323088 - RGB values slightly off in canvas after drawWindow()

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Kathleen :Brade]

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) (email my personal email if necessary)]

Attachment: patch

Can you test this patch, please?

Comment 2

[[e-mail]]

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]]

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) (email my personal email if necessary)]

(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] (needinfo me, slow to respond)]

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)]

Comment on attachment 208237 [details] [diff] [review]
patch

sr=shaver.  Note: I did not check the values in the table.  :)

Comment 7

[Kathleen :Brade]

roc--can you land this on the trunk or can I?

Comment 8

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

please land it. I was going to, but I haven't been online enough lately.

Comment 9

[Mark Smith [:mcs]]

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]]

Requesting permission to land on the branches.

Comment 11

[Daniel Veditz [:dveditz]]

Not blocking a security update

Comment 12

[Mark Smith [:mcs]]

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) (email my personal email if necessary)]

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]]

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]]

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) (email my personal email if necessary)]

All of the "tab preview" extensions, plus other snapshotting extensions or XUL apps, get slightly wrong colors.

Comment 17

[Kathleen :Brade]

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]]

Comment on attachment 208237 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 19

[Mark Smith [:mcs]]

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.