getImageData is supposed to allow out of bounds rects

RESOLVED FIXED in mozilla5

Status

()

Core
Canvas: 2D
P4
normal
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: pete, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla5
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

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

8 years ago
Created attachment 380572 [details] [diff] [review]
getImageData (Out of bounds)

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

8 years ago
Created attachment 380573 [details]
Testcase

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

8 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

8 years ago
Created attachment 381445 [details] [diff] [review]
getImageData (Out of bounds)

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)
(Reporter)

Comment 6

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

8 years ago
Created attachment 385049 [details]
Integer over/underflow testcase

Attaching test to verify that overflow/underflow don't go undetected

Comment 8

8 years ago
Created attachment 385050 [details] [diff] [review]
Patch allowing out of bounds rects

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

8 years ago
Created attachment 385090 [details] [diff] [review]
Patch allowing out of bounds rects

Removed tabs from previous patch
Attachment #385050 - Attachment is obsolete: true
Attachment #385090 - Flags: review?(vladimir)
Attachment #385050 - Flags: review?(dbaron)
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Blocks: 622842
(Assignee)

Comment 11

6 years ago
Created attachment 510314 [details] [diff] [review]
Patch v2

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

6 years ago
Depends on: 629894
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Updated

6 years ago
Attachment #510314 - Flags: review?(vladimir) → review?(bzbarsky)
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

6 years ago
Whiteboard: [needs review] → [needs patch]
(Assignee)

Comment 13

6 years ago
Created attachment 524432 [details] [diff] [review]
Patch for checkin
Attachment #510314 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [needs patch]
This patch does not apply cleanly any more.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d5905c6cfafa
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Keywords: dev-doc-needed
https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.