433004 - Support canvas.getContext("2d").createImageData()

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jeff Walden [:Waldo]]

It'll speed up canvas ops that splash an ImageData on the canvas, because the canvas won't have to typecheck a bunch of JS values.  There are also wins in resolution independence, but I'm not familiar with the arguments over that particular wrinkle.

The spec as of this commit:

http://lists.whatwg.org/htdig.cgi/commit-watchers-whatwg.org/2008/000396.html

...requires throwing a type mismatch error if putImageData is called with an object which wasn't created by createImageData, but that seems a bit harsh at least for now -- we should wait for existing content to catch up to the spec on this.

Comment 1

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

Wha?  I had no idea this was added... throwing the error seems like failure, given that the current spec was working fine.  Blah.  But yes, overall this seems like a good idea (adding a createImageData object); I don't buy the resolution independence argument, though.

Comment 2

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

Attachment: initial impl

Here's an initial implementation that just returns a JS object.  It doesn't do the clamping on pixel assignment as required by the spec.  We should grab this just to have most of source-compat with the spec, and then do a real native ImageData object with clamping and optimized drawing (backed by a gfxImageSurface) as a separate patch.

Comment 3

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

Attachment: let's try that again, without the built-in integer overflow

Ahem.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't get it, we're just going to throw this patch away when we do the real patch, so what's the value of taking this patch?

+  // ImageData createImageData(in float w, in float h);
+  void createImageData();

And even if there is value in just returning a JS object temporarily, why not make this "void createImageData(in float w, in float h)"?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 323968 [details] [diff] [review]
let's try that again, without the built-in integer overflow

See comment #4.

Perhaps Eric would have time to do a real implementation here? It would be great to have the real DOM object here instead of poking the JS engine.

Comment 6

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

Yeah, we talked about it; it's a bit complicated because it requires a scriptable helper and possibly some fast-paths to make performance not be abysmal..

Comment 7

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

Attachment: a patch to implement a DOM object, clamping, the whole bit

Here's a patch that implements the DOM object, clamping, the whole bit.  However, it is horribly, horribly slow -- the per-element access overhead is atrocious.  The only thing that's sped up here is putImageData with an ImageData Object -- my test goes from 259 ms -> 16 ms, so that's a decent speedup.  But pixel manipulation slows down by a factor of 17 or 3, depending on how you're accessing the data, and that doesn't seem like a good tradeoff.

I don't think putImageData was ever the bottleneck here, but instead the image manipulation was.. though people are arguing that there are cases where someone wants to compute once and put multiple times.  For that case, creating a second canvas, putting the data to that canvas, and then using drawImage with that canvas as a source will be fast end more flexible (you can use that canvas as a pattern, can transform it, etc.).

It's been suggested to me that creating a custom JS object would be a way to get back performance, but I don't think I can do this in time for fx 3.1 -- so I suggest we go with my original patch and try to get to this later.  The only thing we lose out on is clamping on assignment as opposed to at put time, which seems quite minor.

Comment 8

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

Comment on attachment 323968 [details] [diff] [review]
let's try that again, without the built-in integer overflow

Let's just go with this for 3.1, to get a createImageData function that does mostly the right thing for compat.  See earlier comment about why we shouldn't do the real fix for now.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 323968 [details] [diff] [review]
let's try that again, without the built-in integer overflow

Interesting.

You probably should send feedback to the list pointing out that, even if you have very fast transitions into and out of the DOM, JS array access might still be faster since the JS runtime gets complete control over data representation and compiler optimizations. Furthermore, having createImageData do the clamping isn't so bad since that can be very easily parallelized, whereas JS execution cannot --- so JS execution is likely to be the bottleneck, not createImageData. (Plus your point about using a temporary canvas if we're only changing a few pixels of the image data, although that might be a bit hard to implement in some cases.)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I thought about comment #9 some more and I'm not sure it's true after all.

If you went all-out and inlined the DOM ImageData array getter/setter into the JS code, the clamping would be basically free and you'd be accessing less memory so you'd probably still be winning with the DOM object even with unlimited parallelism available.

So basically we should go ahead with this and later enable tracing through the ImageData getter/setter for great justice.

Comment 11

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

"go ahead with this" -- the JS bandaid, or the full DOM thing?  I think the full DOM thing is definitely the way to go, I'm just not sure if we can get it performant for 1.9.1.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

the bandaid's OK

Comment 13

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

Checked in; filed bug 453355 on doing the real pixel data object in the future.

Comment 14

[Eric Shepherd [:sheppy]]

Added this article, which covers createImageData() among other things:

https://developer.mozilla.org/En/HTML/Canvas/Pixel_manipulation_with_canvas