959958 - Implement ImageData constructor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

See bug 959925 for the use case. .createImageData is unavailable from workers.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This isn't in the spec, though.

Comment 2

[Masatoshi Kimura [:emk]]

Actually it is.
http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#hitregionoptions
> [Constructor (Uint8ClampedArray data, unsigned long width, unsigned long height)]   
> interface ImageData {
>   readonly attribute unsigned long width;
>   readonly attribute unsigned long height;
>   readonly attribute Uint8ClampedArray data;
> };

Comment 3

[Masatoshi Kimura [:emk]]

Oh, yet another discrepancy between WHATWG and W3C :(
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#hitregionoptions

Comment 4

[Masatoshi Kimura [:emk]]

OTOH, WHATWG's ImageData has resolution but W3C's one doesn't.
I don't understand why W3C bugzilla 21045 and 21046 were closed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think we should do this, fwiw.

Comment 6

[(no longer active)]

(In reply to comment #5)
> I think we should do this, fwiw.

Yeah, and fix the whatwg spec to add a ctor there too!

Comment 7

[Masatoshi Kimura [:emk]]

The problem is, the WHATWG spec has an attribute what the W3C spec does not.
How the constructor should be?
[Constructor (Uint8ClampedArray data, unsigned long width, unsigned long height, optional double resolution)]?
[Constructor (ImageDataInit initDict)]?
Or whatever else?

Comment 8

[Hixie (not reading bugmail)]

resolution's going away; the constructor as specced on the w3c side is probably ok.

Comment 9

[Hixie (not reading bugmail)]

Actually the W3C one's arguments don't make much sense... I'll spec something on the WHATWG side.

Comment 10

[Hixie (not reading bugmail)]

Done. There's two constructors:

   new ImageData(w, h); // returns a blank ImageData of the given dimensions

   new ImageData(data, w[, h]); // returns a new ImageData that references the given data, using the given width

See: http://www.whatwg.org/html#dom-imagedata

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Implement ImageData constructor

https://tbpl.mozilla.org/?tree=Try&rev=953f0204d579

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Implement ImageData constructor

https://tbpl.mozilla.org/?tree=Try&rev=e696d5f383d3

Comment 13

[Masatoshi Kimura [:emk]]

ping?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8362161 [details] [diff] [review]
Implement ImageData constructor

>+  if (aWidth <= 0 || aHeight <= 0) {

They're unsigned, so just == 0, please.

>+  if (length <= 0 || length & 3) {

Likewise.  Also, "% 4" would be clearer than "& 3" here, I think.

>+  length >>= 2;

length /= 4, please.  The compiler will strength-reduce as needed, and the code is clearer.

>+  if (aWidth <= 0) {

Again, just ==.  Also, please file a spec issue; the spec doesn't do this check, but should.

r=me with that

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> >+  if (aWidth <= 0) {
> 
> Again, just ==.  Also, please file a spec issue; the spec doesn't do this
> check, but should.

It is covered by the text "If length is not an integral multiple of sw," in the spec. If sw is zero, length will never be an integral multiple of sw. Our explicit zero check is just an implementation detail.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a801fc05819

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It is covered by the text "If length is not an integral multiple of sw," in the spec.

I think that assumes an unwarranted level of mathematical sophistication on the part of spec readers.  The spec should just have an explicit check for 0 here before that test.

Comment 17

[Masatoshi Kimura [:emk]]

Hm, indeed I forgot the check until the test crashed despite that programmers must be careful about the zero division. Filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=24429>.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a801fc05819

Comment 19

[Rik Cabanier]

This API was implemented incorrectly.
The spec says that imagedata should keep a reference to the data but it seems to create a copy.

Comment 20

[Rik Cabanier]

(In reply to Hixie (not reading bugmail) from comment #9)
> Actually the W3C one's arguments don't make much sense... I'll spec
> something on the WHATWG side.

The W3C level 2 version should just be a copy of the WhatWG. Not sure what was there before...

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Rik Cabanier from comment #19)
> This API was implemented incorrectly.
> The spec says that imagedata should keep a reference to the data but it
> seems to create a copy.

Our implementation doesn't create a copy.
 < var a=new Uint8ClampedArray([1,2,3,4]);var i=new ImageData(a,1);i.data.set([5,6,7,8]);a;
 > Uint8ClampedArray [ 5, 6, 7, 8 ]
I even added an automated test to verify that. Why do you think it creates a copy?

(In reply to Rik Cabanier from comment #20)
> (In reply to Hixie (not reading bugmail) from comment #9)
> > Actually the W3C one's arguments don't make much sense... I'll spec
> > something on the WHATWG side.
> 
> The W3C level 2 version should just be a copy of the WhatWG. Not sure what
> was there before...

Because I referred a wrong version on the W3C site.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24298#c6

Comment 22

[Boris Zbarsky [:bzbarsky]]

I agree with Masatoshi Kimura.  The code (which is _really_ simple), and basic tests, all indicate the constructor takes a reference.  Why do you think it creates a copy?

Comment 23

[Rik Cabanier]

I looked at the wrong constructor.
Sorry about that!

Comment 24

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/ImageData
https://developer.mozilla.org/en-US/docs/Web/API/ImageData.ImageData
https://developer.mozilla.org/en-US/Firefox/Releases/29