Last Comment Bug 323088 - RGB values slightly off in canvas after drawWindow()
: RGB values slightly off in canvas after drawWindow()
Status: RESOLVED FIXED
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-11 15:24 PST by Kathleen Brade
Modified: 2006-04-03 17:40 PDT (History)
5 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2-
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.17 KB, patch)
2006-01-11 15:53 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
vladimir: review+
shaver: superreview+
roc: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description Kathleen Brade 2006-01-11 15:24:20 PST
On Linux, with a 16-bit depth display, drawWindow() doesn't produce color values that are perfectly correct.  For example, white (255,255,255) pixels in the original window are rendered on the canvas as (248, 252, 248).  Solid yellow becomes (248,252,0).

This sounds similar to bug 322647 but that was filed as a Windows bug.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-11 15:53:15 PST
Created attachment 208237 [details] [diff] [review]
patch

Can you test this patch, please?
Comment 2 neil@parkwaycc.co.uk 2006-01-12 06:49:03 PST
Note: from a binary perspective I'd choose the following values:
1111111111111111 = 65535
0101010101010101 = 85 * 257
0010010010010010 = 9362 ~ 65535 / 7
0001000100010001 = 17 * 257
0000100001000010 = 2114 ~ 65535 / 31
0000010000010000 = 1040 ~ 65535 / 63
0000001000000100 = 516 ~ 65535 / 127
0000000100000001 = 257
Comment 3 Mark Smith (:mcs) 2006-01-12 10:42:47 PST
The patch seems to work well on Linux at both 16-bit and 24-bit color depth (no effect on 24-bit).  Tested with my own MOZILLA_1_8_BRANCH build of Firefox.

I am not sure what the best table values are for ComputeScaleFactor(), but the ones in the patch seem OK to me.  There are still some oddities; for example, a 50% gray color which should by (128,128,128) ends up as (131,129,131).  But that kind of thing is probably unavoidable when different numbers of bits are used to represent different color components.

It would be really nice to have this fix land on the branch.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-12 12:05:57 PST
(In reply to comment #3)
> 50% gray color which should by (128,128,128) ends up as (131,129,131).

50% gray is either 15/31 or 16/31, mapping to 123.870967741935 or 132.129032258065 respectively ... so this is just lossiness inherent in the conversion through 5 or 6 bits. It can't be avoided, except by making the offscreen surface for drawWindow always be a 24-bit surface, which we may or may not do later.

I'll request review, we'll land this on trunk, and then we'll think about branch approval. 1.8.1 (Firefox2) branch approval should be easy to get, but I dunno about 1.8.0 approval.
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-23 11:06:05 PST
Comment on attachment 208237 [details] [diff] [review]
patch

I'd love some MMX goodness to optimize this at some point, but I guess this code will go away with 1.9 anyway (hopefully, in any case).

r=vladimir
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-01-24 17:52:39 PST
Comment on attachment 208237 [details] [diff] [review]
patch

sr=shaver.  Note: I did not check the values in the table.  :)
Comment 7 Kathleen Brade 2006-02-02 08:01:29 PST
roc--can you land this on the trunk or can I?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-02-02 20:47:54 PST
please land it. I was going to, but I haven't been online enough lately.
Comment 9 Mark Smith (:mcs) 2006-02-03 14:02:49 PST
Fix committed to the trunk:

mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp
new revision: 1.36; previous revision: 1.35
  Bug 323088 - RGB values slightly off in canvas after drawWindow().
    Use a table indexed by bit depth to provide a better "scale factor" when
    copying pixels from native surfaces to the canvas.
    Patch by roc, r=vladimir, sr=shaver.
Comment 10 Mark Smith (:mcs) 2006-02-03 14:03:45 PST
Requesting permission to land on the branches.
Comment 11 Daniel Veditz [:dveditz] 2006-02-14 15:10:31 PST
Not blocking a security update
Comment 12 Mark Smith (:mcs) 2006-03-01 08:06:41 PST
Reinterating the request for 1.8.1 and 1.8.0.x. approval.  The patch is small and the changes only affect canvas drawWindow().
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-01 14:32:37 PST
Comment on attachment 208237 [details] [diff] [review]
patch

approved for 1.8.1.

I recommend landing it for 1.8.0.3 too. The risk is infinitesimal.
Comment 14 Mark Smith (:mcs) 2006-03-16 06:35:40 PST
Committed to MOZILLA_1_8_BRANCH:
mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp
new revision: 1.22.2.5; previous revision: 1.22.2.4
  Merge in fix from the trunk:
  Bug 323088 - RGB values slightly off in canvas after drawWindow().
    Use a table indexed by bit depth to provide a better "scale factor" when
    copying pixels from native surfaces to the canvas.
    Patch by roc, r=vladimir, sr=shaver, a=roc.
Comment 15 Daniel Veditz [:dveditz] 2006-03-27 12:04:58 PST
Can you explain who in the real world is actually affected by this such that it needs to go into a security/stability release? Minusing for 1.8.0.3 for now since it doesn't appear to fit the release criteria.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-27 12:35:11 PST
All of the "tab preview" extensions, plus other snapshotting extensions or XUL apps, get slightly wrong colors.
Comment 17 Kathleen Brade 2006-03-27 13:22:04 PST
Seeking reconsideration (I hope I'm not being annoying).

I view this as a stability fix for canvas.  We ran into this bug with Page Saver.  I think it is a small but valuable patch which should be taken.  As the canvas feature becomes more widely used, I would expect more users to encounter the problem if it is not fixed.
Comment 18 Daniel Veditz [:dveditz] 2006-04-03 12:27:54 PDT
Comment on attachment 208237 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 19 Mark Smith (:mcs) 2006-04-03 17:40:12 PDT
Thanks for the 1.8.0 approval.  Committed to MOZILLA_1_8_0_BRANCH:
mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp
new revision: 1.22.2.3.2.2; previous revision: 1.22.2.3.2.1
Merge in fix from the trunk and 1.8 branch:
  Bug 323088 - RGB values slightly off in canvas after drawWindow().
    Use a table indexed by bit depth to provide a better "scale factor" when
    copying pixels from native surfaces to the canvas.
    Patch by roc, r=vladimir, sr=shaver, a=dveditz.

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