Closed
Bug 392751
Opened 17 years ago
Closed 14 years ago
getImageData is supposed to allow out of bounds rects
Categories
(Core :: Graphics: Canvas2D, defect, P4)
Core
Graphics: Canvas2D
Tracking
()
RESOLVED
FIXED
mozilla5
People
(Reporter: pete_a90, Assigned: Ms2ger)
References
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007072104 Minefield/3.0a7pre
Build Identifier:
http://www.whatwg.org/specs/web-apps/current-work/multipage/section-the-canvas.html#getimagedata
3.14.11.1.10. Pixel manipulation
"Pixels outside the canvas must be returned as transparent black"
I'm guessing that putImageData should do something similar but the spec is silent.
Reproducible: Always
Comment 1•17 years ago
|
||
This is definitely what the standard says, and we're definitely not returning that in that case (at least from reading the code), but I don't know how big an issue this is.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P4
Comment 2•16 years ago
|
||
This patch changes the returned error when width=0 or height=0 from SYNTAX_ERR to INDEX_SIZE_ERR (as per spec) and allows for negative dimensions and out of bound rects
Attachment #380572 -
Flags: review?(vladimir)
Comment 3•16 years ago
|
||
No exceptions should be thrown (current builds throw SYNTAX_ERR) and the testcase should only display success messages (Note: this testcase is a minor modification of the test_2d.imageData.get.source.outside.html test allowing it to run without the mochitest framework)
Comment 4•16 years ago
|
||
Comment on attachment 380572 [details] [diff] [review]
getImageData (Out of bounds)
The patch causes the rendering to pass the test at http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.get.source.outside.html but fail the one at http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.get.source.negative.html.
My initial interpretation of the spec requirements was that the 4 corners determined by the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx, sy+sh) form a rectangle for which pixels should be returned from the top left to the bottom right - since the data is being returned in a CanvasPixelArray object (which the spec specifically requires to order the data in left-to-right, top-to-bottom format).
However, the test at http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.get.source.negative.html seems to have a different interpretation, instead expecting the bottom right pixel to appear first in the array (where bottom right in that specific test would be the point 85,25). Did I miss something very obvious about all this in the spec?
I had filed this issue at https://bugzilla.mozilla.org/show_bug.cgi?id=494744, and that bug also pointed out a simple error in the test (should be imgdata1.data.length, not imgdata1.length)
Attachment #380572 -
Flags: review?(vladimir)
Comment 5•16 years ago
|
||
I confirmed with Philip Taylor that the above test that fails is actually incorrect, so I'm resubmitting the patch
Attachment #380572 -
Attachment is obsolete: true
Attachment #381445 -
Flags: review?(vladimir)
Some things that stand out to me (I haven't ran your code, these are just red flags that are jarring to me):
What if w or h are INT_MIN in the negative dimensions fixups?
What happens if x or y underflows in the negative dimension fixups?
What if x+w or y+h overflows in the fill temp surface calculations?
What if x or y are INT_MIN in the -(int)x, -(int)y calculations?
I don't even know why the spec allows negative sizes anyway. Zero sized lengths make sense to be but are disallowed. Negative lengths make no sense to me but are allowed. Go figure.
I'm hoping there is some way to change the standard to disallow negative sizes (and allow 0 sizes).
Comment 7•16 years ago
|
||
Attaching test to verify that overflow/underflow don't go undetected
Comment 8•16 years ago
|
||
I'm not sure whether allowing -ve dimensions does any harm, but then again, this may not be the right avenue for that discussion.
Most of the additional code in this patch will simply be skipped in the majority of cases (where +ve dimensions are supplied)
Attachment #381445 -
Attachment is obsolete: true
Attachment #385050 -
Flags: review?(dbaron)
Attachment #381445 -
Flags: review?(vladimir)
Comment 9•16 years ago
|
||
You should request review from vlad rather than me. (But I can say right now that you shouldn't have tabs in the code; you need to indent using spaces.)
Comment 10•16 years ago
|
||
Removed tabs from previous patch
Attachment #385050 -
Attachment is obsolete: true
Attachment #385090 -
Flags: review?(vladimir)
Attachment #385050 -
Flags: review?(dbaron)
Updated•15 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 11•14 years ago
|
||
This can do a lot less work. I hope you don't mind me taking over.
Assignee: nobody → Ms2ger
Attachment #385090 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #510314 -
Flags: review?(vladimir)
Attachment #385090 -
Flags: review?(vladimir)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Assignee | ||
Updated•14 years ago
|
Attachment #510314 -
Flags: review?(vladimir) → review?(bzbarsky)
Comment 12•14 years ago
|
||
Comment on attachment 510314 [details] [diff] [review]
Patch v2
> + if (!srcRect.Contains(destRect)) {
Add a comment that this is trying to optimize out the Rectangle() call in the common case, ok?
r=me
Attachment #510314 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review] → [needs patch]
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #510314 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [needs patch]
Comment 15•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 16•14 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•