392751 - getImageData is supposed to allow out of bounds rects

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P4)

Description

[pete]

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

[Joe Drew (not getting mail)]

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.

Comment 2

[Saint Wesonga]

Attachment: 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

Comment 3

[Saint Wesonga]

Attachment: 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

[Saint Wesonga]

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)

Comment 5

[Saint Wesonga]

Attachment: getImageData (Out of bounds)

I confirmed with Philip Taylor that the above test that fails is actually incorrect, so I'm resubmitting the patch

Comment 6

[pete]

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

[Saint Wesonga]

Attachment: Integer over/underflow testcase

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

Comment 8

[Saint Wesonga]

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

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Saint Wesonga]

Attachment: Patch allowing out of bounds rects

Removed tabs from previous patch

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

This can do a lot less work. I hope you don't mind me taking over.

Comment 12

[Boris Zbarsky [: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

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 14

[(no longer active)]

This patch does not apply cleanly any more.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d5905c6cfafa

Comment 16

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes