1141979 - [FoxEye] Extend ImageBitmap with interfaces to access its underlying image data

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

In the FoxEye framework (Bug 1100203), ImageBitmap (Bug 1044102) is used as a container to pass image data to developers (in JavaScript code). However, ImageBitmap is originally designed as a opaque handler which does not match our FoxEye's requirement. Open a bug for discussion.

Comment 1

[Boris Zbarsky [:bzbarsky]]

If you want a non-opaque container, should't you be using ImageData instead of ImageBitmap?  ImageBitmap is designed opaque for a reason (e.g. in the case of an ImageBitmap the image bits might not even exist in a form the browser can access without doing a GPU readback).

Comment 2

[Tzuhao Kuo [:kaku]]

(In reply to Not doing reviews right now from comment #1)
> If you want a non-opaque container, should't you be using ImageData instead
> of ImageBitmap?  ImageBitmap is designed opaque for a reason (e.g. in the
> case of an ImageBitmap the image bits might not even exist in a form the
> browser can access without doing a GPU readback).

That is the point and also the trade-off. One of the project FoxEye's goals is to streamline the video processing framework on the web platform. It provides web applications a way to hook a script to a video track (of a MediaStream) so that each frame will be processed accordingly. However, the decoded raw data of an video frame might exists in either CPU or GPU memory and that's the reason ImageBitmap is suitable for passing such data. 

On the other hand, we need to consider how would developers process the frames. The user script might processes the frames via JavaScript(/asm.js) or WebGL. If it uses WebGL then passing the frames via ImageBitmp is reasonable. But if it uses JavaScript(/asm.js) to process the frames, then we need a way to access the raw data via an ImageBitmp and this is the scenario which I would like to enable in this bug.

Does it make sense?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sure.  I assume drawing the ImageBitmap to a 2d canvas and then using getImageData feels like too roundabout a way of doing things?

Comment 4

[Tzuhao Kuo [:kaku]]

(In reply to Not doing reviews right now from comment #3)
> Sure.  I assume drawing the ImageBitmap to a 2d canvas and then using
> getImageData feels like too roundabout a way of doing things?

Yes that is what FoxEye would like to avoid. BTW, the conclusion in comment 2 was came out wit roc and Jeff Muizelaar. Also, for HtmlImageElement case, Jeff has filed a bug 870026 to avoid the redundant go-through-canvas step.

Comment 5

[Tzuhao Kuo [:kaku]]

Hi Rob, 

In the last correspondence email, you wrote some rough APIs, I quote them here:

> So as well as getting access to the image data efficiently (ideally so that asm.js code can read it), we also need some format negotiation so native code can operate on the data without having to do format conversion.
> 
> One way to do that would be to have a WebIDL enum ImageFormat defining a list of known formats, and APIs on ImageBitmap like
> // Find the best format for receiving data. Returns one of the possibleFormats or the empty string if no format in the list is supported.
> ImageFormat findOptimalFormat(ImageFormat[] possibleFormats);
> // Return the length of the mapped data in format 'format'. Throws if 'format' is not supported.
> long mappedDataLength(ImageFormat format);
> // Logically, makes a copy of the underlying image data in the given format to 'array' at offset 'offset', filling at most 'length' bytes. If 'offset' and 'length' are page-aligned, we can able to implement this with an mmap operation to avoid copying the data (plus an mprotect handler to provide copy-on-write semantics so it works like a normal array) ... depends on the underlying ImageBitmap though, since the data may need to be copied from GPU memory anyway.
> void mapDataInto(ImageFormat format, TypedArray array, long offset, long length);
> 
> I need to talk to the JS team about such an mmap API. That sort of API is already needed for other reasons, though.

Could you explain the idea of the first API, findOptimalFormat(), in more detail? I am somewhat confused with it. Is it platform-wise or per-ImageBitmap-wise? Should the optimal/best format be the one the called ImageBitmap is formatted? If it is the case, why not provide "getImageFormat()" instead?



Except the above question, I would like to discuss some implementation details. In the current Gecko code base, video data from both video callback and getUserMedia are presented as a kind of layers::Image. To my understanding, most of them are PlanarYCbCrImage (and its subclasses GrallocImage, GonkCameraImage) but still other formats are possible. On the other hand, in ImageBitmap implementation (Bug 1044102, although not granted yet), we use a gfx::SourceSurface to represent the underlying data buffer. Currently, we use nsLayoutUtils::SurfaceFromElements() to get a gfx::SourceSurface from HtmlImage/Video/CanvasElements. In the video case, this utility function calls layers::Image::GetAsSourceSurface() which converts the original video data into RGBA format and copies into a newly created gfx::SourceSurface object. 

So, in order to get rid of unnecessary color format conversion, I would propose to have the ImageBitmap stores data in the original color format into a gfx::SourceSurfaceAlignedRawData. If users ask for other kind of color format, we do the conversion for them while mapping data out. If the ImageBimtap is going to be drawn into Canvas or WebGL, then we prepare the platform-optimized SourceSurface for the drawing functions. However, here is the trade-off between generalized-for-processing and optimized-for-platform, what do you think?

One step further, considering how will users process an image/frame, I think what matters to image processing algorithms is the "color format" (RGB, YUV, Gray) instead of the permutation of pixels. So, we should expose all kinds of YUV family (includes YUV4XX, YCbCr, NV12, NV21...) to YUV444 and all kinds of RGB family (includes RGBA, BGRA, RGB565...) into RGBA8888. Do you think it is reasonable for the API design?

Comment 6

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

(In reply to Tzuhao Kuo [:kaku] from comment #5)
> Could you explain the idea of the first API, findOptimalFormat(), in more
> detail? I am somewhat confused with it. Is it platform-wise or
> per-ImageBitmap-wise? Should the optimal/best format be the one the called
> ImageBitmap is formatted? If it is the case, why not provide
> "getImageFormat()" instead?

I assume that there is some fixed set of formats that your image-processing code can handle. And, I assume that for any given ImageBitmap the browser can convert it from its internal format to at least one standard format, but often more than one. I also assume that in some cases (e.g. GPU readback) we can convert it from one format to another cheaply when we get the data.

Then, getImageFormat() is not good enough for at least two reasons:
1) The native format might not be a standard format, in which case it's not clear what to return. You could return a format F that's close to the native format, but then you might have to convert from the native format to F and then to another format.
2) If the application wants to use format F, and we can cheaply convert the image to F when we read back the data, we should return F as the image format.

> Except the above question, I would like to discuss some implementation
> details. In the current Gecko code base, video data from both video callback
> and getUserMedia are presented as a kind of layers::Image. To my
> understanding, most of them are PlanarYCbCrImage (and its subclasses
> GrallocImage, GonkCameraImage) but still other formats are possible. On the
> other hand, in ImageBitmap implementation (Bug 1044102, although not granted
> yet), we use a gfx::SourceSurface to represent the underlying data buffer.
> Currently, we use nsLayoutUtils::SurfaceFromElements() to get a
> gfx::SourceSurface from HtmlImage/Video/CanvasElements. In the video case,
> this utility function calls layers::Image::GetAsSourceSurface() which
> converts the original video data into RGBA format and copies into a newly
> created gfx::SourceSurface object.
>
> So, in order to get rid of unnecessary color format conversion, I would
> propose to have the ImageBitmap stores data in the original color format
> into a gfx::SourceSurfaceAlignedRawData. If users ask for other kind of
> color format, we do the conversion for them while mapping data out. If the
> ImageBimtap is going to be drawn into Canvas or WebGL, then we prepare the
> platform-optimized SourceSurface for the drawing functions. However, here is
> the trade-off between generalized-for-processing and optimized-for-platform,
> what do you think?

I think an ImageBitmap should contain either a layers::Image or a gfx::SourceSurface. We should convert it from one to the other lazily as required.

> One step further, considering how will users process an image/frame, I think
> what matters to image processing algorithms is the "color format" (RGB, YUV,
> Gray) instead of the permutation of pixels. So, we should expose all kinds
> of YUV family (includes YUV4XX, YCbCr, NV12, NV21...) to YUV444 and all
> kinds of RGB family (includes RGBA, BGRA, RGB565...) into RGBA8888. Do you
> think it is reasonable for the API design?

I would recommend something like what we have for PlanarYCbCr. So, each ImageFormat is parameterized by a fixed set of parameters that let you describe pixel layouts. I don't know if it makes sense to have different ImageFormats for RGB565 vs RGBA32 ... in theory those can be described by a single format with parameters that specify a mask and shift for each color channel, but that may not easily be used by an existing RGBA-processing implementation.

Possibly ImageFormat could contain both some simple very specific formats and some very generic formats with many parameters.

Comment 7

[Tzuhao Kuo [:kaku]]

Agree with the findOptimalFormat() idea!

> I think an ImageBitmap should contain either a layers::Image or a
> gfx::SourceSurface. We should convert it from one to the other lazily as
> required.
layers::Image might keeps a buffer form video memory and we cannot assume how users use ImageBimap, 
so let ImageBitmap keeps a layers::Image might drain the video buffer.

> I would recommend something like what we have for PlanarYCbCr. So, each
> ImageFormat is parameterized by a fixed set of parameters that let you
> describe pixel layouts. I don't know if it makes sense to have different
> ImageFormats for RGB565 vs RGBA32 ... in theory those can be described by a
> single format with parameters that specify a mask and shift for each color
> channel, but that may not easily be used by an existing RGBA-processing
> implementation.
That is exactly what I concern. I think application developers (especially, image processing applications) are not interested in handling different pixel layouts of single color space; the variety of pixel layouts should be handled inside the browser. This API should provides data with the most generic layout for one single color space. So, the WebIDL enum ImageFormat would be something likes this:

enum MozImageFormat {
  "RGBA32",
  "GRAY8",
  "YUV444P",
  // and other standard color spaces: ex: "HSV", "Lab",
  ""
};

I pick the most generic layouts with one rule: one data presentation for each channel in a pixel.

PlanarYCbCrData is good for describing different pixel layouts of the YUV color space. A similar one for RGB space would be helpful. This kind of helper structures could be used internally to keep the variety of pixel layouts. When users call the _mapDataInto()_ API, we do the pixel re-layout or even color space conversion for users.

Let me append a draft patch for concrete discussion.

Comment 8

[Tzuhao Kuo [:kaku]]

Attachment: bug1044102_imagebitmap_implementation.patch

Patches of this bug depends on the ImageBitmap implementation.

Comment 9

[Tzuhao Kuo [:kaku]]

Attachment: A-draft-patch.patch

This is the first draft patch which might helps discussion. I am wondering if the idea of hiding pixel layouts inside browser makes sense?

Comment 10

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

(In reply to Tzuhao Kuo [:kaku] from comment #7)
> > I think an ImageBitmap should contain either a layers::Image or a
> > gfx::SourceSurface. We should convert it from one to the other lazily as
> > required.
> layers::Image might keeps a buffer form video memory and we cannot assume
> how users use ImageBimap, 
> so let ImageBitmap keeps a layers::Image might drain the video buffer.

Yes, but I think that's an acceptable tradeoff.

If it turns out to be a problem then under some conditions we could automatically convert from Image to SourceSurface after a short amount of time has elapsed.

> > I would recommend something like what we have for PlanarYCbCr. So, each
> > ImageFormat is parameterized by a fixed set of parameters that let you
> > describe pixel layouts. I don't know if it makes sense to have different
> > ImageFormats for RGB565 vs RGBA32 ... in theory those can be described by a
> > single format with parameters that specify a mask and shift for each color
> > channel, but that may not easily be used by an existing RGBA-processing
> > implementation.
> That is exactly what I concern. I think application developers (especially,
> image processing applications) are not interested in handling different
> pixel layouts of single color space; the variety of pixel layouts should be
> handled inside the browser. This API should provides data with the most
> generic layout for one single color space. So, the WebIDL enum ImageFormat
> would be something likes this:
> 
> enum MozImageFormat {
>   "RGBA32",
>   "GRAY8",
>   "YUV444P",
>   // and other standard color spaces: ex: "HSV", "Lab",
>   ""
> };
> 
> I pick the most generic layouts with one rule: one data presentation for
> each channel in a pixel.
> 
> PlanarYCbCrData is good for describing different pixel layouts of the YUV
> color space. A similar one for RGB space would be helpful. This kind of
> helper structures could be used internally to keep the variety of pixel
> layouts. When users call the _mapDataInto()_ API, we do the pixel re-layout
> or even color space conversion for users.
> 
> Let me append a draft patch for concrete discussion.

I read the draft patch but I still don't understand.

Are you saying that the browser should convert camera input (NV21? NV12?) to some "standard" YUV format before we run image processing code on it? Because I thought that was unacceptable for performance.

Comment 11

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> (In reply to Tzuhao Kuo [:kaku] from comment #7)
> > > I think an ImageBitmap should contain either a layers::Image or a
> > > gfx::SourceSurface. We should convert it from one to the other lazily as
> > > required.
> > layers::Image might keeps a buffer form video memory and we cannot assume
> > how users use ImageBimap, 
> > so let ImageBitmap keeps a layers::Image might drain the video buffer.
> 
> Yes, but I think that's an acceptable tradeoff.
> 
> If it turns out to be a problem then under some conditions we could
> automatically convert from Image to SourceSurface after a short amount of
> time has elapsed.

To be clear: I'd like to see the patches in bug 1044102 land ASAP as-is, but I'd like to then extend ImageBitmap to support containing an Image. Then cases like HTMLVideoElement -> ImageBitmap -> WebGLContext can be handled without requiring read-back.

Comment 12

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I read the draft patch but I still don't understand.
> 
> Are you saying that the browser should convert camera input (NV21? NV12?) to
> some "standard" YUV format before we run image processing code on it?
> Because I thought that was unacceptable for performance.

Yes, that is what I thought. However, I think it is not really necessary now since that the 3rd API, mapDataInto(), provides a way for users to extract data with a given format, which could be the "standard" one. Also, there might be some scenarios that users could work with NV12(or NV21) without conversion if they only care about the Y-channel (the intensity).

Comment 13

[Tzuhao Kuo [:kaku]]

Attachment: WebIDL-draft.patch

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> I would recommend something like what we have for PlanarYCbCr. So, each
> ImageFormat is parameterized by a fixed set of parameters that let you
> describe pixel layouts. 

For exposing the variety of pixel layouts, we need a new WebIDL interface ImageFormatPixelLayout, a extended version of PlanarYCbCrData in Gecko, and make the 3rd API returns an ImageFormatPixelLayout object to describe the mapped data. 

Here is the draft WebIDL.

Comment 14

[Boris Zbarsky [:bzbarsky]]

What does mozMapDataInto actually do?  Why does it need the "buffer" argument?  Does it return a new object each time or not?

In general, any IDL that's not coming from a spec should have documentation comments that are the equivalent of a spec.  Certainly if you want people to comment on it intelligently...

Comment 15

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

Comment on attachment 8588945 [details] [diff] [review]
WebIDL-draft.patch

Review of attachment 8588945 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/ImageBitmap.webidl
@@ +42,5 @@
> +  "YUV444P",
> +  "YUV422P",
> +  "YUV420P",
> +  "YUV420SP_NV12",
> +  "YUV420SP_NV21",

Please document all these very carefully. Document what kinds of ChannelPixelLayouts they can return, and what 'channel1'...'channel4' mean for each format.

Is it not possible to represent NV12 and NV21 using ChannelPixelLayouts?

@@ +49,5 @@
> +  ""
> +};
> +
> +[Exposed=(Window,Worker)]
> +interface MozChannelPixelLayout

Please stop introducing prefixes. We don't want to ship new prefixed APIs. Same for all the prefixes below.

@@ +52,5 @@
> +[Exposed=(Window,Worker)]
> +interface MozChannelPixelLayout
> +{
> +  [Constant, StoreInSlot]
> +  readonly attribute Uint8ClampedArray data; // includes buffer, width, height

The actual image data is not here, it gets mapped into the ArrayBuffer below. So what exactly is this array?

@@ +56,5 @@
> +  readonly attribute Uint8ClampedArray data; // includes buffer, width, height
> +  [Constant]
> +  readonly attribute unsigned long stride;
> +  [Constant]
> +  readonly attribute unsigned long skip;

Please document these.

Comment 16

[Tzuhao Kuo [:kaku]]

(In reply to Not doing reviews right now from comment #14)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)

Sorry for the no-comment code and I will add the documentation comments.

> Is it not possible to represent NV12 and NV21 using ChannelPixelLayouts?
I think it is possible to use _skip_ property to present the interleaving layout.

> Please stop introducing prefixes. We don't want to ship new prefixed APIs.
OK.

> @@ +52,5 @@
> > +[Exposed=(Window,Worker)]
> > +interface MozChannelPixelLayout
> > +{
> > +  [Constant, StoreInSlot]
> > +  readonly attribute Uint8ClampedArray data; // includes buffer, width, height
> 
> The actual image data is not here, it gets mapped into the ArrayBuffer
> below. So what exactly is this array?
What I need here is an _offset_ property (which is relative to the beginning position of the given ArrayBuffer) to indicate the beginning position of a certain channel. I was thinking about using a TypedArray to encapsulate the given ArrayBuffer and the _offset_ for users' convenience but I think a _offset_ property alone is much more clear now.

Comment 17

[Tzuhao Kuo [:kaku]]

Attachment: WebIDL-draft.patch

Comment 18

[Tzuhao Kuo [:kaku]]

Attachment: WebIDL-draft.patch

Remove spaces and correct typos.

Comment 19

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

Comment on attachment 8591520 [details] [diff] [review]
WebIDL-draft.patch

Review of attachment 8591520 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good!

I suspect Jeff might have some good opinions here :-)

::: dom/webidl/ImageBitmap.webidl
@@ +260,5 @@
> +   * Channel order: Y, V, U
> +   * Channel size: full yuv-channel, quarter uv-channels
> +   * Pixel layout: planar y-channel, interleaving vu-channels
> +   */
> +  "YUV420SP_NV21",

I wonder whether we need these specific formats listed, or whether we should just have generic YUV and RGB formats with the details exposed via ChannelPixelLayout etc.

I guess one reason to have more detailed formats here is to specify constraints on the YUV format (for example). E.g. if the code can work with YUV420 or YUV422 but no other YUV variants, you might want to specify that to the API so the browser converts if necessary.

So, maybe the most reasonable thing to do is have all these specific formats, plus "GenericYUV" and "GenericRGB" formats which don't constrain the channel layouts at all.

@@ +276,5 @@
> +  "Lab",
> +  /*
> +   * empty string for error reporting
> +   */
> +  ""

Not sure why you need this? We can throw an exception in error cases.

@@ +322,5 @@
> +  readonly attribute unsigned long skip;
> +};
> +
> +[Exposed=(Window,Worker)]
> +interface ColorFormatPixelLayout

Maybe call this ColorFormatDetails? We might want to put color space data in here later, for example.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8591520 [details] [diff] [review]
WebIDL-draft.patch

Review of attachment 8591520 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/ImageBitmap.webidl
@@ +384,5 @@
> +   *
> +   * @return a ColorFormatPixelLayout object which describes the pixel layout.
> +   */
> +  [Throws]
> +  ColorFormatPixelLayout mapDataInto(ColorFormat format, ArrayBuffer buffer, long offset, long length);

I thought this was going to be asynchronous. That would be better.

Comment 21

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Comment on attachment 8591520 [details] [diff] [review]
> WebIDL-draft.patch
> 
> Review of attachment 8591520 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/webidl/ImageBitmap.webidl
> @@ +260,5 @@
> > +   * Channel order: Y, V, U
> > +   * Channel size: full yuv-channel, quarter uv-channels
> > +   * Pixel layout: planar y-channel, interleaving vu-channels
> > +   */
> > +  "YUV420SP_NV21",
> 
> I wonder whether we need these specific formats listed, or whether we should
> just have generic YUV and RGB formats with the details exposed via
> ChannelPixelLayout etc.
> 
> I guess one reason to have more detailed formats here is to specify
> constraints on the YUV format (for example). E.g. if the code can work with
> YUV420 or YUV422 but no other YUV variants, you might want to specify that
> to the API so the browser converts if necessary.
Yes, I think that people design/code an image processing algorithm based on a specific color format. If the system/framework does not provide the specific format they want, they do conversion before sending data into their algorithm engine.

> So, maybe the most reasonable thing to do is have all these specific
> formats, plus "GenericYUV" and "GenericRGB" formats which don't constrain
> the channel layouts at all.
I am not sure the use case of "GenericXXX color formats". How will people do something on an image data without knowing the exact color format in advance?

And agree for the others.

Comment 22

[Tzuhao Kuo [:kaku]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> Comment on attachment 8591520 [details] [diff] [review]
> WebIDL-draft.patch
> 
> Review of attachment 8591520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/ImageBitmap.webidl
> @@ +384,5 @@
> > +   *
> > +   * @return a ColorFormatPixelLayout object which describes the pixel layout.
> > +   */
> > +  [Throws]
> > +  ColorFormatPixelLayout mapDataInto(ColorFormat format, ArrayBuffer buffer, long offset, long length);
> 
> I thought this was going to be asynchronous. That would be better.

Do you mean something like this?
Promise<ColorFormatPixelLayout> mapDataInto(ColorFormat format, ArrayBuffer buffer, long offset, long length);

I am not sure about the criteria of designing an API to be sync or async? Is it for the computation time? This API might spends time on 1) color conversion and 2) copy data into the provided ArrayBuffer. Indeed, it might takes a while if the image dimension is large.

Comment 23

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

(In reply to Tzuhao Kuo [:kaku] from comment #21)
> I am not sure the use case of "GenericXXX color formats". How will people do
> something on an image data without knowing the exact color format in advance?

For example, a GenericYUV format with ChannelPixelLayout data would be usable, just a bit slow. So an algorithm could have two code paths: one that handles YUV444P, and and a slower path that handles all YUV formats. Then the code could request [ YUV444P, GenericYUV ].

(In reply to Tzuhao Kuo [:kaku] from comment #22)
> > I thought this was going to be asynchronous. That would be better.
> 
> Do you mean something like this?
> Promise<ColorFormatPixelLayout> mapDataInto(ColorFormat format, ArrayBuffer
> buffer, long offset, long length);
> 
> I am not sure about the criteria of designing an API to be sync or async? Is
> it for the computation time? This API might spends time on 1) color
> conversion and 2) copy data into the provided ArrayBuffer. Indeed, it might
> takes a while if the image dimension is large.

Yes I think Jeff's right, because the conversion could be slow, we should probably make this async.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tzuhao Kuo [:kaku] from comment #22)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> > Comment on attachment 8591520 [details] [diff] [review]
> > WebIDL-draft.patch
> > 
> > Review of attachment 8591520 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/ImageBitmap.webidl
> > @@ +384,5 @@
> > > +   *
> > > +   * @return a ColorFormatPixelLayout object which describes the pixel layout.
> > > +   */
> > > +  [Throws]
> > > +  ColorFormatPixelLayout mapDataInto(ColorFormat format, ArrayBuffer buffer, long offset, long length);
> > 
> > I thought this was going to be asynchronous. That would be better.
> 
> Do you mean something like this?
> Promise<ColorFormatPixelLayout> mapDataInto(ColorFormat format, ArrayBuffer
> buffer, long offset, long length);

Yes. That seems reasonable.

Comment 25

[Tzuhao Kuo [:kaku]]

Hi, roc and Jeff,

So far, we have extended the ImageBitmap with methods to read its underlying data, however, we still need a method to write data back into an ImageBitmap because the processor scenario of the VideoWorker framework needs developers to write the processed image data back into the "outputImageBitmap" property. (or, generally, the framework needs developers to write the processed back in some way so that the framework can handle the processed data thereafter......)

I suggest to add the following method to the ImageBitmap interface.

[Throw]
boolean setDataFrom(ColorFormat fromat, ColorFromatPixelLayout layout, ArrayBuffer buffer, long offset, long length);

I would like to listen to your thoughts on this setter method.
The main concern is that how could developers create a "ColorFromatPixelLayout" object by their own? However, this is somewhat tricky since that developers could always use the setDataFrom() method with the mapDataInto() method beforehand and so the ColorFromatLayout object is already there, returned from the mapDataInto() method.

BTW, since ctai is preparing to propose the VideoWorker to the W3C, I also wrote this ImageBitmap Extenstions specification draft in the W3C format. 
The link of draft: http://kakukogou.github.io/spec-imagebitmap-extension/.
The link of github: https://github.com/kakukogou/spec-imagebitmap-extension/.
People from Intel are now working with ctai to integrate the VideoWorker with their mediacapture-depth specification so they propose that ImageBitmap should also be able to be a container of a depth image which means that there should be a "DEPTH" format in the "ColorFromat enumeration". Discussions are on the Github repository.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> ArrayBuffer buffer, 

BufferSource is probably better.

> The main concern is that how could developers create a "ColorFromatPixelLayout" object

Can we not just give it a constructor?

Comment 27

[Tzuhao Kuo [:kaku]]

(In reply to Not doing reviews right now from comment #26)
> > ArrayBuffer buffer, 
> 
> BufferSource is probably better.
Are you saying something like the AudioBuffer in the WebAudio?

> 
> > The main concern is that how could developers create a "ColorFromatPixelLayout" object
> 
> Can we not just give it a constructor?
Yes, we can. What I concern is that filling out the ColorFromatPixelLayout object correctly is tedious..., but yes, it can be done by providing a constructor.

Comment 28

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Tzuhao Kuo [:kaku] from comment #27)
> (In reply to Not doing reviews right now from comment #26)
> > > ArrayBuffer buffer, 
> > 
> > BufferSource is probably better.
> Are you saying something like the AudioBuffer in the WebAudio?
I think this is what bz want.
https://heycam.github.io/webidl/#idl-buffer-source-types


> 
> > 
> > > The main concern is that how could developers create a "ColorFromatPixelLayout" object
> > 
> > Can we not just give it a constructor?
> Yes, we can. What I concern is that filling out the ColorFromatPixelLayout
> object correctly is tedious..., but yes, it can be done by providing a
> constructor.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I think this is what bz want.

Yep.  Specifically http://heycam.github.io/webidl/#common-BufferSource

That's the normal type for "just pass me some bytes" unless there are overriding considerations for doing something else.

Comment 30

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

(In reply to Tzuhao Kuo [:kaku] from comment #25)
> So far, we have extended the ImageBitmap with methods to read its underlying
> data, however, we still need a method to write data back into an ImageBitmap
> because the processor scenario of the VideoWorker framework needs developers
> to write the processed image data back into the "outputImageBitmap"
> property. (or, generally, the framework needs developers to write the
> processed back in some way so that the framework can handle the processed
> data thereafter......)

We don't want ImageBitmaps to be writeable. We can't do that and also make them shareable across Workers. Right now ImageBitmaps are designed to be immutable (except for close(), which only affects the worker's ImageBitmap object and doesn't actually modify any data).

If necessary we should add a new ImageBitmap constructor instead.

Comment 31

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> (In reply to Tzuhao Kuo [:kaku] from comment #25)
> > So far, we have extended the ImageBitmap with methods to read its underlying
> > data, however, we still need a method to write data back into an ImageBitmap
> > because the processor scenario of the VideoWorker framework needs developers
> > to write the processed image data back into the "outputImageBitmap"
> > property. (or, generally, the framework needs developers to write the
> > processed back in some way so that the framework can handle the processed
> > data thereafter......)
> 
> We don't want ImageBitmaps to be writeable. We can't do that and also make
> them shareable across Workers. Right now ImageBitmaps are designed to be
> immutable (except for close(), which only affects the worker's ImageBitmap
> object and doesn't actually modify any data).
> 
> If necessary we should add a new ImageBitmap constructor instead.

The behavior of setDataFrom() method is totally reset the data of an ImageBitmap so that we could implement it by copy-on-write mechanism which can still keep sharing across workers. However, copy-on-write is not so much difference between providing a new CTOR.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

I would prefer a new CTOR.

Comment 33

[Tzuhao Kuo [:kaku]]

Attachment: [WIP] Part0-WebIDL.patch

WIP.

Comment 34

[Tzuhao Kuo [:kaku]]

Attachment: [WIP] Part1-Test-cases.patch

Comment 35

[Tzuhao Kuo [:kaku]]

Attachment: [WIP] Part2-Implementation.patch

WIP.

Only support the RGBA/YUV color spaces now.

Comment 36

[Tzuhao Kuo [:kaku]]

Attachment: [WIP] Part2-Implementation.patch

In this patch, I build up the structure of the extensions implementation, which focus on two issues, 1) support all types of layers::Image and 2) convert between different color formats. 

For the 1st issue, I create a wrapper, ImageUtils, to all kinds of layers::Images. I unify the interfaces for collecting information in different layers::Images.

For the 2nd issue, I create a type trait, ImageFormatTrait, and partial-specialize it for each kind of color formats. 

Not all the details are implemented, however, I would like to listen to your comments on the structure of the code.

Comment 37

[u459114]

Comment on attachment 8661139 [details] [diff] [review]
[WIP] Part2-Implementation.patch

Review of attachment 8661139 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/ImageUtils.h
@@ +44,5 @@
> +                                                       uint32_t aOffset,
> +                                                       uint32_t aBufferLength,
> +                                                       ImageBitmapFormat aFormat,
> +                                                       ErrorResult& aRv) const;
> +

I prefer use static member function here.
static ImageBitmapFormat GetFormat(layers::Image* aImage);
static uint32_t GetBufferLength(layers::Image* aImage);
static already_AddRefed<ImageFormatPixelLayout> MapDataInto(layers::Image* aImage....);

Then, you don't even need to use pimp pattern here.

Comment 38

[u459114]

Comment on attachment 8661139 [details] [diff] [review]
[WIP] Part2-Implementation.patch

Review of attachment 8661139 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/ImageBitmap.h
@@ +200,5 @@
> +
> +  /*
> +   * This is used in the ImageBitmap-Extensions implementation.
> +   */
> +  ImageUtils* mUtils;

"ImageBitmap has a ImageUtils" is kinda weird relationship.

::: dom/canvas/ImageBitmapExtension.cpp
@@ +63,5 @@
> +static already_AddRefed<layers::Image>
> +CreateImageFromRawData(const uint8_t*aBufferData, uint32_t aBufferLength,
> +                       mozilla::dom::ImageBitmapFormat aFormat,
> +                       mozilla::dom::ImageFormatPixelLayout& aLayout,
> +                       const Maybe<gfx::IntRect>& aCropRect, ErrorResult& aRv)

Move all CreateImageFromXXX functions to ImageUtils?

::: dom/canvas/ImageBitmapUtils.h
@@ +54,5 @@
> +{
> +public:
> +  static unsigned char const channels = 3;
> +
> +  static uint32_t NeededBufferSize(uint32_t width, uint32_t height)

Make a trait more simple, move NeeddedBufferSize out of Trait
And, I don't really understand why you need trait here?

Comment 39

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8655272 [details] [diff] [review]
[WIP] Part0-WebIDL.patch

Review of attachment 8655272 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: dom/webidl/ImageBitmap.webidl
@@ +5,5 @@
>   *
>   * The origin of this IDL file is
>   * https://html.spec.whatwg.org/multipage/webappapis.html#images
> + * The origin of the extented IDL file is
> + * http://kakukogou.github.io/spec-imagebitmap-extension/

Change to W3C github.

@@ +105,5 @@
> +// [NoInterfaceObject, Exposed=(Window,Worker)]
> +// partial interface ImageBitmapFactories {
> +//     Promise<ImageBitmap> createImageBitmap (BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
> +//     Promise<ImageBitmap> createImageBitmap (BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout, long aSx, long aSy, long aSw, long aSh);
> +// };

Redundant codes?

Comment 40

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8661139 [details] [diff] [review]
[WIP] Part2-Implementation.patch

Review of attachment 8661139 [details] [diff] [review]:
-----------------------------------------------------------------

Like CJ say, don't see the benefit of trait. We might be able to find a solution later.

::: dom/base/nsGlobalWindow.h
@@ +53,5 @@
>  #include "nsSize.h"
>  #include "nsCheapSets.h"
>  #include "mozilla/dom/ImageBitmapSource.h"
>  
> +

Why do we need this empty line?

::: dom/canvas/ChannelPixelLayout.cpp
@@ +20,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +ChannelPixelLayout::ChannelPixelLayout()
> +: mOffset(-1)

You should change the spec to point out those default values are -1, right?

@@ +27,5 @@
> +, mDataType(DataType::Uint8)
> +, mStride(-1)
> +, mSkip(-1)
> +{
> +    // Add |MOZ_COUNT_CTOR(ChannelPixelLayout);| for a non-refcounted object.

Are those comment necessary? I think those are auto generated comments.

@@ +32,5 @@
> +}
> +
> +ChannelPixelLayout::~ChannelPixelLayout()
> +{
> +    // Add |MOZ_COUNT_DTOR(ChannelPixelLayout);| for a non-refcounted object.

diddo

::: dom/canvas/ChannelPixelLayout.h
@@ +35,5 @@
> +  ~ChannelPixelLayout();
> +
> +public:
> +  // TODO: return something sensible here, and change the return type
> +  ChannelPixelLayout* GetParentObject() const

Should the parent be ImageFormatPixelLayout?

::: dom/canvas/ImageBitmapExtension.cpp
@@ +21,5 @@
> +namespace dom {
> +
> +// external functions
> +extern void
> +AsyncFulfillImageBitmapPromise(Promise* aPromise, ImageBitmap* aImageBitmap);

Do you mind to point out where we can find the function in comment?

@@ +69,5 @@
> +  switch(aFormat) {
> +  case ImageBitmapFormat::RGBA32:
> +  case ImageBitmapFormat::BGRA32:
> +  {
> +    const nsTArray<nsRefPtr<ChannelPixelLayout>>& channels = aLayout.GetChannels();

Could channels empty?

@@ +178,5 @@
> +    bufferView.ComputeLengthAndData();
> +    bufferData = bufferView.Data();
> +    bufferLength = bufferView.Length();
> +  } else {
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

Should we just return right there?

@@ +210,5 @@
> +ImageBitmapFormat
> +ImageBitmap::FindOptimalFormat(const Optional<Sequence<ImageBitmapFormat>>& aPossibleFormats)
> +{
> +  MOZ_ASSERT(mUtils, "No ImageBitmapFormatUtils functionalities.");
> +  return mUtils->GetFormat();

Please add TODO in this function. You should do some logic to choose the optimal format.

@@ +218,5 @@
> +ImageBitmap::MappedDataLength(ImageBitmapFormat aFormat, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(mUtils, "No ImageBitmapFormatUtils functionalities.");
> +
> +  const ImageBitmapFormat optimalFormat = mUtils->GetFormat();

rename to platformFormat

::: dom/canvas/ImageBitmapUtils.h
@@ +54,5 @@
> +{
> +public:
> +  static unsigned char const channels = 3;
> +
> +  static uint32_t NeededBufferSize(uint32_t width, uint32_t height)

Could we just use isA?

::: dom/canvas/ImageFormatPixelLayout.cpp
@@ +47,5 @@
> +  uint8_t channelCount = GetChannelCountOfImageFormat(aFormat);
> +
> +  // set mChannels
> +  switch (aFormat) {
> +  case ImageBitmapFormat::RGBA32:

indents.

@@ +164,5 @@
> +                                        uint32_t aUWidth, uint32_t aUHeight, uint32_t aUStride,
> +                                        uint32_t aVWidth, uint32_t aVHeight, uint32_t aVStride)
> +{
> +  nsRefPtr<ImageFormatPixelLayout> layout(new ImageFormatPixelLayout(ImageBitmapFormat::YUV420P));
> +  nsRefPtr<ChannelPixelLayout> *ychannel = layout->mChannels.AppendElement(); (*ychannel) = new ChannelPixelLayout();

new line before (*ychannel) = new ChannelPixelLayout();

::: dom/canvas/ImageUtils.cpp
@@ +54,5 @@
> +  }
> +}
> +
> +static ImageBitmapFormat
> +GetImageBitmapFormatFromPlanarYCbCrData(layers::PlanarYCbCrData const *data) 

trailing space.

@@ +709,5 @@
> +}
> +
> +already_AddRefed<ImageFormatPixelLayout>
> +ImageUtils::MapDataInto(uint8_t* aBuffer,
> +                                    uint32_t aOffset,

alignment

Comment 41

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8661139 [details] [diff] [review]
[WIP] Part2-Implementation.patch

Review of attachment 8661139 [details] [diff] [review]:
-----------------------------------------------------------------

Like CJ say, don't see the benefit of trait. We might be able to find a solution later.

::: dom/base/nsGlobalWindow.h
@@ +53,5 @@
>  #include "nsSize.h"
>  #include "nsCheapSets.h"
>  #include "mozilla/dom/ImageBitmapSource.h"
>  
> +

Why do we need this empty line?

::: dom/canvas/ChannelPixelLayout.cpp
@@ +20,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +ChannelPixelLayout::ChannelPixelLayout()
> +: mOffset(-1)

You should change the spec to point out those default values are -1, right?

@@ +27,5 @@
> +, mDataType(DataType::Uint8)
> +, mStride(-1)
> +, mSkip(-1)
> +{
> +    // Add |MOZ_COUNT_CTOR(ChannelPixelLayout);| for a non-refcounted object.

Are those comment necessary? I think those are auto generated comments.

@@ +32,5 @@
> +}
> +
> +ChannelPixelLayout::~ChannelPixelLayout()
> +{
> +    // Add |MOZ_COUNT_DTOR(ChannelPixelLayout);| for a non-refcounted object.

diddo

::: dom/canvas/ChannelPixelLayout.h
@@ +35,5 @@
> +  ~ChannelPixelLayout();
> +
> +public:
> +  // TODO: return something sensible here, and change the return type
> +  ChannelPixelLayout* GetParentObject() const

Should the parent be ImageFormatPixelLayout?

::: dom/canvas/ImageBitmapExtension.cpp
@@ +21,5 @@
> +namespace dom {
> +
> +// external functions
> +extern void
> +AsyncFulfillImageBitmapPromise(Promise* aPromise, ImageBitmap* aImageBitmap);

Do you mind to point out where we can find the function in comment?

@@ +69,5 @@
> +  switch(aFormat) {
> +  case ImageBitmapFormat::RGBA32:
> +  case ImageBitmapFormat::BGRA32:
> +  {
> +    const nsTArray<nsRefPtr<ChannelPixelLayout>>& channels = aLayout.GetChannels();

Could channels empty?

@@ +178,5 @@
> +    bufferView.ComputeLengthAndData();
> +    bufferData = bufferView.Data();
> +    bufferLength = bufferView.Length();
> +  } else {
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

Should we just return right there?

@@ +210,5 @@
> +ImageBitmapFormat
> +ImageBitmap::FindOptimalFormat(const Optional<Sequence<ImageBitmapFormat>>& aPossibleFormats)
> +{
> +  MOZ_ASSERT(mUtils, "No ImageBitmapFormatUtils functionalities.");
> +  return mUtils->GetFormat();

Please add TODO in this function. You should do some logic to choose the optimal format.

@@ +218,5 @@
> +ImageBitmap::MappedDataLength(ImageBitmapFormat aFormat, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(mUtils, "No ImageBitmapFormatUtils functionalities.");
> +
> +  const ImageBitmapFormat optimalFormat = mUtils->GetFormat();

rename to platformFormat

::: dom/canvas/ImageBitmapUtils.h
@@ +54,5 @@
> +{
> +public:
> +  static unsigned char const channels = 3;
> +
> +  static uint32_t NeededBufferSize(uint32_t width, uint32_t height)

Could we just use isA?

::: dom/canvas/ImageFormatPixelLayout.cpp
@@ +47,5 @@
> +  uint8_t channelCount = GetChannelCountOfImageFormat(aFormat);
> +
> +  // set mChannels
> +  switch (aFormat) {
> +  case ImageBitmapFormat::RGBA32:

indents.

@@ +164,5 @@
> +                                        uint32_t aUWidth, uint32_t aUHeight, uint32_t aUStride,
> +                                        uint32_t aVWidth, uint32_t aVHeight, uint32_t aVStride)
> +{
> +  nsRefPtr<ImageFormatPixelLayout> layout(new ImageFormatPixelLayout(ImageBitmapFormat::YUV420P));
> +  nsRefPtr<ChannelPixelLayout> *ychannel = layout->mChannels.AppendElement(); (*ychannel) = new ChannelPixelLayout();

new line before (*ychannel) = new ChannelPixelLayout();

::: dom/canvas/ImageUtils.cpp
@@ +54,5 @@
> +  }
> +}
> +
> +static ImageBitmapFormat
> +GetImageBitmapFormatFromPlanarYCbCrData(layers::PlanarYCbCrData const *data) 

trailing space.

@@ +709,5 @@
> +}
> +
> +already_AddRefed<ImageFormatPixelLayout>
> +ImageUtils::MapDataInto(uint8_t* aBuffer,
> +                                    uint32_t aOffset,

alignment

Comment 42

[Tzuhao Kuo [:kaku]]

Attachment: Part0-WebIDL.patch

Comment 43

[Tzuhao Kuo [:kaku]]

Attachment: Part1-Test-cases.patch

Comment 44

[Tzuhao Kuo [:kaku]]

Attachment: Part2-Implementation.patch

Comment 45

[Tzuhao Kuo [:kaku]]

Attachment: Part3-Preference.patch

Comment 46

[Tzuhao Kuo [:kaku]]

Hi roc,

I have implemented the overall structure of the ImageBitmap-extensions which includes a generic algorithm to access all kinds of layers::Image and a specialized one for the PlanarYCbCrImage. Also, the current implementation only supports users to access data in RGBA format. 

I would like to request for your review on the current implementation first and then open new bugs for 
(1) optimize for other sub-classes of layers::Image and 
(2) support other color formats.

Comment 47

[Tzuhao Kuo [:kaku]]

Comment on attachment 8685396 [details] [diff] [review]
Part0-WebIDL.patch

Review of attachment 8685396 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/ImageBitmap.webidl
@@ +38,5 @@
> +  // underlying image data
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmapFromBufferSource(BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmapFromBufferSource(BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout, long aSx, long aSy, long aSw, long aSh);

In the draft spec., these two extended methods are both named ImageBitmapFactories::createImageBitmap which overloads the original ones. However, our tool-chain does not support overriding via union types (ImageBitmapSource and BufferSource), so I rename these two methods here. We can simply modify the spec. or should we keep the original naming and open a bug for the overriding issue?

@@ +45,5 @@
> +// Extensions
> +// Bug 1141979 - [FoxEye] Extend ImageBitmap with interfaces to access its
> +// underlying image data
> +
> +enum ImageBitmapFormat {

In the draft spec., this enumeration is named "ImageFormat". However, our code base already has mozilla::ImageFormat which is used extensively without scope resolution, so I simply rename this enumeration here and should also be ok to modify it on the spec.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Isn't the right spec thing to just change ImageBitmapSource to the definition that's actually wanted?

In terms of our implementation, that's certainly what you should do: just add BufferSource to the ImageBitmapSource union.

> so I simply rename this enumeration here and should also be ok to modify it on the spec.

Making it ImageBitmapFormat in the spec makes sense to me.  Improves searchability, and the names of enums are not anything anyone on the web ever has to type so there is no real drawback to length.

Comment 49

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

Comment on attachment 8685396 [details] [diff] [review]
Part0-WebIDL.patch

Review of attachment 8685396 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/ImageBitmap.webidl
@@ +38,5 @@
> +  // underlying image data
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmapFromBufferSource(BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmapFromBufferSource(BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout, long aSx, long aSy, long aSw, long aSh);

Maybe we can create a WebIDL dictionary type which contains a buffer, an offset, a length and the format, and make ImageBitmapSource take that dictionary type as an option?

Or, can we overload createImageBitmap just by reordering the parameters so that ImageBitmapFormat is the first parameter?

Comment 50

[Boris Zbarsky [:bzbarsky]]

The latter wouldn't help in terms of our code, because ImageBitmapSource is itself a union and our overload resolution implementation doesn't deal with a union as the distinguishing argument.  We could try to fix that, but it's quite a bit of complexity....

Comment 51

[Tzuhao Kuo [:kaku]]

Attachment: error.txt

Comment 52

[Tzuhao Kuo [:kaku]]

(In reply to Boris Zbarsky [:bz] from comment #48)
> Isn't the right spec thing to just change ImageBitmapSource to the
> definition that's actually wanted?
> 
> In terms of our implementation, that's certainly what you should do: just
> add BufferSource to the ImageBitmapSource union.

I tried this idea and it is doable. 
However, the APIs will then become:

ImageBitmapFactories::createImageBitmap(ImageBitmapSource aSource);
ImageBitmapFactories::createImageBitmap(ImageBitmapSource aSource, 
                                        long aOffset, long aLength, 
                                        ImageBitmapFormat aFormat, 
                                        ImageFormatPixelLayout aLayout);

I think the 2nd one might confuse people because the 2nd one is only reasonable wile the _aSource_ is a BufferSource but, in this design, it could also be other sources such as HTML(Image/Video/Canvas)Element.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> Maybe we can create a WebIDL dictionary type which contains a buffer, an
> offset, a length and the format, and make ImageBitmapSource take that
> dictionary type as an option?

Does this dictionary type includes the _ImageFormatPixelLayout_? 
If no, then this proposal will face the same problem of bz's proposal.

If yes, then I think it could be a solution, however when I tried to implement it, I encountered into another tool-chain error. 

Modified webidl:

dictionary ImageBitmapBufferSourceInit {
  required BufferSource buffer;
  required long offset;
  required long length;
  required ImageBitmapFormat format;
  required ImageFormatPixelLayout layout;
};

typedef (HTMLImageElement or
         HTMLVideoElement or
         HTMLCanvasElement or
         Blob or
         ImageData or
         CanvasRenderingContext2D or
         ImageBitmap or
         ImageBitmapBufferSourceInit) ImageBitmapSource;

The error message is: (full log is at Attachment 8685805 [details])
TypeError: Cycle collection for type ImageBitmapBufferSourceInit (Wrapper) is not supported

Is this another tool-chain limitation?

Comment 53

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

(In reply to Tzuhao Kuo [:kaku] from comment #52)
> I tried this idea and it is doable. 
> However, the APIs will then become:
> 
> ImageBitmapFactories::createImageBitmap(ImageBitmapSource aSource);
> ImageBitmapFactories::createImageBitmap(ImageBitmapSource aSource, 
>                                         long aOffset, long aLength, 
>                                         ImageBitmapFormat aFormat, 
>                                         ImageFormatPixelLayout aLayout);
> 
> I think the 2nd one might confuse people because the 2nd one is only
> reasonable wile the _aSource_ is a BufferSource but, in this design, it
> could also be other sources such as HTML(Image/Video/Canvas)Element.

I think probably we should spec it as 
  createImageBitmap(ImageBitmapSource aSource);
  createImageBitmap(BufferSource aBuffer, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);

(BTW can we change ImageFormatPixelLayout to ImagePixelLayout?)

then we can implement it for now as

  createImageBitmap(ImageBitmapSource aSource);
  createImageBitmap(ImageBitmapSource aSource, long aOffset, long aLength, ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
and add BufferSource to ImageBitmapSource ... until it's possible to use the spec definition.

Comment 54

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> I think probably we should spec it as 
>   createImageBitmap(ImageBitmapSource aSource);
>   createImageBitmap(BufferSource aBuffer, long aOffset, long aLength,
> ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
I prefer this one also.
 
> (BTW can we change ImageFormatPixelLayout to ImagePixelLayout?)
Any specific reasons?
 
> then we can implement it for now as
> 
>   createImageBitmap(ImageBitmapSource aSource);
>   createImageBitmap(ImageBitmapSource aSource, long aOffset, long aLength,
> ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
> and add BufferSource to ImageBitmapSource ... until it's possible to use the
> spec definition.
Are you already reviewing other patches? Should I upload the modified ones according to this work around for you to review?

Comment 55

[Tzuhao Kuo [:kaku]]

One more thing. Overloaded functions cannot have different "extended attributes", so I cannot add preference on the extended version of ImageBitmapFactories::createImageBitmap(). To work around, I will then check the preference at run time and throw if needed.

Comment 56

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

(In reply to Tzuhao Kuo [:kaku] from comment #54)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> > I think probably we should spec it as 
> >   createImageBitmap(ImageBitmapSource aSource);
> >   createImageBitmap(BufferSource aBuffer, long aOffset, long aLength,
> > ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
> I prefer this one also.
>  
> > (BTW can we change ImageFormatPixelLayout to ImagePixelLayout?)
> Any specific reasons?

It's shorter and less redundant.

> > then we can implement it for now as
> > 
> >   createImageBitmap(ImageBitmapSource aSource);
> >   createImageBitmap(ImageBitmapSource aSource, long aOffset, long aLength,
> > ImageBitmapFormat aFormat, ImageFormatPixelLayout aLayout);
> > and add BufferSource to ImageBitmapSource ... until it's possible to use the
> > spec definition.
> Are you already reviewing other patches? Should I upload the modified ones
> according to this work around for you to review?

Please make that change and re-upload.

Comment 57

[Boris Zbarsky [:bzbarsky]]

> Is this another tool-chain limitation?

Looks like yes.  In particular, if you have an owning union that has a dictionary member and that dictionary has members that need to be traversed/unlinked, the codegen currently doesn't know how to output the right traverse/unlink bits.

That should not be so difficult to fix, I would think.  We would need to spit out ImplCycleCollectionUnlink/ImplCycleCollectionTraverse implementations for dictionaries with such members.  I'm happy to guide you through doing that, or if needed can find the time to do it myself.

Comment 58

[Tzuhao Kuo [:kaku]]

Attachment: Part0-WebIDL.patch

Comment 59

[Tzuhao Kuo [:kaku]]

Attachment: Part1-Test-cases.patch

Comment 60

[Tzuhao Kuo [:kaku]]

Attachment: Part2-Implementation.patch

Basically, the functionality of ImageBitmap-extensions are completed with two utility classes.
(1) ImageUtils: this provides unified methods to access data of different sub-classes of layers::Image.
(2) ImageBitmapFormatUtils: this provides unified methods to deal with different ImageBitmapFormats, includes getting information, creating ImagePixelLayouts and color conversion.

Comment 61

[Tzuhao Kuo [:kaku]]

Attachment: Part3-Preference.patch

Comment 62

[Tzuhao Kuo [:kaku]]

(In reply to Boris Zbarsky [:bz] from comment #57)
> > Is this another tool-chain limitation?
> 
> Looks like yes.  In particular, if you have an owning union that has a
> dictionary member and that dictionary has members that need to be
> traversed/unlinked, the codegen currently doesn't know how to output the
> right traverse/unlink bits.
> 
> That should not be so difficult to fix, I would think.  We would need to
> spit out ImplCycleCollectionUnlink/ImplCycleCollectionTraverse
> implementations for dictionaries with such members.  I'm happy to guide you
> through doing that, or if needed can find the time to do it myself.

Thank you and this will be a very good opportunity for me to learn something deeper. 
So, we raised two issues here, 
(1)in comment50, let overload resolution implementation deal with a union as the distinguishing argument.
(2)let union type handle cycle collection of its dictionary member.

I will then open two bugs for there two issues and we can discuss there.

Comment 63

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

Comment on attachment 8686376 [details] [diff] [review]
Part0-WebIDL.patch

Review of attachment 8686376 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/ImageBitmap.webidl
@@ +76,5 @@
> +    "YUV420SP_NV12",
> +    "YUV420SP_NV21",
> +    "HSV",
> +    "Lab",
> +    "DEPTH"

we need to carefully document all these at some point.

Comment 64

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

Comment on attachment 8686377 [details] [diff] [review]
Part1-Test-cases.patch

Review of attachment 8686377 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/test/imagebitmap_extension.html
@@ +58,5 @@
> +//  | 255  | 255  | 255  | 255  | 0    | 0    | 0    |
> +//  | 255  | 0    | 255  | 0    | 255  | 0    | 255  |
> +//  |      |      |      |      |      |      |      |
> +//  ^      ^      ^      ^      ^      ^      ^      ^
> +//  0      46     92     138    184    230    276    319

I don't understand this comment

@@ +71,5 @@
> +//    2.1) "pixel": the coordinate of this pixel (x, y).
> +//    2.2) "expectedColor": the expected color of this pixel (r, g, b, a).
> +//    2.3) "tolerance": the acceptable tolerance of pixel values.
> +//
> +var TEST_BITMAPS = [

Where are these used?

Seems like TEST_BITMAPS isn't the best name, since they're not bitmaps. Maybe BITMAP_TESTS?

What is the croppingArea for?

::: dom/canvas/test/imagebitmap_extension_on_worker.js
@@ +82,5 @@
> +//    2.1) "pixel": the coordinate of this pixel (x, y).
> +//    2.2) "expectedColor": the expected color of this pixel (r, g, b, a).
> +//    2.3) "tolerance": the acceptable tolerance of pixel values.
> +//
> +var TEST_BITMAPS = [

Can't we share test code instead of duplicating it?

Comment 65

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

Comment on attachment 8686382 [details] [diff] [review]
Part2-Implementation.patch

Review of attachment 8686382 [details] [diff] [review]:
-----------------------------------------------------------------

Please split this up. ChannelPixelLayout can go into its own patch, then ImagePixelLayout in its own patch, then the ImageBitmap stuff, then the nsGlobalWindow implementation, then the WorkerScope implementation, then the WebIDL for everything and then the tests. All prefixes of that patch list should build.

You might find that using MozReview is easier than uploading patches.

Comment 66

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> Comment on attachment 8686377 [details] [diff] [review]
> Part1-Test-cases.patch
> 
> Review of attachment 8686377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/test/imagebitmap_extension.html
> @@ +58,5 @@
> > +//  | 255  | 255  | 255  | 255  | 0    | 0    | 0    |
> > +//  | 255  | 0    | 255  | 0    | 255  | 0    | 255  |
> > +//  |      |      |      |      |      |      |      |
> > +//  ^      ^      ^      ^      ^      ^      ^      ^
> > +//  0      46     92     138    184    230    276    319
> 
> I don't understand this comment
> 
> @@ +71,5 @@
> > +//    2.1) "pixel": the coordinate of this pixel (x, y).
> > +//    2.2) "expectedColor": the expected color of this pixel (r, g, b, a).
> > +//    2.3) "tolerance": the acceptable tolerance of pixel values.
> > +//
> > +var TEST_BITMAPS = [
> 
> Where are these used?
> 
> Seems like TEST_BITMAPS isn't the best name, since they're not bitmaps.
> Maybe BITMAP_TESTS?
> 
> What is the croppingArea for?
The TEST_BITMAPS is not used. I forgot to remove this part while creating this test from the old ImageBitmap test. Sorry for my mistake.

> ::: dom/canvas/test/imagebitmap_extension_on_worker.js
> @@ +82,5 @@
> > +//    2.1) "pixel": the coordinate of this pixel (x, y).
> > +//    2.2) "expectedColor": the expected color of this pixel (r, g, b, a).
> > +//    2.3) "tolerance": the acceptable tolerance of pixel values.
> > +//
> > +var TEST_BITMAPS = [
> 
> Can't we share test code instead of duplicating it?
We can and I will then create a JS file for the shared functionality.

Comment 67

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> Comment on attachment 8686382 [details] [diff] [review]
> Part2-Implementation.patch
> 
> Review of attachment 8686382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please split this up. ChannelPixelLayout can go into its own patch, then
> ImagePixelLayout in its own patch, then the ImageBitmap stuff, then the
> nsGlobalWindow implementation, then the WorkerScope implementation, then the
> WebIDL for everything and then the tests. All prefixes of that patch list
> should build.
> 
> You might find that using MozReview is easier than uploading patches.

OK and I will try to use MozReview.

Comment 68

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Bug 1141979 - part0 - setup preference utilities.

Comment 69

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Bug 1141979 - part1 - WebIDL for native implementation.

Comment 70

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Bug 1141979 - part2 - implement ChannelPixelLayout.

Comment 71

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Bug 1141979 - part3 - implement ImagePixelLayout.

Comment 72

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Bug 1141979 - part4 - implement utilities, ImageUtils and ImageBitmapFormatUtils.

Comment 73

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Bug 1141979 - part5 - implement ImageBitmap extensions.

Comment 74

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Bug 1141979 - part6 - implement ImageBitmapFactories extensions.

Comment 75

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Bug 1141979 - part7 - export to nsGlobalWindow.

Comment 76

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Bug 1141979 - part8 - export to WorkerGlobalScope.

Comment 77

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Bug 1141979 - part9 - WebIDL for bindings.

Comment 78

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Bug 1141979 - part10 - mochitest.

Comment 79

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25537/diff/1-2/

Comment 80

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25539/diff/1-2/

Comment 81

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25541/diff/1-2/

Comment 82

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/1-2/

Comment 83

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25545/diff/1-2/

Comment 84

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/1-2/

Comment 85

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/1-2/

Comment 86

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/1-2/

Comment 87

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25537/diff/1-2/

Comment 88

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/1-2/

Comment 89

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25539/diff/1-2/

Comment 90

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/1-2/

Comment 91

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25541/diff/1-2/

Comment 92

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/1-2/

Comment 93

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/1-2/

Comment 94

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/1-2/

Comment 95

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/1-2/

Comment 96

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/1-2/

Comment 97

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/1-2/

Comment 98

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/1-2/

Comment 99

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/1-2/

Comment 100

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/1-2/

Comment 101

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/1-2/

Comment 102

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/1-2/

Comment 103

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/1-2/

Comment 104

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/1-2/

Comment 105

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/1-2/

Comment 106

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/1-2/

Comment 107

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/1-2/

Comment 108

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/1-2/

Comment 109

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

Comment on attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

https://reviewboard.mozilla.org/r/25537/#review23257

::: modules/libpref/init/all.js:754
(Diff revision 1)
> +pref("canvas.imagebitmap_extensions.enabled", false);

Should be enabled in non-release builds, right?

Comment 110

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

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

https://reviewboard.mozilla.org/r/25539/#review23259

::: dom/webidl/ImageBitmap.webidl:31
(Diff revision 1)
> +	 BufferSource) ImageBitmapSource;

Since you're adding this in this patch, don't you also need in this patch to add dynamic checks for all existing functions taking an ImageBitmapSource to reject a BufferSource?

::: dom/webidl/ImageBitmap.webidl:65
(Diff revision 1)
> +    "DEPTH"

These need to be documented.

Comment 111

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

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

https://reviewboard.mozilla.org/r/25541/#review23261

::: dom/canvas/ChannelPixelLayout.h:45
(Diff revision 1)
> +  ChannelPixelLayout(const ChannelPixelLayout&) = default;

Do we need this copy constructor? Maybe we can just remove it.

::: dom/canvas/ChannelPixelLayout.cpp:30
(Diff revision 1)
> +}

Why do we need this constructor? Would be better not to have it if we don't need it.

Comment 112

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

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

https://reviewboard.mozilla.org/r/25543/#review23263

You need DOM Peer review for WebIDL changes.

::: dom/canvas/ImagePixelLayout.h:34
(Diff revision 1)
> +  ImagePixelLayout(const ImagePixelLayout&) = default;

You may not need the default and copy constructors.

Comment 113

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

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

https://reviewboard.mozilla.org/r/25545/#review23269

::: dom/canvas/ImageBitmapUtils.h:29
(Diff revision 1)
> + * (2) Store the number of channels of each kind of ImageBitmapFormats.

"for each kind"?

::: dom/canvas/ImageBitmapUtils.h:30
(Diff revision 1)
> + * (3) Calculate the needed buffer size of each kind of ImageBitmapFormats with

"each kind of ImageBitmapFormat" (singular)

::: dom/canvas/ImageBitmapUtils.h:40
(Diff revision 1)
> +class ImageBitmapFormatUtils

You don't really explain what ImageBitmapFormatUtils is. It's clearly not a singleton ... is there one per ImageBitmapFormat?

If there's one per ImageBitmapFormat, how do callers get the correct singleton object?

::: dom/canvas/ImageBitmapUtils.h:186
(Diff revision 1)
> +CvtColor(ImageBitmapFormat aSrcFormat,

Give this a full name and explain what it does.

Also explain how the parameters work. Also mention whether it always does a copy, or not.

::: dom/canvas/ImageUtils.h:40
(Diff revision 1)
> + * that are only meaningful to the ImageBitmap.

Can this go into a separate patch after ImageBitmapUtils?

Comment 114

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

Please upload a new set of patches after fixing those.

Comment 115

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25537/#review23257

> Should be enabled in non-release builds, right?

Yes, it could. Just not sure when to *enable in non-release build* v.s. *diasble in all builds*?

Comment 116

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25539/#review23259

> Since you're adding this in this patch, don't you also need in this patch to add dynamic checks for all existing functions taking an ImageBitmapSource to reject a BufferSource?

It has been done *implicitly* in the [`ImageBitmap::create()` method](https://dxr.mozilla.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1136) while checking the type of ImageBitmapSource union. I think can do it explicitly and eralier in the `nsGlobalWindow::CreateImageBitmap()` and `WorkerGlobalScope::CreateImageBitmap()`.

> These need to be documented.

OK.

Comment 117

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25541/#review23261

> Do we need this copy constructor? Maybe we can just remove it.

You are right, the copy CTOR is not used now. I will then remove this line.

> Why do we need this constructor? Would be better not to have it if we don't need it.

This one is used in the ChannelPixelLayout::Constructor(), the constructor exposed to JS.

Comment 118

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25543/#review23263

> You may not need the default and copy constructors.

Yes, they are not used and will be removed at next patch.

Comment 119

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25541/#review23261

> This one is used in the ChannelPixelLayout::Constructor(), the constructor exposed to JS.

Sorry, I was wrong. The default CTOR is used ImagePixelLayout::AppendChannel(), which we append a new ChannelPixelLayout object into the ImagePixelLayout's data member, mChannels.

Comment 120

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25545/#review23269

> "for each kind"?

How about "Store the channel counts of each ImageBitmapFormat." 
Would it be more clear?

> "each kind of ImageBitmapFormat" (singular)

OK

> You don't really explain what ImageBitmapFormatUtils is. It's clearly not a singleton ... is there one per ImageBitmapFormat?
> 
> If there's one per ImageBitmapFormat, how do callers get the correct singleton object?

I will then add more comments. 

The ImageBitmapFormatUtils is not designed as a singletion because the ImageBitmapFormatUtils class and its derivered classes do not represent something that is natually single. They are essentially designed to encapsulate properties and behaviors of each ImageBitmapFormat.

> Give this a full name and explain what it does.
> 
> Also explain how the parameters work. Also mention whether it always does a copy, or not.

OK. How about rename it as "CopyAndCovertColor()"? It does always copy the data in the source buffer to the destination buffer and performs color conversion if the source format and the destination format are different.

> Can this go into a separate patch after ImageBitmapUtils?

Yes, it could. I will split them into two patches.

Comment 121

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25537/diff/1-2/

Comment 122

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25539/diff/1-2/

Comment 123

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25541/diff/1-2/

Comment 124

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/1-2/

Comment 125

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25545/diff/1-2/

Comment 126

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

Bug 1141979 - part5 - implement ImageUtils.

Comment 127

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/1-2/

Comment 128

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/1-2/

Comment 129

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/1-2/

Comment 130

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/1-2/

Comment 131

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/1-2/

Comment 132

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/1-2/

Comment 133

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

https://reviewboard.mozilla.org/r/25545/#review23269

> How about "Store the channel counts of each ImageBitmapFormat." 
> Would it be more clear?

ok

> I will then add more comments. 
> 
> The ImageBitmapFormatUtils is not designed as a singletion because the ImageBitmapFormatUtils class and its derivered classes do not represent something that is natually single. They are essentially designed to encapsulate properties and behaviors of each ImageBitmapFormat.

In that case you should say there's one per ImageBitmapFormat.

> OK. How about rename it as "CopyAndCovertColor()"? It does always copy the data in the source buffer to the destination buffer and performs color conversion if the source format and the destination format are different.

OK, call it CopyAndConvertColor and put what you said in comments.

Comment 134

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

Comment on attachment 8686383 [details] [diff] [review]
Part3-Preference.patch

Review of attachment 8686383 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this is obsoleted by the MozReview patches.

Comment 135

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

https://reviewboard.mozilla.org/r/25537/#review23689

Comment 136

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

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

https://reviewboard.mozilla.org/r/25539/#review23697

Needs DOM peer review for WebIDL

::: dom/webidl/ImageBitmap.webidl:118
(Diff revision 2)
> +   * Pixel layout: planar yuv-channels

These format descriptions aren't completely clear and for some of these, they're ambiguous.

It might be better to write out the layout assuming 4 pixels labeled like so:
1 2
3 4
One line per plane.

RGBA32:
R1 G1 B1 A1 R2 G2 B2 A2 R3 G3 B3 A3 R4 G4 B4 A4

BGRA32:
B1 G1 R1 A1 B2 G2 R2 A2 B3 G3 R3 A3 B4 G4 R4 A4

RGB24:
R1 G1 B1 R2 G2 B2 R3 G3 B3 R4 G4 B4

GRAY8:
GRAY1 GRAY2 GRAY3 GRAY4

YUV444P:
Y1 Y2 Y3 Y4
U1 U2 U3 U4
V1 V2 V3 V4

YUV422P:
Y1 Y2 Y3 Y4
U1 U3
V1 V3

YUV420P:
Y1 Y2 Y3 Y4
U1
V1

YUV420SP_NV12
Y1 Y2 Y3 Y4
U1 V1

YUV420SP_NV21
Y1 Y2 Y3 Y4
V1 U1

etc. Or something like that...

Comment 137

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

https://reviewboard.mozilla.org/r/25543/#review23699

Need DOM peer review for this patch and the previous patch.

Comment 138

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

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

https://reviewboard.mozilla.org/r/25545/#review23703

::: dom/canvas/ImageBitmapUtils.h:13
(Diff revision 2)
> +using Layout = ImagePixelLayout;

Why this shortcut? Seems to me we might as well just spell out ImagePixelLayout.

::: dom/canvas/ImageBitmapUtils.h:53
(Diff revision 2)
> +  static ImageBitmapFormatUtils* Create(ImageBitmapFormat aFormat);

So is the intended usage that you call ImageBitmapFormatUtils::Create every time you need an ImageBitmapFormatUtils? And does the resulting object need to be destroyed in any way?

::: dom/canvas/ImageBitmapUtils.h:59
(Diff revision 2)
> +  CreateCustomizedLayout(ImageBitmapFormat aFormat, layers::Image* aImage);

Explain what the role of aImage is here.

::: dom/canvas/ImageBitmapUtils.h:194
(Diff revision 2)
> +  ConvertFrom(ImageBitmapFormatUtils_YUV420P* aSrcFormat, const uint8_t* aSrcBuffer, const Layout* aSrcLayout, uint8_t* aDstBuffer) override;

This double dispatch approach might be a bit confusing. It might be simpler to just have logic in each ConvertTo method that checks the destination type. Not sure...

::: dom/canvas/ImageBitmapUtils.h:227
(Diff revision 2)
> +CopyAndConvertColor(ImageBitmapFormat aSrcFormat,

This interface looks good.

Why "CopyAndConvertColor"? Maybe "CopyAndConvertImageData" would be better... "Color" isn't really the right noun here.

::: dom/canvas/ImageBitmapUtils.h:235
(Diff revision 2)
> +                       const Sequence<ImageBitmapFormat>& aCandidates);

Explain what this does.

Comment 139

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

https://reviewboard.mozilla.org/r/25541/#review23705

::: dom/canvas/ChannelPixelLayout.h:104
(Diff revision 2)
> +  uint32_t mSkip;

Please document these members, especially mOffset and mSkip which aren't obvious.

Comment 140

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25539/#review23697

> These format descriptions aren't completely clear and for some of these, they're ambiguous.
> 
> It might be better to write out the layout assuming 4 pixels labeled like so:
> 1 2
> 3 4
> One line per plane.
> 
> RGBA32:
> R1 G1 B1 A1 R2 G2 B2 A2 R3 G3 B3 A3 R4 G4 B4 A4
> 
> BGRA32:
> B1 G1 R1 A1 B2 G2 R2 A2 B3 G3 R3 A3 B4 G4 R4 A4
> 
> RGB24:
> R1 G1 B1 R2 G2 B2 R3 G3 B3 R4 G4 B4
> 
> GRAY8:
> GRAY1 GRAY2 GRAY3 GRAY4
> 
> YUV444P:
> Y1 Y2 Y3 Y4
> U1 U2 U3 U4
> V1 V2 V3 V4
> 
> YUV422P:
> Y1 Y2 Y3 Y4
> U1 U3
> V1 V3
> 
> YUV420P:
> Y1 Y2 Y3 Y4
> U1
> V1
> 
> YUV420SP_NV12
> Y1 Y2 Y3 Y4
> U1 V1
> 
> YUV420SP_NV21
> Y1 Y2 Y3 Y4
> V1 U1
> 
> etc. Or something like that...

Okay, then I will add a "layout example" section in the comments.

Comment 141

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25539/#review23697

Okay, I will add smaug after updating this patch.

Comment 142

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25541/#review23705

> Please document these members, especially mOffset and mSkip which aren't obvious.

Okay.

Comment 143

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25545/#review23703

> Why this shortcut? Seems to me we might as well just spell out ImagePixelLayout.

Just for fitting some lines into 80-characters..., could be just ImagePixelLayout.

> So is the intended usage that you call ImageBitmapFormatUtils::Create every time you need an ImageBitmapFormatUtils? And does the resulting object need to be destroyed in any way?

Yes, users need to create one every time they need one and destroy it after finishing using it. 
I use an nsAutoPt to manager it when I use it.

> Explain what the role of aImage is here.

So, this fucntion extracs infromation form the aImage parameter to customize the ImagePixelLayout object.
Will add this as a comment.

> This double dispatch approach might be a bit confusing. It might be simpler to just have logic in each ConvertTo method that checks the destination type. Not sure...

Assume that we wrote the logic in each ConvertTo() method, we still write methods like the ConvertFrom() for each type and use a switch-case to dispatch the operations.
I think the advantage of the current approach is hiding the switch-case into the overloading mechanism, which keeps the code clean but need a little bit thought to understand.
So, if you agree, I would like to keep the current implementation.

> This interface looks good.
> 
> Why "CopyAndConvertColor"? Maybe "CopyAndConvertImageData" would be better... "Color" isn't really the right noun here.

Ok.

> Explain what this does.

This function tries to find the best ImageBitmapFormat, from the aCandiates, which can be converted from the aSrcFormat.
The algorithm now merely returns the FIRST one, from the aCandidates, which can be converted from the aSrcFormat.
The algorithm should be updated after we implement optimizations for different platforms (different kinds of layers::Image), considering that some convertion might be cheaper through hardware...
Again, will add this as a comment.

Comment 144

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25543/#review23699

Okay, will add smaug in next iteration with the update of part1.

Comment 145

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

https://reviewboard.mozilla.org/r/25545/#review23831

::: dom/canvas/ImageBitmapUtils.h:53
(Diff revision 2)
> +  static ImageBitmapFormatUtils* Create(ImageBitmapFormat aFormat);

In that case why not just merge this into ImageBitmapFormat?

Comment 146

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

Comment on attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

https://reviewboard.mozilla.org/r/26195/#review23833

::: dom/canvas/ImageUtils.h:30
(Diff revision 1)
> + * (2) GetBufferLength() returns the number of memories that are used to store

"number of bytes"?

::: dom/canvas/ImageUtils.cpp:49
(Diff revision 1)
> +          return ImageBitmapFormat::YUV444P;

Don't we need to check mCbSkip/mCrSkip before we return?

::: dom/canvas/ImageUtils.cpp:58
(Diff revision 1)
> +          if (aData->mCbChannel < aData->mCrChannel) {

I think we need a more precise check here to ensure that they are really interleaved properly.

::: dom/canvas/ImageUtils.cpp:93
(Diff revision 1)
> +        Some(GetImageBitmapFormatFromSurfaceFromat(Surface()->GetFormat()));

This doesn't handle the PlanarCbCr cases that return EndGuard. I think in those cases we need to fall back to Impl.

::: dom/canvas/ImageUtils.cpp:187
(Diff revision 1)
> +    return mFormat.value();

Caching the format in mFormat doesn't seem worth it to me.

::: dom/canvas/ImageUtils.cpp:238
(Diff revision 1)
> +  using IMAGETYPE = layers::CairoImage;

CairoImage (or rather, SourceSurfaceImage!) instead of IMAGETYPE. Similar for other IMAGETYPEs above.

Comment 147

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

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

https://reviewboard.mozilla.org/r/25547/#review23835

::: dom/canvas/ImageBitmap.h:207
(Diff revision 2)
> +  ImageUtils* mDataWrapper;

Shouldn't this be an nsAutoPtr?

::: dom/canvas/ImageBitmap.cpp:1251
(Diff revision 2)
> +// ImageBitmap-exntensions.

"ImageBitmap extensions"

::: dom/canvas/ImageBitmap.cpp:1264
(Diff revision 2)
> +    // ImageBitmapFormat::EndGuard_ and the binding layers returns empty string.

This sounds bad. Our C++ type signature says we return a valid ImageBitmapFormat so we should return one, and not rely on the bindings handling invalid values in a certain way.

In fact it seems to me that we should not return an empty string here. As far as I know, we should always be able to convert any format into any other format, except I guess to/from DEPTH. Maybe we should throw an exception in that case?

::: dom/canvas/ImageBitmap.cpp:1459
(Diff revision 2)
> +                         int32_t aOffset, int32_t aLength, ErrorResult& aRv)

We don't really need callers to pass in aLength, right, since it must match the MappedDataLength we get for the image?

Comment 148

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

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

https://reviewboard.mozilla.org/r/25549/#review23837

::: dom/canvas/ImageBitmap.cpp:1593
(Diff revision 2)
> + * main thread to create a layers::CairoImage from an BufferSource raw data.

I think the ultimate plan is to make CairoImage (i.e. SourceSurfaceImage) constructible off the main thread, right? If so please note that here.

Comment 149

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

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

https://reviewboard.mozilla.org/r/25551/#review23839

Comment 150

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

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

https://reviewboard.mozilla.org/r/25553/#review23841

Comment 151

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

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

https://reviewboard.mozilla.org/r/25555/#review23843

Comment 152

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

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

https://reviewboard.mozilla.org/r/25557/#review23845

::: dom/canvas/test/imagebitmap_extensions.js:78
(Diff revision 2)
> +    promiseThrows(callMappedDataLengthWithUnsupportedFormat("DEPTH"), "[Exception] DEPTH is not supported yet.")

I think we need to support all formats, with conversion as necessary. Or did we decide something else?

::: dom/canvas/test/imagebitmap_extensions.js:163
(Diff revision 2)
> +                  function(ev) { failed(ev); reject(); });                

trailing whitespace

::: dom/canvas/test/test_imagebitmap_extensions_on_worker.html:26
(Diff revision 2)
> +    	  };

here too

Comment 153

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

Right now the implementation here does a copy for every mapInto operation and a copy for every ImageBitmap construction operation. I think it's really important that from the beginning we have at least one path that avoids copies as much as possible, so we know both our implementation structure and the API support this.

With an underlying platform API like gralloc, we should be able to support mapInto read-only with no copies, and creating an ImageBitmap from a fresh ArrayBuffer with no copies. I've lost track of where our discussion ended up on these issues and how we ended up with the current API, but it doesn't seem to support either of those cases.

Comment 154

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25545/#review23831

> In that case why not just merge this into ImageBitmapFormat?

Image Bitmap Format is a WebIDL enumeration whose native implementation is a enum class generated by the binding tools.
Is there a way to map a WebIDL enumeration into other hand-writing implementation?

Comment 155

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25545/#review23831

> Image Bitmap Format is a WebIDL enumeration whose native implementation is a enum class generated by the binding tools.
> Is there a way to map a WebIDL enumeration into other hand-writing implementation?

typo: "Image Bitmap Format" -> ImageBitmapFormat.

Comment 156

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25545/#review23831

> typo: "Image Bitmap Format" -> ImageBitmapFormat.

As we talked on the IRC, I will then use the C++ static initialize to implement singleton for each ImageBitmapFormatUtils_xxx subclass.

Comment 157

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/26195/#review23833

> "number of bytes"?

Yes, thanks.

> Don't we need to check mCbSkip/mCrSkip before we return?

I will modify this function with more precise logic.

> I think we need a more precise check here to ensure that they are really interleaved properly.

Same here, I will modify this function with more precise logic.

> This doesn't handle the PlanarCbCr cases that return EndGuard. I think in those cases we need to fall back to Impl.

I will remove the cache variable here too, so there is no need to handle the EndGuard return value anymore. In default, all layers::Image types that we haven't optimized, we fall all of them back to Impl. Not sure if I got your point?

> Caching the format in mFormat doesn't seem worth it to me.

I was thinking that GetImageBitmapFormatFromPlanarYCbCrData() is somewhat complicate and do not need to go through the logic every time. However, I can remove the cache code.

> CairoImage (or rather, SourceSurfaceImage!) instead of IMAGETYPE. Similar for other IMAGETYPEs above.

Okay.

Comment 158

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25547/#review23835

> Shouldn't this be an nsAutoPtr?

Yes, it should, thanks!

> "ImageBitmap extensions"

Ok.

> This sounds bad. Our C++ type signature says we return a valid ImageBitmapFormat so we should return one, and not rely on the bindings handling invalid values in a certain way.
> 
> In fact it seems to me that we should not return an empty string here. As far as I know, we should always be able to convert any format into any other format, except I guess to/from DEPTH. Maybe we should throw an exception in that case?

Agree, I will then file a spec issue.

> We don't really need callers to pass in aLength, right, since it must match the MappedDataLength we get for the image?

Agree here. However, I think it could be larger or equal to the return value of MappedDataLength() method.

Comment 159

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25549/#review23837

> I think the ultimate plan is to make CairoImage (i.e. SourceSurfaceImage) constructible off the main thread, right? If so please note that here.

Yes and sure. I will also modify this part to not using the ErrorResult object cross-thread. (Bug 1224647)

Comment 160

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25557/#review23845

> I think we need to support all formats, with conversion as necessary. Or did we decide something else?

Yes, we will.
I just want to scope this bug to only BGRA32 and RGBA32. 
I would like to file separate bugs for other formats.

> trailing whitespace

Ok.

> here too

Ok.

Comment 161

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25537/diff/2-3/

Comment 162

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25539/diff/2-3/

Comment 163

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25541/diff/2-3/

Comment 164

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/2-3/

Comment 165

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25545/diff/2-3/

Comment 166

[Tzuhao Kuo [:kaku]]

Comment on attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26195/diff/1-2/

Comment 167

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/2-3/

Comment 168

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/2-3/

Comment 169

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/2-3/

Comment 170

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/2-3/

Comment 171

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/2-3/

Comment 172

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/2-3/

Comment 173

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

https://reviewboard.mozilla.org/r/25539/#review23999

::: dom/webidl/ImageBitmap.webidl:19
(Diff revision 3)
> +// distinguishing argument which means we cannot overload funcions via union

s/funcions/functions/

::: dom/webidl/ImageBitmap.webidl:247
(Diff revision 3)
> +enum DataType {

DataType as enum name sounds too common.
This patch doesn't show where it is going to be used so hard to say anything exact about it

Comment 174

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

https://reviewboard.mozilla.org/r/25539/#review24017

Looks like DataType should be called ChannelPixelLayoutDataType or some such. I know it is a big long, but
DataType is just way too generic.

Comment 175

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

https://reviewboard.mozilla.org/r/25541/#review24019

::: dom/webidl/ImageBitmap.webidl:386
(Diff revision 3)
> + Constructor(unsigned long aOffset, unsigned long aWidth, unsigned long aHeight, DataType aDataType, unsigned long aStride, unsigned long aSkip)]

overlong line. Please put some params to the next line.

::: dom/webidl/ImageBitmap.webidl:387
(Diff revision 3)
> +interface ChannelPixelLayout {

I don't understand ChannelPixelLayout. It looks like a dictionary, even from implementation point of view. Why do we need an interface for this?

Comment 176

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

MozReview is silly and doesn't have a way to r-

Comment 177

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

I think here too using dictionaries and sequences would make both implementation and API simpler.

If you disagree, please explain and ask review again.

Comment 178

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Clearing this request since it depends on what will be done to those interfaces 
(whether or not convert them to dictionaries / sequences)

Comment 179

[Tzuhao Kuo [:kaku]]

(In reply to Olli Pettay [:smaug] from comment #177)
> Comment on attachment 8689113 [details]
> MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc,
> smaug
> 
> I think here too using dictionaries and sequences would make both
> implementation and API simpler.
> 
> If you disagree, please explain and ask review again.
After reading through the WebIDL spec, using dictionary for ChannelPixelLayout and ImagePixelLayout seems reasonable to me. These two structures only contain data members and, according to the WebIDL spec, the dictionary type also support [Constructor] and [Exposed]. 

Although, by this way, I have to expose the implementation of some help methods, such as ImagePixelLayout::AppendChannel(), to the caller side.

BTW, bug1044102 comment17 struck me while I was thing this issue because the ImageBitmap::mapDataInto() method also returns a Promise which will be resolved into an ImagePixelLayout. However, I found that the ToJSValue() accepts a WebIDL dictionary type (https://dxr.mozilla.org/mozilla-central/source/dom/bindings/ToJSValue.h#224), so this is not a problem now. Need more experimental implementation so that I can be more confident.

@roc, @anssi, @ctai, WDYT?

Comment 180

[Olli Pettay [:smaug][[e-mail]]]

So I would assume ImagePixelLayout to become just
sequence<ChannelPixelLayout> (so, not a dictionary) and then on C++ side it is effectively normal 
nsTArray which has Append* etc
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingDeclarations.h?rev=975e8b161839#447

Do we even need constructor?
var channel1 = { offset: 123, width: 456, height: 789, dataType: "some_data_type", stride: 0, skip: 0};
var channel2 = { offset: 123, width: 456, height: 789, dataType: "some_data_type", stride: 0, skip: 0};
var pxLayout = [ channel1, channel2 ];
doesn't look too bad to me - or in other words, what does using constructor give us?

Comment 181

[Tzuhao Kuo [:kaku]]

You are right, we do not need CTORs in this way.

Comment 182

[Tzuhao Kuo [:kaku]]

While trying to add [Exposed=(Window,Worker)] to these dictionaries, I encountered into the following error, is this a tool chain limitation?



 0:12.52 Traceback (most recent call last):
 0:12.52   File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 162, in _run_module_as_main
 0:12.52     "__main__", fname, loader, pkg_name)
 0:12.52   File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
 0:12.52     exec code in run_globals
 0:12.52   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/python/mozbuild/mozbuild/action/webidl.py", line 19, in <module>
 0:12.52     sys.exit(main(sys.argv[1:]))
 0:12.52   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/python/mozbuild/mozbuild/action/webidl.py", line 15, in main
 0:12.52     manager.generate_build_files()
 0:12.52   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/dom/bindings/mozwebidlcodegen/__init__.py", line 245, in generate_build_files
 0:12.52     self._parse_webidl()
 0:12.52   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/dom/bindings/mozwebidlcodegen/__init__.py", line 326, in _parse_webidl
 0:12.52     parser.parse(data, path)
 0:12.52   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/dom/bindings/parser/WebIDL.py", line 6560, in parse
 0:12.52     self._productions.extend(self.parser.parse(lexer=self.lexer, tracking=True))
 0:12.52   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/other-licenses/ply/ply/yacc.py", line 263, in parse
 0:12.52     return self.parseopt(input,lexer,debug,tracking,tokenfunc)
 0:12.53   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/other-licenses/ply/ply/yacc.py", line 710, in parseopt
 0:12.53     p.callable(pslice)
 0:12.53   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/dom/bindings/parser/WebIDL.py", line 5112, in p_Definitions
 0:12.53     p[2].addExtendedAttributes(p[1])
 0:12.53   File "/Volumes/firefoxos/Gecko-hg/mozilla-central/dom/bindings/parser/WebIDL.py", line 1698, in addExtendedAttributes
 0:12.53     assert len(attrs) == 0
 0:12.53 AssertionError
 0:12.57 make[5]: *** [codegen.pp] Error 1
 0:12.57 make[4]: *** [dom/bindings/export] Error 2
 0:12.57 make[4]: *** Waiting for unfinished jobs....
 0:12.90 There are no private exports.
 0:12.91 make[3]: *** [export] Error 2
 0:12.91 make[2]: *** [default] Error 2
 0:12.91 make[1]: *** [realbuild] Error 2
 0:12.91 make: *** [build] Error 2
 0:12.96 288 compiler warnings present.

Comment 183

[Olli Pettay [:smaug][[e-mail]]]

You don't need Exposed with dictionaries.
Just having a dictionary doesn't add anything to global scope (like having normal interfaces does), so
dictionaries just describe (from bindings point of view) how js is converted to a certain C++ struct or
C++ to JS.

Comment 184

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

LGTM for changing to Dictionary.

(In reply to Tzuhao Kuo [:kaku] from comment #179)
> (In reply to Olli Pettay [:smaug] from comment #177)
> > Comment on attachment 8689113 [details]
> > MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc,
> > smaug
> > 
> > I think here too using dictionaries and sequences would make both
> > implementation and API simpler.
> > 
> > If you disagree, please explain and ask review again.
> After reading through the WebIDL spec, using dictionary for
> ChannelPixelLayout and ImagePixelLayout seems reasonable to me. These two
> structures only contain data members and, according to the WebIDL spec, the
> dictionary type also support [Constructor] and [Exposed]. 
> 
> Although, by this way, I have to expose the implementation of some help
> methods, such as ImagePixelLayout::AppendChannel(), to the caller side.
> 
> BTW, bug1044102 comment17 struck me while I was thing this issue because the
> ImageBitmap::mapDataInto() method also returns a Promise which will be
> resolved into an ImagePixelLayout. However, I found that the ToJSValue()
> accepts a WebIDL dictionary type
> (https://dxr.mozilla.org/mozilla-central/source/dom/bindings/ToJSValue.
> h#224), so this is not a problem now. Need more experimental implementation
> so that I can be more confident.
> 
> @roc, @anssi, @ctai, WDYT?

Comment 185

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

(In reply to Tzuhao Kuo [:kaku] from comment #179)
> @roc, @anssi, @ctai, WDYT?

Sure, these changes sound good. Also, can you discuss comment #153?

Comment 186

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #185)
> (In reply to Tzuhao Kuo [:kaku] from comment #179)
> > @roc, @anssi, @ctai, WDYT?
> 
> Sure, these changes sound good. Also, can you discuss comment #153?

Sorry that I completely missed comment #153.

The current API design was driven out by 
(1) the use cases of cooperating with ASM.js applications which needs a copy between the ImageBitmap and the ASM.js run-time heap.
2) keeping the ImageBitmap interface immutable.

At the very beginning, I proposed to extend the ImageBitmap interface with an ArrayBuffer data member, which is a reference to the ImageBitmap object's underlying data. 

partial interface ImageBitmap {
    readonly attribute ArrayBuffer data;
    // plus other fields that describes the _data_.
}

By this way, if an AMS.js-version application would like to access this ImageBitmap, we need to copy the ImageBitmap.data into the ASM.js-verison application's run-time heap.

Two problems here:
(1) With exposing the ImageBitmap's underlying data via an ArrayBuffer, the ImageBitmap then becomes mutable which is not what we want.
(2) The copy operation between the ImageBitmap and the ASM.js run-time heap is expensive. 
    The number was: For a 1920x1080x4(channels) image, it takes about 1.8 ms on a Macbook pro.
                                                                about 33ms on Flame.
                    For a 640x480x4(channels) image, it takes around 5ms on Flame. 

Then, we came out with the draft of the current API design (please see comment #5). The current APIs design still keep one copy operation, however, it keeps the ImageBitmap immutable.

We also discussed the possibility of providing mmap-like API with ASM.js people at the Whistler work week, however, they seems not plain to provide the mmap-like API soon. (Sorry that I cannot recall the specific reason now.)

Back to the comment #153,

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #153)
> With an underlying platform API like gralloc, we should be able to support
> mapInto read-only with no copies, 
Do you mean that mapDataInto() a "real-only buffer" with no copies? If yes, what exactly a "read-only buffer" is in the existing JavaScript or DOM APIs? 

> ... and creating an ImageBitmap from a fresh ArrayBuffer with no copies. 
Indeed, it's possible to create an ImageBitmap form an ArrayBuffer without copies via making the ImageBitmap's underlying data be a reference to the ArrayBuffer's underlying data. However, as far as I can tell, the ArrayBuffer is always writable, then how can we keep the ImageBitmap immutable? Or, the same question above, is there something likes "read-only" buffer? or can we make an ArrayBuffer read-only?

Comment 187

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

Thanks for reminding me about the asm.js heap issue!

(In reply to Tzuhao Kuo [:kaku] from comment #186)
> Indeed, it's possible to create an ImageBitmap form an ArrayBuffer without
> copies via making the ImageBitmap's underlying data be a reference to the
> ArrayBuffer's underlying data. However, as far as I can tell, the
> ArrayBuffer is always writable, then how can we keep the ImageBitmap
> immutable? Or, the same question above, is there something likes "read-only"
> buffer? or can we make an ArrayBuffer read-only?

We can neuter the ArrayBuffer so that its contents are no longer accessible. We do something similar to transfer ArrayBuffers between Workers without copying.

So I think we should have a way to construct an ImageBitmap that consumes the ArrayBuffer (neutering it). But probably that should use a method instead of a constructor, since the other ImageBitmap constructors do not destroy their parameters. So what you have now is fine and we should add the neutering approach later, as a separate method. That method would not take offset/length parameters.

(In reply to Tzuhao Kuo [:kaku] from comment #158)
> > We don't really need callers to pass in aLength, right, since it must match the MappedDataLength we get for the image?
> 
> Agree here. However, I think it could be larger or equal to the return value
> of MappedDataLength() method.

And if it's less, we throw ... I guess I can't think of an approach that's obviously better. OK.

Comment 188

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

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

https://reviewboard.mozilla.org/r/25545/#review25013

::: dom/canvas/ImageBitmapUtils.h:29
(Diff revision 3)
> + * static method.

I think this comment needs to be changed now.

::: dom/canvas/ImageBitmapUtils.h:70
(Diff revision 3)
> +  virtual ~ImageBitmapFormatUtils() = default;

Just write {} instead of = default.

::: dom/canvas/ImageBitmapUtils.h:92
(Diff revision 3)
> +  }

Please document all the public methods!

::: dom/canvas/ImageBitmapUtils.h:229
(Diff revision 3)
> +                          uint32_t aWidth, uint32_t aHeight);

And document these.

Comment 189

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

Comment on attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

https://reviewboard.mozilla.org/r/26195/#review25035

::: dom/canvas/ImageUtils.cpp:64
(Diff revision 2)
> +

Remove these unnecessary blank lines here and above and below

::: dom/canvas/ImageUtils.cpp:125
(Diff revision 2)
> +    return mBufferLength.value();

We don't need to cache mBufferLength.

::: dom/canvas/ImageUtils.cpp:246
(Diff revision 2)
> +// version.

Why does CairoSurfaceImpl even exist at this point?

Comment 190

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

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

https://reviewboard.mozilla.org/r/25547/#review25037

Comment 191

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

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

https://reviewboard.mozilla.org/r/25549/#review25039

Comment 192

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25539/#review24017

I filed an issue on the github to discuss the name. I will use ChannelPixelLayoutDataType for now and modify it later, onece we have further agreement on the naming.

Comment 193

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25545/#review25013

> I think this comment needs to be changed now.

Yes and thank you.

> Just write {} instead of = default.

Okay.

> Please document all the public methods!

Okay and I will move the ImageBitmapFormatUtils class and its subclasses into .cpp file because they are not used directly by others.

> And document these.

Okay.

Comment 194

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/26195/#review25035

> Remove these unnecessary blank lines here and above and below

Okay.

> We don't need to cache mBufferLength.

Okay.

> Why does CairoSurfaceImpl even exist at this point?

Hmm......, it is not needed, will drop it.

Comment 195

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25537/diff/3-4/

Comment 196

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25539/diff/3-4/

Comment 197

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25541/diff/3-4/

Comment 198

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25543/diff/3-4/

Comment 199

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25545/diff/3-4/

Comment 200

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part4.1 - move ImageBitmapFormatUtils and its subclasses into cpp file; r?roc

Review commit: https://reviewboard.mozilla.org/r/28459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28459/

Comment 201

[Tzuhao Kuo [:kaku]]

Comment on attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26195/diff/2-3/

Comment 202

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/3-4/

Comment 203

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/3-4/

Comment 204

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/3-4/

Comment 205

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/3-4/

Comment 206

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/3-4/

Comment 207

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/3-4/

Comment 208

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

https://reviewboard.mozilla.org/r/25539/#review25541

Comment 209

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

https://reviewboard.mozilla.org/r/25541/#review25543

Comment 210

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

https://reviewboard.mozilla.org/r/25543/#review24027

::: dom/webidl/ImageBitmap.webidl:404
(Diff revision 3)
> +interface ImagePixelLayout {

The spec doesn't have ImagePixelLayout interface (it is called ImageFormatPixelLayout), and could it be expressed as a sequence of ChannelPixelLayout dictionaries?

Comment 211

[Olli Pettay [:smaug][[e-mail]]]

/me kicks MozReview.  I didn't add that comment to this review, but somehow MozReview decided to use it here.
Anyhow, that comment is now invalid since there isn't interface at all, just typedef

Comment 212

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

https://reviewboard.mozilla.org/r/25555/#review25547

How stable is the spec? Should we put the new methods under some Pref or Func and enable the relevant pref only in nightlies for now?

Comment 213

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

mozreview really can't deal well more than one reviewer.
Marking this explicitly r- so that whoever reads this bug knows that there is still something to do with this patch (that something is possibly to just clarify how stable the relevant spec is)

Comment 214

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

Please fold patch 4.1 back into patch 4. We don't want to land a patch that creates ImageBitmapUtils followed by another patch that moves much of it elsewhere.

Comment 215

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25555/#review25547

Sorry that I somehow mis-discard the preference tags in webidl while I was re-organizing these patches. The spec is not stable yet (I think) and I will re-upload this patch with the [Func="mozilla::dom::ImageBitmap::PrefEnabled"] on the extened methods of ImageBitmap interface. For extended methods of ImageBitmapFactory, our tool-chain now does not support using different "extended attributes" on overloadded functions, so I check the preference at runtime and throw if needed.

The preference related utilities are implemented in patch0.

Comment 216

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #214)
> Please fold patch 4.1 back into patch 4. We don't want to land a patch that
> creates ImageBitmapUtils followed by another patch that moves much of it
> elsewhere.

Okay.

Comment 217

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25545/diff/4-5/

Comment 218

[Tzuhao Kuo [:kaku]]

Comment on attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26195/diff/3-4/

Comment 219

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25547/diff/4-5/

Comment 220

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25549/diff/4-5/

Comment 221

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25551/diff/4-5/

Comment 222

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25553/diff/4-5/

Comment 223

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25555/diff/4-5/

Comment 224

[Tzuhao Kuo [:kaku]]

Comment on attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25557/diff/4-5/

Comment 225

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

https://reviewboard.mozilla.org/r/25555/#review25665

Not super nice but I don't have better ideas for that overload issue.

Comment 226

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #214)
> Please fold patch 4.1 back into patch 4. We don't want to land a patch that
> creates ImageBitmapUtils followed by another patch that moves much of it
> elsewhere.

Hi roc, 
Patch4 and patch4.1 had been merged into one.
Please review the following two patches, thanks!

For part4 (https://bugzilla.mozilla.org/attachment.cgi?id=8689114), 
please review version3 v.s. version5.(https://reviewboard.mozilla.org/r/25545/diff/3-5/)

For part5 (https://bugzilla.mozilla.org/attachment.cgi?id=8691863), 
please review version2 v.s. version4.(https://reviewboard.mozilla.org/r/26195/diff/2-4/)

Comment 227

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

Comment on attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

https://reviewboard.mozilla.org/r/25545/#review26971

::: dom/canvas/ImageBitmapUtils.h:19
(Diff revision 5)
> +using ImagePixelLayout = nsTArray<ChannelPixelLayout>;

Use "typedef" instead, it's more obvious.

::: dom/canvas/ImageBitmapUtils.h:27
(Diff revision 5)
> +CreateDefaultLayout(ImageBitmapFormat aFormat, uint32_t aWidth, uint32_t aHeight, uint32_t aStride);

Call this CreateDefaultPixelLayout. mozilla::dom::CreateDefaultLayout isn't so clear about what it means (it could refer to CSS layout).

::: dom/canvas/ImageBitmapUtils.h:36
(Diff revision 5)
> +CreateCustomizedLayout(ImageBitmapFormat aFormat, layers::Image* aImage);

CreateCustomizedPixelLayout

Why does this take aFormat? Seems to me that parameter is not needed, and if we leave it out, we avoid potential problems where aFormat doesn't match aImage.

::: dom/canvas/ImageBitmapUtils.h:42
(Diff revision 5)
> +IsSupportedFormat(ImageBitmapFormat aFormat);

IsSupportedImageFormat.

But what does this mean? What formats wouldn't we support? Shouldn't we support all formats?

::: dom/canvas/ImageBitmapUtils.h:55
(Diff revision 5)
> +CalculateNeededBufferSize(ImageBitmapFormat aFormat,

CalculateImageBufferSize

::: dom/canvas/ImageBitmapUtils.h:68
(Diff revision 5)
> + * needed size could be found by the ImageBitmapFormatUtils::NeededBufferSize()

Fix this comment. Callers should check the above function instead.

::: dom/canvas/ImageBitmapUtils.cpp:30
(Diff revision 5)
> +struct DoNotDelete { void operator()(void* p) {} };
> +using UtilsUniquePtr = UniquePtr<ImageBitmapFormatUtils, DoNotDelete>;

Instead of doing this, why not just return a raw pointer to ImageBitmapFormatUtils and overload ImageBitmapFormatUtils::delete with a private declaration but no implementation? Then anyone calling 'delete' will get a compile error.

But since this is not a public interface I don't think we need this kind of protection here.

::: dom/canvas/ImageBitmapUtils.cpp:307
(Diff revision 5)
> +    return nullptr;

When are we going to support the other formats?

If we just land all these patches as-is, then can't people cause crashes just by using unsupported formats?

::: dom/canvas/ImageBitmapUtils.cpp:444
(Diff revision 5)
> +  layers::CairoImage* src = static_cast<layers::CairoImage*> (aImage);

How do we know this is a CairoImage?

::: dom/canvas/ImageBitmapUtils.cpp:608
(Diff revision 5)
> +  return nullptr;

Why are these returning null?

::: dom/canvas/ImageBitmapUtils.cpp:725
(Diff revision 5)
> +    return true;

Why only these formats?

Comment 228

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

Comment on attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

https://reviewboard.mozilla.org/r/26195/#review26973

::: dom/canvas/ImageUtils.h:26
(Diff revision 4)
> +using ImagePixelLayout = nsTArray<ChannelPixelLayout>;

Use typedef

::: dom/canvas/ImageUtils.h:29
(Diff revision 4)
> + * ImageUtils is a wrapper to the layers::Image. It provides three unified

"a wrapper around layers::Image"

::: dom/canvas/ImageUtils.h:45
(Diff revision 4)
> +class ImageUtils

Should this be MOZ_STACK_CLASS?

::: dom/canvas/ImageUtils.h:64
(Diff revision 4)
> +              ImageBitmapFormat aFormat, ErrorResult& aRv) const;

When is the data unmapped? When the ImageUtils destructor runs?

::: dom/canvas/ImageUtils.cpp:36
(Diff revision 4)
> +    return ImageBitmapFormat::EndGuard_;

Why aren't we supporting more surface types here?

::: dom/canvas/ImageUtils.cpp:47
(Diff revision 4)
> +        aData->mCrChannel >= aData->mCbChannel + aData->mCbCrSize.height * aData->mCbCrStride) { // Three planes.

Why do we need these checks? I don't think we do.

Comment 229

[Tzuhao Kuo [:kaku]]

Hi roc, 

Thanks for your review. 
Several comments are related to the unsupported formats. 
We will support all formats in the end, however, I want to scope this bug to the basic structure of the ImageBitmap-extensions and support only BGRA32 and RGBA32. After this bug is landed, I will file separate bugs for other formats and implement them.

WDYT?

Comment 230

[Tzuhao Kuo [:kaku]]

(In reply to Tzuhao Kuo [:kaku] from comment #229)
> Hi roc, 
> 
> Thanks for your review. 
> Several comments are related to the unsupported formats. 
> We will support all formats in the end, however, I want to scope this bug to
> the basic structure of the ImageBitmap-extensions and support only BGRA32
> and RGBA32. After this bug is landed, I will file separate bugs for other
> formats and implement them.
> 
> WDYT?

Add NI flag.

Comment 231

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

Sorry about the delayed review, by the way.

(In reply to Tzuhao Kuo [:kaku] from comment #230)
> (In reply to Tzuhao Kuo [:kaku] from comment #229)
> > Hi roc, 
> > 
> > Thanks for your review. 
> > Several comments are related to the unsupported formats. 
> > We will support all formats in the end, however, I want to scope this bug to
> > the basic structure of the ImageBitmap-extensions and support only BGRA32
> > and RGBA32. After this bug is landed, I will file separate bugs for other
> > formats and implement them.
> > 
> > WDYT?
> 
> Add NI flag.

It's confusing. These patches add partial support so it's not clear which missing parts are bugs or stuff you plan to add later.

I also don't want mozilla-central to get into a state where you can crash just by passing an unsupported format value.

How much more work it is to add support for the other formats? If it's not too much more work, I'd suggest just doing the work here.

Otherwise I'd suggest removing all mention of the unsupported formats from your patches (commenting out the unsupported values from the WebIDL).

Comment 232

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #231)
> Sorry about the delayed review, by the way.
NP, I spent my time on the bug 1235301 (the HTMLMediaElement.SeekToNextFrame() bug).

> How much more work it is to add support for the other formats? If it's not
> too much more work, I'd suggest just doing the work here.
> 
> Otherwise I'd suggest removing all mention of the unsupported formats from
> your patches (commenting out the unsupported values from the WebIDL).
I think I could do it in this bug.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #228)
> Comment on attachment 8691863 [details]
> MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc
> ::: dom/canvas/ImageUtils.h:64
> (Diff revision 4)
> > +              ImageBitmapFormat aFormat, ErrorResult& aRv) const;
> 
> When is the data unmapped? When the ImageUtils destructor runs?
The data is actually copied into the given ArrayBuffer, so we don't need to unmap it.
The decstructor will be called while releasing an ImageBitmap, a.k.a. an ImageBitmap owns an ImageUtils object.
Not sure whether do I answer your question or not?

And, Okay to other comments.

Comment 233

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #228)
> ::: dom/canvas/ImageUtils.h:45
> (Diff revision 4)
> > +class ImageUtils
> 
> Should this be MOZ_STACK_CLASS?
I don't think this must be a MOZ_STACK_CLASS. Actually, the ImageBitmap now holding a nsAutoPtr (maybe I should use UniquePtr) to an ImageUtils, which was new-ed at the ImageBitmap's constructor. May I know why are you suggesting it to be a MOZ_STACK_CLASS?

Comment 234

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

(In reply to Tzuhao Kuo [:kaku] from comment #232)
> The data is actually copied into the given ArrayBuffer, so we don't need to
> unmap it.
> The decstructor will be called while releasing an ImageBitmap, a.k.a. an
> ImageBitmap owns an ImageUtils object.
> Not sure whether do I answer your question or not?

That's fine. Thanks.

(In reply to Tzuhao Kuo [:kaku] from comment #233)
> I don't think this must be a MOZ_STACK_CLASS. Actually, the ImageBitmap now
> holding a nsAutoPtr (maybe I should use UniquePtr) to an ImageUtils, which
> was new-ed at the ImageBitmap's constructor. May I know why are you
> suggesting it to be a MOZ_STACK_CLASS?

You're right, never mind.

Comment 235

[Tzuhao Kuo [:kaku]]

Hi roc, 

I am going to implement other color formats supporting now (sorry for no response for a long time). Before that, I would like to look for some advices about the implementation.

I am going to utilize the libyuv for most conversions between YUV-family and RGB-family. However, the libyuv does not implement all the conversions between any two YUV-format and RGB-fromat. Actually, the libyuv uses YUV420P and BGRA32 as gate ways to convert between two color families, a.k.a. YUV420P could be converted to any format of RGB-family but other YUV-formats cannot be converted directly to the RGB-family; inversely, BGRA32 is able to be converted to any format of YUV-family but the other formats of RGB-family cannot not be converted directly to the YUV-family. 

So, to implement our color conversion, we have following ways:
1) Utilizes the YUV420P and BGRA32 as gate ways to support all conversions between YUV-family and RGB-family. This method utilizes the libyuv most (especially for some SIMD optimization) and it is the simplest way to write our code. But some conversions need a middle-format.

2) Implements the missing conversions of libyuv. This is also doable but I am not sure about where should I write the code. Looks like that the libyuv is forked from Chromium project, can I write new codes in the libyuv source codes in Gecko? Or, should I just write codes under the dom/canvas/?

For the conversions involves HSV and Lab, I will write codes in our dom/canvas directly and I would like to use RGB as a gate way to/from YUV-family.

Comment 236

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

I'm not sure. I think either approach would be OK. How long do you think it will take to write all the missing conversions?

Comment 237

[Tzuhao Kuo [:kaku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #236)
> I'm not sure. I think either approach would be OK. How long do you think it
> will take to write all the missing conversions?
It won't take too much time to write the pure C version (without SIMD optimization), I will say one to two days for the pure color conversion codes and then plus two days for integrating into the ImageBitmap extension and writing test cases.

It will take much longer time to implement SIMD version (fro me who is not familiar with SIMD), but I think we don't need to do the optimization for now, especially that the ImageBitmap-extensions is not yet standardized. However, follow this thought, is it worth writing the missing part but just utilize the libyuv to do the two-pass conversion of approach (1).

Comment 238

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

I'd say just write the C version. We'd need it for the cases where SIMD isn't available, anyway.

Comment 239

[Tzuhao Kuo [:kaku]]

It turns out to be a huge amount of work, sorry for my bad prediction. 
I have finished the conversions of "(RGBA/BGRA/RGB/BGR) <--> (YUV444P/YUV422P/YUV420P/NV12/NV21)".

Keep working on the conversions (1)among different YUV formats, (2) To Gray, (3)To/From HSV and (4) To/From Lab.

Comment 240

[Tzuhao Kuo [:kaku]]

I finally realized that our layers::PlanarYCbCrImage does not support storing data in NV12/NV21 formats. It transform the input NV12/NV21 data into YUV420P format in the PlanarYCbCrImage::SetData() method.

Comment 241

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

https://reviewboard.mozilla.org/r/25547/#review36111

::: dom/canvas/ImageBitmap.h:137
(Diff revision 3)
> +              int32_t aOffset, ErrorResult& aRv);

It seems like this exposes data races to JS as they will be able to observe the data being copied into the array. I'm not sure people will like that. If you use a SharedArrayBuffer that will make it obvious that this can happen.

Comment 242

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41313/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41313/

Comment 243

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r=jrmuizel, r=smaug

Review commit: https://reviewboard.mozilla.org/r/41315/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41315/

Comment 244

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

Review commit: https://reviewboard.mozilla.org/r/41317/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41317/

Comment 245

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

Review commit: https://reviewboard.mozilla.org/r/41319/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41319/

Comment 246

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part4 - Add NVImage; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41321/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41321/

Comment 247

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part5 - Add R8G8B8, B8G8R8, HSV, Lab and Depth into gfx::SurfaceFormat; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41323/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41323/

Comment 248

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part6 - implement ImageBitmapFormatUtils; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41325/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41325/

Comment 249

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part7 - implement ImageUtils; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41327/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41327/

Comment 250

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part8 - implement ImageBitmap extensions; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41329/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41329/

Comment 251

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part9 - implement ImageBitmapFactories extensions; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41331/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41331/

Comment 252

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part10 - hanlde drawing RGB24/BGR24/HSV/Lab onto canvas element; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41333/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41333/

Comment 253

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part11 - handle cases that mapDataInto() should throw; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41335/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41335/

Comment 254

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part12 - export to nsGlobalWindow; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41337/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41337/

Comment 255

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part13 - export to WorkerGlobalScope; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41339/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41339/

Comment 256

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

Review commit: https://reviewboard.mozilla.org/r/41341/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41341/

Comment 257

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part15 - mochitest - basic operations; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41343/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41343/

Comment 258

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part16 - mochitest - color conversion; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41345/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41345/

Comment 259

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part17 - mochitest - draw special color formats onto canvas; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41347/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41347/

Comment 260

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1141979 - part18 - mochitest - cases while calling mapDataInto should throw; r=jrmuizel

Review commit: https://reviewboard.mozilla.org/r/41349/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41349/

Comment 261

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25547/#review36111

> It seems like this exposes data races to JS as they will be able to observe the data being copied into the array. I'm not sure people will like that. If you use a SharedArrayBuffer that will make it obvious that this can happen.

Since this is WebIDL issue, I opened an issue at the W3C github: https://github.com/w3c/mediacapture-worker/issues/45.

Comment 262

[Olli Pettay [:smaug][[e-mail]]]

Trying to get to these patches tomorrow.

Comment 263

[Tzuhao Kuo [:kaku]]

(In reply to Olli Pettay [:smaug] from comment #262)
> Trying to get to these patches tomorrow.

Hi smaug, 

The WebIDL files are not modified except that the "Data type" of HSV and Lab formats are changed from "unsigned char" to "floating point".

However, Jeff mentioned a spec issue which is opened as an issue at the W3C github: https://github.com/w3c/mediacapture-worker/issues/45. May I have your thought on this issue?

Comment 264

[Jeff Muizelaar [:jrmuizel]]

Sorry this review is taking so long, my queue is long. I'll try to get to it this week.

Comment 265

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8732737 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41315/#review39891

::: dom/base/nsGlobalWindow.cpp:14030
(Diff revision 1)
>  already_AddRefed<Promise>
>  nsGlobalWindow::CreateImageBitmap(const ImageBitmapSource& aImage,
>                                    ErrorResult& aRv)
>  {
> +  if (aImage.IsArrayBuffer() || aImage.IsArrayBufferView()) {
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

Could we throw here TypeError, so that we don't expose Gecko specific exceptions to web.
TypeError would be used anyhow if webidl bindings were used for type checking.

::: dom/base/nsGlobalWindow.cpp:14043
(Diff revision 1)
>  nsGlobalWindow::CreateImageBitmap(const ImageBitmapSource& aImage,
>                                    int32_t aSx, int32_t aSy, int32_t aSw, int32_t aSh,
>                                    ErrorResult& aRv)
>  {
> +  if (aImage.IsArrayBuffer() || aImage.IsArrayBufferView()) {
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

ditto

::: dom/workers/WorkerScope.cpp:399
(Diff revision 1)
>  already_AddRefed<Promise>
>  WorkerGlobalScope::CreateImageBitmap(const ImageBitmapSource& aImage,
>                                       ErrorResult& aRv)
>  {
> +  if (aImage.IsArrayBuffer() || aImage.IsArrayBufferView()) {
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

ditto

::: dom/workers/WorkerScope.cpp:412
(Diff revision 1)
>  WorkerGlobalScope::CreateImageBitmap(const ImageBitmapSource& aImage,
>                                       int32_t aSx, int32_t aSy, int32_t aSw, int32_t aSh,
>                                       ErrorResult& aRv)
>  {
> +  if (aImage.IsArrayBuffer() || aImage.IsArrayBufferView()) {
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

ditto.

(perhaps the exception is changing, or removed, in some other patch, but don't know that when reviewing this patch.)

Comment 266

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8732738 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41317/#review39897

Comment 267

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8732739 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41319/#review39903

Comment 268

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8732751 [details]
MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41341/#review39905

::: dom/webidl/ImageBitmap.webidl:428
(Diff revision 1)
>  };
>  
>  typedef sequence<ChannelPixelLayout> ImagePixelLayout;
> +
> +partial interface ImageBitmap {
> +    [Throws, Func="mozilla::dom::ImageBitmap::PrefEnabled"]

Could we call the method something else. Perhaps mozilla::dom::ImageBitmap::ExtensionsEnabled(), since the basic ImageBitmap is enabled by default already.

Comment 269

[Jeff Muizelaar [:jrmuizel]]

Have you looked at code-coverage data for this new code with the tests that you added? It's a lot of code so it would be good if we had solid tests.

Comment 270

[Jeff Muizelaar [:jrmuizel]]

I haven't looked thoroughly, but it seems like things would be simpler if we dropped support for cropping outside of the source area. Have you considered this?

Comment 271

[Tzuhao Kuo [:kaku]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #269)
> Have you looked at code-coverage data for this new code with the tests that
> you added? It's a lot of code so it would be good if we had solid tests.
No, I haven't and I have no idea how to get the data..., is this MDN article (https://developer.mozilla.org/en-US/docs/Measuring_Code_Coverage_on_Firefox) still valid?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #270)
> I haven't looked thoroughly, but it seems like things would be simpler if we
> dropped support for cropping outside of the source area. Have you considered
> this?
Yes, it would be much simpler. However, the extension API will then have different behavior with the existing API, so I decided to follow the original behavior. I am not sure if it is ok to have different behavior.

Comment 272

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/25535/#review41121

::: dom/canvas/ImageBitmap.h:256
(Diff revision 6)
>    /*
> +   * This is used in the ImageBitmap-Extensions implementation.
> +   * ImageUtils is a wrapper to layers::Image, which add some common methods for
> +   * accessing the layers::Image's data.
> +   */
> +  nsAutoPtr<ImageUtils> mDataWrapper;

Use UniquePtr instead of nsAutoPtr

::: dom/canvas/ImageBitmap.cpp:1881
(Diff revision 6)
> +    const int32_t srcStride = channels[0].mStride;
> +    const IntSize srcSize(channels[0].mWidth, channels[0].mHeight);
> +
> +    // Wrap the source buffer into a SourceSurface.
> +    RefPtr<DataSourceSurface> srcDataSurface =
> +      Factory::CreateWrappingDataSourceSurface(const_cast<uint8_t*>(aBufferData),

Since you're just accessing aBufferData you shouldn't need to create a WrappingDataSourceSurface and can just copy them in to dst surface directly instead of mapping the srcDataSurface.

Comment 273

[Tzuhao Kuo [:kaku]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #269)
> Have you looked at code-coverage data for this new code with the tests that
> you added? It's a lot of code so it would be good if we had solid tests.

Here is the code-coverage data (line coverage / function coverage):
ImageBitmap.cpp : 85.4% / 85.3 %
ImageBitmapColorUtils.cpp : 82.0% / 85.8 %
ImageBitmapUtils.cpp : 93.3% / 90.9 % 	
ImageUtils.cpp : 98.8% / 100.0 %

Detail information: http://people.mozilla.org/~tkuo/coverage_imagebitmap/dom/canvas/index.html

The data above comes from running all ImageBitmap-related mochitests under dom/canvas/test/.

Comment 274

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25535/#review41121

Have you finished a review phase?
Should I address these two items and upload new patches now?
Or, should I wait for other reviews?

Comment 275

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/41341/#review39905

> Could we call the method something else. Perhaps mozilla::dom::ImageBitmap::ExtensionsEnabled(), since the basic ImageBitmap is enabled by default already.

Sure. I will change it.

Comment 276

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/41315/#review39891

> Could we throw here TypeError, so that we don't expose Gecko specific exceptions to web.
> TypeError would be used anyhow if webidl bindings were used for type checking.

Ok. I will then use TypeError. Thanks!

Comment 277

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tzuhao Kuo [:kaku] from comment #273)
> Here is the code-coverage data (line coverage / function coverage):
> ImageBitmap.cpp : 85.4% / 85.3 %
> ImageBitmapColorUtils.cpp : 82.0% / 85.8 %
> ImageBitmapUtils.cpp : 93.3% / 90.9 % 	
> ImageUtils.cpp : 98.8% / 100.0 %

Great. Is it possible to get test coverage for the remaining functions in ImageBitmapColorUtils.cpp?


(In reply to Tzuhao Kuo [:kaku] from comment #274)
> https://reviewboard.mozilla.org/r/25535/#review41121
> 
> Have you finished a review phase?
> Should I address these two items and upload new patches now?
> Or, should I wait for other reviews?

I haven't really finished, but it's fine to upload new patches. Sorry the review is taking so long...

Comment 278

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tzuhao Kuo [:kaku] from comment #271)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #269)
> > Have you looked at code-coverage data for this new code with the tests that
> > you added? It's a lot of code so it would be good if we had solid tests.
> No, I haven't and I have no idea how to get the data..., is this MDN article
> (https://developer.mozilla.org/en-US/docs/
> Measuring_Code_Coverage_on_Firefox) still valid?
> 
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #270)
> > I haven't looked thoroughly, but it seems like things would be simpler if we
> > dropped support for cropping outside of the source area. Have you considered
> > this?
> Yes, it would be much simpler. However, the extension API will then have
> different behavior with the existing API, so I decided to follow the
> original behavior. I am not sure if it is ok to have different behavior.

I agree that would be somewhat weird. What if we dropped the cropping variant from the API? It seems like there should be no need for it especially if we provide the ability to specify a stride.

Comment 279

[Tzuhao Kuo [:kaku]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #277)
> Great. Is it possible to get test coverage for the remaining functions in
> ImageBitmapColorUtils.cpp?
"The remaining functions" are those functions which are actually also implemented by libyuv with SIMD optimization. In the ImageBitmapUitls.cpp, the client of ImageBitmapColorUtils, I choose to use libyuv's implementation and that is why mochitest does not hit "the remaining functions".

Hoever, the ImageBitmapColorUtils.cpp is actually full-covered by gtest (under ./dom/canvas/gtest/TestImageBitmapColorUtils.cpp). The coverage is 99.8% / 100%.
Please refer to the updated data: http://people.mozilla.org/~tkuo/coverage_imagebitmap/dom/canvas/index.html

> I haven't really finished, but it's fine to upload new patches. Sorry the
> review is taking so long...
Sure, I will update it. And it's ok, let me know if you need any information.

Comment 280

[Tzuhao Kuo [:kaku]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #278)
> I agree that would be somewhat weird. What if we dropped the cropping
> variant from the API? It seems like there should be no need for it
> especially if we provide the ability to specify a stride.
Do you mean just get rid of the 2nd overloaded function in the ImageBitmapFactories extensions (http://w3c.github.io/mediacapture-worker/#imagebitmapfactories-interface-extensions)?

Comment 281

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tzuhao Kuo [:kaku] from comment #280)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #278)
> > I agree that would be somewhat weird. What if we dropped the cropping
> > variant from the API? It seems like there should be no need for it
> > especially if we provide the ability to specify a stride.
> Do you mean just get rid of the 2nd overloaded function in the
> ImageBitmapFactories extensions
> (http://w3c.github.io/mediacapture-worker/#imagebitmapfactories-interface-
> extensions)?

Yes.

Comment 282

[Tzuhao Kuo [:kaku]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #281)
> (In reply to Tzuhao Kuo [:kaku] from comment #280)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #278)
> > > I agree that would be somewhat weird. What if we dropped the cropping
> > > variant from the API? It seems like there should be no need for it
> > > especially if we provide the ability to specify a stride.
> > Do you mean just get rid of the 2nd overloaded function in the
> > ImageBitmapFactories extensions
> > (http://w3c.github.io/mediacapture-worker/#imagebitmapfactories-interface-
> > extensions)?
> 
> Yes.

I think this is good. 
Actually, we have filed two exceptions to handle the cropping of buffers:
Issue 43: https://github.com/w3c/mediacapture-worker/issues/43
Issue 46: https://github.com/w3c/mediacapture-worker/issues/46

By dropping the cropping variant, these two exceptions would be no longer needed.
I will then remove the cropping-related codes and then single you for reviewing, ok?

Comment 283

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tzuhao Kuo [:kaku] from comment #282)
> I will then remove the cropping-related codes and then single you for
> reviewing, ok?

Sounds great.

Comment 284

[Tzuhao Kuo [:kaku]]

(In reply to Tzuhao Kuo [:kaku] from comment #282)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #281)
> > (In reply to Tzuhao Kuo [:kaku] from comment #280)
> > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #278)
> > > > I agree that would be somewhat weird. What if we dropped the cropping
> > > > variant from the API? It seems like there should be no need for it
> > > > especially if we provide the ability to specify a stride.
> > > Do you mean just get rid of the 2nd overloaded function in the
> > > ImageBitmapFactories extensions
> > > (http://w3c.github.io/mediacapture-worker/#imagebitmapfactories-interface-
> > > extensions)?
> > 
> > Yes.
> 
> I think this is good. 
> Actually, we have filed two exceptions to handle the cropping of buffers:
> Issue 43: https://github.com/w3c/mediacapture-worker/issues/43
> Issue 46: https://github.com/w3c/mediacapture-worker/issues/46
> 
> By dropping the cropping variant, these two exceptions would be no longer
> needed.
> I will then remove the cropping-related codes and then single you for
> reviewing, ok?

I filed an github issue on this discussion at https://github.com/w3c/mediacapture-worker/issues/48.
@samug, may I have your thought on this?

Comment 285

[Olli Pettay [:smaug][[e-mail]]]

Commented in github. Defining first just one method in the spec and implementing it and shipping it sounds ok to me, and then if people see need for the other, more complicated version, that can be added later.

Comment 286

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732736 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41313/diff/1-2/

Comment 287

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732737 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41315/diff/1-2/

Comment 288

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732738 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41317/diff/1-2/

Comment 289

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732739 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41319/diff/1-2/

Comment 290

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732741 [details]
MozReview Request: Bug 1141979 - part4 - Add NVImage; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41321/diff/1-2/

Comment 291

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732742 [details]
MozReview Request: Bug 1141979 - part5 - Add R8G8B8, B8G8R8, HSV, Lab and Depth into gfx::SurfaceFormat; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41323/diff/1-2/

Comment 292

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732743 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmapFormatUtils; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41325/diff/1-2/

Comment 293

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732744 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageUtils; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41327/diff/1-2/

Comment 294

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732745 [details]
MozReview Request: Bug 1141979 - part8 - implement ImageBitmap extensions; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41329/diff/1-2/

Comment 295

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732746 [details]
MozReview Request: Bug 1141979 - part9 - implement ImageBitmapFactories extensions; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41331/diff/1-2/

Comment 296

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732747 [details]
MozReview Request: Bug 1141979 - part10 - hanlde drawing RGB24/BGR24/HSV/Lab onto canvas element; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41333/diff/1-2/

Comment 297

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732748 [details]
MozReview Request: Bug 1141979 - part11 - handle cases that mapDataInto() should throw; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41335/diff/1-2/

Comment 298

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732749 [details]
MozReview Request: Bug 1141979 - part12 - export to nsGlobalWindow; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41337/diff/1-2/

Comment 299

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732750 [details]
MozReview Request: Bug 1141979 - part13 - export to WorkerGlobalScope; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41339/diff/1-2/

Comment 300

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732751 [details]
MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41341/diff/1-2/

Comment 301

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732752 [details]
MozReview Request: Bug 1141979 - part15 - mochitest - basic operations; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41343/diff/1-2/

Comment 302

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732753 [details]
MozReview Request: Bug 1141979 - part16 - mochitest - color conversion; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41345/diff/1-2/

Comment 303

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732754 [details]
MozReview Request: Bug 1141979 - part17 - mochitest - draw special color formats onto canvas; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41347/diff/1-2/

Comment 304

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732755 [details]
MozReview Request: Bug 1141979 - part18 - mochitest - cases while calling mapDataInto should throw; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41349/diff/1-2/

Comment 305

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/25535/#review46153

::: dom/canvas/ImageBitmap.h:280
(Diff revision 7)
> +   * source rectangle so that it contains any transparent black pixels (cropping
> +   * area is outside of the source image).
> +   * This is used in mapDataInto() to check if we should reject promise with
> +   * IndexSizeError.
> +   */
> +  bool mIsCroppingAreaOutSideOfSourceImage;

Can we just compute this from mPictureRect when we need it?

Comment 306

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/25535/#review46153

> Can we just compute this from mPictureRect when we need it?

It is not ok since the *mData* is not always the original source data and the *mPictureRect* is not always the original cropping area, these two variables are optimized out while creating from (1)an ImageData, (2)a Blob or (3)an canvas element with WebGL rendering context.

While creating an ImageBitmap from the above three cases **with cropping requst**, we crop the source data immediately and the mPictureRect is the size of the final cropped data. It was implemented as this way for optimization (please refere to the 4th part of bug 1044102 comment 210). So, we cannot always compute the "mIsCroppingAreaOutSideOfSourceImage" from "mPictureRect" in these cases.

Comment 307

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tzuhao Kuo [:kaku] from comment #306)
> https://reviewboard.mozilla.org/r/25535/#review46153
> 
> > Can we just compute this from mPictureRect when we need it?
> 
> It is not ok since the *mData* is not always the original source data and
> the *mPictureRect* is not always the original cropping area, these two
> variables are optimized out while creating from (1)an ImageData, (2)a Blob
> or (3)an canvas element with WebGL rendering context.
> 
> While creating an ImageBitmap from the above three cases **with cropping
> requst**, we crop the source data immediately and the mPictureRect is the
> size of the final cropped data. It was implemented as this way for
> optimization (please refere to the 4th part of bug 1044102 comment 210). So,
> we cannot always compute the "mIsCroppingAreaOutSideOfSourceImage" from
> "mPictureRect" in these cases.

Ok, I understand this now. I think it might be worth considering making map() just work with cropping outside of the source image, but I'm not sure.

Overall, the patchset seems reasonable. I'll try to give it a finer look tomorrow and then we can finally land this and make any necessary adjustments in tree.

Comment 308

[Tzuhao Kuo [:kaku]]

Thanks a lot, Jeff!

Comment 309

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732736 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=jrmuizel

https://reviewboard.mozilla.org/r/41313/#review52566

Comment 310

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732737 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41315/#review52568

Comment 311

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732738 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41317/#review52570

Comment 312

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732739 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41319/#review52572

Comment 313

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732741 [details]
MozReview Request: Bug 1141979 - part4 - Add NVImage; r=jrmuizel

https://reviewboard.mozilla.org/r/41321/#review52574

Comment 314

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732742 [details]
MozReview Request: Bug 1141979 - part5 - Add R8G8B8, B8G8R8, HSV, Lab and Depth into gfx::SurfaceFormat; r=jrmuizel

https://reviewboard.mozilla.org/r/41323/#review52576

Comment 315

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/41323/#review52580

Comment 316

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732743 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmapFormatUtils; r=jrmuizel

https://reviewboard.mozilla.org/r/41325/#review52582

Comment 317

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732744 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageUtils; r=jrmuizel

https://reviewboard.mozilla.org/r/41327/#review52584

Comment 318

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732745 [details]
MozReview Request: Bug 1141979 - part8 - implement ImageBitmap extensions; r=jrmuizel

https://reviewboard.mozilla.org/r/41329/#review52586

Comment 319

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732746 [details]
MozReview Request: Bug 1141979 - part9 - implement ImageBitmapFactories extensions; r=jrmuizel

https://reviewboard.mozilla.org/r/41331/#review52588

Comment 320

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732747 [details]
MozReview Request: Bug 1141979 - part10 - hanlde drawing RGB24/BGR24/HSV/Lab onto canvas element; r=jrmuizel

https://reviewboard.mozilla.org/r/41333/#review52590

Comment 321

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732748 [details]
MozReview Request: Bug 1141979 - part11 - handle cases that mapDataInto() should throw; r=jrmuizel

https://reviewboard.mozilla.org/r/41335/#review52592

Comment 322

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732749 [details]
MozReview Request: Bug 1141979 - part12 - export to nsGlobalWindow; r=jrmuizel

https://reviewboard.mozilla.org/r/41337/#review52594

Comment 323

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732750 [details]
MozReview Request: Bug 1141979 - part13 - export to WorkerGlobalScope; r=jrmuizel

https://reviewboard.mozilla.org/r/41339/#review52596

Comment 324

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732751 [details]
MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41341/#review52598

Comment 325

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732752 [details]
MozReview Request: Bug 1141979 - part15 - mochitest - basic operations; r=jrmuizel

https://reviewboard.mozilla.org/r/41343/#review52600

Comment 326

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732754 [details]
MozReview Request: Bug 1141979 - part17 - mochitest - draw special color formats onto canvas; r=jrmuizel

https://reviewboard.mozilla.org/r/41347/#review52602

Comment 327

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732751 [details]
MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41341/#review52606

Comment 328

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732753 [details]
MozReview Request: Bug 1141979 - part16 - mochitest - color conversion; r=jrmuizel

https://reviewboard.mozilla.org/r/41345/#review52608

Comment 329

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/41347/#review52610

Comment 330

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732755 [details]
MozReview Request: Bug 1141979 - part18 - mochitest - cases while calling mapDataInto should throw; r=jrmuizel

https://reviewboard.mozilla.org/r/41349/#review52612

Comment 331

[Tzuhao Kuo [:kaku]]

Jeff, thanks for the review.

This bug also depends on the bug 1255688 which fix the TypeFromSize() utility function to support odd width/height. Could you please also review bug 1255688?

BTW, I am going to rebase the patch set and may ask your review again if there are big changes.

Comment 332

[Tzuhao Kuo [:kaku]]

Rebasing is smooth, only bug 1276411 was found.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=051999fc0575

Comment 333

[Tzuhao Kuo [:kaku]]

Two problems found in the try:
(1) Compile error/warning in ImageBitmapColorUtils.cpp, this should be easy to fix.
    (a) truncation from 'double' to 'float'
    (b) bad implicit conversion constructor for 'Image'
(2) Test cases fail in Android, this one needs more investigation.

Comment 334

[Tzuhao Kuo [:kaku]]

(In reply to Tzuhao Kuo [:kaku] from comment #333)
> Two problems found in the try:
> (1) Compile error/warning in ImageBitmapColorUtils.cpp, this should be easy
> to fix.
>     (a) truncation from 'double' to 'float'
>     (b) bad implicit conversion constructor for 'Image'
This is fixed.

> (2) Test cases fail in Android, this one needs more investigation.
It seems that libyuv::I444ToARGB() does not work well on the Android platform, I instead replace this function with our own implementation YUV444PToBGRA32() in ImageBitmapColorUtils.h.

While converting amoung YUV-family formats in the Android platform, libyuv also behaves slightly differently to the other desktop platforms; however, the difference in this case is quite small which is one or two pixel values. I would instead enlarge the tolerance in these test case to work around.

The new try, based on the above mentioned modification, looks good!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0448af80275f

I am now going to wait for the landing of bug 1276411 and bug 1255688, then we can rebase and these patches.

Comment 335

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732736 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41313/diff/2-3/

Comment 336

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732737 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41315/diff/2-3/

Comment 337

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732738 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41317/diff/2-3/

Comment 338

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732739 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41319/diff/2-3/

Comment 339

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732741 [details]
MozReview Request: Bug 1141979 - part4 - Add NVImage; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41321/diff/2-3/

Comment 340

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732742 [details]
MozReview Request: Bug 1141979 - part5 - Add R8G8B8, B8G8R8, HSV, Lab and Depth into gfx::SurfaceFormat; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41323/diff/2-3/

Comment 341

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732743 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmapFormatUtils; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41325/diff/2-3/

Comment 342

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732744 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageUtils; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41327/diff/2-3/

Comment 343

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732745 [details]
MozReview Request: Bug 1141979 - part8 - implement ImageBitmap extensions; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41329/diff/2-3/

Comment 344

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732746 [details]
MozReview Request: Bug 1141979 - part9 - implement ImageBitmapFactories extensions; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41331/diff/2-3/

Comment 345

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732747 [details]
MozReview Request: Bug 1141979 - part10 - hanlde drawing RGB24/BGR24/HSV/Lab onto canvas element; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41333/diff/2-3/

Comment 346

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732748 [details]
MozReview Request: Bug 1141979 - part11 - handle cases that mapDataInto() should throw; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41335/diff/2-3/

Comment 347

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732749 [details]
MozReview Request: Bug 1141979 - part12 - export to nsGlobalWindow; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41337/diff/2-3/

Comment 348

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732750 [details]
MozReview Request: Bug 1141979 - part13 - export to WorkerGlobalScope; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41339/diff/2-3/

Comment 349

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732751 [details]
MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41341/diff/2-3/

Comment 350

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732752 [details]
MozReview Request: Bug 1141979 - part15 - mochitest - basic operations; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41343/diff/2-3/

Comment 351

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732753 [details]
MozReview Request: Bug 1141979 - part16 - mochitest - color conversion; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41345/diff/2-3/

Comment 352

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732754 [details]
MozReview Request: Bug 1141979 - part17 - mochitest - draw special color formats onto canvas; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41347/diff/2-3/

Comment 353

[Tzuhao Kuo [:kaku]]

Comment on attachment 8732755 [details]
MozReview Request: Bug 1141979 - part18 - mochitest - cases while calling mapDataInto should throw; r=jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41349/diff/2-3/

Comment 354

[Tzuhao Kuo [:kaku]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=754cd24755b3&selectedJob=21801210

Thanks for the review!

Comment 355

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da10e075f516
part0 - setup preference utilities; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/347754ae7af6
part1 - WebIDL for native implementation; r=jrmuizel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0be727802ec7
part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7efdd144b8db
part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2212812dce1a
part4 - Add NVImage; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee4b94da77fa
part5 - Add R8G8B8, B8G8R8, HSV, Lab and Depth into gfx::SurfaceFormat; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c8fafec1d9
part6 - implement ImageBitmapFormatUtils; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac2b72200dd
part7 - implement ImageUtils; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/88751aaefb92
part8 - implement ImageBitmap extensions; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/0282afe01d9e
part9 - implement ImageBitmapFactories extensions; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/88544efdd4bd
part10 - hanlde drawing RGB24/BGR24/HSV/Lab onto canvas element; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6887901264
part11 - handle cases that mapDataInto() should throw; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/603da76d89e7
part12 - export to nsGlobalWindow; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e194ddd9ae5f
part13 - export to WorkerGlobalScope; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2062a9112b
part14 - WebIDL for bindings; r=jrmuizel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e46ee796c1d5
part15 - mochitest - basic operations; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a8bb1e52cd
part16 - mochitest - color conversion; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e661c7c4e07
part17 - mochitest - draw special color formats onto canvas; r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/df4c70f4b9f6
part18 - mochitest - cases while calling mapDataInto should throw; r=jrmuizel

Comment 356

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da10e075f516
https://hg.mozilla.org/mozilla-central/rev/347754ae7af6
https://hg.mozilla.org/mozilla-central/rev/0be727802ec7
https://hg.mozilla.org/mozilla-central/rev/7efdd144b8db
https://hg.mozilla.org/mozilla-central/rev/2212812dce1a
https://hg.mozilla.org/mozilla-central/rev/ee4b94da77fa
https://hg.mozilla.org/mozilla-central/rev/f6c8fafec1d9
https://hg.mozilla.org/mozilla-central/rev/7ac2b72200dd
https://hg.mozilla.org/mozilla-central/rev/88751aaefb92
https://hg.mozilla.org/mozilla-central/rev/0282afe01d9e
https://hg.mozilla.org/mozilla-central/rev/88544efdd4bd
https://hg.mozilla.org/mozilla-central/rev/3d6887901264
https://hg.mozilla.org/mozilla-central/rev/603da76d89e7
https://hg.mozilla.org/mozilla-central/rev/e194ddd9ae5f
https://hg.mozilla.org/mozilla-central/rev/4d2062a9112b
https://hg.mozilla.org/mozilla-central/rev/e46ee796c1d5
https://hg.mozilla.org/mozilla-central/rev/e4a8bb1e52cd
https://hg.mozilla.org/mozilla-central/rev/2e661c7c4e07
https://hg.mozilla.org/mozilla-central/rev/df4c70f4b9f6

Comment 357

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da10e075f516
https://hg.mozilla.org/mozilla-central/rev/347754ae7af6
https://hg.mozilla.org/mozilla-central/rev/0be727802ec7
https://hg.mozilla.org/mozilla-central/rev/7efdd144b8db
https://hg.mozilla.org/mozilla-central/rev/2212812dce1a
https://hg.mozilla.org/mozilla-central/rev/ee4b94da77fa
https://hg.mozilla.org/mozilla-central/rev/f6c8fafec1d9
https://hg.mozilla.org/mozilla-central/rev/7ac2b72200dd
https://hg.mozilla.org/mozilla-central/rev/88751aaefb92
https://hg.mozilla.org/mozilla-central/rev/0282afe01d9e
https://hg.mozilla.org/mozilla-central/rev/88544efdd4bd
https://hg.mozilla.org/mozilla-central/rev/3d6887901264
https://hg.mozilla.org/mozilla-central/rev/603da76d89e7
https://hg.mozilla.org/mozilla-central/rev/e194ddd9ae5f
https://hg.mozilla.org/mozilla-central/rev/4d2062a9112b
https://hg.mozilla.org/mozilla-central/rev/e46ee796c1d5
https://hg.mozilla.org/mozilla-central/rev/e4a8bb1e52cd
https://hg.mozilla.org/mozilla-central/rev/2e661c7c4e07
https://hg.mozilla.org/mozilla-central/rev/df4c70f4b9f6