241939 - content with CSS3 opacity <1 is invisible in 256color mode

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect, P1)

Description

[:aceman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040423 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040423 Firefox/0.8.0+

Content with opacity < 1 is not rendered at all. At 1, the content is OK. I know
implementing opacity correctly would be hard in 256 colors. But not displaying
such content is not acceptable. Maybe we should ignore opacity specification in
this case and make the content fully opaque.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

WFM Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040427
Firefox/0.8.0+

Perhaps a GFX problem in Windows98? Reporter you also have Windows 2000, what
happens there?

Comment 2

[:aceman]

Attachment: It is the same in Win98 and 2000 (different machine and graphics card).

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok confirming with that testcase. 

I agree that text with less opacity should be visible indstead of hidding it. I
know from other bugs that the support isn't provided for 256 colors. But this
issue doesn't reflect the UI instead the rendering is faulty and that should be
fixed. We can't hide visible content from the user. I think this color mode was
missed to check when this feature was implemented.

Comment 4

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

Attachment: fix

Comment 5

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

Attachment: fix2

oops. DoOpaqueBlend has never handled the two-source case.

Comment 6

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

Comment on attachment 147301 [details] [diff] [review]
fix2

Never mind. this is wrong too

Comment 7

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

Attachment: real fix

It's too early in the morning.

DoOpaqueBlend never handled the two-source case (rendered content has an alpha
channel), which wasn't *much* of a problem since when opacity == 1.0 we don't
call into the blender. But anyway, we just don't want to call it from Do32,
Do24 and Do16, there's no point in providing a wrong fast path for a case that
never gets here.

This patch changes it to Do8Blend and lets it handle the two-source case
assuming that the rendered content has 1-bit opacity (and treating aOpacity as
0 = transparent, any other value = opaque). Reasonable assumption, and we can't
recover more bits of alpha in any reasonable way, anyway.

Comment 8

[:aceman]

Were you able to confirm also the second problem? In the testcase, if you set
opacity: 0.5, the background is light green (16bit, win2000). Is that correct?

Comment 9

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

That sounds bad.

Ere, we're assuming that the color layout in the 16-bit words is fixed (it's
compiled into nsBlender). Is that true? It sounds like this guy's setup has a
different layout.

Comment 10

[:aceman]

Which guy? Anyway, the color issue seems to be bug 228399.

I can't test your fix for this bug, so if Henrik Skupin can confirm it works,
you can close this bug.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry, I'm not able to compile it for myself. So I can't test your patch.

Comment 12

[:aceman]

Roc, is the patch checked into somomething? Is it enough to wait for the next
release (or get a nightly) and test it there?

Comment 13

[:aceman]

Regarding comment 9: it depends on the way you display it. I hope it is the same
on all systems when you push the values using some standard Win API. But on
lower level, videocards differ in the order of the color components.

Comment 14

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

It's not checked in because Blizzard has not reviewed it.

Comment 15

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

fix checked in.

Comment 16

[:aceman]

I'm sorry but it seems the fix is not enough. Attachment following...

Comment 17

[:aceman]

Attachment: The testcase viewed with Firefox 20040528, which should have the fix.

It looks like the patch changed somethin. The less opaque div is now completely
black instead of white (or fully invisible). We probably didn't want this...
The text was changed to red to test if the text is rendered at all in the less
opaque div. It would be black on black. But it is still not visible, which
seems to indicate the text is not rendered at all. Everything inside the div is
black regardless of any styles.

Comment 18

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

Attachment: fix

This should do it. There was a simple logic error in the other patch.

The first source is the content painted onto a black background, the second
source is the same content painted onto a white background. If the two pixels
are different, that means the content pixel was not opaque and so we should
*not* update the destination. If the two pixels are the same, then the content
pixel was opauqe and we *should* update the destination.

Comment 19

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

Comment on attachment 149714 [details] [diff] [review]
fix

I think this is right, but I can't really test it (setting 8 bit mode on my
machine seems to crash it). Would be nice if aceman or someone else on this bug
could try the change.

Comment 20

[:aceman]

I would gladly test it, but I can't compile Mozilla on my own :(

Comment 21

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

checked in

Comment 22

[:aceman]

I still see this on firefox 1.0. Was it checked into that branch or is it in the
trunk only?

Comment 23

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

trunk only