312921 - Image having an alpha channel is rendered incorrectly in a translucent window

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Alex Sirota (iosart)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: 

I'm creating a translucent XUL window (hidechrome = "true", style =
"background-color:transparent"). Inside the window there is a single 'image'
element with a transparent PNG image. I position this translucent window against
a white background and the alpha blended result looks wrong, compared to the
same image rendered against a white background inside Firefox content area (or
rendered by any other program).

Reproducible: Always

Steps to Reproduce:
1. Open the attached grad.png file in Firefox (make sure the background is white)
2. Open the attached grad.xul file with "firefox -chrome" or XULRunner
3. Drag the opened translucent window and position it under the image in Firefox
4. Compare the two images

Actual Results:  
The images look different over a white background.

Expected Results:  
The same image (grad.png), rendered inside Firefox over a white backgound and
rendered by a translucent window over the same background should look identical.

I'm seeing this with XULRunner built from the trunk yesterday.

Comment 1

[Alex Sirota (iosart)]

Attachment: A screenshot demonstrating the problem

Comment 2

[Alex Sirota (iosart)]

Attachment: Test image

Comment 3

[Alex Sirota (iosart)]

Attachment: Test XUL file using the above image

Open this XUL file as a top-level window

Comment 4

[Alex Sirota (iosart)]

Ok, I hope I'm not way off here, but I think I know what the problem is, and, at
least partially, how to solve it. 

When the PNG with transparency is being painted, it sees that the image has an
alpha channel so it's alpha blended onto the background. But, the background is
actually 'transparent', so there's nothing there. So, the result is that the
image is blended with a black background. Then, the result is given to the OS
(UpdateLayeredWindow) and then the OS does another alpha blending step with it.
But we actually need only the second step! 

So, if the original image color was C with the alpha value A, after the image is
painted, we have value C' = (C*A)/255 (instead of C) and the alpha value is
still A. 

Now, the solution as far as I can see, is as follows. Before sending the color
to UpdateLayeredWindow, each channel is currently multiplied by A. To get the
correct color the result would need to be (C*A)/255, but because we have C' its
(C'*A)/255 = (((C*A)/255)*A)/255. So, basically, if we don't actually perform
this pre-multiplication of every channel by A, we will send the correct value -
(C*A)/255 (as if we multiplied the correct pixel value). I tested this theory by
commenting out the three 'FAST_DIVIDE_BY_255(pPixel [...], *pAlpha * pPixel
[...]);' lines in nsWindow.cpp and the results are now correct. 

I'm attaching a patch to demonstrate what I did, I think there are a couple of
other places that need to be treated similarly. 

Comment 5

[Alex Sirota (iosart)]

Attachment: Partial fix for the problem

Comment 6

[Alex Sirota (iosart)]

Attachment: Fixes the problem for the 16 bit screen mode too

Comment 7

[Alex Sirota (iosart)]

Guys, any comments on the above?

Comment 8

[Dainis Jonitis]

Interesting. I have not noticed it before because my test octopus.gif and alphapng.png did not looked suspiciously wrong, especially on my black desktop.

I used premultiplication because MSDN says we should do that:
  http://msdn.microsoft.com/library/en-us/gdi/bitmaps_3b3m.asp?frame=true

"Note that the APIs use premultiplied alpha, which means that the red, green and blue channel values in the bitmap must be premultiplied with the alpha channel value. For example, if the alpha channel value is x, the red, green and blue channels must be multiplied by x and divided by 0xff prior to the call."

Probably I misread this note and confused "alpha channel" with case when SourceConstantAlpha != 0xFF.
If Robert can confirm that blender provides correct pixel values then your change should be correct. It will be pretty nice speed optimization. Great!
Just throw away these lines instead of commenting them out.

Comment 9

[Dainis Jonitis]

I would even recommend to modify 32-bit loop to point pPixel to alpha part to avoid pPixel [3] inside the loop and thus make it even faster.

    if (hMemoryDC)
    {
      PRUint8* pPixel = w2k.mMemoryBits + 3;    // Point to Alpha component
      PRUint8* pAlpha = mAlphaMask;
      PRInt32 pixels = mBounds.width * mBounds.height;

      for (PRInt32 cnt = 0 ; cnt < pixels ; cnt++)
      {
        *pPixel = *pAlpha;

        pPixel += 4;
        pAlpha++;
      }

Comment 10

[Dainis Jonitis]

I will attach new patch with speed optimizations for both loops in a couple of hours.

Comment 11

[Dainis Jonitis]

After some more thinking and looking at sources of other Win32 alpha blending applications there was no problem with current Win32 widget code. UpdateLayeredWindow indeed requires the premultiplication of each of R, G & B component with this pixel's alpha value.

What Alex was sugesting was just working around the problem that blender already passed the premultiplied values to the widget code. By removing the Win32 code he just avoided the doubled effect.

I think that the blender should instead pass the non-premultiplied image and widget code should do it's blending in the way that platform API requires. For example if some platform at the API level works with non premultiplied values then it would be stupid to force them to restore the original image data from one that was supplied by blender.

If there are changes made to pass the non-premultiplied values to the widget code then for platforms that support only 1-bit transparency (e.g. Win95) widget code will have to be adjusted to multiply RGB values with alpha to get the color 
intensities right. The best that they can get will be the same blend on top of black background.

At least the comment in nsIWidget.h should explicitly say whether the pixel data is premultiplied by alpha or not.

Robert, what do you think?

Comment 12

[Alex Sirota (iosart)]

I'm not very familiar with Mozilla rendering and blending mechanisms, but the problem as I see it is as follows: each element is rendered onto a canvas, which either contains the background or some pixels already rendered by the elements above it. If the element contains an alpha channel it is blended onto the background, otherwise it's just painted over it. Now, let's say we are blending an image with an alpha channel onto some canvas. Some pixels of that canvas are 'transparent' or 'empty', so no alpha blending should be performed on them (the OS will do that later), while others are opaque, and those pixels should be blended with the image pixels. So, it looks like we need to perform different operations on different pixels! 

What happens now, as far as I can see is that the image is blended (all its pixels), but because there is no such thing as an 'empty' background, all the 'empty' pixels are black by default. The OS indeed requires us to do pre-mulitiplication, but if the pixels have already been blended, this will produce an incorrect effect. So, we can either try and remove the initial blending (seems like a difficult thing to do), or to cancel this effect by removing the final pre-multiplication step. Hope all this makes sense :)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> I think that the blender should instead pass the non-premultiplied image and
> widget code should do it's blending in the way that platform API requires. For
> example if some platform at the API level works with non premultiplied values
> then it would be stupid to force them to restore the original image data from
> one that was supplied by blender.

I think most APIs use premultiplied. Cairo does. Let's keep the pixel data going down into widget/ as premultiplied, and comment that in nsIWidget.h

Comment 14

[Dainis Jonitis]

Attachment: Faster loops in Win32 widget code + comments in nsIWidget.h

Comment 15

[Ere Maijala (slow)]

Comment on attachment 201295 [details] [diff] [review]
Faster loops in Win32 widget code + comments in nsIWidget.h

+  w2k.mMemoryBits     = NULL;   // RGB components are already premultiplied with alpha

I find this comment confusing as it implies setting mMemoryBits is somehow related to the premultiplication. 

Now that I look at it, you should also change needConversion to
PRBool needConversion = (depth == 24);

This would make it more obvious that the conversion branch only works when the depth is 24.
 
With these fixed, r=ere.

Comment 16

[Dainis Jonitis]

Attachment: Fix Ere review notes

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 201346 [details] [diff] [review]
Fix Ere review notes

nice

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Dainis, do you have CVS access yet?

If not, why not? :-)

Comment 19

[Dainis Jonitis]

Robert, I do not have CVS access. As I do not make patches often enough I think it is better to stay away from trouble :) I will appreciate if you check in the patch for me.

BTW is there any chance that both Bug 312921 and Bug 313927 will be eventually included in 1.8 branch? Most likely it is too late for Firefox 1.5, but what about 1.5.0.2? It is so unfortunate that these two bugs were discovered so long after initial checkin and just before major release.

And many thanks to Alex for finding and fixing this problem!!

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, I'll check it in for you.

I think that these patches are not suitable for the FF1.5 branch unless we find someone who has an application or extension that really needs them.

Comment 21

[Alex Sirota (iosart)]

(In reply to comment #19)
> 
> BTW is there any chance that both Bug 312921 and Bug 313927 will be eventually
> included in 1.8 branch? Most likely it is too late for Firefox 1.5, but what
> about 1.5.0.2? It is so unfortunate that these two bugs were discovered so long
> after initial checkin and just before major release.
> 

Thanks a lot to everyone for your quick response!

Dainis, I totally agree - this is very unfortunate. 
Robert, can we pull some strings to get Bug 312921 and Bug 313927 into Firefox 1.5? I went over the patches and it looks like the regression potential of these patches is minimal, as they both affect only translucent windows. Translucency is a major feature in 1.5 IMHO, and if there are trivial fixes for the two problems, maybe they should be allowed in? 

I've started looking into translucensy to maybe use this very nice feature in one of my extensions, that's how I discovered this bug... I think that maybe this is a "if you build it, they'll come" situation - if this feature is not broken in FF 1.5, people will surelly do very cool things with it, which will greately promote Firefox. And again, the patches' regression potential seems so small :)

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The thing is, these bugs don't break the feature too badly. If you're going to push for 1.5 inclusion you need to explain exactly what you were wanting to do that you can't do without these fixes.

Comment 23

[Benjamin Smedberg]

This is not 1.5-fodder.

Comment 24

[Alex Sirota (iosart)]

(In reply to comment #22)
> The thing is, these bugs don't break the feature too badly. If you're going to
> push for 1.5 inclusion you need to explain exactly what you were wanting to do
> that you can't do without these fixes.
> 
Well, I just started to play with this, so I'm afraid I don't have anything tangible at this point. I agree, these bugs don't really break the feature, but because the whole purpose of the feature is being 'smooth' and 'sexy', these artifacts might really ruin the effect for people that want to use it, IMHO.
I'm attaching another screenshot of the problem. This was one of the first things I tried, and right away I noticed that something was wrong, because the effect seemed a bit 'off'. 

I'm not sure what Mozilla policy on branch checkins is (especally so close to the release, and this not being a blocker), but I believe that there is some kind of a value/danger trade-off. I agree that the value here is not critical, but on the other hand the danger is not so high too, IMHO.

Comment 25

[Alex Sirota (iosart)]

Attachment: Another example of the problem

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked into trunk.

Comment 27

[Alex Sirota (iosart)]

Comment on attachment 201346 [details] [diff] [review]
Fix Ere review notes

This landed on the trunk a long time ago (before Firefox 1.5) - I'd hate to see Firefox 2.0 released without this fix...

David: I hope you're the right person for this :)

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does the author of the patch think the patch should go in?  Bugzilla users in general shouldn't make approval requests; they should either (1) make blocking requests or (2) ask the patch author to make an approval request if the patch author thinks the patch is appropriate for whatever branch.  One of the principles of requests like this is that two people have to agree:  the patch author and one of the author's peers.  Other users requesting approval skips that step: you get only one person, the peer, and not the author.

Comment 29

[Alex Sirota (iosart)]

David - thanks for the explanation - I admit I didn't understand the exact process very well... 

In comment 19 Dainis said:
--------------------
BTW is there any chance that both Bug 312921 and Bug 313927 will be eventually
included in 1.8 branch? Most likely it is too late for Firefox 1.5, but what
about 1.5.0.2? It is so unfortunate that these two bugs were discovered so long
after initial checkin and just before major release.
--------------------

Dainis - do you think the patch should land on the 1.8.1 branch? I believe that because it only affects translucent windows, its regression potential is minimal...

Comment 30

[Darin Fisher]

Comment on attachment 201346 [details] [diff] [review]
Fix Ere review notes

a=darin on behalf of drivers

Comment 31

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/public/nsIWidget.h 	3.140.4.3
mozilla/widget/src/windows/nsWindow.cpp 	3.569.2.30