Closed Bug 498826 Opened 11 years ago Closed 10 years ago

canvas putImageData doesn't implement optional arguments

Categories

(Core :: Canvas: 2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: Mook, Assigned: wesongathedeveloper)

References

()

Details

Attachments

(3 files, 4 obsolete files)

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.
wtf, where did those arguments come from?
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
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.
Assignee: nobody → wesongathedeveloper
Attached patch Patch v1 (obsolete) — Splinter Review
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?
Attachment #400377 - Flags: review?(vladimir)
Quick URL - the tests for this bug are at http://philip.html5.org/tests/canvas/suite/tests/index.2d.imageData.put.dirty.html
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.
Attached patch WIP (obsolete) — Splinter Review
Still need to fix 6 failing canvas mochitests
Attachment #400377 - Attachment is obsolete: true
Attachment #400377 - Flags: review?(vladimir)
Attached patch Patch (obsolete) — Splinter Review
Attachment #451995 - Attachment is obsolete: true
Attachment #452046 - Flags: review?(vladimir)
Status: NEW → ASSIGNED
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.
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.
test video for previous tile test
(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).
blocking2.0: --- → ?
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.
blocking2.0: ? → -
The patch looks fine, but should be rewritten to use the CheckedInt stuff we have now for overflow checking.
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!
Also a showstopper here. With the recent fast development pace for Firefox 4 I've been hopeful this patch would make it in time.
Attached patch Patch (obsolete) — Splinter Review
> ... but should be rewritten to use the CheckedInt stuff we have now for overflow checking.

Done!
Attachment #452046 - Attachment is obsolete: true
Attachment #492891 - Flags: review?(vladimir)
Attachment #452046 - Flags: review?(vladimir)
Attached patch PatchSplinter Review
Minor optimization, only one:

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

check is needed if no dirty rect was specified.
Attachment #492891 - Attachment is obsolete: true
Attachment #492904 - Flags: review?(vladimir)
Attachment #492891 - Flags: review?(vladimir)
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.
Attachment #492904 - Flags: review?(vladimir)
Attachment #492904 - Flags: review+
Attachment #492904 - Flags: approval2.0+
Keywords: checkin-needed
> Minor cleanup you could do as a followup, if you really wanted to -- change

hopefully after semester's done...
http://hg.mozilla.org/mozilla-central/rev/3cfa2011c831
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
This change has now been documented, btw.
You need to log in before you can comment on or make changes to this bug.