Last Comment Bug 298375 - drawWindow() reverses red and blue channels in 16 bit color desktop
: drawWindow() reverses red and blue channels in 16 bit color desktop
Status: RESOLVED FIXED
[rft-dl]
: fixed1.8.1, verified1.8.0.2
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Mark Smith (:mcs)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-21 11:40 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2006-05-21 17:30 PDT (History)
12 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tab preview of page with opacity over Remote Desktop (19.73 KB, image/png)
2005-12-03 16:27 PST, Vitorio
no flags Details
The actual page with opacity over Remote Desktop (41.97 KB, image/png)
2005-12-03 16:30 PST, Vitorio
no flags Details
possible fix (1.33 KB, patch)
2006-01-25 15:32 PST, Mark Smith (:mcs)
roc: review+
roc: superreview+
roc: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
Simple drawWindow testcase (799 bytes, text/html)
2006-02-24 18:58 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details

Description Ted Mielczarek [:ted.mielczarek] 2005-06-21 11:40:32 PDT
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 Ted Mielczarek [:ted.mielczarek] 2005-06-21 11:57:15 PDT
Rolled back to 6.14.10.6476, driver date 8/25/2004, still not working right.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2005-06-22 11:55:19 PDT
Now I can't reproduce this anymore.  There's something very strange about this.
Comment 3 neil@parkwaycc.co.uk 2005-11-30 15:46:51 PST
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 2005-11-30 20:34:21 PST
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) 2005-12-01 08:40:09 PST
On these non-working configurations, does 'opacity' work?
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2005-12-01 11:29:08 PST
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 2005-12-03 16:27:21 PST
Created attachment 204920 [details]
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 2005-12-03 16:30:39 PST
Created attachment 204921 [details]
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 2005-12-04 21:22:37 PST
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 2005-12-04 21:25:39 PST
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] 2005-12-04 22:40:39 PST
(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 2005-12-04 23:18:39 PST
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 2005-12-04 23:23:21 PST
Anyone want to verify if their computer reproduces bug out in 16 color mode?
Comment 14 Ted Mielczarek [:ted.mielczarek] 2005-12-05 07:49:36 PST
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 2005-12-05 19:25:34 PST
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) 2005-12-15 15:23:04 PST
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] 2005-12-15 16:38:22 PST
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 2006-01-10 02:15:07 PST
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 2006-01-11 14:48:43 PST
related to bug 293726? (Solaris bug where red and blue are swapped)
Comment 20 Mark Smith (:mcs) 2006-01-23 10:49:10 PST
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) 2006-01-25 15:23:20 PST
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) 2006-01-25 15:32:46 PST
Created attachment 209636 [details] [diff] [review]
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) 2006-01-25 19:16:30 PST
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) 2006-01-26 13:37:00 PST
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) 2006-01-26 14:48:28 PST
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 2006-01-27 08:43:01 PST
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 [:bsmedberg] 2006-01-27 08:45:58 PST
Comment on attachment 209636 [details] [diff] [review]
possible fix

1.8.0.1 is already gone
Comment 28 Mark Smith (:mcs) 2006-02-16 06:53:52 PST
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 Ted Mielczarek [:ted.mielczarek] 2006-02-16 07:17:03 PST
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) 2006-02-16 07:29:58 PST
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) 2006-02-16 08:21:59 PST
I agree that the risk is minimal.
Comment 32 Daniel Veditz [:dveditz] 2006-02-22 12:21:22 PST
Comment on attachment 209636 [details] [diff] [review]
possible fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 33 Mark Smith (:mcs) 2006-02-22 13:48:55 PST
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] 2006-02-24 14:48:45 PST
please attach a testcase to this bug
Comment 35 Ted Mielczarek [:ted.mielczarek] 2006-02-24 18:58:36 PST
Created attachment 213143 [details]
Simple drawWindow testcase

Testcase, requires signed.applets.codebase_principal_support set to true.  You should see two red boxes.
Comment 36 Ted Mielczarek [:ted.mielczarek] 2006-03-02 21:49:27 PST
Hopefully davel doesn't mind me twiddling his whiteboard flags...
Comment 37 Jay Patel [:jay] 2006-03-08 18:14:34 PST
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 2006-05-21 10:08:49 PDT
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] 2006-05-21 10:15:33 PDT
(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 2006-05-21 10:27:05 PDT
> (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 2006-05-21 17:30:24 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.