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




Canvas: 2D
9 years ago
8 years ago


(Reporter: Waldo, Assigned: vlad)


({dev-doc-complete, perf})

dev-doc-complete, perf

Firefox Tracking Flags

(Not tracked)




(2 attachments, 1 obsolete attachment)

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:

...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.
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.
Assignee: nobody → vladimir
Created attachment 323967 [details] [diff] [review]
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.
Attachment #323967 - Flags: review?(roc)
Created attachment 323968 [details] [diff] [review]
let's try that again, without the built-in integer overflow

Attachment #323967 - Attachment is obsolete: true
Attachment #323968 - Flags: review?(roc)
Attachment #323967 - Flags: review?(roc)
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 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.
Attachment #323968 - Flags: review?(roc)
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..
Created attachment 333786 [details] [diff] [review]
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 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.
Attachment #323968 - Flags: review?(roc)
Comment on attachment 323968 [details] [diff] [review]
let's try that again, without the built-in integer overflow


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.)
Attachment #323968 - Flags: superreview+
Attachment #323968 - Flags: review?(roc)
Attachment #323968 - Flags: review+
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.
"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.
the bandaid's OK


9 years ago
Keywords: perf
Checked in; filed bug 453355 on doing the real pixel data object in the future.
Last Resolved: 9 years ago
Resolution: --- → FIXED


8 years ago
Keywords: dev-doc-needed
Added this article, which covers createImageData() among other things:
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.