[BUG+FIX] Blitting wrong alpha values (Burnings and IImage)

[chronologicaldot]

Sorry for the terse title. Anyways...
When copying an image with alpha (the "source" image) to another image with alpha (the "destination" image), either via IImage::copyToWithAlpha() or via the Burnings Renderer (both of which call the same offending function), the source image's alpha channel completely replaces that of the destination alpha rather than blending the two. The offending code is in SoftwareDriver2_helper.h, function inline u32 PixelBlend32 ( const u32 c2, const u32 c1 ), line 347. Problem affects all current svn revisions up to 5589 (checked out today).

Code: Select all

 
/*!
    Pixel = dest * ( 1 - SourceAlpha ) + source * SourceAlpha
*/
inline u32 PixelBlend32 ( const u32 c2, const u32 c1 )
{
    // alpha test
    u32 alpha = c1 & 0xFF000000;
 
    if ( 0 == alpha )
        return c2;
 
    if ( 0xFF000000 == alpha )
    {
        return c1;
    }
 
    alpha >>= 24;
 
    // add highbit alpha, if ( alpha > 127 ) alpha += 1;
    alpha += ( alpha >> 7);
 
    u32 srcRB = c1 & 0x00FF00FF;
    u32 srcXG = c1 & 0x0000FF00;
 
    u32 dstRB = c2 & 0x00FF00FF;
    u32 dstXG = c2 & 0x0000FF00;
 
 
    u32 rb = srcRB - dstRB;
    u32 xg = srcXG - dstXG;
 
    rb *= alpha;
    xg *= alpha;
    rb >>= 8;
    xg >>= 8;
 
    rb += dstRB;
    xg += dstXG;
 
    rb &= 0x00FF00FF;
    xg &= 0x0000FF00;
 
    return (c1 & 0xFF000000) | rb | xg;
}
 

The trail of function calls results in c1 being the source color and c2 being the destination color. With that in mind, it's should be clearly evident that the last line is using only the alpha of the source color.
Again, the correct behavior should be blending the two. I propose the last line be replaced with the following fix:

Code: Select all

 
    u32 alphaSum = (c1 >> 24) + (c2 >> 24);
    alphaSum = (0x000000FF*((alphaSum & 0x0000FF00) > 0)) | (alphaSum & 0x000000FF);
    return (alphaSum << 24) | rb | xg;
 

That's a semi-crammed-but-proven-tested-way of saying this:

Code: Select all

 
    u32 alphaSum = (c1 >> 24) + (c2 >> 24);
    if ( alphaSum & 0x0000FF00 > 0 )
        return 0xff000000 | rb | xg;
    else
        return ((alphaSum & 0x000000FF)<<24) | rb | xg;
 

Take your pick. I only tested the first version, and it did what I wanted.
I could send you an image, but it's probably easy enough for you to pick a couple random images from your computer and use this test:

Code: Select all

 
#include <irrlicht.h>
 
using namespace irr;
using namespace video;
 
int main() {
    IrrlichtDevice* dev = createDevice(EDT_NULL);
    IVideoDriver* vid = dev->getVideoDriver();
    io::IFileSystem* fsys = dev->getFileSystem();
 
    io::path myBackImagePath = ""; // set this to the path of your first image
    io::path myFrontImagePath = ""; // set this to the path of your second image
 
    IImage* backImage = vid->createImageFromFile(myBackImagePath);
    IImage* frontImage = vid->createImageFromFile(myFrontImagePath);
 
    if ( !backImage || !frontImage )
        return 1;
 
    video::SColor white(255,255,255,255);
    core::vector2di zeroVector(0,0);
    core::dimension2du frontImageSize = frontImage->getDimension();
    core::dimension2di frontImageSizeI(frontImageSize);
    core::recti frontImageSrcRect(zeroVector, core::vector2di(frontImageSizeI));
    core::vector2di frontImageULC(0,0);
 
    frontImage->copyToWithAlpha(backImage, frontImageULC, frontImageSrcRect, white);
    vid->writeImageToFile(backImage, io::path("out.png"));
 
    backImage->drop();
    frontImage->drop();
 
    return 0;
}
 

What you'll be looking for is a rough edge around any partially-transparent objects in the image. Ideally, use a front image with low a low alpha channel.
Side notes:
I tried rendering using OpenGL and it also gave unsatisfactory results. I'm not sure if Burnings is simply trying to mimic OpenGL activity or not, but it's rather annoying. I tried various values for the "white" color you see in my given example test as well as various render settings and materials before finding out it was this pixel blending function.

[CuteAlien]

When just reading the function-name I rather would have expected this to be the correct way to handle it (as it copies with alpha... and doesn't mention "blend" as it usual does in Irrlicht with blending functions). But from documentation it seems indeed to expect to use alpha as mask. Thought not sure if it means blending or mask as in on/off. No wonder when implementations then mess up because it's not even exactly obvious what is correct *sigh*.

If all drivers behave wrong in the same way then it's maybe better to create a new function for it which works different (people still using software-renders these days tend to be coders which work with it because it never changes -meaning our software drivers are the only renderers out there which have perfect identical render-results independent of graphic-cards which is quite important in some software and something you don't get with D3D/OpenGL). Maybe call it copyWithBlending or so.

Not sure if I'll get to checking this on weekend already as I wanted to code some other stuff, but I'll try to find some time looking at this. Thanks for reporting and even finding the bug.

[chronologicaldot]

Update: I just checked the original software driver. Turns out, it was also calling the same function, so all of these are tied together, which explains why its the same.

I'm also toying with OpenGL, but there seems to be another bug there, wherein calling draw2DRectangle prior to drawImage() causes images not to appear... other than that similar jagged partial-alpha edge. I'm going to mess around with it abit more to see if its related.

If it is, in fact, "expected" functionality, then I second your proposition to add copyWithBlending(). Also, the documentation would then need to reflect this change. This still leaves the task of incorporating this into the video drivers.
Edit: For IVideoDriver::draw2DImage(), we could add a parameter "blend=true". This would preserve backwards compatibility, distinguish from just using alpha channel copying, and save us from having to add a new function to the API.

Edit (snipping excess): I was considering it to be a copy, but I as dug into the code, I found that both the original and Burnings drivers treat useAlphaChannelOfTexture as a switch for blending (the original just defers everything to the IImage interface). Copying the alpha channel values directly is done by turning that switch off. Currently, PixelBlend32(u32 c1, u32 c2) is broken because it can't decide whether to treat it as a mask or as a blend. Most of the function is designed for blending, and in fact, there is another function called PixelBlend(u32 c1, u32 c2, u32 alpha) that doesn't have this confused identity because it uses a given alpha value... which it uses for blending. The fact that pretty much everything in the code as far as nomenclature suggests this is BLENDING means that it's supposed to be blending.

In short, I still say it's a bug.
If useAlphaChannelOfTexture is supposed to turn alpha on and off, then we would have to treat the "false" version of this as buggy too since it copies the alpha channel.

Historical note: It's easy to see how this went hidden for so long. If you set the background color for IVideoDriver::beginScene() to black, you would think that the rendering of the image-with-alpha values was rendering solid black in the partially-transparent section when in actuality, that was the filler from beginScene(). Only now that we have render targets was I able to export the renders to file and see the bug.

[devsh]

This is a bit off-topic... but have you thought of separating the Image-blob handling from the IImage class.

Basically a lot more formats and loading methods could be supported without a mess of un-implemented copyToScaling() etc functions in IImage or painfully extending the IImage class which affects the software drivers.

[CuteAlien]

@chronologicaldot: On first view I tend to agree with you. But alpha calculations can become tricky when you keep an alpha intact which is already applied once (because if you continue to use the alpha it will become applied twice). So could maybe be something about reverting alpha for those situations or about chaining alpha image calculations. It seems the other PixelBlend32 function simply resets the alpha to 0.

I'll ask Thomas who wrote this code, maybe he still has an idea about it.

@devsh: I didn't touch this code before. Right now I'm more confused about CImage using code from a file called SoftwareDriver2_helper.h (general code using code which sounds like specific driver code...) The copyToScaling function looks implemented to me, but maybe you mean not for all formats (like compressed formats)? Anyway, currently no plans of rewriting it unless I have to.

[CuteAlien]

@chronologicaldot: Why do you add the alphas? This would mean the more half-transparent images you put over each other the more transparent it becomes? Shouldn't it be multiplied? (edit: Yeah - that was stupid. I thought 255 is full transparent when I wrote that, it's certainly full opaque.)

But I'm still trying to wrap my head around what having an alpha in the result here really means (aka - how it would be used in further calculations).

[CuteAlien]

Been reading a little bit about alpha composition (https://en.wikipedia.org/wiki/Alpha_compositing).
According to that, the correct alpha should be:
alpha = srcAlpha + destAlpha*(1-srcAlpha)
(using 0-1 range, we certainly have 0-255 range in Irrlicht and this function should be fast so doing that with bit-shifting operations etc)

[chronologicaldot]

That's the correct formula, but that's not what the function is actually doing. The destination alpha isn't even incorporated into the result, hence the "destAlpha*(1-srcAlpha)" isn't implemented in the function.
The alpha composition should be additive because adding alpha makes it LESS transparent. It gets less and less transparent as you add images on top of each other, sort of like how plastic becomes harder to see through when you add more layers of plastic. My fancy little bit tricks in my suggestion were done so as to keep the final calculated alpha within the range 0-255. Just adding the values would create overflow - lopping the top part off - when in actuality, it would mean "max alpha" (255), so I incorporated the overflow bits in. Multiplying the alpha values would be wrong because that DEcreases the final alpha value.

Think about it: Let's say you keep the values as integers and multiply them. If c1 alpha = 255 and c2 alpha = 255, then the final alpha is 65025. That's not right, so we divide by the range (255, so that it's normalized to 1), and we get 255. But suppose that c1 alpha = 1 and c2 alpha = 1. Multiplying them gets 1, but then the normalization results in, well, truncated to zero, so the image disappears. We could pick values other than 1 for more visibilty, but the reality is that anything less than 100% of the range multiplied by something with the same alpha value is going to decrease in visibility.

It's easier to understand with floating point numbers. If our range is 0-1, and our alpha values are 0.4 and 0.4, then the final value is 0.16, which is hardly visible. Or, we could pick 0.8 and 0.8, giving us 0.64, not 0.8, which is what we might expect.

Now, if we are to obey the documentation formula (copied verbatum from the file):
Pixel = dest * ( 1 - SourceAlpha ) + source * SourceAlpha
then either (a) the formula says nothing about the final alpha value or (b) we would need to see the rest of the equation be implemented in the function for alpha, not just for the color values (which it does correctly). It might look something equivalent to:

Code: Select all

 
u32 srcA = (c2 & 0xff000000)>>24;
u32 alpha = ((c1 & 0xff000000)>>24) * (1 - srcA) + srcA;
//... later...
alpha <<= 24;
 

And it would be located somewhere near the top of the function. However, it doesn't exist in any equivalent form in the function. Instead, as I said, it tries to cheat by simply returning the source alpha value.

EDIT: Addendum. Looking at that link you posted, I can see where some confusion arises. From the looks of it, Irrlicht is using "straight" rather than pre-multiplied alpha. After all, you can set all of the color values to full scale. We change the final rendered alpha using only the alpha channel. That makes Irrlicht images "straight" alpha. And yet, this particular function is acting as though the alpha is pre-multiplied.

Taking the original equation and rearranging it:

Code: Select all

 
Pixel = dest * ( 1 - sourceAlpha ) + source * sourceAlpha
= dest - dest * sourceAlpha + source * sourceAlpha
= dest - sourceAlpha * (dest - source)
= dest + sourceAlpha * (source - dest)
 

Within the function, the entire formula is actually present for all of the channels... except for the part about adding together the alpha channels, which is exactly what I'm proposing we implement. Following the blending function:

Code: Select all

 
alpha = destAlpha + sourceAlpha * (sourceAlpha - destAlpha);
 

In code, it might be:

Code: Select all

 
alpha = (c1>>24 + (c2>>24) * ( (c2>>24) - (c1>>24) )) << 24;
 

Notably, this looks different from the earlier form, but I showed above they are mathematically equivalent.

[CuteAlien]

Yes, I did see it's not implemented like that in the function. I was already talking about how to change it. And you are right - alpha is certainly about opacity not transparency, so higher value is less transparency not more (sorry, I often got confused which one is 0 and 255 because we often speak about more alpha when we mean more transparency, I just got that one mixed up - blame it on trying to think on the weekend).

Not yet understanding why you don't like the Wikipedia solution which you say is correct.

[chronologicaldot]

Sorry, I updated my post above, so you'll have to read after the "EDIT". The Wikipedia one is correct. I had to do some mathematically rearranging to make it look like the actual calculations used in PixelBlend32, and fortunately everything agrees.
Update, looking at my code, it looks like I added too much alpha. You're right. So to cut back on the alpha requires the difference factor. That means my first line of my fix becomes:

Code: Select all

 
    u32 alphaSum = (c1 >> 24) + (c2 >> 24) * ( (c2 >> 24) - (c1 >> 24) );
 

[CuteAlien]

Thanks! I'll wait a few days if Thomas gives some feedback, but if he's fine with it I'll change it.

[chronologicaldot]

You reply quick. I added the formula change to my previous post.

[CuteAlien]

OK, I'll check that calculation once more when I get to it (right now a little confused how the 1-srcAlpha from Wiki became destAlph-srcAlpha, but probably works out once I work with other number-range or so).

[CuteAlien]

Thanks for the feedback.
edit: Got an addional mail with more info. So it looks like it's fine to change this :-)
edit2: Somehow burningreggae's answer above with more info vanished again?

[chronologicaldot]

Cool. So we can mark this thread "Resolved" then.