Open sezero opened 3 years ago
@slouken, @icculus: This was marked as target-2.0.16
in original bugzilla: Needs you guys' love & care.
If I may correct my criticism a bit (sorry, I'm a bit rusty on compositing...)
Treating every colour component the same is only possible when you use what's called premultiplied alpha. This is so common in other libraries that I've worked on that I forgot the situation was different with straight (non-premultiplied) alpha, which from the way BlitRGBtoRGBPixelAlpha()
handles the RGB components, is clearly the case here.
For those not familiar with the distinction, there's a good description on Wikipedia. It's the difference between this for premultiplied alpha:
and this for straight alpha:
(Those formulae for αo are actually equivalent.)
The old SDL 1.2 code is consistent with straight alpha formula for Co, if you assume αb = αo = 1. In other words, they're correct if you take the source image to be ARGB with straight alpha, and the destination image to be XRGB (i.e. assumed opaque, with the most-significant 8 bits ignored). The problem is that SDL 2.0 appears to be applying this function to cases where the most-significant 8 bits of the destination image are also interpreted as alpha, and the only part of the function that has been changed is that for calculating the alpha channel, not the RGB channels.
To be fair, the function name BlitRGBtoRGBPixelAlpha
is ambiguous about whether both images had per-pixel alpha or not.
So the questions to be answered are:
[edit: corrected link to Wikipedia]
I think we're out of time for 2.0.18 on this one, so I'm bumping to 2.0.20.
Moving to SDL 3.0, to review our alpha blending handling.
@sezero, feel free to look at this sooner, but software rendering on an ARM device isn't really a priority given most devices are set up for hardware acceleration at this point. (Feel free to point out important exceptions!)
Moving to SDL 3.0, to review our alpha blending handling.
@sezero, feel free to look at this sooner
It's not my thing at all, it's @bavison's: I just opened the ticket here after his comments.
Originally posted by @bavison in https://github.com/libsdl-org/SDL-1.2/issues/777#issuecomment-871726562:
I think I can see what's happened. In SDL 2.0, the way
BlitRGBtoRGBPixelAlpha()
handled the alpha channel changed in an incompatible way. To be precise, in this commit:https://github.com/libsdl-org/SDL/commit/89bc80f1ae5bd99db448eccbc19ba2981722da1f
There is no equivalent commit in SDL 1.2. My ARM assembly optimisations - both the SIMD and NEON versions - faithfully copied the SDL 1.2 behaviour (this was, after all, my primary target at the time). It looks like nobody, including myself, noticed the difference between the two branches.
First conclusion: I think the code can safely be re-enabled on SDL 1.2.
Having looked more closely at the SDL 2.0 code, I think the new handling of alpha is actually incorrect, according to its own specification. It's desirable that every colour component is treated identically (not least because it makes SIMD processing easier). Look at the least-significant byte of
d1
/s1
(blue) - we can ignore the AND mask because the intermediate value can't overflow for any combination of input values - we can rearrangeto
If you substitute
s1
withalpha
andd1
withdalpha
, this givesBy contrast, we can rearrange
to
These are not equivalent, except (almost) when
alpha == 0xFF
- but that's already been special-cased a few lines above!Some worked examples:
source 0x0f0f0f0f, destination 0x00000000, SDL1.2 result 0x00000000, SDL2.0 result 0x0f000000
source 0x0f0f0f0f, destination 0xffffffff, SDL1.2 result 0xfff0f0f0, SDL2.0 result 0xfef0f0f0
Ideally, I'd have said that we should be aiming for every colour component to be treated the same, which would result in 0x00000000 and 0xf0f0f0f0 respectively for these examples. (There's also an argument that some rounding should be done rather than simple truncation of the 16-bit intermediate product, but I won't go into that now.)
If I were to re-work the SDL2.0 assembly, would it be acceptable to treat all components the same in this way? I wouldn't have to revisit it yet again if the equation is changed a year or two down the line. I can change
BlitRGBtoRGBPixelAlpha()
to match this behaviour easily enough, but would need some assistance from someone familiar with MMX and 3dNOW to make those cases match.