Closed Bug 312921 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: XUL, defect)

1.8 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: alex, Assigned: jonitis)

Details

(Keywords: fixed1.8.1)

Attachments

(5 files, 3 obsolete files)

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.
Attached image Test image
Open this XUL file as a top-level window
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. 
Summary: Image having an alpha channel is rendered incorrectly in a translucent window → Image having an alpha channel is rendered incorrectly in a translucent window
Attached patch Partial fix for the problem (obsolete) — Splinter Review
Attachment #200025 - Attachment is obsolete: true
Guys, any comments on the above?
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.
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++;
      }
I will attach new patch with speed optimizations for both loops in a couple of hours.
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?
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 :)
(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
Attachment #200038 - Attachment is obsolete: true
Attachment #201295 - Flags: superreview?(roc)
Attachment #201295 - Flags: review?(emaijala)
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.
Attachment #201295 - Flags: review?(emaijala) → review+
Attachment #201295 - Attachment is obsolete: true
Attachment #201346 - Flags: superreview?(roc)
Attachment #201295 - Flags: superreview?(roc)
Comment on attachment 201346 [details] [diff] [review]
Fix Ere review notes

nice
Attachment #201346 - Flags: superreview?(roc) → superreview+
Dainis, do you have CVS access yet?

If not, why not? :-)
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!!
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.
(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 :)



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.
This is not 1.5-fodder.
(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.
checked into trunk.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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 :)
Attachment #201346 - Flags: approval-branch-1.8.1?(dbaron)
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.
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...
Attachment #201346 - Flags: approval-branch-1.8.1?(dbaron) → approval1.8.1?
Comment on attachment 201346 [details] [diff] [review]
Fix Ere review notes

a=darin on behalf of drivers
Attachment #201346 - Flags: approval1.8.1? → approval1.8.1+
Assignee: nobody → Dainis_Jonitis
mozilla/widget/public/nsIWidget.h 	3.140.4.3
mozilla/widget/src/windows/nsWindow.cpp 	3.569.2.30
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1beta2
Version: Trunk → 1.8 Branch
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.