498826 - canvas putImageData doesn't implement optional arguments

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[:Mook]

https://developer.mozilla.org/index.php?title=En/HTML/Canvas/Pixel_manipulation_with_canvas&revision=11#section_3 documents that there are four optional arguments - dirtyX, dirtyY, dirtyWidth, dirtyHeight - that putImageData can take.  Unfortunately, that's documenting to the spec and not the implementation.

Filing this bug so I have a reference when I go delete the documentation ;)

See http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-putimagedata for the spec.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

wtf, where did those arguments come from?

Comment 2

[inamru]

The agruments (In reply to comment #1)
> wtf, where did those arguments come from?
The arguments are documented here:
https://developer.mozilla.org/index.php?title=En/HTML/Canvas/Pixel_manipulation_with_canvas&revision=11#section_3

Comment 3

[:Mook]

I suspect vlad actually wants http://html5.org/tools/web-apps-tracker?from=1196&to=1197&context=10 instead, which would be 2008-02-03 01:43:50 -0800.

Comment 4

[Saint Wesonga]

Attachment: Patch v1

I manually coded overflow/underflow checks in this patch. Is there a standardized way to do this in the code base that I may have missed, or is this fine?

Comment 5

[Saint Wesonga]

Quick URL - the tests for this bug are at http://philip.html5.org/tests/canvas/suite/tests/index.2d.imageData.put.dirty.html

Comment 6

[Daniël]

Looks like it's still not fully implemented

ctx.putImageData(imagedata, 10, 10, 20, 40, 20, 40);
//works in WebKit and not in Gecko


ctx.putImageData(imagedata, 0, 0, 20, 40, 20, 40);
//works in both.

Comment 7

[Saint Wesonga]

Attachment: WIP

Still need to fix 6 failing canvas mochitests

Comment 8

[Saint Wesonga]

Attachment: Patch

Comment 9

[Silvia Pfeiffer]

Running Minefield on OSX: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre

Is this in trunk already? I've just stumbled across the fact that I cannot use putImageData to tile an image or video frame. I will attach my test case.

Comment 10

[Silvia Pfeiffer]

Attachment: tries to tile a video - works in Safari/Google Chrome

The tiling doesn't work in Firefox, but works in Webkit.
Instead, all I see is the original video at no offset.
I will attach a test video for this test case, too.

Comment 11

[Silvia Pfeiffer]

Attachment: test video for test case

test video for previous tile test

Comment 12

[pino_sb]

(In reply to comment #9)
> Running Minefield on OSX: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6;
> rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre
> 
> Is this in trunk already? I've just stumbled across the fact that I cannot use
> putImageData to tile an image or video frame. I will attach my test case.

No, it isn't. The patch is still waiting to be reviewed, then hopefully the bug can be fixed. I hope this makes it into Firefox 4, since it is an important part of canvas (for my project at least).

Comment 13

[Joe Drew (not getting mail)]

This doesn't block Firefox 4, but I would take this patch. I suggest you ping vlad on irc.mozilla.org channel #gfx to get his attention.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The patch looks fine, but should be rewritten to use the CheckedInt stuff we have now for overflow checking.

Comment 15

[Dmitry Skavish]

This bug is really a show stopper for us here. Do you guys plan on taking this patch anytime soon or you absolutely need it to be rewritten using CheckedInt stuff? Thank you!

Comment 16

[Aku Kotkavuo]

Also a showstopper here. With the recent fast development pace for Firefox 4 I've been hopeful this patch would make it in time.

Comment 17

[Saint Wesonga]

Attachment: Patch

> ... but should be rewritten to use the CheckedInt stuff we have now for overflow checking.

Done!

Comment 18

[Saint Wesonga]

Attachment: Patch

Minor optimization, only one:

if (dirtyRect.Width() <= 0 || dirtyRect.Height() <= 0)

check is needed if no dirty rect was specified.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 492904 [details] [diff] [review]
Patch

looks good, thanks!

Minor cleanup you could do as a followup, if you really wanted to -- change putImageData_explicit to use a nsIntRect for x/y/w/h and dirtyx/dirtyy/etc., and then add a overload that doesn't take the extra dirty intrect which will just pass the imagerect in.  Then compare whether imagerect != dirtyrect to decide whether to do the checks.  But not a big deal.

Comment 20

[Saint Wesonga]

> Minor cleanup you could do as a followup, if you really wanted to -- change

hopefully after semester's done...

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3cfa2011c831

Comment 22

[Eric Shepherd [:sheppy]]

This change has now been documented, btw.