canvas putImageData doesn't implement optional arguments

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Canvas: 2D
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Mook, Assigned: Saint Wesonga)

Tracking

Trunk
mozilla2.0b8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 2

8 years ago
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
(Reporter)

Comment 3

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

Updated

8 years ago
Assignee: nobody → wesongathedeveloper
(Assignee)

Comment 4

8 years ago
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?
Attachment #400377 - Flags: review?(vladimir)
(Assignee)

Comment 5

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

8 years ago
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.
(Assignee)

Comment 7

7 years ago
Created attachment 451995 [details] [diff] [review]
WIP

Still need to fix 6 failing canvas mochitests
Attachment #400377 - Attachment is obsolete: true
Attachment #400377 - Flags: review?(vladimir)
(Assignee)

Comment 8

7 years ago
Created attachment 452046 [details] [diff] [review]
Patch
Attachment #451995 - Attachment is obsolete: true
Attachment #452046 - Flags: review?(vladimir)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED

Comment 9

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

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

7 years ago
Created attachment 466068 [details]
test video for test case

test video for previous tile test

Comment 12

7 years ago
(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.

Comment 15

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

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

Comment 17

7 years ago
Created attachment 492891 [details] [diff] [review]
Patch

> ... 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)
(Assignee)

Comment 18

7 years ago
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.
Attachment #492891 - Attachment is obsolete: true
Attachment #492891 - Flags: review?(vladimir)
Attachment #492904 - 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+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

7 years ago
> 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
Last Resolved: 7 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.