Last Comment Bug 498826 - canvas putImageData doesn't implement optional arguments
: canvas putImageData doesn't implement optional arguments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla2.0b8
Assigned To: Saint Wesonga
:
: Milan Sreckovic [:milan]
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-16 22:56 PDT by :Mook
Modified: 2011-11-03 09:26 PDT (History)
19 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch v1 (7.69 KB, patch)
2009-09-13 12:19 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
WIP (7.18 KB, patch)
2010-06-17 10:43 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Patch (9.49 KB, patch)
2010-06-17 12:38 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
tries to tile a video - works in Safari/Google Chrome (1.39 KB, text/html)
2010-08-14 20:04 PDT, Silvia Pfeiffer
no flags Details
test video for test case (1.30 MB, video/ogg)
2010-08-14 20:05 PDT, Silvia Pfeiffer
no flags Details
Patch (9.57 KB, patch)
2010-11-23 19:12 PST, Saint Wesonga
no flags Details | Diff | Splinter Review
Patch (6.44 KB, patch)
2010-11-23 19:49 PST, Saint Wesonga
vladimir: review+
vladimir: approval2.0+
Details | Diff | Splinter Review

Description :Mook 2009-06-16 22:56:40 PDT
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] 2009-06-17 00:37:21 PDT
wtf, where did those arguments come from?
Comment 2 inamru 2009-06-17 02:58:38 PDT
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 2009-06-17 09:07:31 PDT
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 2009-09-13 12:19:39 PDT
Created attachment 400377 [details] [diff] [review]
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 2009-09-13 12:25:48 PDT
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 2010-02-12 07:35:55 PST
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 2010-06-17 10:43:46 PDT
Created attachment 451995 [details] [diff] [review]
WIP

Still need to fix 6 failing canvas mochitests
Comment 8 Saint Wesonga 2010-06-17 12:38:19 PDT
Created attachment 452046 [details] [diff] [review]
Patch
Comment 9 Silvia Pfeiffer 2010-08-14 20:02:47 PDT
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 2010-08-14 20:04:34 PDT
Created attachment 466067 [details]
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 2010-08-14 20:05:42 PDT
Created attachment 466068 [details]
test video for test case

test video for previous tile test
Comment 12 pino_sb 2010-08-17 05:31:29 PDT
(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) 2010-08-18 09:33:56 PDT
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] 2010-08-19 14:48:18 PDT
The patch looks fine, but should be rewritten to use the CheckedInt stuff we have now for overflow checking.
Comment 15 Dmitry Skavish 2010-10-27 20:35:38 PDT
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 2010-11-01 05:00:50 PDT
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 2010-11-23 19:12:11 PST
Created attachment 492891 [details] [diff] [review]
Patch

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

Done!
Comment 18 Saint Wesonga 2010-11-23 19:49:56 PST
Created attachment 492904 [details] [diff] [review]
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] 2010-11-29 13:28:59 PST
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 2010-11-29 22:31:30 PST
> 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] 2010-12-04 07:45:45 PST
http://hg.mozilla.org/mozilla-central/rev/3cfa2011c831
Comment 22 Eric Shepherd [:sheppy] 2011-11-03 09:26:24 PDT
This change has now been documented, btw.

Note You need to log in before you can comment on or make changes to this bug.