297563 - Transparency doesn't seem to work in 16bit color depth

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

I've tried the testcases in bug 113232.
They don't work for me in 16 bit color depth, but when I set the video card to
32 bit color depth, they do work for me (after that I can reset to 16 bit color
depth and then it still works).
The result is that I don't see anything, while in 32 bit color depth, I can see
the transparent demo.

I'm using WinXP, NVidia GMX200 32MB.

Comment 1

[Dainis Jonitis]

SetDIBits call in nsWindow::UpdateTranslucentWindow fails in 16bit mode.
Investigating.

Comment 2

[Dainis Jonitis]

Ere, Tim, Arron,

Any ideas why ::SetDIBits fails in 16-bit mode, but works fine in 32bit mode?
SDK documentation says that bitmap should not be selected in any device context,
but moving
  ::SelectObject(hMemoryDC, hAlphaBitmap);
line after ::SetDIBits does not help. The problem is same both on ATI and nVidia
cards, so this is not a video driver problem.

Comment 3

[ArronM (:paper)]

UpdateTranslucentWindow() is using ::CreateCompatibleDC, which means no alpha
information will be stored in our bitmap unless the display mode is precisely
32-bit.   We have to use CreateDC instead since we need a 32-bit DC for storing
32-bit values in our bitmap.

Comment 4

[Dainis Jonitis]

I had this thought, too. But then ::UpdateLayeredWindow fails. 

Comment 5

[Dainis Jonitis]

Attachment: nsWindow::UpdateTranslucentWindow that uses CreateDIBSection - better, but still wrong

I replaced the CreateBitmap/SetDIBits with CreateDIBSection. It improves things
slightly, but it is still buggy:
1. Animated octopus displays only the black animated outline, everything else
is transparent
2. The gradient square has some black rectangles by right side
3. Buttons are drawn corrupted - mostly transparent
4. Simple animated gif from 3 pixels with R,G & B works fine, though.

Could be some rounding errors during color conversion from 16-bit compatible
bitmap to 32-bit one in GetDIBits? I am out of ideas what exactly causes these
problems :(
The bug 228399 mentioned some hardware/driver specific bugs with 16-bit display
modes. Anyone remembers what exactly was screwed up?
I guess I will try to not use the compatible bitmap for memory backstore, but
always enforce at least 24 bits. How significantly this will reduce
performance?
Have you any better suggestions? Thanx

Comment 6

[Dainis Jonitis]

Attachment: Fix for 16-bit color modes

This patch like in bug 228399 enforces at least 24bit device independent memory
bitmaps to draw into. If dispaly is in 32bit color mode then no conversion is
required and we can directly access RGBA memory bitmap bits to prmultiply color
values with alpha. If display is in 24bit or less color mode then we create
temporary 32bit RGBA bitmap to use it in call to UpdateLayeredWindow.

Comment 7

[Dainis Jonitis]

Would be nice to have this working correctly in all video modes in upcoming
Firefox 1.1

Comment 8

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

I must be missing something, but where do we clean up mMemoryDC, mMemoryBitmap
and mMemoryBits?

Also we need some comments in nsWindow.h stating what these are for.

Comment 9

[Dainis Jonitis]

  mMemoryDC and mMemoryBitmap GDI handles are released in
SetupTranslucentWindowMemoryBitmap (PR_FALSE), which is called from Destroy ().
There are no GDI object or memory leaks.
Only the mMemoryBits variable was added in this patch. It is allocated and
controlled by GDI subsystem after a call to CreateDIBSection. This memory goes
away after call to DeleteObject(w2k.mMemoryBitmap) in
SetupTranslucentWindowMemoryBitmap ().
  I am not sure that I can in a brief comment say anything better than
"mMemoryBitmap is a memory bitmap" :) I think I used enough comments (compared
to others, of course) in fix for bug 252067 and in this bug I have not
introduced anything new. 

Comment 10

[Dainis Jonitis]

Microsoft engineer Lou Amadio was kind enough to investigate the SetDIBits
problem and document the GDI alpha related limitations in his blog. Thank you
very much for being helpful, Lou!

Here is the qoute:
  "It turns out that SetDIBits doesn't do exactly what you think it should"

And here is the blog entry about alpha aware GDI functions:
  "http://ooeygui.typepad.com/ooey_gui/2005/07/gdi_is_to_alpha.html"

Comment 11

[Ere Maijala (slow)]

Comment on attachment 187203 [details] [diff] [review]
Fix for 16-bit color modes

After a reminder I noticed I've already reviewed this but forgot to add r+.
Sorry about that.

Comment 12

[Josh Aas]

Does the patch on this bug need an approval request?

Comment 13

[Dainis Jonitis]

Yes, this bug needs approval request. It is very low risk - affects only Win32
new feature that was not present in previous version of Firefox and only in
65536 color video mode. Please also consider bug 300297 for 1.8b4, which is also
the low risk and also fixes the transparency problem on Win32, but can cause
undesirable effect that your parent window disappears.

I do not have access rights to check this in, so someone else should help with
getting all approvals and checking into the tree.

Comment 14

[Martijn Wargers (dead)]

You need to set the approval1.8b4 ? flag on the patch, not?

Comment 15

[Dainis Jonitis]

Comment on attachment 187203 [details] [diff] [review]
Fix for 16-bit color modes

asking for approval

Comment 16

[Ere Maijala (slow)]

Comment on attachment 187203 [details] [diff] [review]
Fix for 16-bit color modes

I couldn't apply the patch cleanly to the current cvs tip in branch or trunk.
Could you create a new diff against the latest versions, please?

Comment 17

[Dainis Jonitis]

Attachment: Updated fix for 16-bit color mode

Updated the patch to the latest trunk to avoid merge conflicts. I also took an
opportunity to replace tab symbols with spaces.

Comment 18

[Dainis Jonitis]

Attachment: Updated fix for 16-bit color mode (for 1.8 branch)

This is for MOZILLA_1_8_BRANCH (without any whitespace changes like in trunk
version).

Comment 19

[Ere Maijala (slow)]

Fixes checked into trunk and branch.