298375 - drawWindow() reverses red and blue channels in 16 bit color desktop

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[(not currently active) Ted Mielczarek]

Running WindowsXP SP2, with an ATI Mobility Fire GL T2, driver version
6.14.10.6512, driver date 02/01/2005.  (Downloaded from Windows Update),
drawWindow() swaps my red and blue channels.  Tested on a VC6 nightly build and
a VC71 self build.

I am going to try rolling back the driver and see if that is indeed the problem.
 These same builds worked fine yesterday before updating my driver.

Comment 1

[(not currently active) Ted Mielczarek]

Rolled back to 6.14.10.6476, driver date 8/25/2004, still not working right.

Comment 2

[(not currently active) Ted Mielczarek]

Now I can't reproduce this anymore.  There's something very strange about this.

Comment 3

[[e-mail]]

I see this with my vc6 trunk self-build, running over Remote Desktop in 16-bit colour. Don't ask me what happens if I run it locally, I've never tried.

Comment 4

[Vitorio]

Alright, I'm using Firefox 1.5 final on Windows XP SP2 with an ATI Radeon 9800 Pro using drivers 6.14.10.6476, dated 8/25/2004.  I see this problem when I use Firefox over Remote Desktop.

Specifically:

When I start a FF1.5 browser session locally, I do not see this problem.

When I log into my PC via Remote Desktop (checked on a variety of machines), I see the red and blue channel swap issue on the browser session that was left open and worked fine locally.

If, via RDP, I exit that same browser session and start a new one, it too will have the problem.

If I log back into my PC locally, with the browser session that I started via RDP still running, it will also continue to exhibit the issue.

If I exit that session and start a new one while logged in locally, it will once again perform normally.

Comment 5

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

On these non-working configurations, does 'opacity' work?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm guessing that this has something to do with our canvas/image code assuming a certain RGB ordering for certain platforms, and windows might change that for RDP.  Thus images that we render will still look correct (since we explicitly tell windows that they're BGR or whatever), but canvas could look swapped because we create an image and then change its pixel data via lockpixels and the like.

Comment 7

[Vitorio]

Attachment: Tab preview of page with opacity over Remote Desktop

This is a screenshot of a tab preview of a page using opacity seen over Remote Desktop.

Comment 8

[Vitorio]

Attachment: The actual page with opacity over Remote Desktop

This is a screenshot of the actual page that the previous tab preview screen shot is of, also over Remote Desktop, showing that the page is rendered correctly, except in the tab preview.

Comment 9

[mnnel3s02]

I'm experiencing the bug with Tab Preview - and I'm not using remote desktop.

I'm on a win2k machine with an nvidia ti4200, just installed firefox 1.5 final, and the following extensions installed:
DOM inspector
talkback
image zoom
gmail notifier
mozilla calender [disabled, incompatible with 1.5]
bugmenot
FLST
viamatic tabnail[disabled since I found your extension]
foobar controls
forecastfox
tabpreview
adblock plus [which I disabled since it was the only thing I installed yesterday, but that didn't fix anything]

Comment 10

[mnnel3s02]

BTW, bug is persistent - started yesterday [I didn't notice the bug until I'd used the extension for a day, not 100% sure if it was active when installed], and continues today through multiple sessions + reboots, every preview page made.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #10)
> BTW, bug is persistent - started yesterday [I didn't notice the bug until I'd
> used the extension for a day, not 100% sure if it was active when installed],
> and continues today through multiple sessions + reboots, every preview page
> made.

Wow, that's just odd.  What are your display resolution and depth settings?

Comment 12

[mnnel3s02]

1600x1200 high color (16 bit)
Changing resolutions - no fix

Fix found: Change to 32 bit color
256 and 16 bit color both produce bug in any resolution, 32 bit color eliminates bug.  I've had 16 bit color on because my monitor doesn't seem to get any benefit or penalty from 32.

(In reply to comment #11)
> (In reply to comment #10)
> > BTW, bug is persistent - started yesterday [I didn't notice the bug until I'd
> > used the extension for a day, not 100% sure if it was active when installed],
> > and continues today through multiple sessions + reboots, every preview page
> > made.
> 
> Wow, that's just odd.  What are your display resolution and depth settings?
> 

Comment 13

[mnnel3s02]

Anyone want to verify if their computer reproduces bug out in 16 color mode?

Comment 14

[(not currently active) Ted Mielczarek]

This is very reproducible for me by switching to 16 bit color.  In fact, if you have Firefox running, you can switch to 16 bit, reproduce the bug, switch back to 32 bit and not see it anymore.  That must have been what happened to me when I originally reported the bug.

I'd also be willing to bet that RDP uses 16 bit color all the time, since Neil said his is 16 bit in comment 3.

Comment 15

[mnnel3s02]

drawWindow() itself may be at fault here, not just the extension.

Bug is reproduceable in Viamatic Tabnail and likely other extensions that use the command.

Comment 16

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

Vlad, can you take this on, since it's presumably windows-only? Or get Pav to take a look?

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I've been meaning to take a look; it's on my todo list.  I have a theory, but we'll see :)

Comment 18

[Kyungjoon Lee]

I see this too, with an ATI 9700 Pro, Windows 2000. CATALYST version 05.13, 2D version 6.14.10.6587.

Comment 19

[Kathleen :Brade]

related to bug 293726? (Solaris bug where red and blue are swapped)

Comment 20

[Mark Smith [:mcs]]

I also see this with our Pearl Crescent Page Saver extension (http://pearlcrescent.com/products/pagesaver/).  Tested on Windows XPsp2 with NVIDIA GeForce FX 5200 video.  I see the same behavior with the Tab Sidebar extension; both extensions use .drawWindow().

Comment 21

[Mark Smith [:mcs]]

Kathy Brade and I debugged this some today.  So far we believe:

- Inside nsCanvasRenderingContext2D::UpdateImageFrame(), the RGB values are correct.

- Inside nsCanvasRenderingContext2D::DrawNativeSurfaces(), the red and blue components are swapped in the destination.  I think the root of the problem is inside nsDrawingSurfaceWin::CreateBitmapInfo() where the pixel format is determined.  Patch coming soon.

Comment 22

[Mark Smith [:mcs]]

Attachment: possible fix

This patch changes the pixel format on Windows when the video depth is less than 32 (swapping the red and green offsets), which seems to fix this bug.  I may be confused by Microsoft's documentation, endianness, etc. but my reading of the description of the biBitCount=24 section on this page tells me the order is blue, green, and red:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/bitmaps_1rw2.asp

I am a little worried this might break something else though.

Comment 23

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

Comment on attachment 209636 [details] [diff] [review]
possible fix

That seems to agree with the MSDN docs. Thanks for finding this. Kathleen can check this in.

Comment 24

[Mark Smith [:mcs]]

Thanks for the review.  I looked around and do not really see any other places in the tree where the pixel format is used.

Fix committed to the trunk:

mozilla/gfx/src/windows/nsDrawingSurfaceWin.cpp
  new revision: 3.22; previous revision: 3.21
    Bug 298375 - drawWindow() reverses red and blue channels in 16 bit color.
      Return correct pixel format information for color depth < 32 on Windows.
      r+sr=roc.

I am not sure what the criteria are for landing this on MOZILLA_1_8_BRANCH, but it seems like it should also land there.

Comment 25

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

Please request review for 1.8.1 at least, and possibly 1.8.0. This patch is trivial and I believe it's very low risk.

Comment 26

[Kathleen :Brade]

Comment on attachment 209636 [details] [diff] [review]
possible fix

requesting that this patch land on various branches (only applies to canvas feature on Windows)

Comment 27

[Benjamin Smedberg]

Comment on attachment 209636 [details] [diff] [review]
possible fix

1.8.0.1 is already gone

Comment 28

[Mark Smith [:mcs]]

Thanks for the 1.8.1 approval.  Committed to MOZILLA_1_8_BRANCH:

mozilla/gfx/src/windows/nsDrawingSurfaceWin.cpp
new revision: 3.21.8.1; previous revision: 3.21
  Commit fix from the trunk (a=roc):
    Bug 298375 - drawWindow() reverses red and blue channels in 16 bit color.
        Return correct pixel format information for color depth < 32 on Windows.
        r+sr=roc.

Comment 29

[(not currently active) Ted Mielczarek]

This would be nice for 1.8.0.2, I consistently get mail from people using Tab Preview in 16 bit color seeing this bug.

Comment 30

[Mark Smith [:mcs]]

I'd also like to reinterate my strong belief that this fix should be approved for 1.8.0.2 (so it is included in an upcoming Firefox 1.5.x release).

Statistics I examined show that about 20% of requests on the web come from browsers that are running in an environment with less than 32 bit color depth (I don't know what percentage of those are from Windows, but probably most of them).  That means all of those users will experience this bug.  And a lot of the new Firefox 1.5 extensions rely on drawWindow(), so this is an important user satisfaction issue.  Here are just a few of the extensions I know about that almost certainly use drawWindow() and are therefore affected by this bug:
  Firefox Showcase (an Extend Firefox contest finalist)
  Reveal (also a finalist)
  Viamatic foXpose (also a finalist)
  Tab Sidebar
  Tab Preview

I am one of the authors of the Pearl Crescent Page Saver and have received quite a few bug reports from people who are running with 16-bit color depth (the most common choice aside from 32-bit).

The other side of the coin is risk.  This is a trivial patch.  After studying the code and poking around in the tree, we determined that the only consumer of the pixel format (which is what this bug fixes) is the canvas drawWindow() code... so there is very little risk of unrelated regressions.

Comment 31

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

I agree that the risk is minimal.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 209636 [details] [diff] [review]
possible fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 33

[Mark Smith [:mcs]]

Thank you very much for the 1.8.0.2 approval!  Committed to MOZILLA_1_8_0_BRANCH:

mozilla/gfx/src/windows/nsDrawingSurfaceWin.cpp
new revision: 3.21.16.1; previous revision: 3.21
  Commit fix from the trunk (a=dveditz):
    Bug 298375 - drawWindow() reverses red and blue channels in 16 bit color.
      Return correct pixel format information for color depth < 32 on Windows.
      r+sr=roc.

Comment 34

[Dave Liebreich [:davel]]

please attach a testcase to this bug

Comment 35

[(not currently active) Ted Mielczarek]

Attachment: Simple drawWindow testcase

Testcase, requires signed.applets.codebase_principal_support set to true.  You should see two red boxes.

Comment 36

[(not currently active) Ted Mielczarek]

Hopefully davel doesn't mind me twiddling his whiteboard flags...

Comment 37

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2, I see both red squares with the simple drawWindow testcase (did not see the first with Firefox 1.5.0.1).

Comment 38

[Alex]

I'm experiencing the same problem on my MacBook Pro. Firefox 1.5 is running fine but I use "reveal" for tab-preview and the colors of the thumbs are wrong. In 16,7mio colors mode every thumb is very cyan and in 32768 colors mode the thumbs are magenta.
Will this be fixed in an upcoming Firefox 1.5.x version?

Comment 39

[Dave Townsend [:mossop]]

(In reply to comment #38)
> Will this be fixed in an upcoming Firefox 1.5.x version?

This should be fixed in firefox 1.5.0.2. You should upgrade to that.

Comment 40

[Alex]

> (In reply to comment #38)
> > Will this be fixed in an upcoming Firefox 1.5.x version?
> 
> This should be fixed in firefox 1.5.0.2. You should upgrade to that.

I'm using Firefox 1.5.0.3

Comment 41

[Kathleen :Brade]

(In reply to comment #38)
> I'm experiencing the same problem on my MacBook Pro. Firefox 1.5 is running
> fine but I use "reveal" for tab-preview and the colors of the thumbs are wrong.
> In 16,7mio colors mode every thumb is very cyan and in 32768 colors mode the
> thumbs are magenta.

The Intel Mac problem is covered in bug 338062.  Please post to that bug and ask that the fix be approved for the branches (so it will make it into an upcoming Firefox release).  So far the fix has landed on the trunk only.