RGB values slightly off in canvas after drawWindow()

RESOLVED FIXED

Status

()

Core
Canvas: 2D
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Kathleen Brade, Assigned: roc)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
x86
Linux
fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 -
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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.
Created attachment 208237 [details] [diff] [review]
patch

Can you test this patch, please?

Comment 2

12 years ago
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

12 years ago
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.
(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.
Attachment #208237 - Flags: superreview?(shaver)
Attachment #208237 - Flags: review?(vladimir)
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
Attachment #208237 - Flags: review?(vladimir) → review+
Comment on attachment 208237 [details] [diff] [review]
patch

sr=shaver.  Note: I did not check the values in the table.  :)
Attachment #208237 - Flags: superreview?(shaver) → superreview+
(Reporter)

Comment 7

12 years ago
roc--can you land this on the trunk or can I?
please land it. I was going to, but I haven't been online enough lately.

Comment 9

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 10

12 years ago
Requesting permission to land on the branches.
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Not blocking a security update
Flags: blocking1.8.0.2? → blocking1.8.0.2-

Comment 12

12 years ago
Reinterating the request for 1.8.1 and 1.8.0.x. approval.  The patch is small and the changes only affect canvas drawWindow().
Flags: blocking1.8.0.3?
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.
Attachment #208237 - Flags: approval1.8.0.3?
Attachment #208237 - Flags: approval-branch-1.8.1+

Comment 14

12 years ago
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.
Whiteboard: fixed1.8.1
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.
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3-
Keywords: fixed1.8.1
Whiteboard: fixed1.8.1
All of the "tab preview" extensions, plus other snapshotting extensions or XUL apps, get slightly wrong colors.
(Reporter)

Comment 17

12 years ago
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.
Flags: blocking1.8.0.3- → blocking1.8.0.3?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 208237 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #208237 - Flags: approval1.8.0.3? → approval1.8.0.3+

Comment 19

12 years ago
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.
Keywords: fixed1.8.0.3
You need to log in before you can comment on or make changes to this bug.