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

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(24 attachments, 26 obsolete attachments)

5.28 KB, text/plain
Details
4.66 KB, patch
Details | Diff | Splinter Review
44.29 KB, patch
Details | Diff | Splinter Review
83.28 KB, patch
Details | Diff | Splinter Review
10.22 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details | Review
(Assignee)

Description

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

Comment 2

3 years ago
(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?
Sure.  I assume drawing the ImageBitmap to a 2d canvas and then using getImageData feels like too roundabout a way of doing things?
(Assignee)

Comment 4

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

Updated

3 years ago
Depends on: 1044102
Summary: [FoyEye] Extend ImageBitmap with interfaces to access its underlying image data → [FoxEye] Extend ImageBitmap with interfaces to access its underlying image data
(Assignee)

Comment 5

2 years ago
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?
Flags: needinfo?(roc)
(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.
(Assignee)

Comment 7

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

Comment 8

2 years ago
Created attachment 8586034 [details] [diff] [review]
bug1044102_imagebitmap_implementation.patch

Patches of this bug depends on the ImageBitmap implementation.
(Assignee)

Comment 9

2 years ago
Created attachment 8586037 [details] [diff] [review]
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?
(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.
Flags: needinfo?(roc)
(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.
(Assignee)

Comment 12

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

Comment 13

2 years ago
Created attachment 8588945 [details] [diff] [review]
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.
Attachment #8586037 - Attachment is obsolete: true
Flags: needinfo?(roc)
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 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.
Flags: needinfo?(roc)
(Assignee)

Comment 16

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

Comment 17

2 years ago
Created attachment 8591464 [details] [diff] [review]
WebIDL-draft.patch
Attachment #8588945 - Attachment is obsolete: true
Attachment #8591464 - Flags: feedback?(roc)
(Assignee)

Comment 18

2 years ago
Created attachment 8591520 [details] [diff] [review]
WebIDL-draft.patch

Remove spaces and correct typos.
Attachment #8591464 - Attachment is obsolete: true
Attachment #8591464 - Flags: feedback?(roc)
Attachment #8591520 - Flags: feedback?(roc)
Blocks: 1100203
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.
Attachment #8591520 - Flags: feedback?(roc)
Attachment #8591520 - Flags: feedback?(jmuizelaar)
Attachment #8591520 - Flags: feedback+
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.
(Assignee)

Comment 21

2 years ago
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 22

2 years ago
(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.
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(roc)
Blocks: 1108950
(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.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 25

2 years ago
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.
Flags: needinfo?(roc)
Flags: needinfo?(jmuizelaar)
> 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?
(Assignee)

Comment 27

2 years ago
(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.
(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.
> 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.
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 31

2 years ago
(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.
I would prefer a new CTOR.
Flags: needinfo?(jmuizelaar)

Updated

2 years ago
Blocks: 1187812
(Assignee)

Updated

2 years ago
Assignee: nobody → tkuo
(Assignee)

Comment 33

2 years ago
Created attachment 8655272 [details] [diff] [review]
[WIP] Part0-WebIDL.patch

WIP.
Attachment #8586034 - Attachment is obsolete: true
Attachment #8591520 - Attachment is obsolete: true
Attachment #8591520 - Flags: feedback?(jmuizelaar)
(Assignee)

Comment 34

2 years ago
Created attachment 8655273 [details] [diff] [review]
[WIP] Part1-Test-cases.patch
(Assignee)

Comment 35

2 years ago
Created attachment 8655274 [details] [diff] [review]
[WIP] Part2-Implementation.patch

WIP.

Only support the RGBA/YUV color spaces now.
(Assignee)

Comment 36

2 years ago
Created attachment 8661139 [details] [diff] [review]
[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.
Attachment #8655274 - Attachment is obsolete: true
Flags: needinfo?(ctai)
Flags: needinfo?(cku)
Attachment #8661139 - Flags: feedback?(ctai)
Attachment #8661139 - Flags: feedback?(cku)
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 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?

Updated

2 years ago
Attachment #8661139 - Flags: feedback?(cku)

Updated

2 years ago
Flags: needinfo?(cku)
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 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 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
Attachment #8661139 - Flags: feedback?(ctai)
Flags: needinfo?(ctai)
(Assignee)

Comment 42

2 years ago
Created attachment 8685396 [details] [diff] [review]
Part0-WebIDL.patch
Attachment #8655272 - Attachment is obsolete: true
Attachment #8685396 - Flags: review?(roc)
(Assignee)

Comment 43

2 years ago
Created attachment 8685397 [details] [diff] [review]
Part1-Test-cases.patch
Attachment #8655273 - Attachment is obsolete: true
Attachment #8685397 - Flags: review?(roc)
(Assignee)

Comment 44

2 years ago
Created attachment 8685399 [details] [diff] [review]
Part2-Implementation.patch
Attachment #8661139 - Attachment is obsolete: true
Attachment #8685399 - Flags: review?(roc)
(Assignee)

Comment 45

2 years ago
Created attachment 8685400 [details] [diff] [review]
Part3-Preference.patch
Attachment #8685400 - Flags: review?(roc)
(Assignee)

Comment 46

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

Comment 47

2 years ago
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.
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 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?
Attachment #8685396 - Flags: review?(roc)
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....
(Assignee)

Comment 51

2 years ago
Created attachment 8685805 [details]
error.txt
(Assignee)

Comment 52

2 years ago
(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?
Flags: needinfo?(roc)
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 54

2 years ago
(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?
Flags: needinfo?(roc)
(Assignee)

Comment 55

2 years ago
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.
(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.
Flags: needinfo?(roc)
> 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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 58

2 years ago
Created attachment 8686376 [details] [diff] [review]
Part0-WebIDL.patch
Attachment #8685396 - Attachment is obsolete: true
Attachment #8686376 - Flags: review?(roc)
(Assignee)

Comment 59

2 years ago
Created attachment 8686377 [details] [diff] [review]
Part1-Test-cases.patch
Attachment #8685397 - Attachment is obsolete: true
Attachment #8685397 - Flags: review?(roc)
Attachment #8686377 - Flags: review?(roc)
(Assignee)

Comment 60

2 years ago
Created attachment 8686382 [details] [diff] [review]
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.
Attachment #8685399 - Attachment is obsolete: true
Attachment #8685399 - Flags: review?(roc)
Attachment #8686382 - Flags: review?(roc)
(Assignee)

Comment 61

2 years ago
Created attachment 8686383 [details] [diff] [review]
Part3-Preference.patch
Attachment #8685400 - Attachment is obsolete: true
Attachment #8685400 - Flags: review?(roc)
Attachment #8686383 - Flags: review?(roc)
(Assignee)

Comment 62

2 years ago
(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 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.
Attachment #8686376 - Flags: review?(roc) → review+
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?
Attachment #8686377 - Flags: review?(roc) → review-
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.
Attachment #8686382 - Flags: review?(roc)
(Assignee)

Comment 66

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

Comment 67

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

Comment 68

2 years ago
Created attachment 8689110 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc

Bug 1141979 - part0 - setup preference utilities.
(Assignee)

Comment 69

2 years ago
Created attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

Bug 1141979 - part1 - WebIDL for native implementation.
(Assignee)

Comment 70

2 years ago
Created attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

Bug 1141979 - part2 - implement ChannelPixelLayout.
(Assignee)

Comment 71

2 years ago
Created attachment 8689113 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug

Bug 1141979 - part3 - implement ImagePixelLayout.
(Assignee)

Comment 72

2 years ago
Created attachment 8689114 [details]
MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc

Bug 1141979 - part4 - implement utilities, ImageUtils and ImageBitmapFormatUtils.
(Assignee)

Comment 73

2 years ago
Created attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

Bug 1141979 - part5 - implement ImageBitmap extensions.
(Assignee)

Comment 74

2 years ago
Created attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

Bug 1141979 - part6 - implement ImageBitmapFactories extensions.
(Assignee)

Comment 75

2 years ago
Created attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

Bug 1141979 - part7 - export to nsGlobalWindow.
(Assignee)

Comment 76

2 years ago
Created attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

Bug 1141979 - part8 - export to WorkerGlobalScope.
(Assignee)

Comment 77

2 years ago
Created attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

Bug 1141979 - part9 - WebIDL for bindings.
(Assignee)

Comment 78

2 years ago
Created attachment 8689120 [details]
MozReview Request: Bug 1141979 - part11 - mochitest; r=roc

Bug 1141979 - part10 - mochitest.
(Assignee)

Comment 79

2 years ago
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/
Attachment #8689110 - Flags: review?(roc)
(Assignee)

Comment 80

2 years ago
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/
Attachment #8689111 - Flags: review?(roc)
(Assignee)

Comment 81

2 years ago
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/
Attachment #8689112 - Flags: review?(roc)
(Assignee)

Comment 82

2 years ago
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/
Attachment #8689113 - Flags: review?(roc)
(Assignee)

Updated

2 years ago
Attachment #8689114 - Flags: review?(roc)
(Assignee)

Comment 83

2 years ago
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/
(Assignee)

Comment 84

2 years ago
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/
Attachment #8689115 - Flags: review?(roc)
(Assignee)

Comment 85

2 years ago
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/
Attachment #8689116 - Flags: review?(roc)
(Assignee)

Updated

2 years ago
Attachment #8689117 - Flags: review?(roc)
(Assignee)

Comment 86

2 years ago
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/
(Assignee)

Comment 87

2 years ago
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/
(Assignee)

Comment 88

2 years ago
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/
Attachment #8689118 - Flags: review?(roc)
(Assignee)

Comment 89

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8689119 - Flags: review?(roc)
(Assignee)

Comment 90

2 years ago
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/
(Assignee)

Comment 91

2 years ago
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/
(Assignee)

Comment 92

2 years ago
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/
Attachment #8689120 - Flags: review?(roc)
(Assignee)

Comment 93

2 years ago
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/
(Assignee)

Comment 94

2 years ago
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/
(Assignee)

Comment 95

2 years ago
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/
(Assignee)

Comment 96

2 years ago
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/
(Assignee)

Comment 97

2 years ago
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/
(Assignee)

Comment 98

2 years ago
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/
(Assignee)

Comment 99

2 years ago
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/
(Assignee)

Comment 100

2 years ago
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/
(Assignee)

Comment 101

2 years ago
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/
(Assignee)

Comment 102

2 years ago
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/
(Assignee)

Comment 103

2 years ago
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/
(Assignee)

Comment 104

2 years ago
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/
(Assignee)

Comment 105

2 years ago
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/
(Assignee)

Comment 106

2 years ago
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/
(Assignee)

Comment 107

2 years ago
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/
(Assignee)

Comment 108

2 years ago
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/
Attachment #8689110 - Flags: review?(roc) → review+
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 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.
Attachment #8689111 - Flags: review?(roc)
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.
Attachment #8689112 - Flags: review?(roc) → review+
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.
Attachment #8689113 - Flags: review?(roc) → review+
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?
Attachment #8689114 - Flags: review?(roc)
Please upload a new set of patches after fixing those.
(Assignee)

Comment 115

2 years ago
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*?
(Assignee)

Comment 116

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

Comment 117

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

Comment 118

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

Comment 119

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

Comment 120

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

Comment 121

2 years ago
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/
(Assignee)

Comment 122

2 years ago
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/
Attachment #8689111 - Flags: review?(roc)
(Assignee)

Comment 123

2 years ago
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/
(Assignee)

Comment 124

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8689114 - Attachment description: MozReview Request: Bug 1141979 - part4 - implement utilities, ImageUtils and ImageBitmapFormatUtils. → MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils.
Attachment #8689114 - Flags: review?(roc)
(Assignee)

Comment 125

2 years ago
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/
(Assignee)

Comment 126

2 years ago
Created attachment 8691863 [details]
MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc

Bug 1141979 - part5 - implement ImageUtils.
Attachment #8691863 - Flags: review?(roc)
(Assignee)

Updated

2 years ago
Attachment #8689115 - Attachment description: MozReview Request: Bug 1141979 - part5 - implement ImageBitmap extensions. → MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions.
(Assignee)

Comment 127

2 years ago
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/
(Assignee)

Comment 128

2 years ago
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/
Attachment #8689116 - Attachment description: MozReview Request: Bug 1141979 - part6 - implement ImageBitmapFactories extensions. → MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions.
(Assignee)

Comment 129

2 years ago
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/
Attachment #8689117 - Attachment description: MozReview Request: Bug 1141979 - part7 - export to nsGlobalWindow. → MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow.
(Assignee)

Updated

2 years ago
Attachment #8689118 - Attachment description: MozReview Request: Bug 1141979 - part8 - export to WorkerGlobalScope. → MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope.
(Assignee)

Comment 130

2 years ago
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/
(Assignee)

Comment 131

2 years ago
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/
Attachment #8689119 - Attachment description: MozReview Request: Bug 1141979 - part9 - WebIDL for bindings. → MozReview Request: Bug 1141979 - part10 - WebIDL for bindings.
(Assignee)

Comment 132

2 years ago
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/
Attachment #8689120 - Attachment description: MozReview Request: Bug 1141979 - part10 - mochitest. → MozReview Request: Bug 1141979 - part11 - mochitest.
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 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.
Attachment #8686383 - Flags: review?(roc)
https://reviewboard.mozilla.org/r/25537/#review23689
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...
Attachment #8689111 - Flags: review?(roc) → review+
https://reviewboard.mozilla.org/r/25543/#review23699

Need DOM peer review for this patch and the previous patch.
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.
Attachment #8689114 - Flags: review?(roc)
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.
(Assignee)

Comment 140

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

Comment 141

2 years ago
https://reviewboard.mozilla.org/r/25539/#review23697

Okay, I will add smaug after updating this patch.
(Assignee)

Comment 142

2 years ago
https://reviewboard.mozilla.org/r/25541/#review23705

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

Okay.
(Assignee)

Comment 143

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

Comment 144

2 years ago
https://reviewboard.mozilla.org/r/25543/#review23699

Okay, will add smaug in next iteration with the update of part1.
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 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.
Attachment #8691863 - Flags: review?(roc)
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?
Attachment #8689115 - Flags: review?(roc)
Attachment #8689116 - Flags: review?(roc)
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.
Attachment #8689117 - Flags: review?(roc) → review+
Comment on attachment 8689117 [details]
MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc

https://reviewboard.mozilla.org/r/25551/#review23839
Attachment #8689118 - Flags: review?(roc) → review+
Comment on attachment 8689118 [details]
MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc

https://reviewboard.mozilla.org/r/25553/#review23841
Attachment #8689119 - Flags: review?(roc) → review+
Comment on attachment 8689119 [details]
MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug

https://reviewboard.mozilla.org/r/25555/#review23843
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
Attachment #8689120 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 154

2 years ago
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?
(Assignee)

Comment 155

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

Comment 156

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

Comment 157

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

Comment 158

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

Comment 159

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

Comment 160

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

Comment 161

2 years ago
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/
Attachment #8689110 - Attachment description: MozReview Request: Bug 1141979 - part0 - setup preference utilities. → MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=roc
(Assignee)

Updated

2 years ago
Attachment #8689111 - Attachment description: MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation. → MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug
Attachment #8689111 - Flags: review?(bugs)
(Assignee)

Comment 162

2 years ago
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/
(Assignee)

Comment 163

2 years ago
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/
Attachment #8689112 - Attachment description: MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout. → MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug
Attachment #8689112 - Flags: review?(bugs)
(Assignee)

Comment 164

2 years ago
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/
Attachment #8689113 - Attachment description: MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout. → MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r?roc, smaug
Attachment #8689113 - Flags: review?(bugs)
(Assignee)

Comment 165

2 years ago
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/
Attachment #8689114 - Attachment description: MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils. → MozReview Request: Bug 1141979 - part4 - implement ImageBitmapFormatUtils; r?roc
Attachment #8689114 - Flags: review?(roc)
(Assignee)

Comment 166

2 years ago
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/
Attachment #8691863 - Attachment description: MozReview Request: Bug 1141979 - part5 - implement ImageUtils. → MozReview Request: Bug 1141979 - part5 - implement ImageUtils; r?roc
Attachment #8691863 - Flags: review?(roc)
(Assignee)

Comment 167

2 years ago
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/
Attachment #8689115 - Attachment description: MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions. → MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r?roc
Attachment #8689115 - Flags: review?(roc)
(Assignee)

Comment 168

2 years ago
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/
Attachment #8689116 - Attachment description: MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions. → MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r?roc
Attachment #8689116 - Flags: review?(roc)
(Assignee)

Comment 169

2 years ago
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/
Attachment #8689117 - Attachment description: MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow. → MozReview Request: Bug 1141979 - part8 - export to nsGlobalWindow; r=roc
(Assignee)

Comment 170

2 years ago
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/
Attachment #8689118 - Attachment description: MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope. → MozReview Request: Bug 1141979 - part9 - export to WorkerGlobalScope; r=roc
(Assignee)

Updated

2 years ago
Attachment #8689119 - Attachment description: MozReview Request: Bug 1141979 - part10 - WebIDL for bindings. → MozReview Request: Bug 1141979 - part10 - WebIDL for bindings; r?roc, smaug
Attachment #8689119 - Flags: review?(bugs)
(Assignee)

Comment 171

2 years ago
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/
(Assignee)

Comment 172

2 years ago
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/
Attachment #8689120 - Attachment description: MozReview Request: Bug 1141979 - part11 - mochitest. → MozReview Request: Bug 1141979 - part11 - mochitest; r=roc
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
Attachment #8689111 - Flags: review?(bugs) → review+

Updated

2 years ago
Attachment #8689111 - Flags: review+
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.

Updated

2 years ago
Attachment #8689112 - Flags: review?(bugs)
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 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-
Attachment #8689112 - Flags: review-
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.
Attachment #8689113 - Flags: review?(bugs) → review-
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)
Attachment #8689119 - Flags: review?(bugs)
(Assignee)

Comment 179

2 years ago
(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?
Flags: needinfo?(roc)
Flags: needinfo?(ctai)
Flags: needinfo?(anssi.kostiainen)
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?
(Assignee)

Comment 181

2 years ago
You are right, we do not need CTORs in this way.
(Assignee)

Comment 182

2 years ago
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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?
Flags: needinfo?(ctai)
(In reply to Tzuhao Kuo [:kaku] from comment #179)
> @roc, @anssi, @ctai, WDYT?

Sure, these changes sound good. Also, can you discuss comment #153?
Flags: needinfo?(roc)
(Assignee)

Comment 186

2 years ago
(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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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.
Attachment #8689114 - Flags: review?(roc)
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?
Attachment #8691863 - Flags: review?(roc)
Attachment #8689115 - Flags: review?(roc) → review+
Comment on attachment 8689115 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc

https://reviewboard.mozilla.org/r/25547/#review25037
Comment on attachment 8689116 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc

https://reviewboard.mozilla.org/r/25549/#review25039
Attachment #8689116 - Flags: review?(roc) → review+
(Assignee)

Comment 192

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

Comment 193

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

Comment 194

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

Comment 195

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8689111 - Flags: review?(bugs)
(Assignee)

Comment 196

2 years ago
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/
(Assignee)

Comment 197

2 years ago
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/
Attachment #8689112 - Flags: review- → review?(bugs)
(Assignee)

Comment 198

2 years ago
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/
Attachment #8689113 - Flags: review- → review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8689114 - Flags: review?(roc)
(Assignee)

Comment 199

2 years ago
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/
(Assignee)

Comment 200

2 years ago
Created attachment 8699863 [details]
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/
Attachment #8699863 - Flags: review?(roc)
(Assignee)

Comment 201

2 years ago
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/
Attachment #8691863 - Flags: review?(roc)
(Assignee)

Comment 202

2 years ago
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/
Attachment #8689115 - Attachment description: MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r?roc → MozReview Request: Bug 1141979 - part6 - implement ImageBitmap extensions; r=roc
(Assignee)

Comment 203

2 years ago
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/
Attachment #8689116 - Attachment description: MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r?roc → MozReview Request: Bug 1141979 - part7 - implement ImageBitmapFactories extensions; r=roc
(Assignee)

Comment 204

2 years ago
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/
(Assignee)

Comment 205

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8689119 - Flags: review?(bugs)
(Assignee)

Comment 206

2 years ago
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/
(Assignee)

Comment 207

2 years ago
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 on attachment 8689111 [details]
MozReview Request: Bug 1141979 - part1 - WebIDL for native implementation; r?roc, smaug

https://reviewboard.mozilla.org/r/25539/#review25541
Attachment #8689111 - Flags: review?(bugs) → review+
Comment on attachment 8689112 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r?roc, smaug

https://reviewboard.mozilla.org/r/25541/#review25543
Attachment #8689112 - Flags: review?(bugs) → review+
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?
Attachment #8689113 - Flags: review?(bugs) → review+
/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

Updated

2 years ago
Attachment #8689119 - Flags: review?(bugs)
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 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)
Attachment #8689119 - Flags: review-
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.
Flags: needinfo?(tkuo)
(Assignee)

Comment 215

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

Comment 216

2 years ago
(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.
Flags: needinfo?(tkuo)
(Assignee)

Updated

2 years ago
Attachment #8699863 - Attachment is obsolete: true
Attachment #8699863 - Flags: review?(roc)
(Assignee)

Comment 217

2 years ago
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/
(Assignee)

Comment 218

2 years ago
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/
(Assignee)

Comment 219

2 years ago
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/
(Assignee)

Comment 220

2 years ago
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/
(Assignee)

Comment 221

2 years ago
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/
(Assignee)

Comment 222

2 years ago
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/
(Assignee)

Comment 223

2 years ago
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/
Attachment #8689119 - Flags: review- → review?(bugs)
(Assignee)

Comment 224

2 years ago
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 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.
Attachment #8689119 - Flags: review?(bugs) → review+
(Assignee)

Updated

2 years ago
Blocks: 1235301
(Assignee)

Comment 226

2 years ago
(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/)
Flags: needinfo?(roc)
Attachment #8689114 - Flags: review?(roc)
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 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.
Attachment #8691863 - Flags: review?(roc)
(Assignee)

Comment 229

2 years ago
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?
Flags: needinfo?(roc)
(Assignee)

Comment 230

2 years ago
(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.
Flags: needinfo?(roc)
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).
Flags: needinfo?(roc)
(Assignee)

Comment 232

2 years ago
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 233

2 years ago
(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?
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 235

2 years ago
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.
Flags: needinfo?(roc)
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?
Flags: needinfo?(roc)
(Assignee)

Comment 237

2 years ago
(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).
Flags: needinfo?(roc)
I'd say just write the C version. We'd need it for the cases where SIMD isn't available, anyway.
Flags: needinfo?(roc)
(Assignee)

Comment 239

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

Comment 240

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

Updated

2 years ago
Attachment #8689110 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689111 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689112 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689113 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689114 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8691863 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689115 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689116 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689117 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689118 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689119 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8689120 - Flags: review?(jmuizelaar)
Attachment #8689115 - Flags: review?(jmuizelaar)
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.
(Assignee)

Comment 242

a year ago
Created attachment 8732736 [details]
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/
Attachment #8732736 - Flags: review?(jmuizelaar)
(Assignee)

Comment 243

a year ago
Created attachment 8732737 [details]
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/
Attachment #8732737 - Flags: review?(jmuizelaar)
Attachment #8732737 - Flags: review?(bugs)
(Assignee)

Comment 244

a year ago
Created attachment 8732738 [details]
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/
Attachment #8732738 - Flags: review?(jmuizelaar)
Attachment #8732738 - Flags: review?(bugs)
(Assignee)

Comment 245

a year ago
Created attachment 8732739 [details]
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/
Attachment #8732739 - Flags: review?(jmuizelaar)
Attachment #8732739 - Flags: review?(bugs)
(Assignee)

Comment 246

a year ago
Created attachment 8732741 [details]
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/
Attachment #8732741 - Flags: review?(jmuizelaar)
(Assignee)

Comment 247

a year ago
Created attachment 8732742 [details]
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/
Attachment #8732742 - Flags: review?(jmuizelaar)
(Assignee)

Comment 248

a year ago
Created attachment 8732743 [details]
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/
Attachment #8732743 - Flags: review?(jmuizelaar)
(Assignee)

Comment 249

a year ago
Created attachment 8732744 [details]
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/
Attachment #8732744 - Flags: review?(jmuizelaar)
(Assignee)

Comment 250

a year ago
Created attachment 8732745 [details]
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/
Attachment #8732745 - Flags: review?(jmuizelaar)
(Assignee)

Comment 251

a year ago
Created attachment 8732746 [details]
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/
Attachment #8732746 - Flags: review?(jmuizelaar)
(Assignee)

Comment 252

a year ago
Created attachment 8732747 [details]
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/
Attachment #8732747 - Flags: review?(jmuizelaar)
(Assignee)

Comment 253

a year ago
Created attachment 8732748 [details]
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/
Attachment #8732748 - Flags: review?(jmuizelaar)
(Assignee)

Comment 254

a year ago
Created attachment 8732749 [details]
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/
Attachment #8732749 - Flags: review?(jmuizelaar)
(Assignee)

Comment 255

a year ago
Created attachment 8732750 [details]
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/
Attachment #8732750 - Flags: review?(jmuizelaar)
(Assignee)

Comment 256

a year ago
Created attachment 8732751 [details]
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/
Attachment #8732751 - Flags: review?(jmuizelaar)
Attachment #8732751 - Flags: review?(bugs)
(Assignee)

Comment 257

a year ago
Created attachment 8732752 [details]
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/
Attachment #8732752 - Flags: review?(jmuizelaar)
(Assignee)

Comment 258

a year ago
Created attachment 8732753 [details]
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/
Attachment #8732753 - Flags: review?(jmuizelaar)
(Assignee)

Comment 259

a year ago
Created attachment 8732754 [details]
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/
Attachment #8732754 - Flags: review?(jmuizelaar)
(Assignee)

Comment 260

a year ago
Created attachment 8732755 [details]
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/
Attachment #8732755 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689110 - Attachment is obsolete: true
Attachment #8689110 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689111 - Attachment is obsolete: true
Attachment #8689111 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689112 - Attachment is obsolete: true
Attachment #8689112 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689113 - Attachment is obsolete: true
Attachment #8689113 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689114 - Attachment is obsolete: true
Attachment #8689114 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8691863 - Attachment is obsolete: true
Attachment #8691863 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689115 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8689116 - Attachment is obsolete: true
Attachment #8689116 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689117 - Attachment is obsolete: true
Attachment #8689117 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689118 - Attachment is obsolete: true
Attachment #8689118 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689119 - Attachment is obsolete: true
Attachment #8689119 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8689120 - Attachment is obsolete: true
Attachment #8689120 - Flags: review?(jmuizelaar)
(Assignee)

Comment 261

a year ago
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.
Trying to get to these patches tomorrow.
(Assignee)

Comment 263

a year ago
(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?
Sorry this review is taking so long, my queue is long. I'll try to get to it this week.
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.)
Attachment #8732737 - Flags: review?(bugs) → review+

Updated

a year ago
Attachment #8732738 - Flags: review?(bugs) → review+
Comment on attachment 8732738 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41317/#review39897
Comment on attachment 8732739 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41319/#review39903
Attachment #8732739 - Flags: review?(bugs) → review+
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.
Attachment #8732751 - Flags: review?(bugs) → review+
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.
Flags: needinfo?(tkuo)
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?
(Assignee)

Updated

a year ago
Flags: needinfo?(tkuo)
(Assignee)

Updated

a year ago
Flags: needinfo?(anssi.kostiainen)
(Assignee)

Comment 271

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

Comment 273

a year ago
(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/.
(Assignee)

Comment 274

a year ago
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?
(Assignee)

Updated

a year ago
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 275

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

Comment 276

a year ago
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!
(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...
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(tkuo)
(Assignee)

Comment 279

a year ago
(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.
Flags: needinfo?(tkuo)
(Assignee)

Comment 280

a year ago
(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)?
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 282

a year ago
(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?
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 284

a year ago
(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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 286

a year ago
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/
Attachment #8732754 - Attachment description: MozReview Request: Bug 1141979 - part 17 - mochitest - draw special color formats onto canvas; r?jrmuizel → MozReview Request: Bug 1141979 - part17 - mochitest - draw special color formats onto canvas; r?jrmuizel
(Assignee)

Comment 287

a year ago
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/
(Assignee)

Comment 288

a year ago
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/
(Assignee)

Comment 289

a year ago
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/
(Assignee)

Comment 290

a year ago
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/
(Assignee)

Comment 291

a year ago
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/
(Assignee)

Comment 292

a year ago
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/
(Assignee)

Comment 293

a year ago
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/
(Assignee)

Comment 294

a year ago
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/
(Assignee)

Comment 295

a year ago
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/
(Assignee)

Comment 296

a year ago
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/
(Assignee)

Comment 297

a year ago
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/
(Assignee)

Comment 298

a year ago
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/
(Assignee)

Comment 299

a year ago
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/
(Assignee)

Comment 300

a year ago
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/
(Assignee)

Comment 301

a year ago
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/
(Assignee)

Comment 302

a year ago
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/
(Assignee)

Comment 303

a year ago
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/
(Assignee)

Comment 304

a year ago
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/
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?
(Assignee)

Comment 306

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

Updated

a year ago
No longer blocks: 1235301
(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.
(Assignee)

Comment 308

a year ago
Thanks a lot, Jeff!
Attachment #8732736 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732736 [details]
MozReview Request: Bug 1141979 - part0 - setup preference utilities; r=jrmuizel

https://reviewboard.mozilla.org/r/41313/#review52566
Attachment #8732737 - Flags: review?(jmuizelaar) → review+
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 on attachment 8732738 [details]
MozReview Request: Bug 1141979 - part2 - implement ChannelPixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41317/#review52570
Attachment #8732738 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732739 [details]
MozReview Request: Bug 1141979 - part3 - implement ImagePixelLayout; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41319/#review52572
Attachment #8732739 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732741 [details]
MozReview Request: Bug 1141979 - part4 - Add NVImage; r=jrmuizel

https://reviewboard.mozilla.org/r/41321/#review52574
Attachment #8732741 - Flags: review?(jmuizelaar) → review+
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
Attachment #8732742 - Flags: review?(jmuizelaar) → review+
https://reviewboard.mozilla.org/r/41323/#review52580
Attachment #8732743 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732743 [details]
MozReview Request: Bug 1141979 - part6 - implement ImageBitmapFormatUtils; r=jrmuizel

https://reviewboard.mozilla.org/r/41325/#review52582
Attachment #8732744 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732744 [details]
MozReview Request: Bug 1141979 - part7 - implement ImageUtils; r=jrmuizel

https://reviewboard.mozilla.org/r/41327/#review52584
Comment on attachment 8732745 [details]
MozReview Request: Bug 1141979 - part8 - implement ImageBitmap extensions; r=jrmuizel

https://reviewboard.mozilla.org/r/41329/#review52586
Attachment #8732745 - Flags: review?(jmuizelaar) → review+
Attachment #8732746 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732746 [details]
MozReview Request: Bug 1141979 - part9 - implement ImageBitmapFactories extensions; r=jrmuizel

https://reviewboard.mozilla.org/r/41331/#review52588
Attachment #8732747 - Flags: review?(jmuizelaar) → review+
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
Attachment #8732748 - Flags: review?(jmuizelaar) → review+
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 on attachment 8732749 [details]
MozReview Request: Bug 1141979 - part12 - export to nsGlobalWindow; r=jrmuizel

https://reviewboard.mozilla.org/r/41337/#review52594
Attachment #8732749 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8732750 [details]
MozReview Request: Bug 1141979 - part13 - export to WorkerGlobalScope; r=jrmuizel

https://reviewboard.mozilla.org/r/41339/#review52596
Attachment #8732750 - Flags: review?(jmuizelaar) → review+
Attachment #8732751 - Flags: review?(jmuizelaar)
Comment on attachment 8732751 [details]
MozReview Request: Bug 1141979 - part14 - WebIDL for bindings; r=jrmuizel, r=smaug

https://reviewboard.mozilla.org/r/41341/#review52598
Attachment #8732752 - Flags: review?(jmuizelaar) → review+