If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement ImageBitmap and createImageBitmap

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jmorton, Assigned: kaku)

Tracking

(Blocks: 5 bugs, {dev-doc-complete})

Trunk
mozilla42
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, relnote-firefox 42+)

Details

Attachments

(6 attachments, 94 obsolete attachments)

42.62 KB, patch
kaku
: review+
Details | Diff | Splinter Review
54.96 KB, patch
kaku
: review+
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review
17.26 KB, patch
kaku
: review+
Details | Diff | Splinter Review
25.18 KB, patch
kaku
: review+
Details | Diff | Splinter Review
3.39 KB, patch
kaku
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
ImageBitmap provides more efficient loading and preparing of image resources for rendering in 2D canvases and WebGL.

Spec: http://www.whatwg.org/specs/web-apps/current-work/#imagebitmap

ImageBitmap also needs to be supported as a CanvasImageSource and a TexImageSource for 2d canvas and WebGL respectively.
(Reporter)

Updated

3 years ago
Assignee: nobody → jmorton
(Reporter)

Comment 1

3 years ago
Created attachment 8467961 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch
Attachment #8467961 - Flags: review?(khuey)
(Reporter)

Comment 2

3 years ago
Created attachment 8467964 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch
Attachment #8467964 - Flags: review?(khuey)
(Reporter)

Comment 3

3 years ago
Created attachment 8467965 [details] [diff] [review]
Part 3 - Implement Cropping and Surface Preparation
Attachment #8467965 - Flags: review?(roc)
(Reporter)

Comment 4

3 years ago
Created attachment 8467967 [details] [diff] [review]
Part 4 - ImageBitmap as a CanvasImageSource
Attachment #8467967 - Flags: review?(roc)
(Reporter)

Comment 5

3 years ago
Created attachment 8467968 [details] [diff] [review]
Part 5 - Tests
Attachment #8467968 - Flags: review?(khuey)
(Reporter)

Comment 6

3 years ago
These patches implement createImageBitmap with Image/Canvas/Video elements as sources and adds ImageBitmap support to canvas 2d. I still need to add Blob/ImageData/ImageBitmap as sources, support texImage in WebGL, and add ImageBitmap as a transferable.
Comment on attachment 8467965 [details] [diff] [review]
Part 3 - Implement Cropping and Surface Preparation

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

In the future please submit patches with -U8 (8 lines of diff context). Otherwise looks great!

::: dom/canvas/ImageBitmap.cpp
@@ +101,5 @@
> +    }
> +
> +    IntPoint dest(
> +      mCropRect.X() < surfPortion.X() ? surfPortion.X() - mCropRect.X() : 0,
> +      mCropRect.Y() < surfPortion.Y() ? surfPortion.Y() - mCropRect.Y() : 0);

std::max(0, surfPortion.X() - mCropRect.X()) etc

@@ +110,5 @@
> +      return mSurface = nullptr;
> +    }
> +    // Make mCropRect match new surface we've cropped to
> +    mCropRect.MoveTo(0, 0);
> +    target->ClearRect(Rect(mCropRect));

I believe this ClearRect is not necessary.
Attachment #8467965 - Flags: review?(roc) → review+
Comment on attachment 8467967 [details] [diff] [review]
Part 4 - ImageBitmap as a CanvasImageSource

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

Great!

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3331,4 @@
>        error.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>        return;
>      }
> +  } else if (!image.IsImageBitmap()) {

Better to move the ImageBitmap case up here so we don't need this !
Attachment #8467967 - Flags: review?(roc) → review+
Comment on attachment 8467961 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

ehsan is going to take a first pass on these.
Attachment #8467961 - Flags: review?(khuey) → review?(ehsan)
Attachment #8467964 - Flags: review?(khuey) → review?(ehsan)
Attachment #8467968 - Flags: review?(khuey) → review?(ehsan)
Comment on attachment 8467961 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

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

Looks good in general, please see my comments below.  The most important issue is whether this object needs to be wrapper-cached.

::: dom/base/nsGlobalWindow.h
@@ +51,4 @@
>  #include "mozilla/dom/EventTarget.h"
>  #include "Units.h"
>  #include "nsComponentManagerUtils.h"
> +#include "mozilla/dom/ImageBitmapSource.h"

Please forward-declare ImageBitmapSource.

::: dom/canvas/ImageBitmap.h
@@ +10,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "mozilla/dom/ImageBitmapSource.h"
> +#include "mozilla/gfx/Rect.h"

I think you should be able to avoid including ErrorResult.h and ImageBitmapSource.h in the header and just forward declare these.  Maybe Rect.h too.

@@ +24,5 @@
> +
> +class Promise;
> +
> +class ImageBitmap MOZ_FINAL : public nsISupports,
> +                              public nsWrapperCache

As far as I can tell, there is no way for the web content to regain access to an ImageBitmap once it has lost its last reference to it.  Based on that, should ImageBitmap be wrapper-cached?  Am I missing something?

@@ +31,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  JSObject* WrapObject(JSContext* aCx);
> +
> +  ImageBitmap* GetParentObject() { return nullptr; }

Nit: const.

Also, I think it would be much better to return a useful value here.  You can use the global object passed to the Create function below.

::: dom/canvas/ImageBitmapSource.h
@@ +1,1 @@
> +

Nit: please include the license header here.

::: dom/webidl/ImageBitmap.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#imagebitmap

Nit: please use a "The origin of this IDL file is" template like other webidl files here.

@@ +26,5 @@
> +interface ImageBitmapFactories {
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);

You should use optional here instead of adding two overloads.

@@ +30,5 @@
> +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
> +};
> +
> +Window implements ImageBitmapFactories;
> +WorkerGlobalScope implements ImageBitmapFactories;

These need to go into Window.webidl and WorkerGlobalScope.webidl respectively.

::: dom/workers/WorkerScope.h
@@ +8,4 @@
>  
>  #include "Workers.h"
>  #include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/ImageBitmapSource.h"

Forware-declare please.
Attachment #8467961 - Flags: review?(ehsan) → feedback+
(Reporter)

Comment 11

3 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)

> Please forward-declare ImageBitmapSource.

This would definitely be the right way, but unfortunately ImageBitmapSource is a typedef of "HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap", and to forward declare it I'd have to reference the original, 110 character gargantuan name everywhere. My solution was to create the ImageBitmapSource.h file with just the typedef so it could be included anywhere needed. I filed Bug 1045867 to suggest support for renaming union type class names.

> > +
> > +class Promise;
> > +
> > +class ImageBitmap MOZ_FINAL : public nsISupports,
> > +                              public nsWrapperCache
> 
> As far as I can tell, there is no way for the web content to regain access
> to an ImageBitmap once it has lost its last reference to it.  Based on that,
> should ImageBitmap be wrapper-cached?  Am I missing something?

Nope, not missing anything, you're definitely right.

> @@ +26,5 @@
> > +interface ImageBitmapFactories {
> > +  [Throws]
> > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
> > +  [Throws]
> > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
> 
> You should use optional here instead of adding two overloads.

Hmm, that's what I was going to do at first, but:
1. Other functions with similar arguments (in WebGLRenderingContext and CanvasRenderingContext2D) did it this way
2. It seems to me that with optional, there are 5 overloads here (1-5 args), but we really just want these two overloads. Can I make the last four arguments optional as a whole? I didn't see a way to do that anywhere.

> Also, I think it would be much better to return a useful value here.  You
> can use the global object passed to the Create function below.

I think I can just remove this function entirely if I don't use wrappercache.

And OK on everything else!
(In reply to Jon Morton [:jmorton] from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> 
> > Please forward-declare ImageBitmapSource.
> 
> This would definitely be the right way, but unfortunately ImageBitmapSource
> is a typedef of
> "HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanv
> asRenderingContext2DOrImageBitmap", and to forward declare it I'd have to
> reference the original, 110 character gargantuan name everywhere. My
> solution was to create the ImageBitmapSource.h file with just the typedef so
> it could be included anywhere needed. I filed Bug 1045867 to suggest support
> for renaming union type class names.

Ah, right.  OK, it's fine to keep the include for ImageBitmapSource here.

> > @@ +26,5 @@
> > > +interface ImageBitmapFactories {
> > > +  [Throws]
> > > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
> > > +  [Throws]
> > > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
> > 
> > You should use optional here instead of adding two overloads.
> 
> Hmm, that's what I was going to do at first, but:
> 1. Other functions with similar arguments (in WebGLRenderingContext and
> CanvasRenderingContext2D) did it this way
> 2. It seems to me that with optional, there are 5 overloads here (1-5 args),
> but we really just want these two overloads. Can I make the last four
> arguments optional as a whole? I didn't see a way to do that anywhere.

Hmm, you're right on both points.  Actually the IDL in the spec is wrong, since it just sets the second argument to the optional.  But the prose does suggest that the spec actually wants to implement your IDL (i.e. allow calling either with 1 or 5 args.)  I filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=26562> for that.  Please keep your existing IDL.  :-)
Comment on attachment 8467964 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +34,5 @@
>    return ImageBitmapBinding::Wrap(aCx, this);
>  }
>  
> +/* static */ already_AddRefed<ImageBitmap>
> +ImageBitmap::CreateFromElement(Element& aElement, ErrorResult& aRv)

Instead of going from a concrete element type to dom::Element and "coming back" to the concrete type inside SurfaceFromElement, it would be better to make this method a template which would call SurfaceFromElement.  That function has all of the overloads that you need here: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h#1775

@@ +129,5 @@
> +  if (!aRv.Failed()) {
> +    MessageLoop::current()->PostTask(
> +      FROM_HERE,
> +      NewRunnableFunction(&FulfillImageBitmapPromise, promise, imageBitmap));
> +  }

You need to reject the promise if an error happens when constructing an ImageBitmap.

::: dom/canvas/ImageBitmap.h
@@ +50,4 @@
>  
>  protected:
>  
> +  ImageBitmap(gfx::SourceSurface* aSurface);

Nit: please mark this as explicit.
Attachment #8467964 - Flags: review?(ehsan) → review-
Comment on attachment 8467968 [details] [diff] [review]
Part 5 - Tests

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

::: dom/canvas/test/test_imagebitmap.html
@@ +4,5 @@
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<link rel="stylesheet" href="/tests/SimpleTest/test.css">
> +<body>
> +
> +<img src="image_anim-gr.gif" id="image" class="resource" />

Nit: XHTML is dead.  please remove the " /".  ;)

@@ +41,5 @@
> +];
> +
> +var canvas, ctx, ctx2, completedImage;
> +
> +function pFail(ex) {

Nit: just call this failed or something.

@@ +51,5 @@
> +
> +  image.src = 'image_rgrg-256x256.png';
> +  var promise = new Promise(function (arg) { resolver = arg; });
> +
> +  var createBitmap = function(def) {

I'm not sure why you don't just do:

function createBitmap(def) {...

here and elsewhere.

@@ +68,5 @@
> +
> +  return promise.then(function() {
> +    TEST_BITMAPS.forEach(function (test) {
> +      if (!test.bitmap) { return; }
> +      ctx.clearRect ( 0, 0, canvas.width, canvas.height );

Nit: no spaces around braces.
Attachment #8467968 - Flags: review?(ehsan) → review+
Keywords: dev-doc-needed
(Reporter)

Comment 15

3 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> std::max(0, surfPortion.X() - mCropRect.X()) etc

Indeed, that's much better.

> I believe this ClearRect is not necessary.

Yep.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Better to move the ImageBitmap case up here so we don't need this !

Yep.

Thanks for the review!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> Instead of going from a concrete element type to dom::Element and "coming
> back" to the concrete type inside SurfaceFromElement, it would be better to
> make this method a template which would call SurfaceFromElement.  That
> function has all of the overloads that you need here:
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> h#1775

Ah, yes. Good call.

> 
> You need to reject the promise if an error happens when constructing an
> ImageBitmap.
> 

If I'm understanding this comment correctly, then: the bindings have a special case for methods that return a Promise and will automatically reject the promise with any errors that are returned.

And SGTM on the rest!
(In reply to Jon Morton [:jmorton] from comment #15)
> > You need to reject the promise if an error happens when constructing an
> > ImageBitmap.
> > 
> 
> If I'm understanding this comment correctly, then: the bindings have a
> special case for methods that return a Promise and will automatically reject
> the promise with any errors that are returned.

OK, then that part looks good.
(Reporter)

Comment 17

3 years ago
It turns out that ImageBitmap does need to be wrappercached, because the promise resolution codepath uses ToJSValue, which only accepts objects which inherit from nsISupports AND WrapperCache: http://mxr.mozilla.org/mozilla-central/source/dom/bindings/ToJSValue.h#141

(thanks khuey)
OK fine!
Blocks: 1100203
(Assignee)

Comment 19

3 years ago
Hi Jon, 
This is Kaku from Mozilla Taipei Media team. I am wondering if you are still working on this bug?
Thanks,
Kaku
Flags: needinfo?(jonanin)
(Reporter)

Comment 20

3 years ago
Hi Kaku,
Sorry about the delay on this. I will try to finish this bug up pretty soon - within the coming few weeks.
Flags: needinfo?(jonanin)
(Assignee)

Updated

3 years ago
Depends on: 1048325
Kaku, why do we have this dependence on bug 1048325?
I don't think we want to implement blob.close() soon, except if there are valid reasons.
Tell me more :)
Flags: needinfo?(tkuo)
(Assignee)

Comment 22

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #21)
> Kaku, why do we have this dependence on bug 1048325?
> I don't think we want to implement blob.close() soon, except if there are
> valid reasons.
> Tell me more :)

Since Blob is one of the source type of ImageBitmap and the "isClosed" property should be checked while constructing the ImageBitmap. The statement is quoted as follows:

"If the Blob object has been disabled through the close() method, then return a promise rejected with an InvalidStateError exception and abort these steps."
Flags: needinfo?(tkuo)
(Assignee)

Comment 23

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #21)
> Kaku, why do we have this dependence on bug 1048325?
> I don't think we want to implement blob.close() soon, except if there are
> valid reasons.
> Tell me more :)

Tn the File API, there is also statements for checking the isClosed property in the readAsXXX() methods, but we do not implement the checks in Gecko. Should I just skip this statement for now?
Yes please. Last time I discussed with bent about implementing a close(), we asked me if really this is needed. We have to discuss it more seriously, but for now, please go on. Maybe file a follow-up about this.
A followup bug, depending on the bug for close(), would be good, so when we add close() we update all the various places that check isClosed.
(Assignee)

Comment 26

3 years ago
Created attachment 8561231 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

Modified according to comments.
Assignee: jonanin → tkuo
Attachment #8467961 - Attachment is obsolete: true
Attachment #8561231 - Flags: review?(ehsan)
(Assignee)

Comment 27

3 years ago
Created attachment 8561232 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

Modified according to comments.
Attachment #8467964 - Attachment is obsolete: true
Attachment #8561232 - Flags: review?(ehsan)
(Assignee)

Comment 28

3 years ago
Created attachment 8561233 [details] [diff] [review]
0003-Bug-1044102-Part-3-Implement-cropping-and-surface-pr.patch

Carry r+ and still modified according to comments.
Attachment #8467965 - Attachment is obsolete: true
Attachment #8561233 - Flags: review+
(Assignee)

Comment 29

3 years ago
Created attachment 8561235 [details] [diff] [review]
0004-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch

Carry r+ and still modified according to comments.
Attachment #8467967 - Attachment is obsolete: true
Attachment #8561235 - Flags: review+
(Assignee)

Comment 30

3 years ago
Created attachment 8561236 [details] [diff] [review]
0005-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

Carry r+ and still modified according to comments.
Attachment #8467968 - Attachment is obsolete: true
Attachment #8561236 - Flags: review+
(Assignee)

Comment 31

3 years ago
Created attachment 8561237 [details] [diff] [review]
0006-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch
Attachment #8561237 - Flags: review?(ehsan)
(Assignee)

Comment 32

3 years ago
Created attachment 8561238 [details] [diff] [review]
0007-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch
Attachment #8561238 - Flags: review?(ehsan)
(Assignee)

Comment 33

3 years ago
Created attachment 8561239 [details] [diff] [review]
0008-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch
Attachment #8561239 - Flags: review?(ehsan)
(Assignee)

Comment 34

3 years ago
These three new patches implement createImageBitmap with ImageData/CanvasRenderingContext2D/ImageBitmap as sources.

Keep working on add Blob as sources, support texImage in WebGL, and add ImageBitmap as a transferable.
The ordering of these patches seems to be off.  The patch to implement the right security checks (which doesn't even exist yet?) should come before the patch that actually returns imagebitmaps for HTML elements.

Incidentally, I don't think the spec's use of the entry settings origin entirely makes sense here....
(Assignee)

Comment 36

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #35)
> The ordering of these patches seems to be off.  The patch to implement the
> right security checks (which doesn't even exist yet?) should come before the
> patch that actually returns imagebitmaps for HTML elements.
> 
> Incidentally, I don't think the spec's use of the entry settings origin
> entirely makes sense here....

Hi Boris, 
Thanks for your comment and after some spec reading and code tracing, here is my proposal for the "security check".
May I have your comments?

------------------------------------------------------------
For the HTMLImageElement and HTMLVideoElement cases:
Step1: Get the Principal object from the incoming global object: (which is the entry settings object)
       nsGlobalWindow::mPrincipal
       ...or...
       WorkerGlobalScope::mWokerPrivate::mPrincipal
Step2: Get the Principal object from the HTMLImage(/Video)Element:
       nsLayoutUtils::SurfaceFromElementResult res = nsLayoutUtils::SurfaceFromElement(...);
       nsLayoutUtils::SurfaceFromElementResult::mPrincipal
Step3: Compare these two Principal objects: (where I am not really sure which method should I use...)
       nsIPrincipal::Equals(...) or nsIPrincipal::Subsumes(...)

For the HTMLCanvasElement and CanvasRenderingContext2D cases:
Just checke the HTMLCanvasElement::isWriteOnly flag.
------------------------------------------------------------

Also, could you talk more about your concern to the usage of "entry settings object" on the spec?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 37

3 years ago
Here are questions raised whiling I was thinking about implementing the blob as input source.
The incoming blob object might comes from two scenarios:

Scenario 1:
If the incoming Blob object is a encoded image file, which might comes from <input> or drag-drop. In this case, I think I can get the InputStream object from the incoming Blob object and then put the InputStream object into imgTools::DecodeImage() to get an imgIContainer object which I can then have a SourceSurface. Is this way feasible?

Scenario 2:
The incoming Blob object contains decoded image raw pixels. This could happen if JavaScript developers use CanvasRenderingContext2D::GetImageData() to get a TypedArray and then construct the Blob object with the underlying ArrayBuffer of the TypedArray. Sould we also support this scenario?
> Step1: Get the Principal object from the incoming global object: (which is the entry
> settings object)

No, it's not.  If you actually want the entry settings object, you want GetEntryGlobal().  Now it's not clear to me whther the spec here makes sense at all; I would have assumed that  the incumbent settings object would make more sense than the entry one.  That's something you should talk to bholley about.  In that case you'd use GetIncumbentGlobal().

Note that you can assert in the HTMLImageElement/HTMLVideoElement cases that you're on mainthread, since DOM elements don't exist on workers.

> Step2: Get the Principal object from the HTMLImage(/Video)Element:

Sadly, no.  The spec's concept of "image origin" doesn't match our concept of "principal of the SurfaceFromElementResult".

You should probably file a separate bug on changing our behavior here (or changing the spec; we need to check whether the spec matches reality), but in the meantime you basically want to assume same-origin if res.mCORSUsed.  Please do file that followup bug, though.

>       nsIPrincipal::Equals(...) or nsIPrincipal::Subsumes(...)

Subsumes.

> Also, could you talk more about your concern to the usage of "entry settings object" on
> the spec?

Sure.  The "entry settings object" is "the window where script first got entered".  It's not clear to me why that should be the thing used for the security check as opposed to "the window containing the script that called the function" or "the window the function is defined in".
Flags: needinfo?(bzbarsky)
> The spec's concept of "image origin" doesn't match our concept of "principal of the
> SurfaceFromElementResult".

Oh, and I assume you _will_ add tests here for cross-origin images, both using CORS and not, which would have caught the need for the mCORSUsed check.
(Assignee)

Comment 40

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #38)
> > Step1: Get the Principal object from the incoming global object: (which is the entry
> > settings object)
> 
> No, it's not.  If you actually want the entry settings object, you want
> GetEntryGlobal().  Now it's not clear to me whther the spec here makes sense
> at all; I would have assumed that  the incumbent settings object would make
> more sense than the entry one.  That's something you should talk to bholley
> about.  In that case you'd use GetIncumbentGlobal().
> 
I agree with you that incumbent settings object is the better one according to the statements in https://dxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h#78.

> The spec's concept of "image origin" doesn't match our concept
> of "principal of the SurfaceFromElementResult".
I am not really sure what's the difference between spec's concept of origin and our concept of SurfaceFromElementResult, could you explain more? And I have no idea how can I check origin if I cannot use the Principal object, could you give me some hints?

> You should probably file a separate bug on changing our behavior here (or
> changing the spec; we need to check whether the spec matches reality), but
> in the meantime you basically want to assume same-origin if res.mCORSUsed. 
> Please do file that followup bug, though.

(In reply to Boris Zbarsky [:bz] from comment #39)
> Oh, and I assume you _will_ add tests here for cross-origin images, both
> using CORS and not, which would have caught the need for the mCORSUsed check. 

Conceptually, here is my understanding about how to do security checking.

if (res.mCORSUsed) {
    // cross-origin image with CORS approval, ---> PASS
}
else {
    if (CHECK_SAME_ORIGIN(HTMLImage/VideoElement, EntrySettingsObject)) {
        // same-origin ---> PASS
    }
    else {
        // cross-origin ---> FAIL
    }
}

And I need to more information about the "image origin" to design the CHECK_SAME_ORIGIN().
Flags: needinfo?(bzbarsky)
> I am not really sure what's the difference between spec's concept of origin and our
> concept of SurfaceFromElementResult, could you explain more?

Sure.  What we store in the SurfaceFromElementResult is two pieces of data: the actual origin of the image and a boolean indicating whether the image passed CORS checks.  If that boolean is set we don't bother performing security checks against the actual origin of the image, exactly as described in your "Conceptually, ..." pseudocode, using the principal in the SurfaceFromElementResult as the image origin.

What the spec does instead is it associates an origin with each image.  If the image passed CORS checks, or was same-origin with the document that started the load to start with and no CORS checks were done, that origin is an alias for the origin of the <img> element's node document.  Otherwise it's a nonce ("unique identifier") origin.

As long as no one uses document.domain, you can't tell apart the spec's behavior and ours.

But if document.domain is used, you can tell them apart.  Say you have two documents, A and B, which are not same-origin but both set document.domain so they have the same effective script origin.  A loads a cross-site image via CORS.  Now say we're sticking with the "entry settings object" bit in the spec and script is entered in document B, which calls a function in document A which tries to create an ImageBitmap from the image.

Per the spec, we'd take the origin of the entry settings object (which is the origin of B) and compare it to the origin of the image (which is the origin of A) and the security check would fail and the image bitmap would not be created.

In our implementation in this situation we don't have "the origin of A" associated with the image in any way.  So we'd apply your pseudocode above and the ImageBitmap creation would succeed.

That's what we need a followup bug on: figuring out what the actual behavior here should be in this situation.  There are similar problems with drawImage on a canvas in terms of the spec not matching what we're actually doing.

Does that help?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 42

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #41)
> Does that help?

Yes, of course, your explanation has helped a lot to get me on track. Thank you.
(Assignee)

Updated

3 years ago
Attachment #8561231 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8561232 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8561237 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8561238 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8561239 - Flags: review?(ehsan)
(Assignee)

Comment 43

3 years ago
Cancel reviews and implement security/status checks first.
(Assignee)

Comment 44

3 years ago
Created attachment 8569718 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch
Attachment #8561231 - Attachment is obsolete: true
Attachment #8569718 - Flags: feedback?(ctai)
(Assignee)

Comment 45

3 years ago
Created attachment 8569719 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch
Attachment #8561232 - Attachment is obsolete: true
Attachment #8569719 - Flags: feedback?(ctai)
(Assignee)

Comment 46

3 years ago
Created attachment 8569720 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

Carry r+ and modified according to comments.
Attachment #8561233 - Attachment is obsolete: true
Attachment #8569720 - Flags: review+
(Assignee)

Comment 47

3 years ago
Created attachment 8569721 [details] [diff] [review]
0004-Bug-1044102-Part-3-Implement-cropping-and-surface-pr.patch

Carry r+ and modified according to comments.
Attachment #8561235 - Attachment is obsolete: true
Attachment #8569721 - Flags: review+
(Assignee)

Comment 48

3 years ago
Created attachment 8569723 [details] [diff] [review]
0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch

Carry r+ and modified according to comments.
Attachment #8561236 - Attachment is obsolete: true
Attachment #8569723 - Flags: review+
(Assignee)

Comment 49

3 years ago
Created attachment 8569724 [details] [diff] [review]
0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

Bug 1044102 Part 6 - ImageData as ImageBitamp sources, code and test case
Attachment #8561237 - Attachment is obsolete: true
Attachment #8569724 - Flags: feedback?(ctai)
(Assignee)

Comment 50

3 years ago
Created attachment 8569726 [details] [diff] [review]
0007-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch

Bug 1044102 Part 7 - CanvasRenderingContext2D as ImageBitamp sources
Attachment #8561238 - Attachment is obsolete: true
Attachment #8569726 - Flags: feedback?(ctai)
(Assignee)

Comment 51

3 years ago
Created attachment 8569728 [details] [diff] [review]
0008-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch

Bug 1044102 Part 7 - CanvasRenderingContext2D as ImageBitamp sources
Attachment #8569728 - Flags: feedback?(ctai)
(Assignee)

Comment 52

3 years ago
Created attachment 8569730 [details] [diff] [review]
0009-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch

Bug 1044102 Part 8 - ImageBitmap as ImageBitamp sources
Attachment #8561239 - Attachment is obsolete: true
Attachment #8569730 - Flags: feedback?(ctai)
(Assignee)

Comment 53

3 years ago
Created attachment 8569731 [details] [diff] [review]
0010-Bug-1044102-Part-9-Blob-as-ImageBitamp-sources.patch

Bug 1044102 Part 9 - Blob as ImageBitamp sources
Attachment #8569731 - Flags: feedback?(ctai)
Comment on attachment 8569718 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +31,5 @@
> +{
> +  return ImageBitmapBinding::Wrap(aCx, this);
> +}
> +
> +/* static */ already_AddRefed<Promise>

ditto: new line for /* static *

::: dom/canvas/ImageBitmap.h
@@ +26,5 @@
> +
> +class Promise;
> +
> +class ImageBitmap MOZ_FINAL : public nsISupports,
> +                              public nsWrapperCache

I think you miss the cycle collection part. 
"Does the object inherit from nsWrapperCache (directly or indirectly)?  If so, it must be cycle collected."
Quoted from http://blog.kylehuey.com/post/27564411715/cycle-collection
Attachment #8569718 - Flags: feedback?(ctai)
Comment on attachment 8569719 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

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

LGTM

::: dom/canvas/ImageBitmap.cpp
@@ +34,5 @@
>    return ImageBitmapBinding::Wrap(aCx, this);
>  }
>  
> +template<class HTMLElementType>
> +/* static */ already_AddRefed<ImageBitmap>

Could you please move /* static */ in front of 	
template<class HTMLElementType>?
Attachment #8569719 - Flags: feedback?(ctai) → feedback+
Comment on attachment 8569724 [details] [diff] [review]
0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

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

Fix some nits.

::: dom/canvas/test/test_imagebitmap.html
@@ +45,5 @@
> +    {'rect': [0, 0, 128, 128],        'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [255, 0, 0, 255], 5]]},
> +    {'rect': [128, 0, 128, 128],      'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [0, 255, 0, 255], 5]]},
> +    {'rect': [230, 230, 128, 128],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [255, 0, 0, 255], 5],
> +                                                                                           [100,  100,  [0, 0, 0, 0],     5]]},
> +    {'rect': [-64, -64, 512, 512],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [0, 0, 0, 0],     5], 

nit: remove the last space

@@ +67,5 @@
> +  function createBitmap(def) {
> +    return createImageBitmap(image, def.rect[0], def.rect[1], def.rect[2], def.rect[3])
> +      .then(function (bitmap) { def.bitmap = bitmap; }, failed);
> +  };
> +  

nit:
remove leading space
Attachment #8569724 - Flags: feedback?(ctai)
Comment on attachment 8569720 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

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

Be careful of the nullptr check.

::: dom/canvas/ImageBitmap.cpp
@@ +21,5 @@
> +  if (res.mIsWriteOnly) {
> +    return false;
> +  }
> +
> +  if (res.mCORSUsed) {

Why not use early return? I guess this is a method which will be called many times.
if (!res.mCORSUsed) { return true;}

@@ +23,5 @@
> +  }
> +
> +  if (res.mCORSUsed) {
> +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();

You should do nullptr check before use it. |PrincipalOrNull| is not guaranteed to return a non-nullptr.

@@ +25,5 @@
> +  if (res.mCORSUsed) {
> +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();
> +    if (!(principal->Subsumes(res.mPrincipal))) {
> +      return false;

if (principal->Subsumes(res.mPrincipal)) {
  return true; 
}
return false;

Will that better?

@@ +39,5 @@
> +  nsresult rv;
> +
> +  nsCOMPtr<imgIRequest> imgRequest;
> +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> +  if (!NS_FAILED(rv) && imgRequest) {

Why not use if (NS_SUSSEEDED(rv) && imgRequest) { ?

@@ +42,5 @@
> +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> +  if (!NS_FAILED(rv) && imgRequest) {
> +    nsCOMPtr<imgIContainer> imgContainer;
> +    rv = imgRequest->GetImage(getter_AddRefs(imgContainer));
> +    if (!NS_FAILED(rv) && imgContainer->GetType() == imgIContainer::TYPE_RASTER) {

ditto.
Is that possible imgContainer a null-ptr?
Attachment #8569720 - Flags: review-
Comment on attachment 8569726 [details] [diff] [review]
0007-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch

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

LGTM.

::: dom/canvas/ImageBitmap.cpp
@@ +225,5 @@
>  
> +/* static */ already_AddRefed<ImageBitmap>
> +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData, ErrorResult& aRv)
> +{
> +  // TODO: check the data of ImageData is neutered or not

Is there a bug related this TODO?

@@ +235,5 @@
> +
> +  array.ComputeLengthAndData();
> +  uint32_t const imageWidth = aImageData.Width();
> +  uint32_t const imageHeight = aImageData.Height();
> +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData

This is based on the assumption that the format of ImageData is RGBA. Be careful here once this assumption is not true.

@@ +239,5 @@
> +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData
> +  uint32_t const dataLength = array.Length();
> +  gfx::IntSize const imageSize(imageWidth, imageHeight);
> +
> +  RefPtr<DataSourceSurface> surface = Factory::CreateDataSourceSurfaceWithStride(imageSize, SurfaceFormat::B8G8R8A8, imageStride);

ditto.
Attachment #8569726 - Flags: feedback?(ctai) → feedback+
Comment on attachment 8569728 [details] [diff] [review]
0008-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch

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

LGTM

::: dom/canvas/ImageBitmap.cpp
@@ +263,5 @@
> +    return nullptr;
> +  }
> +
> +  IntSize surfaceSize = surface->GetSize();
> +  if (surfaceSize.width == 0 || surfaceSize.height == 0) {

Is that possible surfaceSize.width < 0?
Attachment #8569728 - Flags: feedback?(ctai) → feedback+
Comment on attachment 8569730 [details] [diff] [review]
0009-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch

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

LGTM

::: dom/canvas/ImageBitmap.cpp
@@ +275,5 @@
>  
> +/* static */ already_AddRefed<ImageBitmap>
> +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageBitmap& aImageBitmap, ErrorResult& aRv)
> +{
> +  RefPtr<DataSourceSurface> dataSurface = aImageBitmap.mSurface->GetDataSurface();

I think you should add NS_ENSURE_TRUE(dataSurface, NS_ERROR_FAILURE) or !dataSurface to make sure everything goes well.
Attachment #8569730 - Flags: feedback?(ctai) → feedback+
Comment on attachment 8569731 [details] [diff] [review]
0010-Bug-1044102-Part-9-Blob-as-ImageBitamp-sources.patch

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

LGTM
Attachment #8569731 - Flags: feedback?(ctai) → feedback+
(Assignee)

Comment 62

3 years ago
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #54)
> Comment on attachment 8569718 [details] [diff] [review]
> 0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch
> 
> Review of attachment 8569718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +31,5 @@
> > +{
> > +  return ImageBitmapBinding::Wrap(aCx, this);
> > +}
> > +
> > +/* static */ already_AddRefed<Promise>
> 
> ditto: new line for /* static *
OK.
 
> ::: dom/canvas/ImageBitmap.h
> @@ +26,5 @@
> > +
> > +class Promise;
> > +
> > +class ImageBitmap MOZ_FINAL : public nsISupports,
> > +                              public nsWrapperCache
> 
> I think you miss the cycle collection part. 
> "Does the object inherit from nsWrapperCache (directly or indirectly)?  If
> so, it must be cycle collected."
> Quoted from http://blog.kylehuey.com/post/27564411715/cycle-collection
Yes, I will use codes generated from code-generator.
(Assignee)

Comment 63

3 years ago
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #57)
> Comment on attachment 8569720 [details] [diff] [review]
> 0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch
> 
> Review of attachment 8569720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Be careful of the nullptr check.
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +21,5 @@
> > +  if (res.mIsWriteOnly) {
> > +    return false;
> > +  }
> > +
> > +  if (res.mCORSUsed) {
> 
> Why not use early return? I guess this is a method which will be called many
> times.
> if (!res.mCORSUsed) { return true;}
>  
> @@ +25,5 @@
> > +  if (res.mCORSUsed) {
> > +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> > +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();
> > +    if (!(principal->Subsumes(res.mPrincipal))) {
> > +      return false;
> 
> if (principal->Subsumes(res.mPrincipal)) {
>   return true; 
> }
> return false;
> 
> Will that better?

These two parts are logical error. It should be "if (!res.mCORSUsed)" and then early returns the false case.

> @@ +23,5 @@
> > +  }
> > +
> > +  if (res.mCORSUsed) {
> > +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> > +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();
> 
> You should do nullptr check before use it. |PrincipalOrNull| is not
> guaranteed to return a non-nullptr.
OK.
 
> @@ +39,5 @@
> > +  nsresult rv;
> > +
> > +  nsCOMPtr<imgIRequest> imgRequest;
> > +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> > +  if (!NS_FAILED(rv) && imgRequest) {
> 
> Why not use if (NS_SUSSEEDED(rv) && imgRequest) { ?
OK.

> @@ +42,5 @@
> > +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> > +  if (!NS_FAILED(rv) && imgRequest) {
> > +    nsCOMPtr<imgIContainer> imgContainer;
> > +    rv = imgRequest->GetImage(getter_AddRefs(imgContainer));
> > +    if (!NS_FAILED(rv) && imgContainer->GetType() == imgIContainer::TYPE_RASTER) {
> 
> ditto.
> Is that possible imgContainer a null-ptr?
The "!NS_FAILED(rv)" checks null-ptr ahead. I will also use NS_SUSSEEDED(rv) here.
(Assignee)

Comment 64

3 years ago
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #56)
> Comment on attachment 8569724 [details] [diff] [review]
> 0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch
> 
> Review of attachment 8569724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fix some nits.
> 
> ::: dom/canvas/test/test_imagebitmap.html
> @@ +45,5 @@
> > +    {'rect': [0, 0, 128, 128],        'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [255, 0, 0, 255], 5]]},
> > +    {'rect': [128, 0, 128, 128],      'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [0, 255, 0, 255], 5]]},
> > +    {'rect': [230, 230, 128, 128],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [255, 0, 0, 255], 5],
> > +                                                                                           [100,  100,  [0, 0, 0, 0],     5]]},
> > +    {'rect': [-64, -64, 512, 512],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [0, 0, 0, 0],     5], 
> 
> nit: remove the last space
> 
> @@ +67,5 @@
> > +  function createBitmap(def) {
> > +    return createImageBitmap(image, def.rect[0], def.rect[1], def.rect[2], def.rect[3])
> > +      .then(function (bitmap) { def.bitmap = bitmap; }, failed);
> > +  };
> > +  
> 
> nit:
> remove leading space

Ok for both cases.
(Assignee)

Comment 65

3 years ago
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #58)
> Comment on attachment 8569726 [details] [diff] [review]
> 0007-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch
> 
> Review of attachment 8569726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +225,5 @@
> >  
> > +/* static */ already_AddRefed<ImageBitmap>
> > +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData, ErrorResult& aRv)
> > +{
> > +  // TODO: check the data of ImageData is neutered or not
> 
> Is there a bug related this TODO?
Sorry that it is a missed part. I will fill it up.
Neutured ImageData (or ArrayBuffer) will be set to zero dimention (length).
Reference bug: bug 1011574

> @@ +235,5 @@
> > +
> > +  array.ComputeLengthAndData();
> > +  uint32_t const imageWidth = aImageData.Width();
> > +  uint32_t const imageHeight = aImageData.Height();
> > +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData
> 
> This is based on the assumption that the format of ImageData is RGBA. Be
> careful here once this assumption is not true.
> 
> @@ +239,5 @@
> > +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData
> > +  uint32_t const dataLength = array.Length();
> > +  gfx::IntSize const imageSize(imageWidth, imageHeight);
> > +
> > +  RefPtr<DataSourceSurface> surface = Factory::CreateDataSourceSurfaceWithStride(imageSize, SurfaceFormat::B8G8R8A8, imageStride);
> 
> ditto.
Ok, I will rearrange this part so that it will be more adaptive to other format.
(Assignee)

Comment 66

3 years ago
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #59)
> Comment on attachment 8569728 [details] [diff] [review]
> 0008-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch
> 
> Review of attachment 8569728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +263,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  IntSize surfaceSize = surface->GetSize();
> > +  if (surfaceSize.width == 0 || surfaceSize.height == 0) {
> 
> Is that possible surfaceSize.width < 0?

No, it won't be. However, the spec only checke the zero dimention case.
(Assignee)

Comment 67

3 years ago
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #60)
> Comment on attachment 8569730 [details] [diff] [review]
> 0009-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch
> 
> Review of attachment 8569730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +275,5 @@
> >  
> > +/* static */ already_AddRefed<ImageBitmap>
> > +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageBitmap& aImageBitmap, ErrorResult& aRv)
> > +{
> > +  RefPtr<DataSourceSurface> dataSurface = aImageBitmap.mSurface->GetDataSurface();
> 
> I think you should add NS_ENSURE_TRUE(dataSurface, NS_ERROR_FAILURE) or
> !dataSurface to make sure everything goes well.

OK!

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #61)
> Comment on attachment 8569731 [details] [diff] [review]
> 0010-Bug-1044102-Part-9-Blob-as-ImageBitamp-sources.patch
> 
> Review of attachment 8569731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM

Thanks.
(Assignee)

Comment 68

3 years ago
Created attachment 8573660 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

Add the cycle collection codes.
Attachment #8569718 - Attachment is obsolete: true
Attachment #8573660 - Flags: feedback?(ctai)
(Assignee)

Comment 69

3 years ago
Created attachment 8573665 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

Fix according to comments.
Attachment #8569720 - Attachment is obsolete: true
Attachment #8573665 - Flags: feedback?(ctai)
(Assignee)

Comment 70

3 years ago
Created attachment 8573667 [details] [diff] [review]
0011-Bug-1044102-Part-10-Add-test-cases-of-security-check.patch

Add test casrs for "security checks".
Attachment #8573667 - Flags: feedback?(ctai)
Comment on attachment 8573660 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

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

LGTM. Thanks!
Attachment #8573660 - Flags: feedback?(ctai) → feedback+
Comment on attachment 8573665 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

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

LGTM. Thanks!

::: dom/canvas/ImageBitmap.cpp
@@ +127,5 @@
> +
> +  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
> +      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +        return nullptr;

nit: alignment.
Attachment #8573665 - Flags: feedback?(ctai) → feedback+
Comment on attachment 8573667 [details] [diff] [review]
0011-Bug-1044102-Part-10-Add-test-cases-of-security-check.patch

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

LGTM.
Attachment #8573667 - Flags: feedback?(ctai) → feedback+
(Assignee)

Comment 74

3 years ago
Created attachment 8573820 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch
Attachment #8573660 - Attachment is obsolete: true
Attachment #8573820 - Flags: review?(ehsan)
(Assignee)

Comment 75

3 years ago
Created attachment 8573821 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

Carry r+.
Attachment #8569719 - Attachment is obsolete: true
Attachment #8573821 - Flags: review+
(Assignee)

Comment 76

3 years ago
Created attachment 8573822 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch
Attachment #8573665 - Attachment is obsolete: true
Attachment #8573822 - Flags: review?(ehsan)
(Assignee)

Comment 77

3 years ago
Comment on attachment 8573821 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

Sorry, my fault. This patch needs to be reviewed.
Attachment #8573821 - Flags: review+ → review?(ehsan)
(Assignee)

Comment 78

3 years ago
Created attachment 8573823 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

Carry r+.
Attachment #8569721 - Attachment is obsolete: true
Attachment #8573823 - Flags: review+
(Assignee)

Comment 79

3 years ago
Created attachment 8573824 [details] [diff] [review]
0004-Bug-1044102-Part-3-Implement-cropping-and-surface-pr.patch

Carry r+.
Attachment #8573823 - Attachment is obsolete: true
Attachment #8573824 - Flags: review+
(Assignee)

Comment 80

3 years ago
Created attachment 8573825 [details] [diff] [review]
0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch

Carry r+.
Attachment #8569723 - Attachment is obsolete: true
Attachment #8573825 - Flags: review+
(Assignee)

Comment 81

3 years ago
Created attachment 8573826 [details] [diff] [review]
0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

Carry r+.
Attachment #8569724 - Attachment is obsolete: true
Attachment #8573826 - Flags: review+
(Assignee)

Comment 82

3 years ago
Created attachment 8573827 [details] [diff] [review]
0007-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch
Attachment #8569726 - Attachment is obsolete: true
Attachment #8573827 - Flags: review?(ehsan)
(Assignee)

Comment 83

3 years ago
Created attachment 8573829 [details] [diff] [review]
0008-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch
Attachment #8569728 - Attachment is obsolete: true
Attachment #8573829 - Flags: review?(ehsan)
(Assignee)

Comment 84

3 years ago
Created attachment 8573831 [details] [diff] [review]
0009-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch
Attachment #8569730 - Attachment is obsolete: true
Attachment #8573831 - Flags: review?(ehsan)
(Assignee)

Comment 85

3 years ago
Created attachment 8573835 [details] [diff] [review]
0010-Bug-1044102-Part-9-Blob-as-ImageBitamp-sources.patch
Attachment #8569731 - Attachment is obsolete: true
Attachment #8573835 - Flags: review?(ehsan)
(Assignee)

Comment 86

3 years ago
Created attachment 8573836 [details] [diff] [review]
0011-Bug-1044102-Part-10-Add-test-cases-of-security-check.patch
Attachment #8573667 - Attachment is obsolete: true
Attachment #8573836 - Flags: review?(ehsan)
(Assignee)

Comment 87

3 years ago
Jon mentioned that we need to support ImageBitmap as texImage in WebGL and add ImageBitmap as a transferable. This sounds convincing and should be very useful. However, I cannot find a written spec for these two scenarios now. Do I miss anything?
Flags: needinfo?(ehsan)
Comment on attachment 8573825 [details] [diff] [review]
0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch

Why is using nullptr for the CanvasPattern principal OK?  At the very least that deserves a very nice comment... but as far as I can tell, the one consumer of that member can't handle null.
Flags: needinfo?(tkuo)
Comment on attachment 8573826 [details] [diff] [review]
0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

Why do you need SpecialPowers stuff here?
I will probably get to these reviews next week.

(In reply to Tzuhao Kuo [:kaku] from comment #87)
> Jon mentioned that we need to support ImageBitmap as texImage in WebGL and
> add ImageBitmap as a transferable. This sounds convincing and should be very
> useful. However, I cannot find a written spec for these two scenarios now.
> Do I miss anything?

I don't think there is a spec for this yet.  I think roc had a write-up for the API, but I can't find it right now.  roc, can you please point to that?
Flags: needinfo?(ehsan) → needinfo?(roc)
Here is the proposal to make ImageBitmap transferable:
https://wiki.whatwg.org/wiki/WorkerCanvas

I don't know of a written proposal to use ImageBitmap with texImage, but it seems a very logical thing to do.
Flags: needinfo?(roc)
(Assignee)

Comment 92

3 years ago
(In reply to Not doing reviews right now from comment #88)
> Comment on attachment 8573825 [details] [diff] [review]
> 0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch
> 
> Why is using nullptr for the CanvasPattern principal OK?  At the very least
> that deserves a very nice comment... but as far as I can tell, the one
> consumer of that member can't handle null.

Sorry that this is a missed part when I was handing security. For doing this part right, an successfully created ImageBitmap should be assigned with an origin. However, there is no description on how to assign an origin to an ImageBitmap in the spec. 

It is intuitive to assign the input source's origin to the newly created ImageBitmap, but not every input source type carries an origin, so this idea is not doable in practical. In other way, we could assign the origin of the script who creates this ImageBitmap to the newly created ImageBitmap. (A discussion is needed for using EnteySettingObject or IncumbentSettingObject.)

Is this way ok?

(In reply to Not doing reviews right now from comment #89)
> Comment on attachment 8573826 [details] [diff] [review]
> 0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch
> 
> Why do you need SpecialPowers stuff here?
No..., I do not use it. I think I could just get rid of it. Thanks.
Flags: needinfo?(tkuo) → needinfo?(bzbarsky)
(Assignee)

Updated

3 years ago
Flags: needinfo?(bzbarsky)
> an successfully created ImageBitmap should be assigned with an origin

Why?

Or to turn this around, what is the actual security model you're trying to create ImageBitmap?  Is it that once you create one you can use it wherever and it never induces tainting (which is what the spec seems like to me)?  If so, it may be sufficient to set mCORSUsed to true, change the assert in DoDrawImageSecurityCheck to only assert aPrincipal if CORSUsed is false, and add some documentation about how mPrincipal can be null if mCORSUsed is true.

If, on the other hand, you want some other security model, I'd like to understand what this other security model is.
(Assignee)

Comment 94

3 years ago
(In reply to Not doing reviews right now from comment #93)
> > an successfully created ImageBitmap should be assigned with an origin
> 
> Why?
> 
> Or to turn this around, what is the actual security model you're trying to
> create ImageBitmap?  Is it that once you create one you can use it wherever
> and it never induces tainting (which is what the spec seems like to me)?  If
> so, it may be sufficient to set mCORSUsed to true, change the assert in
> DoDrawImageSecurityCheck to only assert aPrincipal if CORSUsed is false, and
> add some documentation about how mPrincipal can be null if mCORSUsed is true.
> 
> If, on the other hand, you want some other security model, I'd like to
> understand what this other security model is.

I was thinking about that the ImageBitmap might taint Canvases or CanvasPatterns. But I think you're right that ImageBitmap never taints others since the spec description about "how to do security check in drawImage() and createPattern() (https://html.spec.whatwg.org/#the-image-argument-is-not-origin-clean)" does not include ImageBitmap case. Thanks for your correction.
(Assignee)

Comment 95

3 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)
> Here is the proposal to make ImageBitmap transferable:
> https://wiki.whatwg.org/wiki/WorkerCanvas
> 
> I don't know of a written proposal to use ImageBitmap with texImage, but it
> seems a very logical thing to do.

Ok, I will file new bugs for those extensions of ImageBitmap. 
* Support structured cloning for ImageBitmap.
* ImageBitmap as a transferable object.
* ImageBitmp as texImage in WebGL.
Also file bugs for those issues need further discussion/attention.
* Blob.isClosed() is needed but not implemented yet.
* HtmlImage/VideoElement's principal does not match the concept of spec's origin.
(Assignee)

Comment 96

3 years ago
Created attachment 8575796 [details] [diff] [review]
0006-Bug-1044102-Part-4.1-Fix-bugs-in-createPattern-code-.patch
Attachment #8575796 - Flags: review?(ehsan)
(Assignee)

Comment 97

3 years ago
Created attachment 8575802 [details] [diff] [review]
Part-4.1-Fix-bugs-in-createPattern-code-.patch

Modified with bz's comment and add EnsureTarget() before calling ImageBitmap::PrepareForDrawTarget().
Attachment #8575796 - Attachment is obsolete: true
Attachment #8575796 - Flags: review?(ehsan)
Attachment #8575802 - Flags: review?(ehsan)
(Assignee)

Comment 98

3 years ago
Created attachment 8575803 [details] [diff] [review]
Part-5-Add-ImageBitmap-test.patch

Get rid of SpecialPowers. Carry r+.
Attachment #8573826 - Attachment is obsolete: true
Attachment #8575803 - Flags: review+
(Assignee)

Comment 99

3 years ago
Created attachment 8575804 [details] [diff] [review]
Part-6-ImageData-as-ImageBitamp-sources-.patch
Attachment #8573827 - Attachment is obsolete: true
Attachment #8573827 - Flags: review?(ehsan)
Attachment #8575804 - Flags: review?(ehsan)
(Assignee)

Comment 100

3 years ago
Created attachment 8575805 [details] [diff] [review]
Part-7-CanvasRenderingContext2D-as-Image.patch
Attachment #8573829 - Attachment is obsolete: true
Attachment #8573829 - Flags: review?(ehsan)
Attachment #8575805 - Flags: review?(ehsan)
(Assignee)

Comment 101

3 years ago
Created attachment 8575806 [details] [diff] [review]
Part-8-ImageBitmap-as-ImageBitamp-source.patch
Attachment #8573831 - Attachment is obsolete: true
Attachment #8573831 - Flags: review?(ehsan)
Attachment #8575806 - Flags: review?(ehsan)
(Assignee)

Comment 102

3 years ago
Created attachment 8575807 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch
Attachment #8573835 - Attachment is obsolete: true
Attachment #8573835 - Flags: review?(ehsan)
(Assignee)

Comment 103

3 years ago
Created attachment 8575809 [details] [diff] [review]
Part-10-Add-test-cases-of-security-check.patch

Add test cases for:
(1) security checks
(2) ImageBitmap as source of CreatePattern().
Attachment #8573836 - Attachment is obsolete: true
Attachment #8573836 - Flags: review?(ehsan)
Attachment #8575809 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8575807 - Flags: review?(ehsan)
Tzuhao, I'm sorry but I will be very busy for the next two weeks or so working on a very high priority project, so I'm planning to defer the reviews here until then.  Is that OK, or is this high priority?

Thanks!
Flags: needinfo?(tkuo)
(Assignee)

Comment 105

3 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #104)
It's ok and thanks for offering to review these patches.
Flags: needinfo?(tkuo)
(Assignee)

Updated

3 years ago
Blocks: 1141979
Attachment #8573820 - Flags: review?(ehsan) → review?(bugs)
Attachment #8573821 - Flags: review?(ehsan) → review?(bugs)
Attachment #8573822 - Flags: review?(ehsan) → review?(bugs)
Attachment #8575802 - Flags: review?(ehsan) → review?(bugs)
Attachment #8575804 - Flags: review?(ehsan) → review?(bugs)
Attachment #8575805 - Flags: review?(ehsan) → review?(bugs)
Attachment #8575806 - Flags: review?(ehsan) → review?(bugs)
Attachment #8575807 - Flags: review?(ehsan) → review?(bugs)
Attachment #8575809 - Flags: review?(ehsan) → review?(bugs)
I'll try to review this stuff tomorrow.
(In reply to Tzuhao Kuo [:kaku] from comment #95)
> > I don't know of a written proposal to use ImageBitmap with texImage, but it
> > seems a very logical thing to do.
...

> Also file bugs for those issues need further discussion/attention.
> * HtmlImage/VideoElement's principal does not match the concept of spec's
> origin.

Did you file this bug? I guess we might need a spec bug and Gecko bug.
There's a spec bug at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28374
Comment on attachment 8573820 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

>+already_AddRefed<Promise>
>+nsGlobalWindow::CreateImageBitmap(const ImageBitmapSource& image,
>+                                  ErrorResult& aRv)
s/image/aImage


>+already_AddRefed<Promise>
>+nsGlobalWindow::CreateImageBitmap(const ImageBitmapSource& image,
>+                                  int32_t sx, int32_t sy, int32_t sw, int32_t sh,
>+                                  ErrorResult& aRv)
aImage, aSx, aSy, aSw, aSh


>+  // ImageBitmapFactories methods
>+  already_AddRefed<mozilla::dom::Promise>
>+  CreateImageBitmap(const mozilla::dom::ImageBitmapSource& image,
>+                    mozilla::ErrorResult& aRv);
>+
>+  already_AddRefed<mozilla::dom::Promise>
>+  CreateImageBitmap(const mozilla::dom::ImageBitmapSource& image,
>+                    int32_t sx, int32_t sy, int32_t sw, int32_t sh,
>+                    mozilla::ErrorResult& aRv);
>+
Same nits here.

>+interface ImageBitmapFactories {
>+  [Throws]
>+  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
>+  [Throws]
>+  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
So for now it I'd like to see these being under some Pref.


>+  // ImageBitmapFactories methods
>+  already_AddRefed<Promise>
>+  CreateImageBitmap(const ImageBitmapSource& image, ErrorResult& aRv);
>+
>+  already_AddRefed<Promise>
>+  CreateImageBitmap(const ImageBitmapSource& image,
>+                    int32_t sx, int32_t sy, int32_t sw, int32_t sh,
>+                    ErrorResult& aRv);
Also here, arguments should be in form aFoo


r+ those fixed, and pref added. We may want to have it enabled first in Nightly/Aurora only, and disabled in beta/release.
Attachment #8573820 - Flags: review?(bugs) → review+
Comment on attachment 8573821 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch


>-ImageBitmap::ImageBitmap(nsIGlobalObject* aGlobal)
>-  : mParent(aGlobal)
>+ImageBitmap::ImageBitmap(nsIGlobalObject* aGlobal, SourceSurface* aSurface)
>+  : mCropRect(0, 0, aSurface->GetSize().width, aSurface->GetSize().height)
Ok, so this is mainthread only ctor, at least for now, given that SourceSurface is expected to be non-null.
Perhaps assert?

>-  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
>+  nsRefPtr<ImageBitmap> imageBitmap;
>+
>+  if (aSrc.IsHTMLImageElement()) {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLImageElement(), aRv);
>+  }
>+  else if (aSrc.IsHTMLVideoElement()) {
} else if (...)

>+    MOZ_ASSERT(!NS_IsMainThread());
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLVideoElement(), aRv);
>+  }
>+  else if (aSrc.IsHTMLCanvasElement()) {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLCanvasElement(), aRv);
>+  }
>+  else {
} else {


>+  if (!aRv.Failed()) {
>+    MessageLoop::current()->PostTask(
>+      FROM_HERE,
>+      NewRunnableFunction(&FulfillImageBitmapPromise, promise, imageBitmap));
Please use normal XPCOM event loop, not the IPC loop.
So, something like NS_DispatchToMainThread(NS_NewRunnableMethodWithArgs(...))
Attachment #8573821 - Flags: review?(bugs) → review-

Updated

3 years ago
Attachment #8575802 - Flags: review?(bugs) → review+
Comment on attachment 8575804 [details] [diff] [review]
Part-6-ImageData-as-ImageBitamp-sources-.patch

>+ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData, ErrorResult& aRv)
>+{
>+  // copy data into SourceSurface
>+  dom::Uint8ClampedArray array;
>+  DebugOnly<bool> inited = array.Init(aImageData.GetDataObject());
>+  MOZ_ASSERT(inited);
>+
>+  array.ComputeLengthAndData();
>+  SurfaceFormat const FORMAT = SurfaceFormat::B8G8R8A8;
>+  uint32_t const BYTES_PER_PIXEL = BytesPerPixel(FORMAT);
>+  uint32_t const imageWidth = aImageData.Width();
>+  uint32_t const imageHeight = aImageData.Height();
>+  uint32_t const imageStride = imageWidth * BYTES_PER_PIXEL;
>+  uint32_t const dataLength = array.Length();
>+  gfx::IntSize const imageSize(imageWidth, imageHeight);
>+
>+  // check the ImageData is neutered or not
>+  if (imageWidth == 0 || imageHeight == 0 ||
>+      (imageWidth*imageHeight*BYTES_PER_PIXEL) != dataLength) {
space before and after *

>+  RefPtr<DataSourceSurface> surface =
>+      Factory::CreateDataSourceSurfaceWithStride(imageSize, FORMAT, imageStride);
2 spaces for indentation.

This needs a review from a gfx peer. I don't know if this stuff is safe in worker threads (I think it is).
And we really must have tests for all this stuff run also in workers.

CreateDataSourceSurfaceWithStride may return null, so you need a null check.


>     imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLCanvasElement(), aRv);
>   }
>+  else if (aSrc.IsImageData()) {
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsImageData(), aRv);
>+  }
>   else {
Again,
} else if (...) {
  ...
} else {

r+ assuming all those handled.
Attachment #8575804 - Flags: review?(bugs) → review+
Comment on attachment 8575803 [details] [diff] [review]
Part-5-Add-ImageBitmap-test.patch

marking this r-, since we really need tests for the worker stuff.
Attachment #8575803 - Flags: review-
Comment on attachment 8575805 [details] [diff] [review]
Part-7-CanvasRenderingContext2D-as-Image.patch

} else if () {
Attachment #8575805 - Flags: review?(bugs) → review+
Comment on attachment 8575806 [details] [diff] [review]
Part-8-ImageBitmap-as-ImageBitamp-source.patch

2 spaces for indentation.
CreateDataSourceSurfaceWithStride may return null, so needs null check.

} else if (...) {
Attachment #8575806 - Flags: review?(bugs) → review+
Comment on attachment 8575809 [details] [diff] [review]
Part-10-Add-test-cases-of-security-check.patch

We need worker tests too.
It would be good to try to share as much as possible the same tests
for main thread and for workers.
Attachment #8575809 - Flags: review?(bugs) → review-
Comment on attachment 8575807 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

>+already_AddRefed<ImageBitmap>
>+ImageBitmap::CreateInternalFromBlobAsync(nsIGlobalObject* aGlobal, File& aBlob, ErrorResult& aRv)
>+{
Please add assertions that this is used only in the main thread.


>+  FileImpl *fileImpl = aBlob.Impl();
FileImpl* fileImpl

>+
>+  // get the internal stream of the blob
>+  nsIInputStream *stream = nullptr;
nsIInputStream* stream


>+  nsresult rv;
>+  rv = fileImpl->GetInternalStream(&stream);
You leak stream here.
Use nsCOMPtr<nsIInputStrea> stream; and then getter_AddRefs when passing it as param.


>+  // get the mine type string of the blob
>+  // the type will be checked in the DecodeImage() method.
>+  nsAutoString mineTypeUTF16;
>+  aBlob.GetType(mineTypeUTF16);

mine? Should be mime



>+  NS_ConvertUTF16toUTF8 mineTypeUTF8(mineTypeUTF16); // NS_ConvertUTF16toUTF8 ---|> nsAutoCString
>+  imgIContainer *imgContainer = nullptr;
>+  rv = aImgTools->DecodeImage(stream, mineTypeUTF8, &imgContainer);
Again, you leak imgContainer.
Please test all these patches in debug build.



>+class CreateImageBitmapFromBlobTask : public nsRunnable
>+{
>+public:
>+  CreateImageBitmapFromBlobTask(Promise* aPromise,
>+                                nsIGlobalObject* aGlobal,
>+                                mozilla::dom::File& aBlob,
>+                                bool aCrop, IntRect const &aCropRect)
>+  :mPromise(aPromise), mGlobalObject(aGlobal), mBlob(&aBlob),
>+   mCrop(aCrop), mCropRect(aCropRect)
>+  {}
>+
>+  NS_IMETHOD Run()
>+  {
>+    ErrorResult error;
>+    nsRefPtr<ImageBitmap> imageBitmap =
>+        ImageBitmap::CreateInternalFromBlobAsync(mGlobalObject, *mBlob, error);
2 spaces for indentation.

>+
>+    // handle errors while creating ImageBitmap
>+    // (1) error occurs during reading of the object
>+    // (2) the image data is not in a supported file format
>+    // (3) the image data is corrupted
>+    // All these three cases should reject promise with null value
>+    if (error.Failed()) {
>+      mPromise->MaybeReject(error);
>+      return error.ErrorCode();
>+    }
>+
>+    if (imageBitmap && mCrop) {
>+      imageBitmap->SetCrop(mCropRect, error);
>+
>+      if (error.Failed()) {
>+        mPromise->MaybeReject(error);
>+        return error.ErrorCode();
>+      }
>+    }
>+
>+    FulfillImageBitmapPromise(mPromise, imageBitmap);
>+    return error.ErrorCode();
>+  }
>+
>+private:
>+  nsRefPtr<Promise> mPromise;
>+  nsCOMPtr<nsIGlobalObject> mGlobalObject;
>+  RefPtr<mozilla::dom::File> mBlob;
>+  bool mCrop;
>+  IntRect mCropRect;
Promise and globalobject must be released and used only in the same thread they are created.

>+  else if (aSrc.IsBlob()) {
>+    // check blob is closed or not. Bug1048325
>+    nsCOMPtr<nsIRunnable> runnable =
>+        new CreateImageBitmapFromBlobTask(promise, aGlobal, aSrc.GetAsBlob(), aCrop, aCropRect);
>+    NS_DispatchToMainThread(runnable);
>+    return promise.forget();
>+  }
This is not safe in workers. You pass worker-only objects to main thread.
Attachment #8575807 - Flags: review?(bugs) → review-
Comment on attachment 8573822 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

This should be update to use the normal coding style.
Foo* variableName
and aFoo for argument naming


This uses GetEntryGlobal() and I thought we'd go with incumbent global.
Also, Get*Global methods may return null.
Attachment #8573822 - Flags: review?(bugs) → review-
(Assignee)

Comment 118

3 years ago
Thank you for the review!
Sorry for lots of coding style problem and even typos, I will fix them all.
Also will I catch up test cases for worker.

(In reply to Olli Pettay [:smaug] from comment #107)
> > Also file bugs for those issues need further discussion/attention.
> > * HtmlImage/VideoElement's principal does not match the concept of spec's
> > origin.
> 
> Did you file this bug? I guess we might need a spec bug and Gecko bug.
(In reply to Olli Pettay [:smaug] from comment #117)
> This uses GetEntryGlobal() and I thought we'd go with incumbent global.
> Also, Get*Global methods may return null.
I haven't filed all the follow-up bugs and will do it when this bug is getting done.
So, for getting this bug done, I will use the incumbent global.

(In reply to Olli Pettay [:smaug] from comment #116)
> Comment on attachment 8575807 [details] [diff] [review]
> Part-9-Blob-as-ImageBitamp-sources.patch
> 
> >+already_AddRefed<ImageBitmap>
> >+ImageBitmap::CreateInternalFromBlobAsync(nsIGlobalObject* aGlobal, File& aBlob, ErrorResult& aRv)
> >+{
> Please add assertions that this is used only in the main thread.
> 
> >+private:
> >+  nsRefPtr<Promise> mPromise;
> >+  nsCOMPtr<nsIGlobalObject> mGlobalObject;
> >+  RefPtr<mozilla::dom::File> mBlob;
> >+  bool mCrop;
> >+  IntRect mCropRect;
> Promise and globalobject must be released and used only in the same thread
> they are created.
> 
> >+  else if (aSrc.IsBlob()) {
> >+    // check blob is closed or not. Bug1048325
> >+    nsCOMPtr<nsIRunnable> runnable =
> >+        new CreateImageBitmapFromBlobTask(promise, aGlobal, aSrc.GetAsBlob(), aCrop, aCropRect);
> >+    NS_DispatchToMainThread(runnable);
> >+    return promise.forget();
> >+  }
> This is not safe in workers. You pass worker-only objects to main thread.
For the blob-as-source case, I use the mozilla::image::RasetrImage to decode the blob and the implementation now is main-thread only. I will add a assertion in the ImageBitmap::Create() method to prevent be called from worker.

And OK for all others.
(Assignee)

Comment 119

3 years ago
Created attachment 8595318 [details] [diff] [review]
Part-1-ImageBitmap-createImageBitmap-Web.patch

Carry r+.
Attachment #8573820 - Attachment is obsolete: true
Attachment #8595318 - Flags: review+
(Assignee)

Comment 120

3 years ago
Created attachment 8595319 [details] [diff] [review]
Part-2-Support-HTML-Image-Video-Canvas-E.patch
Attachment #8573821 - Attachment is obsolete: true
Attachment #8595319 - Flags: review?(bugs)
(Assignee)

Comment 121

3 years ago
Created attachment 8595321 [details] [diff] [review]
Part-2.1-Security-and-status-checks.patch
Attachment #8573822 - Attachment is obsolete: true
Attachment #8595321 - Flags: review?(bugs)
(Assignee)

Comment 122

3 years ago
Created attachment 8595322 [details] [diff] [review]
Part-6-ImageData-as-ImageBitamp-sources-.patch

Carry r+.
Attachment #8575804 - Attachment is obsolete: true
Attachment #8595322 - Flags: review+
(Assignee)

Comment 123

3 years ago
Created attachment 8595323 [details] [diff] [review]
Part-7-CanvasRenderingContext2D-as-Image.patch

Carry r+.
Attachment #8575805 - Attachment is obsolete: true
Attachment #8595323 - Flags: review+
(Assignee)

Comment 124

3 years ago
Created attachment 8595324 [details] [diff] [review]
Part-8-ImageBitmap-as-ImageBitamp-source.patch

Carry r+.
Attachment #8575806 - Attachment is obsolete: true
Attachment #8595324 - Flags: review+
(Assignee)

Comment 125

3 years ago
Created attachment 8595325 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch
Attachment #8575807 - Attachment is obsolete: true
Attachment #8595325 - Flags: review?(bugs)
(Assignee)

Comment 126

3 years ago
Fix coding style problem, types, memory leakage (tried with debug build), main-thready-only assertion, null pointer checks, XPCOM event loop and the setting object issue.

Upload patches which are related to the above issues first and keep writing test cases for worker.
Comment on attachment 8595319 [details] [diff] [review]
Part-2-Support-HTML-Image-Video-Canvas-E.patch

>+  NS_IMETHOD Run() {
Nit, { goes to its own line.

>+  if (aSrc.IsHTMLImageElement()) {
>+    MOZ_ASSERT(NS_IsMainThread(), "Create ImageBitmap from HTMLImageElement off main thread.");
"Createing ImageBitmap from HTMLImageElement off the main thread.", or something like that I think. Also elsewhere.
Attachment #8595319 - Flags: review?(bugs) → review+
Comment on attachment 8595325 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

We support Blobs on workers, so CreateImageBitmapFromBlobTask etc. need to be able to deal with both main thread and worker threads.
SourceSurface doesn't seem to have threadsafe refcounting, so perhaps you need to
create SourceSurface on the main thread always, and then in case of workers just pass the data it contains
to worker thread.


Nit, aImgTools has 'a' as prefix, but 'a' should be used with arguments only.

Please test this all in debug build and test workers and main thread.
Attachment #8595325 - Flags: review?(bugs) → review-
Comment on attachment 8595321 [details] [diff] [review]
Part-2.1-Security-and-status-checks.patch

>+CheckNonBitmapSource(HTMLImageElement& aImageEl)
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<imgIRequest> imgRequest;
>+  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
Just declare rv here where you use it first time.

> ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, HTMLVideoElement& aVideoEl, ErrorResult& aRv)
> {
>-  // TODO: check network state
>-  // TODO: check origin
>-  // TODO: check ready state
>+  if (aVideoEl.NetworkState() == HTMLMediaElement::NETWORK_EMPTY) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return nullptr;
>+  }
>+
>+  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
>+      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return nullptr;
>+  }
Why you aren't doing the security check for HTMLVideoElement as the spec says?
Attachment #8595321 - Flags: review?(bugs) → review-
(Assignee)

Comment 130

3 years ago
(In reply to Olli Pettay [:smaug] from comment #128)
> Comment on attachment 8595325 [details] [diff] [review]
> Part-9-Blob-as-ImageBitamp-sources.patch
> 
> We support Blobs on workers, so CreateImageBitmapFromBlobTask etc. need to
> be able to deal with both main thread and worker threads.
> SourceSurface doesn't seem to have threadsafe refcounting, so perhaps you
> need to
> create SourceSurface on the main thread always, and then in case of workers
> just pass the data it contains
> to worker thread.
But it seems that the image decoding code path (through RasterImage) is main thread only now, do you mean passing the Blob to main thread, decoding it there and then pass the decoded image raw data back to worker thread?
 
> Nit, aImgTools has 'a' as prefix, but 'a' should be used with arguments only.
> 
> Please test this all in debug build and test workers and main thread.
OK.
Flags: needinfo?(bugs)
(Assignee)

Comment 131

3 years ago
(In reply to Olli Pettay [:smaug] from comment #129)
> Comment on attachment 8595321 [details] [diff] [review]
> Part-2.1-Security-and-status-checks.patch
> 
> >+CheckNonBitmapSource(HTMLImageElement& aImageEl)
> >+{
> >+  nsresult rv;
> >+
> >+  nsCOMPtr<imgIRequest> imgRequest;
> >+  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> Just declare rv here where you use it first time.
> 
> > ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, HTMLVideoElement& aVideoEl, ErrorResult& aRv)
> > {
> >-  // TODO: check network state
> >-  // TODO: check origin
> >-  // TODO: check ready state
> >+  if (aVideoEl.NetworkState() == HTMLMediaElement::NETWORK_EMPTY) {
> >+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> >+    return nullptr;
> >+  }
> >+
> >+  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
> >+      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {
> >+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> >+    return nullptr;
> >+  }
> Why you aren't doing the security check for HTMLVideoElement as the spec
> says?
The security checking is modularized in the CheckSecurityForHTMLElements() function which is called in the CreateFromElement() method. The order of these checks is somewhat different from the spec addressed, is it ok?
(Assignee)

Comment 132

2 years ago
Created attachment 8598589 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

Enable creating ImageBitmap from Blob in both main-thread and worker.
Currently, we cannot decode a blob off the main thread, so in this patch, I work around by dispatching a decoding-blob-task to main thread.
Attachment #8595325 - Attachment is obsolete: true
Attachment #8598589 - Flags: review?(bugs)
(Assignee)

Comment 133

2 years ago
Created attachment 8598590 [details] [diff] [review]
Part-10-Extend-test-cases.patch

Added the following test cases:
1. blob as source
2. security checks
3. usage of ctx.createPattern
4. test exceptions from
   (1) neutured ImageBitmap
   (2) corrupted Blob
   (3) non-image Blob
5. Worker scenario (HTMLXXXElements and CanvasRenderingContext2d are not allowed in worker.)
Attachment #8575809 - Attachment is obsolete: true
Attachment #8598590 - Flags: review?(bugs)
Comment on attachment 8598589 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

>+ImageBitmap::CreateInternalFromRawData(nsIGlobalObject* aGlobal, uint8_t* aData, gfx::IntSize& aSize, uint32_t aStride, gfx::SurfaceFormat& aFormat, ErrorResult& aRv)
Overlong line.


>+{
>+  RefPtr<DataSourceSurface> surface =
>+    Factory::CreateWrappingDataSourceSurface(aData, aStride, aSize, aFormat);
I don't know whether this is safe to use on any thread. Needs review from a gfx peer.

>+RejectImageBitmapPromise(Promise* aPromise, nsresult aErrorCode)
>+{
>+  aPromise->MaybeReject(aErrorCode);
>+}
Why not just call
promise->MaybeReject(errorCode) in the caller?
Don't really see a reason for RejectImageBitmapPromise


>+  NS_IMETHOD Cancel() override {
{ goes to the next line.

>+    RejectImageBitmapPromise(mPromise, NS_ERROR_ABORT);
I don't think we can really do anything in case Cancel is called. The worker is being killed at that point
So just leave Cancel() empty and return null or something like that.
Same with other Cancel()s too.

>+class CreateImageBitmapFromRawDataTask : public nsCancelableRunnable
>+{
>+public:
>+  CreateImageBitmapFromRawDataTask(nsRefPtr<Promise>& aPromise,
>+                                   nsCOMPtr<nsIGlobalObject>& aGlobal,
>+                                   nsRefPtr<mozilla::dom::File>& aBlob,
>+                                   bool aCrop, IntRect const &aCropRect,
>+                                   uint8_t* aData, uint32_t aStride,
>+                                   IntSize& aSize, SurfaceFormat aFormat,
>+                                   nsresult aRv)
>+  :mCrop(aCrop), mCropRect(aCropRect),
>+   mData(aData), mStride(aStride), mSize(aSize), mFormat(aFormat),
>+   mError(aRv)
>+  {
>+    MOZ_ASSERT(NS_IsMainThread());
>+    mPromise.swap(aPromise);
>+    mGlobalObject.swap(aGlobal);
>+    mBlob.swap(aBlob);
The issue with this class is that nothing guarantees it is ever run or released on the worker thread.
The worker might have been closed already.
And if that is the case, DecodeImageFromBlobInTheMainThreadTask::Run might be the place where the
object is deleted, and that means that mPromise, mGlobalObject and mBlob are possibly deleted on the wrong thread.
Sounds like you want to use something like WorkerMainThreadRunnable in case you're in worker.
(GetCurrentThreadWorkerPrivate() gives you the thing WorkerMainThreadRunnable ctor needs.)
Sorry, I should have realized this earlier.

>+  // decode the bolb into a DataSourceSurface
blob

>+      }
>+      else {
} else {
Flags: needinfo?(bugs)
Attachment #8598589 - Flags: review?(bugs) → review-
Comment on attachment 8598590 [details] [diff] [review]
Part-10-Extend-test-cases.patch

>+    promiseThrows(createImageBitmapWithNeuturedImageData(), "createImageBitmap should thorw with neutured ImageData"),
>+    promiseThrows(createImageBitmapWithCorruptedBlob(), "createImageBitmap should thorw with corrupted blob"),
>+    promiseThrows(createImageBitmapWithNonImageFile(), "createImageBitmap should thorw with non-image blob"),
throw, not thorw


>+// The follwing tests is not enabled in Worker now:
following
Attachment #8598590 - Flags: review?(bugs) → review+
(Assignee)

Comment 136

2 years ago
(In reply to Olli Pettay [:smaug] from comment #134)
> >+class CreateImageBitmapFromRawDataTask : public nsCancelableRunnable
> >+{
> >+public:
> >+  CreateImageBitmapFromRawDataTask(nsRefPtr<Promise>& aPromise,
> >+                                   nsCOMPtr<nsIGlobalObject>& aGlobal,
> >+                                   nsRefPtr<mozilla::dom::File>& aBlob,
> >+                                   bool aCrop, IntRect const &aCropRect,
> >+                                   uint8_t* aData, uint32_t aStride,
> >+                                   IntSize& aSize, SurfaceFormat aFormat,
> >+                                   nsresult aRv)
> >+  :mCrop(aCrop), mCropRect(aCropRect),
> >+   mData(aData), mStride(aStride), mSize(aSize), mFormat(aFormat),
> >+   mError(aRv)
> >+  {
> >+    MOZ_ASSERT(NS_IsMainThread());
> >+    mPromise.swap(aPromise);
> >+    mGlobalObject.swap(aGlobal);
> >+    mBlob.swap(aBlob);
> The issue with this class is that nothing guarantees it is ever run or
> released on the worker thread.
> The worker might have been closed already.
> And if that is the case, DecodeImageFromBlobInTheMainThreadTask::Run might
> be the place where the
> object is deleted, and that means that mPromise, mGlobalObject and mBlob are
> possibly deleted on the wrong thread.
> Sounds like you want to use something like WorkerMainThreadRunnable in case
> you're in worker.
> (GetCurrentThreadWorkerPrivate() gives you the thing
> WorkerMainThreadRunnable ctor needs.)
> Sorry, I should have realized this earlier.
Yes, that is what I expect, thanks for your explanation and hint!

Ok to all the others.
(Assignee)

Comment 137

2 years ago
Created attachment 8599757 [details] [diff] [review]
Part-7-CanvasRenderingContext2D-as-Image.patch

Carry r+.
Add an assertion while creating an ImageBitmap from CanvasRenderingContext2D off the main thread.
Attachment #8595323 - Attachment is obsolete: true
Attachment #8599757 - Flags: review+
(Assignee)

Comment 138

2 years ago
Created attachment 8599758 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch
Attachment #8598589 - Attachment is obsolete: true
Attachment #8599758 - Flags: review?(bugs)
(Assignee)

Comment 139

2 years ago
Created attachment 8599760 [details] [diff] [review]
Part-10-Extend-test-cases.patch

Carry r+.
Attachment #8598590 - Attachment is obsolete: true
Attachment #8599760 - Flags: review+
Comment on attachment 8599758 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

>-static void
>+static inline void
> FulfillImageBitmapPromise(Promise* aPromise, ImageBitmap* aImageBitmap)
> {
>   MOZ_ASSERT(aPromise);
>   aPromise->MaybeResolve(aImageBitmap);
> }
The code might be easier to read if you didn't have this method at all, and
just called promise->MaybeResolve(imageBitmap); when needed

> 
>-class FulfillImageBitmapPromiseTask : public nsRunnable
>+class IFulfillImageBitmapPromise
Why 'I' prefix?  just drop it

>+class ICreateImageBitmapFromBlob
I would drop the first 'I' also here. 


>+  ICreateImageBitmapFromBlob(Promise* aPromise,
>+                             nsIGlobalObject* aGlobal,
>+                             File& aBlob,
>+                             bool aCrop,
>+                             IntRect const &aCropRect)
nit, & goes with the type, not the variable name

>+  // This is a synchronous task.
>+  class DecodeBlobInMainThreadSyncedTask final : public WorkerMainThreadRunnable
s/Synced/Sync/

>+
>+  bool WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
>+  {
>+    ErrorResult error;
>+    CreateImageBitmapFromBlob(error);
>+    return error.Failed() ? false : true;

ErrorResult.Failed() returns boolean, so no need for ?:
Attachment #8599758 - Flags: review?(bugs) → review+
(Assignee)

Comment 141

2 years ago
Created attachment 8601846 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

Carry r+.
Attachment #8599758 - Attachment is obsolete: true
Attachment #8601846 - Flags: review+
(Assignee)

Comment 142

2 years ago
Comment on attachment 8575803 [details] [diff] [review]
Part-5-Add-ImageBitmap-test.patch

(In reply to Olli Pettay [:smaug] from comment #112)
> Comment on attachment 8575803 [details] [diff] [review]
> Part-5-Add-ImageBitmap-test.patch
> 
> marking this r-, since we really need tests for the worker stuff.

Hi smaug, 
Test cases for the worker have been added in the  Part-10-Extend-test-cases.patch (Attachment #8599760 [details] [diff]). Can we turn this patch into r+?
Attachment #8575803 - Flags: review+ → review?(bugs)
(Assignee)

Comment 143

2 years ago
Comment on attachment 8595321 [details] [diff] [review]
Part-2.1-Security-and-status-checks.patch

(In reply to Olli Pettay [:smaug] from comment #129)
> Comment on attachment 8595321 [details] [diff] [review]
> Part-2.1-Security-and-status-checks.patch
> 
> > ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, HTMLVideoElement& aVideoEl, ErrorResult& aRv)
> > {
> >-  // TODO: check network state
> >-  // TODO: check origin
> >-  // TODO: check ready state
> >+  if (aVideoEl.NetworkState() == HTMLMediaElement::NETWORK_EMPTY) {
> >+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> >+    return nullptr;
> >+  }
> >+
> >+  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
> >+      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {
> >+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> >+    return nullptr;
> >+  }
> Why you aren't doing the security check for HTMLVideoElement as the spec
> says?

The security checking is modularized in the CheckSecurityForHTMLElements() function which is called in the CreateFromElement() method. So for cases from HTMLImage/Video/CanvasElement, the CreateInternal(...) methods call CreateFromElement() in the end and check the origin security there. The order of these checks is somewhat different from the spec addressed, is it ok?
Attachment #8595321 - Flags: review- → review?(bugs)
(Assignee)

Comment 144

2 years ago
Comment on attachment 8595318 [details] [diff] [review]
Part-1-ImageBitmap-createImageBitmap-Web.patch

Hi smaug, 
Since this patch modify the WebAPI, can you also do the superreview for this patch?
Attachment #8595318 - Flags: superreview?(bugs)
(Assignee)

Comment 145

2 years ago
Comment on attachment 8573825 [details] [diff] [review]
0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch

Hi smaug, 
Same as the art-1-ImageBitmap-createImageBitmap-Web.patch, this patch modify the WebAPI, can you also do the superreview for this patch?
Attachment #8573825 - Flags: superreview?(bugs)
(Assignee)

Comment 146

2 years ago
Comment on attachment 8595322 [details] [diff] [review]
Part-6-ImageData-as-ImageBitamp-sources-.patch

(In reply to Olli Pettay [:smaug] from comment #111)
> Comment on attachment 8575804 [details] [diff] [review]
> Part-6-ImageData-as-ImageBitamp-sources-.patch
> >+  RefPtr<DataSourceSurface> surface =
> >+      Factory::CreateDataSourceSurfaceWithStride(imageSize, FORMAT, imageStride);
> 2 spaces for indentation.
> 
> This needs a review from a gfx peer. I don't know if this stuff is safe in
> worker threads (I think it is).

Hi roc,
May I have your review on this patch, especially on the function call to the Factory::CreateDataSourceSurfaceWithStride() smaug mentioned above? We want to make sure that if this utility function is safe in worker thread.
Attachment #8595322 - Flags: review?(roc)
(Assignee)

Comment 147

2 years ago
Comment on attachment 8601846 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

(In reply to Olli Pettay [:smaug] from comment #134)
> Comment on attachment 8598589 [details] [diff] [review]
> Part-9-Blob-as-ImageBitamp-sources.patch
> >+{
> >+  RefPtr<DataSourceSurface> surface =
> >+    Factory::CreateWrappingDataSourceSurface(aData, aStride, aSize, aFormat);
> I don't know whether this is safe to use on any thread. Needs review from a
> gfx peer.

Hi roc, 
Same as the comment #146, this patch also uses the Factory::CreateWrappingDataSourceSurface() utility function which we would like to have a review by a gfx peer to make sure if it is safe in the worker thread.
Attachment #8601846 - Flags: review?(roc)
So this all is still missing structured cloning for ImageBitmap, right?
I've been kind of expecting that, since without it ImageBitmap on workers isn't too useful.
Do we want to deal structured cloning in a separate bug? If so, let's not expose Imagebitmap yet
on workers by default (perhaps enable it using a pref, so that it can be tested?), only on Window context.
But I don't think structured cloning is actually too much work, so why not implement that now?

Updated

2 years ago
Attachment #8573825 - Flags: superreview?(bugs) → superreview+
Comment on attachment 8595318 [details] [diff] [review]
Part-1-ImageBitmap-createImageBitmap-Web.patch

So I'll sr this once we either have support for structured cloning, so that ImageBitmap on workers is useful thing, or Worker support is enabled only if some testing pref is set.
Attachment #8595318 - Flags: superreview?(bugs)
Comment on attachment 8575803 [details] [diff] [review]
Part-5-Add-ImageBitmap-test.patch

>+  // TODO: Add (1) ImageData and (2) Blob as input sources.
I think we really have to have tests also for ImageData and Blob as sources before we can think of releasing this.

(So much Promise usage - makes my uneducated-to-modern-JS-eyes hurt, but no need to change that because of
me)
Attachment #8575803 - Flags: review?(bugs) → review-

Updated

2 years ago
Attachment #8595321 - Flags: review?(bugs) → review+
Attachment #8595322 - Flags: review?(roc) → review+
(In reply to Olli Pettay [:smaug] from comment #148)
> So this all is still missing structured cloning for ImageBitmap, right?

Yes, but we should also be able to transfer it, since that's what people will actually want to do for efficiency.
Comment on attachment 8601846 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +572,5 @@
> +      if (mError->Failed()) {
> +        return false;
> +      }
> +
> +      // Read the raw data out from the SoruceSurface and pass the raw data

SourceSurface

@@ +643,5 @@
> +      mPromise->MaybeReject(aRv);
> +      return nullptr;
> +    }
> +
> +    // create SoruceSurface object from raw data

SourceSurface
Attachment #8601846 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #151)
> (In reply to Olli Pettay [:smaug] from comment #148)
> > So this all is still missing structured cloning for ImageBitmap, right?
> 
> Yes, but we should also be able to transfer it, since that's what people
> will actually want to do for efficiency.
I don't see transferring in the spec.
(and I imagine even structured cloning could just share the data in case of ImageBitmap)
(Assignee)

Comment 154

2 years ago
(In reply to Olli Pettay [:smaug] from comment #150)
> Comment on attachment 8575803 [details] [diff] [review]
> Part-5-Add-ImageBitmap-test.patch
> 
> >+  // TODO: Add (1) ImageData and (2) Blob as input sources.
> I think we really have to have tests also for ImageData and Blob as sources
> before we can think of releasing this.
> 
> (So much Promise usage - makes my uneducated-to-modern-JS-eyes hurt, but no
> need to change that because of
> me)

Test for ImageData has been added in Part-6-ImageData-as-ImageBitamp-sources-.patch.
Test for Blob has been added in  Part-10-Extend-test-cases.patch.
Sorry for my bad patches arrangement.
Would you like to see one single patch with all test cases?
Flags: needinfo?(bugs)
(Assignee)

Comment 155

2 years ago
(In reply to Olli Pettay [:smaug] from comment #153)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #151)
> > (In reply to Olli Pettay [:smaug] from comment #148)
> > > So this all is still missing structured cloning for ImageBitmap, right?
> > 
> > Yes, but we should also be able to transfer it, since that's what people
> > will actually want to do for efficiency.
> I don't see transferring in the spec.
> (and I imagine even structured cloning could just share the data in case of
> ImageBitmap)

It is not in standard spec yes but roc provides a proposal spec:
https://wiki.whatwg.org/wiki/WorkerCanvas
Part of this proposal is on-going in Bug 709490.
(Assignee)

Comment 156

2 years ago
Created attachment 8604463 [details] [diff] [review]
Part-11-Support-Structured-Clone.patch

This is a WIP patch for Structured Clone and I will keep working on Transferable.
The implementation in this patch sticks to the spec (https://html.spec.whatwg.org/#structured-clone) which makes the newly constructed ImageBitmap's data as a COPY of the input ImageBitmap's data.
Attachment #8604463 - Flags: feedback?(bugs)
Do we need support for transferable, if we can just share the data between threads?
I mean, the backend of shared ImageBitmap could be threadsafe, right?

(and does the spec say anywhere we should support transferable? I don't see ImageBitmap in https://html.spec.whatwg.org/multipage/infrastructure.html#transferable-objects)
Flags: needinfo?(bugs)
Comment on attachment 8604463 [details] [diff] [review]
Part-11-Support-Structured-Clone.patch

So this keeps the backend non-threadsafe (SourceSurface doesn't seem to have threadsafe refcounting at least). What if we stored the data in some such way that it would be threadsafely refcounted and then pass that data to 
SourceSurface when needed? (but SourceSurface shouldn't own the data ever in that case). 
In a way something similar to Blob, where the underlying data is just shared.
Attachment #8604463 - Flags: feedback?(bugs) → feedback+
Oh, I missed comment 155. But I still don't quite understand why transferring is needed.
Why can't the data just be shared.
Flags: needinfo?(roc)
(In reply to Tzuhao Kuo [:kaku] from comment #154)
> Test for ImageData has been added in
> Part-6-ImageData-as-ImageBitamp-sources-.patch.
> Test for Blob has been added in  Part-10-Extend-test-cases.patch.
> Sorry for my bad patches arrangement.
> Would you like to see one single patch with all test cases?

that would be great yes.
(Assignee)

Comment 161

2 years ago
Created attachment 8604621 [details] [diff] [review]
Part-5-Test-cases.patch

Merge all test case into one single patch.
Merge from the original whole part-5, whole part-10 and partial part-6.
Attachment #8575803 - Attachment is obsolete: true
Attachment #8599760 - Attachment is obsolete: true
Attachment #8604621 - Flags: review?(bugs)
(Assignee)

Comment 162

2 years ago
Created attachment 8604624 [details] [diff] [review]
Part-6-ImageData-as-ImageBitamp-sources.patch

Split the test cases into part-5 patch. The others remain the same.

Carry r+ from smaug and roc.
Attachment #8595322 - Attachment is obsolete: true
Attachment #8604624 - Flags: review+
(Assignee)

Comment 163

2 years ago
Created attachment 8604625 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

Modify typos.

Carry r+ from smaug and roc.
Attachment #8601846 - Attachment is obsolete: true
Attachment #8604625 - Flags: review+
Comment on attachment 8604621 [details] [diff] [review]
Part-5-Test-cases.patch

"A soruce object is passed into worker."
source
Attachment #8604621 - Flags: review?(bugs) → review+
(Assignee)

Comment 165

2 years ago
Either supporting transferable or implementing StructuredClone with shared buffer, the back-end buffer should be thread-safe, right? 
At Bug 1141979, roc proposed to have ImageBitmap keeps a layers::Image as its back-end so that no read-back is needed while put the ImageBitmap into WebGLContext. 
I think we could create a new class, which is extended from the layers::Image, to be the threadsafely-refcounted back-end buffer.
What do you think, smaug?
Flags: needinfo?(bugs)
(In reply to Tzuhao Kuo [:kaku] from comment #165)
> Either supporting transferable or implementing StructuredClone with shared
> buffer, the back-end buffer should be thread-safe, right? 
Yeah, and since the spec talks about support for structured clone only, and since it
makes it possible to reuse the same data in several threads, I'd prefer that.


> I think we could create a new class, which is extended from the
> layers::Image, to be the threadsafely-refcounted back-end buffer.
> What do you think, smaug?
I'm not familiar with layers::Image. Is it safe to access it from several threads?
Though, perhaps we'd just refcnt the thing on several threads, but actually use the data
only in one thread.
Whatever makes us reuse the same thing safely on several threads sounds good to me :)
Flags: needinfo?(bugs)
Blocks: 1108950
I guess it's true that if ImageBitmap is cloneable but not transferable, we can still get predictable resource usage by sharing the contents with a thread-safe refcount (and supporting a close() method which destroys a thread's ImageBitmap and releases one reference to the contents). Supporting transfer as well might be convenient but we can deal with that later.
Flags: needinfo?(roc)
(Assignee)

Comment 168

2 years ago
Hi smaug,

I replaced the back-end of ImageBitmap to a newly created layers::Image class.
Since this is a large modification, I would like to ask your feedback first.
I also implement the StrucuredClone based on sharing the same back-end mechanism.
(Assignee)

Comment 169

2 years ago
Created attachment 8608668 [details] [diff] [review]
Bug-1044102-Part-0-Test-cases.patch

Add test cases for StructuredClone
Attachment #8604621 - Attachment is obsolete: true
Attachment #8608668 - Flags: review?(bugs)
(Assignee)

Comment 170

2 years ago
Created attachment 8608669 [details] [diff] [review]
Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch

This is the newly created class inherited from layers::Image.
This class is used as the back-end of ImageBitmap.
Attachment #8608669 - Flags: feedback?(bugs)
(Assignee)

Comment 171

2 years ago
Created attachment 8608670 [details] [diff] [review]
Bug-1044102-Part-2-ImageBitmap-createImageBitmap-Web.patch

The whole ImageBitmap implementation based on the new backend.
Attachment #8608670 - Flags: feedback?(bugs)
(Assignee)

Comment 172

2 years ago
Created attachment 8608671 [details] [diff] [review]
Bug-1044102-Part-3-Support-ImageBitmap-as-a-CanvasIm.patch

This patch contains those interaction with CanvasRenderingContext.
Codes in this part are not modified.
(Assignee)

Comment 173

2 years ago
Created attachment 8608672 [details] [diff] [review]
Bug-1044102-Part-4-StructuredClone.patch

Implement the StrucuredClone based on the shared back-end.
Attachment #8608672 - Flags: feedback?(bugs)
Comment on attachment 8608668 [details] [diff] [review]
Bug-1044102-Part-0-Test-cases.patch

Non test files don't usually start with test_
so rename test_imagebitmap_on_worker.js and test_imagebitmap_structuredclone.js
And could you add a simple test to actually create ImageBitMap in worker and pass that to main thread.
(perhaps just add event.data.bitmap2 which is created on worker side.)
Attachment #8608668 - Flags: review?(bugs) → review+
Comment on attachment 8608669 [details] [diff] [review]
Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch

I don't understand the need for Clone(). Why aren't we sharing the data?
Attachment #8608669 - Flags: feedback?(bugs)
Comment on attachment 8608670 [details] [diff] [review]
Bug-1044102-Part-2-ImageBitmap-createImageBitmap-Web.patch

This is which uses cloning, not sure why.


One thing to verify with ImageBitmapImage is that it is safe to use it in several thread, and also delete on any of the threads it is using.
I'm not familiar with gfx::Image, so don't know if there are some limitations.
Attachment #8608670 - Flags: feedback?(bugs)
Comment on attachment 8608672 [details] [diff] [review]
Bug-1044102-Part-4-StructuredClone.patch

>+ImageBitmap::WriteStructuredClone(JSContext* aCx, JSStructuredCloneWriter* aWriter, ImageBitmap* aImageBitmap)
>+{
>+  const uint32_t cropRectX = BitwiseCast<uint32_t>(aImageBitmap->mCropRect.x);
>+  const uint32_t cropRectY = BitwiseCast<uint32_t>(aImageBitmap->mCropRect.y);
>+  const uint32_t cropRectWidth = BitwiseCast<uint32_t>(aImageBitmap->mCropRect.width);
>+  const uint32_t cropRectHeight = BitwiseCast<uint32_t>(aImageBitmap->mCropRect.height);
>+
>+  bool writeResult =  JS_WriteUint32Pair(aWriter, SCTAG_DOM_IMAGEBITMAP, 0) &&
>+                      JS_WriteUint32Pair(aWriter, cropRectX, cropRectY) &&
>+                      JS_WriteUint32Pair(aWriter, cropRectWidth, cropRectHeight) &&
>+                      JS_WritePtr(aWriter, aImageBitmap->mBackend);
What makes sure mBackend isn't deleted while it is in the written form?


baku should also look at this.
Attachment #8608672 - Flags: feedback?(bugs) → feedback?(amarchesini)
(our structured clone handling is a great mess atm, unfortunately. Way too many places copy-pasting almost the same code and one never knows which version is used in which case)
Comment on attachment 8608672 [details] [diff] [review]
Bug-1044102-Part-4-StructuredClone.patch

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

Tell me more about this backend. A generic void* seems scary.

::: dom/base/nsJSEnvironment.cpp
@@ +2546,5 @@
>      return result;
>  #else
>      return nullptr;
>  #endif
> +  } else if (tag == SCTAG_DOM_IMAGEBITMAP) {

Move this code in the nsGlobalWindow and in the correct WorkerPrivate callback.

@@ +2610,5 @@
>  
> +  // Handle imageBitmap cloning
> +  ImageBitmap* imageBitmap;
> +  if (NS_SUCCEEDED(UNWRAP_OBJECT(ImageBitmap, obj, imageBitmap))) {
> +    return ImageBitmap::WriteStructuredClone(cx, writer, imageBitmap);

Same here.

::: dom/canvas/ImageBitmap.cpp
@@ +803,5 @@
> +  int32_t cropRectHeight = BitwiseCast<int32_t>(cropRectHeight_);
> +  layers::Image* backend = static_cast<layers::Image*>(backend_);
> +
> +  // Get the current global object.
> +  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

I don't like this part. In the main-thread you always have a window and you should use it.
For workers, the parent is not important and you can use a nullptr global.

@@ +821,5 @@
> +    return nullptr;
> +  }
> +
> +  // Wrap it in a JS::Value.
> +  result = imageBitmap->WrapObject(aCx, JS::NullPtr());

use GetOrCreateDOMReflector

@@ +828,5 @@
> +}
> +
> +/* static */
> +bool
> +ImageBitmap::WriteStructuredClone(JSContext* aCx, JSStructuredCloneWriter* aWriter, ImageBitmap* aImageBitmap)

You are not using aCx. Remove it.

@@ +829,5 @@
> +
> +/* static */
> +bool
> +ImageBitmap::WriteStructuredClone(JSContext* aCx, JSStructuredCloneWriter* aWriter, ImageBitmap* aImageBitmap)
> +{

MOZ_ASSERT(aWriter);
MOZ_ASSERT(aImageBitmap);

@@ +838,5 @@
> +
> +  bool writeResult =  JS_WriteUint32Pair(aWriter, SCTAG_DOM_IMAGEBITMAP, 0) &&
> +                      JS_WriteUint32Pair(aWriter, cropRectX, cropRectY) &&
> +                      JS_WriteUint32Pair(aWriter, cropRectWidth, cropRectHeight) &&
> +                      JS_WritePtr(aWriter, aImageBitmap->mBackend);

What is this backend? What keeps it alive?
Attachment #8608672 - Flags: feedback?(amarchesini) → feedback-
(Assignee)

Comment 180

2 years ago
(In reply to Olli Pettay [:smaug] from comment #175)
> Comment on attachment 8608669 [details] [diff] [review]
> Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch
> 
> I don't understand the need for Clone(). Why aren't we sharing the data?
(In reply to Olli Pettay [:smaug] from comment #176)
> Comment on attachment 8608670 [details] [diff] [review]
> Bug-1044102-Part-2-ImageBitmap-createImageBitmap-Web.patch
> 
> This is which uses cloning, not sure why.

I was trying to clone the ImageBitmapImage while creating an ImageBitmap from another ImageBitmap. But, yes, it is not needed and they could share the same ImageBitmapImage.

> 
> One thing to verify with ImageBitmapImage is that it is safe to use it in
> several thread, and also delete on any of the threads it is using.

Two levels of thread-safe should be considered here, reference counting thread-safe and data accessing thread-safe.

Since ImageBitmapImage inherits layers::Image and layers::Image implements "NS_INLINE_DECL_THREADSAFE_REFCOUNTING" which I think layers::Image should be thread safely refcnted and so does ImageBitmapImage. Is it right?

About data accessing thread-safe, I think there is no such issue. Since after an ImageBitmapImage was created, it might be shared in multiple threads and be read there, however, no "write operation" will be performed on an ImageBitmapImage so we do not need to protect the data in such context.

> I'm not familiar with gfx::Image, so don't know if there are some
> limitations.
Maybe we can ask for roc's review on this part after he come back office.
Flags: needinfo?(bugs)
Yes, refcounting is threadsafe then, and if there is no writing to the data ever, things should be ok.
Flags: needinfo?(bugs)
(Assignee)

Comment 182

2 years ago
(In reply to Andrea Marchesini (:baku) from comment #179)
> Review of attachment 8608672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tell me more about this backend. A generic void* seems scary.

The backend is the underlying data buffer of an ImageBitmap. A backend is an instance of any subclasses of layers::Image class. For now, it could only be layers::PlanarYCbCrImage or layers::ImageBitmapImage.

Smaug suggests to implement the StructuredClone of ImageBitmap by sharing the same backend between the original ImageBitmap and the cloned ImageBitmap, so, here, I try to pass the thread-safe backend pointer to another thread to create a new ImageBitmap object.

> @@ +838,5 @@
> > +
> > +  bool writeResult =  JS_WriteUint32Pair(aWriter, SCTAG_DOM_IMAGEBITMAP, 0) &&
> > +                      JS_WriteUint32Pair(aWriter, cropRectX, cropRectY) &&
> > +                      JS_WriteUint32Pair(aWriter, cropRectWidth, cropRectHeight) &&
> > +                      JS_WritePtr(aWriter, aImageBitmap->mBackend);
> 
> What is this backend? What keeps it alive?
A backend is always kept by an ImageBitmap, so, as long as the ImageBitmap is alive, the backend is alive. 
I missed that it might be freed during the StructuredClone. 
Here are two proposals to handle it:

1. 
Before passing the backend into JS_WritePtr(), AddRef it once, and Release it after the new ImageBitmap is created. 
(The backend will be AddRef-ed aging in the ImageBitmap CTOR.) 
If the StructuredClone failed, Release it at the StructuredCloneErrorOp function.

2. 
Append the ImageBitmap object, which keeps the backend we want to pass to another thread, in to the clonedObjects (from aClosure), then the ImageBitmap and its backend will be kept alive during the StructuredClone process. 
(Since the layers::Image does not inherit nsISupport but ImageBitmap does, so I can only append the ImageBitmap into clonedObjects.)

Anyone is better than another?
Flags: needinfo?(bugs)
Flags: needinfo?(amarchesini)
(In reply to Tzuhao Kuo [:kaku] from comment #182)
> > What is this backend? What keeps it alive?
> A backend is always kept by an ImageBitmap, so, as long as the ImageBitmap
> is alive, the backend is alive. 
> I missed that it might be freed during the StructuredClone. 
> Here are two proposals to handle it:
> 
> 1. 
> Before passing the backend into JS_WritePtr(), AddRef it once, and Release
> it after the new ImageBitmap is created. 
> (The backend will be AddRef-ed aging in the ImageBitmap CTOR.) 
> If the StructuredClone failed, Release it at the StructuredCloneErrorOp
> function.
Something like that sounds good to me, but I'd prefer baku to comment on this.

> 
> 2. 
> Append the ImageBitmap object, which keeps the backend we want to pass to
> another thread, in to the clonedObjects (from aClosure), then the
> ImageBitmap and its backend will be kept alive during the StructuredClone
> process. 
I don't understand this. What thing keeps the ImageBitmap alive?
Remember, StructuredClone is read asynchronously after it has been written.
Flags: needinfo?(bugs)
Sorry, it took too much before answering.

> 1. 
> Before passing the backend into JS_WritePtr(), AddRef it once, and Release
> it after the new ImageBitmap is created. 
> (The backend will be AddRef-ed aging in the ImageBitmap CTOR.) 
> If the StructuredClone failed, Release it at the StructuredCloneErrorOp
> function.

I like this. Happy to review this patch.
Flags: needinfo?(amarchesini)
Blocks: 1172796
(Assignee)

Comment 185

2 years ago
Created attachment 8628654 [details] [diff] [review]
Bug-1044102-Part-0-Test-cases.patch

(In reply to Olli Pettay [:smaug] from comment #174)
> Comment on attachment 8608668 [details] [diff] [review]
> Bug-1044102-Part-0-Test-cases.patch
> 
> Non test files don't usually start with test_
> so rename test_imagebitmap_on_worker.js and
> test_imagebitmap_structuredclone.js
> And could you add a simple test to actually create ImageBitMap in worker and
> pass that to main thread.
> (perhaps just add event.data.bitmap2 which is created on worker side.)

Add one test case that create an ImageBitmap in worker and then pass it to the main thread.
Attachment #8608668 - Attachment is obsolete: true
Attachment #8628654 - Flags: review?(bugs)
(Assignee)

Comment 186

2 years ago
Created attachment 8628655 [details] [diff] [review]
Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch

layers::ImageBitmapImage is a newly created class inherited from layers::Image.
This class is used as the back-end of ImageBitmap.
Attachment #8608669 - Attachment is obsolete: true
Attachment #8628655 - Flags: review?(roc)
Attachment #8628655 - Flags: review?(bugs)
(Assignee)

Comment 187

2 years ago
Created attachment 8628656 [details] [diff] [review]
Bug-1044102-Part-2-ImageBitmap-implementation.patch

The whole ImageBitmap implementation based on the new backend, layers::Image.
Attachment #8608671 - Attachment is obsolete: true
Attachment #8628656 - Flags: review?(bugs)
(Assignee)

Comment 188

2 years ago
Created attachment 8628657 [details] [diff] [review]
Bug-1044102-Part-3-Support-ImageBitmap-as-a-CanvasIm.patch

This patch contains those interaction with CanvasRenderingContext.
Attachment #8608670 - Attachment is obsolete: true
Attachment #8628657 - Flags: review?(bugs)
(Assignee)

Comment 189

2 years ago
Created attachment 8628658 [details] [diff] [review]
Bug-1044102-Part-4-Support-StructuredClone.patch

Implement the StructuredClone based on the WorkerStructuredCloneClosure.
Attachment #8608672 - Attachment is obsolete: true
Attachment #8628658 - Flags: review?(bugs)
Attachment #8628658 - Flags: review?(amarchesini)
Comment on attachment 8628658 [details] [diff] [review]
Bug-1044102-Part-4-Support-StructuredClone.patch

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

Add a test where you transfer more than 1 imageBitmap.

::: dom/canvas/ImageBitmap.cpp
@@ +785,5 @@
>  }
>  
> +/* static */
> +JSObject*
> +ImageBitmap::ReadStructuredClone(JSContext* aCx, JSStructuredCloneReader* aReader, void *aClosure)

80chars

@@ +786,5 @@
>  
> +/* static */
> +JSObject*
> +ImageBitmap::ReadStructuredClone(JSContext* aCx, JSStructuredCloneReader* aReader, void *aClosure)
> +{

MOZ_ASSERT(aReader);
MOZ_ASSERT(aClosure);

@@ +804,5 @@
> +  int32_t cropRectHeight = BitwiseCast<int32_t>(cropRectHeight_);
> +
> +  // Get the current global object.
> +  auto* closure = static_cast<WorkerStructuredCloneClosure*>(aClosure);
> +  nsCOMPtr<nsIGlobalObject> parent = do_QueryInterface(closure->mParentWindow);

MOZ_ASSERT(!closure->mClonedImages.IsEmpty());

@@ +808,5 @@
> +  nsCOMPtr<nsIGlobalObject> parent = do_QueryInterface(closure->mParentWindow);
> +
> +  // Create a new ImageBitmap.
> +  nsRefPtr<ImageBitmap> imageBitmap = new ImageBitmap(parent, closure->mClonedImages[0].get());
> +  closure->mClonedImages.Clear();

Probably her eyou want: closure->mClonedImages.RemoveElementAt(0);

@@ +812,5 @@
> +  closure->mClonedImages.Clear();
> +
> +  ErrorResult error;
> +  imageBitmap->SetCrop(IntRect(cropRectX, cropRectY, cropRectWidth, cropRectHeight), error);
> +  if (error.Failed()) {

NS_WARN_IF

@@ +828,5 @@
> +}
> +
> +/* static */
> +bool
> +ImageBitmap::WriteStructuredClone(JSStructuredCloneWriter* aWriter, WorkerStructuredCloneClosure &aClosure, ImageBitmap* aImageBitmap)

80chars

@@ +844,5 @@
> +      NS_WARN_IF(!JS_WriteUint32Pair(aWriter, cropRectWidth, cropRectHeight))) {
> +    return false;
> +  }
> +
> +  aClosure.mClonedImages.AppendElement(aImageBitmap->mBackend);

wait... here you store mBackend in the array, but in the array you use just the first one.
This means that if we transfer 2 images, we don't the second backend, right?

::: dom/canvas/ImageBitmap.h
@@ +73,5 @@
>    Create(nsIGlobalObject* aGlobal, const ImageBitmapSource& aSrc,
>           bool aCrop, const gfx::IntRect& aCropRect, ErrorResult& aRv);
>  
> +  static JSObject*
> +  ReadStructuredClone(JSContext* aCx, JSStructuredCloneReader* aReader, void *aClosure);

80chars

@@ +76,5 @@
> +  static JSObject*
> +  ReadStructuredClone(JSContext* aCx, JSStructuredCloneReader* aReader, void *aClosure);
> +
> +  static bool
> +  WriteStructuredClone(JSStructuredCloneWriter* aWriter, workers::WorkerStructuredCloneClosure &aClosure, ImageBitmap* aImageBitmap);

80chars.

::: dom/workers/WorkerPrivate.cpp
@@ +88,5 @@
>  #include "nsQueryObject.h"
>  #include "nsSandboxFlags.h"
>  #include "prthread.h"
>  #include "xpcpublic.h"
> +#include "ImageContainer.h"

alphabetic order.

@@ +634,5 @@
>        ReadFormData(aCx, aReader, /* aIsMainThread */ false,  aData, &formData);
>        return formData;
>      }
> +    // See if the object is an ImageBitmap.
> +    else if (aTag == SCTAG_DOM_IMAGEBITMAP) {

no 'else' after a return.

@@ +823,5 @@
>        ReadFormData(aCx, aReader, /* aIsMainThread */ true,  aData, &formData);
>        return formData;
>      }
> +    // See if the object is a ImageBitmap
> +    else if (aTag == SCTAG_DOM_IMAGEBITMAP) {

no 'else' after a return.

::: dom/workers/WorkerStructuredClone.h
@@ +39,5 @@
>    nsCOMPtr<nsPIDOMWindow> mParentWindow;
>  
>    nsTArray<nsCOMPtr<nsISupports>> mClonedObjects;
>  
> +  // This is used for sharing the backend of ImageBitmaps.

Write that this layers::Image must be thread-safe objects.
Attachment #8628658 - Flags: review?(amarchesini) → review-
It is likely that I won't be able to review these in 24h.
(Assignee)

Comment 192

2 years ago
Comment on attachment 8628658 [details] [diff] [review]
Bug-1044102-Part-4-Support-StructuredClone.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +844,5 @@
> +      NS_WARN_IF(!JS_WriteUint32Pair(aWriter, cropRectWidth, cropRectHeight))) {
> +    return false;
> +  }
> +
> +  aClosure.mClonedImages.AppendElement(aImageBitmap->mBackend);

Did you miss a verb at the last sentence?
I guess that you want to say something like "read", right?
If yes, than that is my fault and your suggestion to replace the "closure->mClonedImages.Clear();" with "closure->mClonedImages.RemoveElementAt(0);" in the ImageBitmap::ReadStructuredClone() should solve this problem.

::: dom/workers/WorkerPrivate.cpp
@@ +634,5 @@
>        ReadFormData(aCx, aReader, /* aIsMainThread */ false,  aData, &formData);
>        return formData;
>      }
> +    // See if the object is an ImageBitmap.
> +    else if (aTag == SCTAG_DOM_IMAGEBITMAP) {

Would you like to remove all the 'else' in this if-else series?
Comment on attachment 8628655 [details] [diff] [review]
Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch

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

I'm confused. How does this different from the (misnamed) CairoImage?
Attachment #8628655 - Flags: review?(roc)
(Assignee)

Comment 194

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #193)
> Comment on attachment 8628655 [details] [diff] [review]
> Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch
> 
> Review of attachment 8628655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm confused. How does this different from the (misnamed) CairoImage?

Sorry that I am just not good at the graphic codebase and missed it. Indeed, CairoImage totally fits to this case (also discussed with Chiajung) and I will re-write the patches.
(Assignee)

Comment 195

2 years ago
Comment on attachment 8628655 [details] [diff] [review]
Bug-1044102-Part-1-Add-layers-ImageBitmapImage.patch

Cancel the review to address roc's comment.
Attachment #8628655 - Flags: review?(bugs)
(Assignee)

Comment 196

2 years ago
Comment on attachment 8628656 [details] [diff] [review]
Bug-1044102-Part-2-ImageBitmap-implementation.patch

Cancel the review to address roc's comment.
Attachment #8628656 - Flags: review?(bugs)
(Assignee)

Comment 197

2 years ago
Created attachment 8629810 [details] [diff] [review]
Bug-1044102-Part-0-Test-cases.patch

Add a new test case that send 2 ImageBitmaps to worker at the same postMessage() function call.
Attachment #8628654 - Attachment is obsolete: true
Attachment #8628654 - Flags: review?(bugs)
Attachment #8629810 - Flags: review?(bugs)
(Assignee)

Comment 198

2 years ago
Created attachment 8629812 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap-createImage.patch

Use the layers::Image as the backend of ImageBitmap. In most case, it will use layers::CairoImage which is a wrapper of a SourceSurface. In the HTMLVideoElement case, it will use what the HTMLVideoElement uses.
Attachment #8628655 - Attachment is obsolete: true
Attachment #8628656 - Attachment is obsolete: true
Attachment #8629812 - Flags: review?(roc)
Attachment #8629812 - Flags: review?(bugs)
(Assignee)

Comment 199

2 years ago
Created attachment 8629813 [details] [diff] [review]
Bug-1044102-Part-2-Support-ImageBitmap-as-a-CanvasIm.patch

This patch contains those interaction with CanvasRenderingContext.
Attachment #8628657 - Attachment is obsolete: true
Attachment #8628657 - Flags: review?(bugs)
Attachment #8629813 - Flags: review?(bugs)
(Assignee)

Comment 200

2 years ago
Created attachment 8629814 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch

Implement the StructuredClone based on the WorkerStructuredCloneClosure and address comments at comment #190.
Attachment #8628658 - Attachment is obsolete: true
Attachment #8628658 - Flags: review?(bugs)
Attachment #8629814 - Flags: review?(bugs)
Attachment #8629814 - Flags: review?(amarchesini)
Comment on attachment 8629814 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +779,5 @@
> +/* static */
> +JSObject*
> +ImageBitmap::ReadStructuredClone(JSContext* aCx,
> +                                 JSStructuredCloneReader* aReader,
> +                                 void *aClosure)

let's make Read and Write consistent: use WorkerStructuredCloneClosure &aClosure.

@@ +813,5 @@
> +  ErrorResult error;
> +  imageBitmap->SetCrop(IntRect(cropRectX, cropRectY, cropRectWidth,
> +                               cropRectHeight), error);
> +  if (NS_WARN_IF(error.Failed())) {
> +    return nullptr;

xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR);

::: dom/canvas/ImageBitmap.h
@@ +74,5 @@
>           bool aCrop, const gfx::IntRect& aCropRect, ErrorResult& aRv);
>  
> +  static JSObject*
> +  ReadStructuredClone(JSContext* aCx, JSStructuredCloneReader* aReader,
> +                      void *aClosure);

workers::WorkerStructuredCloneClosure &aClosure

::: dom/workers/WorkerPrivate.cpp
@@ +635,5 @@
>        return formData;
>      }
> +    // See if the object is an ImageBitmap.
> +    else if (aTag == SCTAG_DOM_IMAGEBITMAP) {
> +      MOZ_ASSERT(!aData);

Do the MOZ_ASSERT(aClosure) + static_cast here.

@@ +824,5 @@
>        return formData;
>      }
> +    // See if the object is a ImageBitmap
> +    else if (aTag == SCTAG_DOM_IMAGEBITMAP) {
> +      return ImageBitmap::ReadStructuredClone(aCx, aReader, aClosure);

same here.
Attachment #8629814 - Flags: review?(amarchesini) → review+
File a follow-up to implement WorkerStructuredCloneClosure::Swap(WorkerStructuredCloneClosure& aClosure).
This will be extremely useful to avoid errors instead using these SwapElements.
Comment on attachment 8629812 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap-createImage.patch

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

This generally looks really good!

::: dom/canvas/ImageBitmap.cpp
@@ +73,5 @@
> +  // call CreateImageFromSurface()
> +  return CreateImageFromSurface(dataSurface, aRv);
> +}
> +
> +static inline bool

Don't use "inline" in any functions here. There's no need.

@@ +102,5 @@
> +  return CheckSecurityForHTMLElements(aRes.mIsWriteOnly, aRes.mCORSUsed, aRes.mPrincipal);
> +}
> +
> +static inline bool
> +CheckNonBitmapSource(HTMLImageElement& aImageEl)

Please add a comment to describe what this function does, or choose a better name. Maybe "HasRasterImage"?

@@ +142,5 @@
> +void
> +ImageBitmap::SetCrop(const gfx::IntRect& aRect, ErrorResult& aRv)
> +{
> +  int32_t width = aRect.Width(), height = aRect.Height(),
> +          x = aRect.X(), y = aRect.Y();

Slightly cleaner to just use
  gfx::IntRect rect = aRect;
and access individual members.

@@ +147,5 @@
> +
> +  // fix up negative dimensions
> +  if (width < 0) {
> +    if (width == INT_MIN) {
> +      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);

Why do we need this? Won't the checked add catch the error? It should!

@@ +204,5 @@
> +
> +    IntRect surfPortion = surfRect.Intersect(mCropRect);
> +
> +    // the crop lies entirely outside the surface area, nothing to draw
> +    if (surfPortion.Width() == 0 || surfPortion.Height() == 0) {

surfPortion.IsEmpty()?

@@ +207,5 @@
> +    // the crop lies entirely outside the surface area, nothing to draw
> +    if (surfPortion.Width() == 0 || surfPortion.Height() == 0) {
> +      mSurface = nullptr;
> +      RefPtr<gfx::SourceSurface> surface(mSurface);
> +      return surface.forget();

Should document somewhere that this can return null when the surface is empty.

@@ +293,5 @@
> +  }
> +
> +  // check ready state
> +  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
> +      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {

ReadState() < HTMLMediaElement::HAVE_CURRENT_DATA

@@ +316,5 @@
> +  }
> +
> +  nsRefPtr<layers::Image> backend = container->LockCurrentImage();
> +  nsRefPtr<ImageBitmap> ret = new ImageBitmap(aGlobal, backend);
> +  container->UnlockCurrentImage();

I'm about to break this code in bug 1143575 but you'll easily be able to fix it.

@@ +334,5 @@
> +  return CreateFromElement(aGlobal, aCanvasEl, aRv);
> +}
> +
> +/* static */
> +already_AddRefed<ImageBitmap>

/* static */ goes on same line as the return type

@@ +354,5 @@
> +
> +  // check the ImageData is neutered or not
> +  if (imageWidth == 0 || imageHeight == 0 ||
> +      (imageWidth * imageHeight * BYTES_PER_PIXEL) != dataLength) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);

For 0x0 ImageData, shouldn't we return an empty ImageBitmap here?

@@ +580,5 @@
> +  nsRefPtr<Promise> mPromise;
> +  nsCOMPtr<nsIGlobalObject> mGlobalObject;
> +  RefPtr<mozilla::dom::Blob> mBlob;
> +  bool mCrop;
> +  IntRect mCropRect;

Use Maybe<IntRect>

@@ +711,5 @@
> +{
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsIRunnable> task =
> +      new CreateImageBitmapFromBlobTask(aPromise, aGlobal, aBlob, aCrop, aCropRect);
> +    NS_DispatchToCurrentThread(task); // Actually, to the main-thread.

Why are we decoding the image on the main thread? Seems like we should be able to avoid that

@@ +715,5 @@
> +    NS_DispatchToCurrentThread(task); // Actually, to the main-thread.
> +  } else {
> +    nsRefPtr<CreateImageBitmapFromBlobWorkerTask> task =
> +      new CreateImageBitmapFromBlobWorkerTask(aPromise, aGlobal, aBlob, aCrop, aCropRect);
> +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext()); // Actually, to the current worker-thread.

Likewise here, we should be able to let the worker run while we decode the image.

@@ +734,5 @@
> +  }
> +
> +  if (aCrop && (aCropRect.Width() == 0 || aCropRect.Height() == 0)) {
> +    aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> +    return promise.forget();

I think we should be able to create a zero-sized ImageBitmap here. Maybe we need to provide spec feedback to that effect.

@@ +763,5 @@
> +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> +  }
> +
> +  if (imageBitmap && aCrop) {
> +    imageBitmap->SetCrop(aCropRect, aRv);

In the cases where we copy the data during CreateInternal, we should crop during the copy so we only do one copy and we only allocate and copy the size of the picture rect. Otherwise this could be very inefficient for anyone who creates ImageBitmaps for small regions of a large image (e.g. sprite sheets).

So generally I think the picture rect should be passed into the CreateInternal functions which should try to handle it. For example PlanarYCbCrImage supports its own picture rect which we can fold this picture rect into (as long as the passed-in picture rect is contained by the PlanarYCbCrImage's existing picture rect).

::: dom/canvas/ImageBitmap.h
@@ +100,5 @@
> +
> +  static already_AddRefed<ImageBitmap>
> +  CreateInternal(nsIGlobalObject* aGlobal, ImageBitmap& aImageBitmap, ErrorResult& aRv);
> +
> +  gfx::IntRect mCropRect;

Document what this rect means and is used for. It would be better to call it mPictureRect for consistency with other gfx code.

Generally it's best to put pointers first in the list of fields, followed by non-pointers and other data in decreasing order of size, for better packing and alignment. So put this last.

@@ +101,5 @@
> +  static already_AddRefed<ImageBitmap>
> +  CreateInternal(nsIGlobalObject* aGlobal, ImageBitmap& aImageBitmap, ErrorResult& aRv);
> +
> +  gfx::IntRect mCropRect;
> +  nsRefPtr<layers::Image> mBackend;

I think mBackend isn't quite the right name for this --- "backend" usually refers to code, not data. How about calling this mData?

@@ +103,5 @@
> +
> +  gfx::IntRect mCropRect;
> +  nsRefPtr<layers::Image> mBackend;
> +  nsCOMPtr<nsIGlobalObject> mParent;
> +  RefPtr<gfx::SourceSurface> mSurface;

Document here the relationship between mBackend/mData and mSurface. As I understand it, mBackend/mData is never null, but mSurface can be null. If mSurface is not null then it holds the same data as mBackend/mData.
Attachment #8629812 - Flags: review?(roc) → review-
Comment on attachment 8629812 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap-createImage.patch

Better to review this once roc's comments have been incorporated into the patch.
Attachment #8629812 - Flags: review?(bugs)

Updated

2 years ago
Attachment #8629810 - Flags: review?(bugs) → review+

Updated

2 years ago
Attachment #8629813 - Flags: review?(bugs) → review+
Comment on attachment 8629814 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch

I don't see how this deals with window<->window messaging.


Also, are we missing testcases for that case?
Attachment #8629814 - Flags: review?(bugs)
Attachment #8629814 - Flags: review-
Attachment #8629814 - Flags: feedback?(amarchesini)
(Assignee)

Comment 206

2 years ago
Comment on attachment 8629812 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap-createImage.patch

Hi Seth, 

As I mentioned in the mail, this is the patch where I try to decode a blob.
Please have a look at the AsyncCreateImageBitmapFromBlob() function in the ImageBitmap.cpp file.

Thanks!
Attachment #8629812 - Flags: feedback?(seth)
(Assignee)

Comment 207

2 years ago
(In reply to Olli Pettay [:smaug] from comment #205)
> Comment on attachment 8629814 [details] [diff] [review]
> Bug-1044102-Part-3-Support-StructuredClone.patch
> 
> I don't see how this deals with window<->window messaging.

Sorry, I missed this part.
I think I should handle this at the PostMessageEvent.cpp. 
Specifically, at PostMessageEvent::ReadStructuredClone() and PostMessageEvent::WriteStructuredClone(), right?

> Also, are we missing testcases for that case?
OK.
Flags: needinfo?(bugs)
Flags: needinfo?(amarchesini)
> I think I should handle this at the PostMessageEvent.cpp. 
> Specifically, at PostMessageEvent::ReadStructuredClone() and
> PostMessageEvent::WriteStructuredClone(), right?

Correct.

> > Also, are we missing testcases for that case?

Yes please :)
Flags: needinfo?(amarchesini)
(In reply to Tzuhao Kuo [:kaku] from comment #206)
> As I mentioned in the mail, this is the patch where I try to decode a blob.
> Please have a look at the AsyncCreateImageBitmapFromBlob() function in the
> ImageBitmap.cpp file.

So I've considered this problem a bit more and I think decoding off-main-thread is going to be tricky if we go through RasterImage, because RasterImage has so many assumptions about things like delivering events.

What may be better instead is to decouple image::Decoder from RasterImage enough that imgTools::DecodeImage can be written without creating a RasterImage object at all. I think this will be relatively straightforward.

I'll file a bug about reimplementing imgTools::DecodeImage to work off-main-thread and assign it to myself. I'll make the new bug block this bug, but if you're almost ready to land, I don't think this issue should stop you from landing. We can rip out the unnecessary ImageBitmap code in a followup once imgTools::DecodeImage works off-main-thread.
Attachment #8629812 - Flags: feedback?(seth)
Depends on: 1181863
(Assignee)

Comment 210

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline July 8-10) from comment #203)
> @@ +354,5 @@
> > +
> > +  // check the ImageData is neutered or not
> > +  if (imageWidth == 0 || imageHeight == 0 ||
> > +      (imageWidth * imageHeight * BYTES_PER_PIXEL) != dataLength) {
> > +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> 
> For 0x0 ImageData, shouldn't we return an empty ImageBitmap here?
Hmm..., what is a 0x0 ImageData? Is it an ImageData with empty size and empty data? or is it a neutered ImageData with its data had been transferred?

The spec says that we should throw if it is a neutered ImageData, and I think there is no way to create an ImageData with empty size and data, right?

> @@ +711,5 @@
> > +{
> > +  if (NS_IsMainThread()) {
> > +    nsCOMPtr<nsIRunnable> task =
> > +      new CreateImageBitmapFromBlobTask(aPromise, aGlobal, aBlob, aCrop, aCropRect);
> > +    NS_DispatchToCurrentThread(task); // Actually, to the main-thread.
> 
> Why are we decoding the image on the main thread? Seems like we should be
> able to avoid that
> 
> @@ +715,5 @@
> > +    NS_DispatchToCurrentThread(task); // Actually, to the main-thread.
> > +  } else {
> > +    nsRefPtr<CreateImageBitmapFromBlobWorkerTask> task =
> > +      new CreateImageBitmapFromBlobWorkerTask(aPromise, aGlobal, aBlob, aCrop, aCropRect);
> > +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext()); // Actually, to the current worker-thread.
> 
> Likewise here, we should be able to let the worker run while we decode the
> image.
So, according to Seth's comment#209, how about we create an follow-up to change the decoding behavior later?
 
> @@ +734,5 @@
> > +  }
> > +
> > +  if (aCrop && (aCropRect.Width() == 0 || aCropRect.Height() == 0)) {
> > +    aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > +    return promise.forget();
> 
> I think we should be able to create a zero-sized ImageBitmap here. Maybe we
> need to provide spec feedback to that effect.
Not sure about the use scenario, any example?

> 
> @@ +763,5 @@
> > +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> > +  }
> > +
> > +  if (imageBitmap && aCrop) {
> > +    imageBitmap->SetCrop(aCropRect, aRv);
> 
> In the cases where we copy the data during CreateInternal, we should crop
> during the copy so we only do one copy and we only allocate and copy the
> size of the picture rect. Otherwise this could be very inefficient for
> anyone who creates ImageBitmaps for small regions of a large image (e.g.
> sprite sheets).
> 
> So generally I think the picture rect should be passed into the
> CreateInternal functions which should try to handle it. For example
> PlanarYCbCrImage supports its own picture rect which we can fold this
> picture rect into (as long as the passed-in picture rect is contained by the
> PlanarYCbCrImage's existing picture rect).
Agree. 

After reviewing the code path, only the following three cases copy data in the CreateInternal() definitely.
(1) the source is an ImageData.
(2) the source is an Blob.
(3) the source is an <canvas> element with WebGL rendering context.

If the source is an <canvas> element with 2D rendering context, then we take a snapshot from the <canvas> and, in most cases, the snapshot refers to the buffer of the context's DrawingTarget. And we do copy-on-write on the snapshot's buffer while the <canvas> element's context is going to process further drawing operations. 

I suggest to crop during the copy for the (1),(2) and (3) cases only. What do you think?

And OK to all other comments.
Flags: needinfo?(roc)
(In reply to Tzuhao Kuo [:kaku] from comment #210)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline July
> 8-10) from comment #203)
> > @@ +354,5 @@
> > > +
> > > +  // check the ImageData is neutered or not
> > > +  if (imageWidth == 0 || imageHeight == 0 ||
> > > +      (imageWidth * imageHeight * BYTES_PER_PIXEL) != dataLength) {
> > > +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> > 
> > For 0x0 ImageData, shouldn't we return an empty ImageBitmap here?
> Hmm..., what is a 0x0 ImageData? Is it an ImageData with empty size and
> empty data? or is it a neutered ImageData with its data had been transferred?
> 
> The spec says that we should throw if it is a neutered ImageData, and I
> think there is no way to create an ImageData with empty size and data, right?

Yes, you're right. Never mind then.

> So, according to Seth's comment#209, how about we create an follow-up to
> change the decoding behavior later?

That's fine.

> > @@ +734,5 @@
> > > +  }
> > > +
> > > +  if (aCrop && (aCropRect.Width() == 0 || aCropRect.Height() == 0)) {
> > > +    aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > > +    return promise.forget();
> > 
> > I think we should be able to create a zero-sized ImageBitmap here. Maybe we
> > need to provide spec feedback to that effect.
> Not sure about the use scenario, any example?

I generally find that allowing creation of 0x0 objects makes code more robust, but I guess it doesn't matter yet. Forget this for now.

> > @@ +763,5 @@
> > > +    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> > > +  }
> > > +
> > > +  if (imageBitmap && aCrop) {
> > > +    imageBitmap->SetCrop(aCropRect, aRv);
> > 
> > In the cases where we copy the data during CreateInternal, we should crop
> > during the copy so we only do one copy and we only allocate and copy the
> > size of the picture rect. Otherwise this could be very inefficient for
> > anyone who creates ImageBitmaps for small regions of a large image (e.g.
> > sprite sheets).
> > 
> > So generally I think the picture rect should be passed into the
> > CreateInternal functions which should try to handle it. For example
> > PlanarYCbCrImage supports its own picture rect which we can fold this
> > picture rect into (as long as the passed-in picture rect is contained by the
> > PlanarYCbCrImage's existing picture rect).
> Agree. 
> 
> After reviewing the code path, only the following three cases copy data in
> the CreateInternal() definitely.
> (1) the source is an ImageData.
> (2) the source is an Blob.
> (3) the source is an <canvas> element with WebGL rendering context.
> 
> If the source is an <canvas> element with 2D rendering context, then we take
> a snapshot from the <canvas> and, in most cases, the snapshot refers to the
> buffer of the context's DrawingTarget. And we do copy-on-write on the
> snapshot's buffer while the <canvas> element's context is going to process
> further drawing operations. 
> 
> I suggest to crop during the copy for the (1),(2) and (3) cases only. What
> do you think?

That sounds fine.
Flags: needinfo?(roc)
(Assignee)

Comment 212

2 years ago
Created attachment 8634556 [details] [diff] [review]
0001-Bug-1044102-Part-0-Test-cases.patch

Add test cases to do structured clone between windows.
Attachment #8629810 - Attachment is obsolete: true
Attachment #8634556 - Flags: review?(bugs)
(Assignee)

Comment 213

2 years ago
Created attachment 8634558 [details] [diff] [review]
0002-Bug-1044102-Part-1-Implement-ImageBitmap.patch

Address comment#203

Mainly:
1) Use MayBe<> on the cropping parameter.
2) If the CreateInternal() method copies data from the source image, we also do cropping if needed.
Attachment #8629812 - Attachment is obsolete: true
Attachment #8634558 - Flags: review?(roc)
Attachment #8634558 - Flags: review?(bugs)
(Assignee)

Comment 214

2 years ago
Created attachment 8634559 [details] [diff] [review]
0003-Bug-1044102-Part-2-Support-ImageBitmap-as-CanvasImag.patch

This patch contains those interaction with CanvasRenderingContext.
Nothing changed.
Attachment #8629813 - Attachment is obsolete: true
Attachment #8634559 - Flags: superreview?(bugs)
(Assignee)

Comment 215

2 years ago
Created attachment 8634560 [details] [diff] [review]
0004-Bug-1044102-Part-3-Support-StructuredClone.patch

Address comment#201 and comment#203.
Support structured clone between windows.
Attachment #8629814 - Attachment is obsolete: true
Attachment #8629814 - Flags: feedback?(amarchesini)
Attachment #8634560 - Flags: review?(bugs)
Attachment #8634560 - Flags: review?(amarchesini)
(Assignee)

Comment 216

2 years ago
Comment on attachment 8634559 [details] [diff] [review]
0003-Bug-1044102-Part-2-Support-ImageBitmap-as-CanvasImag.patch

This patch contains those interaction with CanvasRenderingContext.
Attachment #8634559 - Flags: superreview?(bugs) → review?(bugs)
Comment on attachment 8634558 [details] [diff] [review]
0002-Bug-1044102-Part-1-Implement-ImageBitmap.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +47,5 @@
> +  const uint32_t dstStride = dstSize.width * BytesPerPixel(format);
> +
> +  // Create a new SourceSurface.
> +  RefPtr<DataSourceSurface> dstDataSurface =
> +    Factory::CreateDataSourceSurfaceWithStride(dstSize, format, dstStride);

Why does this need to be a DataSourceSurface? Can't we make this a surface of the same type as aSurface? That could be much faster since e.g. on Windows we could use a Direct2D surface and avoid readback. Should also be simpler code since we would use a DrawTarget to copy the rectangle. We might need to convert mSurface to a DataSourceSurface if we later find that the surface is not compatible with the DrawTarget we want to use it for.

@@ +413,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return nullptr;
> +  }
> +
> +  RefPtr<SourceSurface> surface = GetSurfaceFromElement(aGlobal, aCanvasEl, aRv);

For WebGL this is pretty bad since it forces readback (and maybe copying?) we don't necessarily need. I don't mind fixing that later, because it's probably significant work, but I think it should be fixed since using ImageBitmaps with WebGL canvas sources is likely to be popular.
(Assignee)

Comment 218

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #217)
> Comment on attachment 8634558 [details] [diff] [review]
> 0002-Bug-1044102-Part-1-Implement-ImageBitmap.patch
> 
> Review of attachment 8634558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +47,5 @@
> > +  const uint32_t dstStride = dstSize.width * BytesPerPixel(format);
> > +
> > +  // Create a new SourceSurface.
> > +  RefPtr<DataSourceSurface> dstDataSurface =
> > +    Factory::CreateDataSourceSurfaceWithStride(dstSize, format, dstStride);
> 
> Why does this need to be a DataSourceSurface? Can't we make this a surface
> of the same type as aSurface? That could be much faster since e.g. on
> Windows we could use a Direct2D surface and avoid readback. Should also be
> simpler code since we would use a DrawTarget to copy the rectangle. We might
> need to convert mSurface to a DataSourceSurface if we later find that the
> surface is not compatible with the DrawTarget we want to use it for.
For general cases, I agree that using a DrawTarget to copy-and-crop via backend APIs is much more efficient.
But here, the _aSurface_ is always a DataSourceSurface since the CropSourceSurface() function is only called in the following 3 cases:
1) CreateInternal() with ImageData as source.
   We use a DataSourceSurface to wrap the ImageData's buffer.

2) CreateInternal() with Blob as source.
   We get the SourceSurface out from the just decoded RasterImage which is not optimized yet, so it is a DataSourceSurface which wraps the image's raw buffer.

3) CreateInternal() with HTMLCanvasElement(with WebGL context) as source.
   We get the SoruceSurface via HTMLCanvasElement::GetSurfaceSnapshot() which reads the WebGL's data back into a DataSourceSurface.

For all other cases, the copping is not handled here, it is handled in the PrepareForDrawTarget() method and is done via a DrawTarget.

So, I think we can keep the CropSourceSurface() function as it is now with some notes/comments?

> 
> @@ +413,5 @@
> > +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> > +    return nullptr;
> > +  }
> > +
> > +  RefPtr<SourceSurface> surface = GetSurfaceFromElement(aGlobal, aCanvasEl, aRv);
> 
> For WebGL this is pretty bad since it forces readback (and maybe copying?)
> we don't necessarily need. I don't mind fixing that later, because it's
> probably significant work, but I think it should be fixed since using
> ImageBitmaps with WebGL canvas sources is likely to be popular.
Okey..., than I will open a follow-up bug to address this issue!?
Flags: needinfo?(roc)
(In reply to Tzuhao Kuo [:kaku] from comment #218)
> For general cases, I agree that using a DrawTarget to copy-and-crop via
> backend APIs is much more efficient.
> But here, the _aSurface_ is always a DataSourceSurface since the
> CropSourceSurface() function is only called in the following 3 cases:
> 1) CreateInternal() with ImageData as source.
>    We use a DataSourceSurface to wrap the ImageData's buffer.
> 
> 2) CreateInternal() with Blob as source.
>    We get the SourceSurface out from the just decoded RasterImage which is
> not optimized yet, so it is a DataSourceSurface which wraps the image's raw
> buffer.
> 
> 3) CreateInternal() with HTMLCanvasElement(with WebGL context) as source.
>    We get the SoruceSurface via HTMLCanvasElement::GetSurfaceSnapshot()
> which reads the WebGL's data back into a DataSourceSurface.
> 
> For all other cases, the copping is not handled here, it is handled in the
> PrepareForDrawTarget() method and is done via a DrawTarget.
> 
> So, I think we can keep the CropSourceSurface() function as it is now with
> some notes/comments?

OK. Perhaps CropSourceSurface should take a DataSourceSurface parameter and return a DataSourceSurface result?

> > @@ +413,5 @@
> > > +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> > > +    return nullptr;
> > > +  }
> > > +
> > > +  RefPtr<SourceSurface> surface = GetSurfaceFromElement(aGlobal, aCanvasEl, aRv);
> > 
> > For WebGL this is pretty bad since it forces readback (and maybe copying?)
> > we don't necessarily need. I don't mind fixing that later, because it's
> > probably significant work, but I think it should be fixed since using
> > ImageBitmaps with WebGL canvas sources is likely to be popular.
> Okey..., than I will open a follow-up bug to address this issue!?

Sure!
Flags: needinfo?(roc)
(Assignee)

Comment 220

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #219)
> (In reply to Tzuhao Kuo [:kaku] from comment #218)
> > For general cases, I agree that using a DrawTarget to copy-and-crop via
> > backend APIs is much more efficient.
> > But here, the _aSurface_ is always a DataSourceSurface since the
> > CropSourceSurface() function is only called in the following 3 cases:
> > 1) CreateInternal() with ImageData as source.
> >    We use a DataSourceSurface to wrap the ImageData's buffer.
> > 
> > 2) CreateInternal() with Blob as source.
> >    We get the SourceSurface out from the just decoded RasterImage which is
> > not optimized yet, so it is a DataSourceSurface which wraps the image's raw
> > buffer.
> > 
> > 3) CreateInternal() with HTMLCanvasElement(with WebGL context) as source.
> >    We get the SoruceSurface via HTMLCanvasElement::GetSurfaceSnapshot()
> > which reads the WebGL's data back into a DataSourceSurface.
> > 
> > For all other cases, the copping is not handled here, it is handled in the
> > PrepareForDrawTarget() method and is done via a DrawTarget.
> > 
> > So, I think we can keep the CropSourceSurface() function as it is now with
> > some notes/comments?
> 
> OK. Perhaps CropSourceSurface should take a DataSourceSurface parameter and
> return a DataSourceSurface result?
Yes, I should do it.
(Assignee)

Comment 221

2 years ago
Created attachment 8635144 [details] [diff] [review]
0002-Bug-1044102-Part-1-Implement-ImageBitmap.patch

Address comment#203 and comment#219

Mainly:
1) Use MayBe<> on the cropping parameter.
2) If the CreateInternal() method copies data from the source image, we also do cropping if needed.
Attachment #8634558 - Attachment is obsolete: true
Attachment #8634558 - Flags: review?(roc)
Attachment #8634558 - Flags: review?(bugs)
Attachment #8635144 - Flags: review?(roc)
Attachment #8635144 - Flags: review?(bugs)
Comment on attachment 8634560 [details] [diff] [review]
0004-Bug-1044102-Part-3-Support-StructuredClone.patch

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

Looks good but I would like to read it again with this small changes. Thanks!

::: dom/base/PostMessageEvent.cpp
@@ +98,5 @@
>      }
>    }
>  
> +  if (tag == SCTAG_DOM_IMAGEBITMAP) {
> +    NS_ASSERTION(!data, "Data should be empty");

You don't use data, remove this assertion.

::: dom/base/PostMessageEvent.h
@@ +50,5 @@
> +  {
> +    return mClonedImages;
> +  }
> +
> +  already_AddRefed<nsGlobalWindow> GetTargetWindow() const

nsGlobalWindow* GetTargetWindow() const
{
  return mTargetWindow;
}

::: dom/canvas/ImageBitmap.cpp
@@ +1014,5 @@
> +
> +/*static*/ JSObject*
> +ImageBitmap::ReadStructuredClone(JSContext* aCx,
> +                                 JSStructuredCloneReader* aReader,
> +                                 PostMessageEvent *aEvent)

Instead having 2 ReadStructuredClone, can you just have:

/*static*/ JSObject*
ImageBitmap::ReadStructuredClone(JSContext* aCx,
                                 JSStructuredCloneReader* aReader,
                                 nsIGlobalObject *aParent,
                                 nsTArray<nsRefPtr<layers::Image>> &aClonedImages);

Same for the Write.

@@ +1037,5 @@
> +  MOZ_ASSERT(aWriter);
> +  MOZ_ASSERT(aEvnet);
> +  MOZ_ASSERT(aImageBitmap);
> +
> +  return WriteStructuredCloneInternal(aWriter, aEvnet->GetImages(), aImageBitmap);

aEvent
Attachment #8634560 - Flags: review?(amarchesini) → review-
Attachment #8635144 - Flags: review?(roc) → review+
(Assignee)

Comment 223

2 years ago
Created attachment 8635800 [details] [diff] [review]
0004-Bug-1044102-Part-3-Support-StructuredClone.patch

(In reply to Andrea Marchesini (:baku) from comment #222)
> Comment on attachment 8634560 [details] [diff] [review]
> 0004-Bug-1044102-Part-3-Support-StructuredClone.patch
> 
> Review of attachment 8634560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but I would like to read it again with this small changes. Thanks!
Sure!
Attachment #8634560 - Attachment is obsolete: true
Attachment #8634560 - Flags: review?(bugs)
Attachment #8635800 - Flags: review?(bugs)
Attachment #8635800 - Flags: review?(amarchesini)
(Assignee)

Comment 224

2 years ago
Comment on attachment 8635144 [details] [diff] [review]
0002-Bug-1044102-Part-1-Implement-ImageBitmap.patch

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

After pushed to the try server, I found several problems.

::: dom/canvas/ImageBitmap.cpp
@@ +91,5 @@
> +  cairoData.mSourceSurface = aSurface;
> +
> +  nsRefPtr<layers::CairoImage> image = new layers::CairoImage();
> +
> +  image->SetData(cairoData);

Here is a problem!

The layers::CairoImage does not keep the reference to the SoruceSurface directly, instead it has a nsCountedRef<nsMainThreadSourceSurfaceRef> mSourceSurface, which is a wrapper to the real SourceSurface reference. It assumes that the SourceSurface only be created and used in the main thread and could be hold by a layers::Image which might be AddRefed/Released off the main thread. So, while AddRefing, the nsMainThreadSourceSurfaceRef asserts it is on the main thread and while Releasing off the main thread, the nsMainThreadSourceSurfaceRef posts an event to the main thread. 
(The definition of nsMainThreadSourceSurfaceRef : https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.h#47)

However, if we create an ImageBitmap from an ImageData off the main thread, then we create and AddRef the SourceSurface off the main thread, which causes the assertion failed.

This problem only happens on the create-from-ImageData case. 
We only support creating ImageBitmap from 1)ImageData, 2)Blob and 3) ImageBitmap off the main thread. 
If the source is a Blob, then we decode it and create the SoruceSurface in the main thread.
If the source is another ImageBitmap, we only AddRef on the layers::Image, not the SoruceSuface.

Two proposal here:
1) Post an event to the main thread to create and AddRef the SoruceSruface from the raw data of an ImageData. (which is what we do for the create-from-blob case now.)
2) Introduce a new layers::Image subclass which wraps the raw memory buffer directly (instead of the SourceSurface) and use this new subclass in the create-from-ImageData cases. 

Would like to hear you thoughts!

::: dom/html/HTMLCanvasElement.h
@@ +35,5 @@
>  class HTMLCanvasPrintState;
>  class PrintCallback;
>  
>  enum class CanvasContextType : uint8_t {
> +  None,

After pushed to the try server, I found that this does not work on the Linux/Android platform since the _None_ is already be defined as a macro somewhere else. I would like to rename this to _NoContext_. Sorry, I should figure out earlier.
Attachment #8635144 - Flags: review?(bugs)
Attachment #8635144 - Flags: feedback?(roc)
Attachment #8635144 - Flags: feedback?(bugs)
(In reply to Tzuhao Kuo [:kaku] from comment #224)
> Two proposal here:
> 1) Post an event to the main thread to create and AddRef the SoruceSruface
> from the raw data of an ImageData. (which is what we do for the
> create-from-blob case now.)

Let's do this, I think. I think it's simpler and more flexible to continue using SourceSurfaces instead of creating yet another wrapper around raw data.
Comment on attachment 8635144 [details] [diff] [review]
0002-Bug-1044102-Part-1-Implement-ImageBitmap.patch

roc gave feedback, and that sounds good to me. Make sure to Addref/Release on the right thread always.
Flags: needinfo?(bugs)
Attachment #8635144 - Flags: feedback?(bugs)

Updated

2 years ago
Attachment #8634556 - Flags: review?(bugs) → review+
Comment on attachment 8635800 [details] [diff] [review]
0004-Bug-1044102-Part-3-Support-StructuredClone.patch

Given all the work baku is now doing to simplifying structured cloning, I think
it is better if just he reviews this stuff.

And looks good anyhow.
Attachment #8635800 - Flags: review?(bugs)
Comment on attachment 8634559 [details] [diff] [review]
0003-Bug-1044102-Part-2-Support-ImageBitmap-as-CanvasImag.patch

interdiff would be nice for showing differences between already reviewed and new patches)
Attachment #8634559 - Flags: review?(bugs) → review+
Comment on attachment 8635800 [details] [diff] [review]
0004-Bug-1044102-Part-3-Support-StructuredClone.patch

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

can I see an interdiff?

::: dom/canvas/ImageBitmap.cpp
@@ +1090,5 @@
> +  // Create a new ImageBitmap.
> +  MOZ_ASSERT(!aClonedImages.IsEmpty());
> +  nsRefPtr<ImageBitmap> imageBitmap =
> +    new ImageBitmap(aParent, aClonedImages[0].get());
> +  aClonedImages.RemoveElementAt(0);

In theory, the 'read' should be 'readonly'. Instead removing the element, pass the index of it. I proposed how in the 'Write'. Add a param |uint32_t aIndex| and here do:

MOZ_ASSERT(aIndex < aClonedImages.Length());
nsRefPtr<ImageBitmap> imageBitmap =
  new ImageBitmap(aParent, aClonedImages[aIndex]);

@@ +1096,5 @@
> +  ErrorResult error;
> +  imageBitmap->SetPictureRect(IntRect(picRectX, picRectY,
> +                                      picRectWidth, picRectHeight), error);
> +  if (NS_WARN_IF(error.Failed())) {
> +    xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR);

Just return nullptr and let the caller to throw the exception.

@@ +1103,5 @@
> +
> +  // Protect the result from a moving GC in ~nsRefPtr.
> +  JS::Rooted<JS::Value> value(aCx);
> +  if (!GetOrCreateDOMReflector(aCx, imageBitmap, &value)) {
> +    xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR);

same here.

@@ +1123,5 @@
> +  const uint32_t picRectY = BitwiseCast<uint32_t>(aImageBitmap->mPictureRect.y);
> +  const uint32_t picRectWidth = BitwiseCast<uint32_t>(aImageBitmap->mPictureRect.width);
> +  const uint32_t picRectHeight = BitwiseCast<uint32_t>(aImageBitmap->mPictureRect.height);
> +
> +  if (NS_WARN_IF(!JS_WriteUint32Pair(aWriter, SCTAG_DOM_IMAGEBITMAP, 0)) ||

instead 0, do:

	
if (NS_WARN_IF(!JS_WriteUint32Pair(aWriter,
                                   SCTAG_DOM_IMAGEBITMAP,
                                   aClonedImages.Length())) ||

::: dom/workers/WorkerPrivate.cpp
@@ +644,5 @@
> +      // Get the current global object.
> +      auto* closure = static_cast<WorkerStructuredCloneClosure*>(aClosure);
> +      nsCOMPtr<nsIGlobalObject> parent = do_QueryInterface(closure->mParentWindow);
> +
> +      return ImageBitmap::ReadStructuredClone(aCx, aReader, parent,

pass 'aData' here as index.

@@ +843,5 @@
> +      auto* closure = static_cast<WorkerStructuredCloneClosure*>(aClosure);
> +      nsCOMPtr<nsIGlobalObject> parent = do_QueryInterface(closure->mParentWindow);
> +
> +      return ImageBitmap::ReadStructuredClone(aCx, aReader, parent,
> +                                              closure->mClonedImages);

aData as index.
Attachment #8635800 - Flags: review?(amarchesini) → review+

Updated

2 years ago
Blocks: 1187812
(Assignee)

Comment 230

2 years ago
Created attachment 8639730 [details] [diff] [review]
Bug-1044102-Part-0-Test-cases.patch

Export ImageBitmap interface to test_interface.js, test_worker_interface.js and test_serviceworker_interface.js.
Attachment #8634556 - Attachment is obsolete: true
Attachment #8639730 - Flags: review?(bugs)
(Assignee)

Comment 231

2 years ago
Created attachment 8639732 [details]
interdiff-part0
(Assignee)

Comment 232

2 years ago
Created attachment 8639734 [details] [diff] [review]
interdiff-part0.patch
Attachment #8639732 - Attachment is obsolete: true
(Assignee)

Comment 233

2 years ago
Created attachment 8639737 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap.patch

Address comment#224 and comment#225 plus some bugs found via try server.
Attachment #8573824 - Attachment is obsolete: true
Attachment #8573825 - Attachment is obsolete: true
Attachment #8575802 - Attachment is obsolete: true
Attachment #8595318 - Attachment is obsolete: true
Attachment #8595319 - Attachment is obsolete: true
Attachment #8595321 - Attachment is obsolete: true
Attachment #8595324 - Attachment is obsolete: true
Attachment #8604624 - Attachment is obsolete: true
Attachment #8604625 - Attachment is obsolete: true
Attachment #8635144 - Attachment is obsolete: true
Attachment #8635144 - Flags: feedback?(roc)
Attachment #8639737 - Flags: review?(roc)
Attachment #8639737 - Flags: review?(bugs)
(Assignee)

Comment 234

2 years ago
Created attachment 8639738 [details] [diff] [review]
interdiff-part1.patch
(Assignee)

Comment 235

2 years ago
Created attachment 8639739 [details] [diff] [review]
Bug-1044102-Part-2-Support-ImageBitmap-as-CanvasImag.patch (carry r+:smaug)

Nothing changed but rebased.
Carry r+ from smaug.
Attachment #8599757 - Attachment is obsolete: true
Attachment #8634559 - Attachment is obsolete: true
(Assignee)

Comment 236

2 years ago
Created attachment 8639741 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch

Address comment#229. 
Rebased so that the original logics in the PostMessageEvent is now replaced to the StructuredCloneHelper.
Attachment #8604463 - Attachment is obsolete: true
Attachment #8635800 - Attachment is obsolete: true
Attachment #8639741 - Flags: review?(amarchesini)
(Assignee)

Comment 237

2 years ago
Created attachment 8639743 [details] [diff] [review]
interdiff-part3.patch

Due to rebase, this interdiff does not completely catch up what happened between two patches and the missed part is replacing the original logics in the PostMessageEvent to the StructuredCloneHelper.
(Assignee)

Comment 238

2 years ago
Comment on attachment 8639738 [details] [diff] [review]
interdiff-part1.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ -121,5 @@
>    }
>  
>    // The temporary cropRect variable is equal to the size of source buffer if we
>    // do not need to crop, or it equals to the given cropping size.
> -  const IntRect& cropRect = aCropRect.refOr(IntRect(0, 0, aSize.width, aSize.height));

This line of code might create a reference to a temporary object which is problematic, so I change it with the valueOr().

@@ +420,5 @@
> +    // non-SourceSurfaceD2D1 surface to a D2D1Image in the following
> +    // CopySurface() step. However, the "partial upload" only supports uploading
> +    // a rectangle starts from the upper-left point, which means it cannot
> +    // upload an arbitrary part of the source surface and this causes problems
> +    // if the mPictureRect is not starts from the upper-left point.

This is where I recently struggled with. The original code made one of my test cases in the Windows8 failed. I will fire a separate bug.
Comment on attachment 8639741 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch

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

Good! Thanks for doing this. My next step is to unify WorkerPrivate::Read/Write callbacks into StructuredCloneHelper.
Please land this code soon! My patches are almost ready.

::: dom/base/StructuredCloneHelper.h
@@ +127,5 @@
>      MOZ_ASSERT(!(mFlags & eMessagePortNotSupported));
>      return mTransferredPorts;
>    }
>  
> +  nsTArray<nsRefPtr<layers::Image>>& GetImages()

nsTArray<nsRefPtr<layers::Image>>& GetImages() const

::: dom/canvas/ImageBitmap.cpp
@@ +15,4 @@
>  #include "libyuv.h"
>  #include "nsLayoutUtils.h"
> +#include "PostMessageEvent.h"
> +#include "WorkerStructuredClone.h"

These 2 are not needed anymore.

@@ +1122,5 @@
> +
> +  ErrorResult error;
> +  imageBitmap->SetPictureRect(IntRect(picRectX, picRectY,
> +                                      picRectWidth, picRectHeight), error);
> +  if (NS_WARN_IF(error.Failed())) {

error.SuppressException();
Attachment #8639741 - Flags: review?(amarchesini) → review+
Comment on attachment 8639737 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap.patch

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

::: dom/canvas/ImageBitmap.cpp
@@ +628,5 @@
> +                                                       imageSize,
> +                                                       aCropRect,
> +                                                       aRv,
> +                                                       getter_AddRefs(data));
> +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext());

Does this mean that in a Worker we return an ImageBitmap which isn't actually usable yet, because the main thread hasn't yet run? It seems to me we should delay fulfilling the promise until this task has completed. Right?
Attachment #8639737 - Flags: review?(roc)
(Assignee)

Comment 241

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #240)
> Comment on attachment 8639737 [details] [diff] [review]
> Bug-1044102-Part-1-Implement-ImageBitmap.patch
> 
> Review of attachment 8639737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +628,5 @@
> > +                                                       imageSize,
> > +                                                       aCropRect,
> > +                                                       aRv,
> > +                                                       getter_AddRefs(data));
> > +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext());
> 
> Does this mean that in a Worker we return an ImageBitmap which isn't
> actually usable yet, because the main thread hasn't yet run? It seems to me
> we should delay fulfilling the promise until this task has completed. Right?

This is a synchronous call since the CreateImageFromRawDataInMainThreadSyncTask inherits from WorkerMainThreadRunnable. I implement this task as a synchronous one so that I do not need to copy the array.Data() to the main thread. So, the ImageBitmap is ready.
Comment on attachment 8639737 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap.patch

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

OK, sorry!
Attachment #8639737 - Flags: review+
(Assignee)

Comment 243

2 years ago
(In reply to Andrea Marchesini (:baku) from comment #239)
> ::: dom/base/StructuredCloneHelper.h
> @@ +127,5 @@
> >      MOZ_ASSERT(!(mFlags & eMessagePortNotSupported));
> >      return mTransferredPorts;
> >    }
> >  
> > +  nsTArray<nsRefPtr<layers::Image>>& GetImages()
> 
> nsTArray<nsRefPtr<layers::Image>>& GetImages() const
Do you mean:
const nsTArray<nsRefPtr<layers::Image>>& GetImages() const;  ?

However, I still need to provide a non-const getter since the ImageBitmap::WriteStructuredClone() requires a non-const array.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 244

2 years ago
(In reply to Tzuhao Kuo [:kaku] from comment #243)
> (In reply to Andrea Marchesini (:baku) from comment #239)
> > ::: dom/base/StructuredCloneHelper.h
> > @@ +127,5 @@
> > >      MOZ_ASSERT(!(mFlags & eMessagePortNotSupported));
> > >      return mTransferredPorts;
> > >    }
> > >  
> > > +  nsTArray<nsRefPtr<layers::Image>>& GetImages()
> > 
> > nsTArray<nsRefPtr<layers::Image>>& GetImages() const
> Do you mean:
> const nsTArray<nsRefPtr<layers::Image>>& GetImages() const;  ?
> 
> However, I still need to provide a non-const getter since the
> ImageBitmap::WriteStructuredClone() requires a non-const array.
No, I am wrong, I mean that we do not use the StructuredCloneHelper in a const way, right?
> > ImageBitmap::WriteStructuredClone() requires a non-const array.
> No, I am wrong, I mean that we do not use the StructuredCloneHelper in a
> const way, right?

Correct. You are right, it cannot be const.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 246

2 years ago
Created attachment 8640354 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch

1) Address the comment#239.
2) Add modification to pass the hazard analysis in ImageBitmap::ReadStructuredClone(), so request review again.
Attachment #8639741 - Attachment is obsolete: true
Attachment #8640354 - Flags: review?(amarchesini)
(Assignee)

Comment 247

2 years ago
Created attachment 8640355 [details] [diff] [review]
interdiff-part3.patch
Attachment #8639743 - Attachment is obsolete: true
Attachment #8640354 - Flags: review?(amarchesini) → review+

Updated

2 years ago
Attachment #8639730 - Flags: review?(bugs) → review+
Comment on attachment 8639737 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap.patch

If CreateImageFromRawData is supposed to be called only on the main thread, could you
MOZ_ASSERT that we're really on the main thread.

I assume deleting layers::Image on worker thread is still thread safe?

(CairoImage has nsCountedRef<nsMainThreadSourceSurfaceRef> as a member variable and all nsMainThreadSourceSurfaceRef handling looks rather error prone to me.
No strong MOZ_ASSERTs, and relying on threadsafety checks on a wrapper on top of some refcounted object is just bizarre. Why not have the checks on the actual classes. But maybe that all works well. I'm not familiar with it.)
Attachment #8639737 - Flags: review?(bugs) → review+
(Assignee)

Comment 249

2 years ago
(In reply to Olli Pettay [:smaug] from comment #248)
> Comment on attachment 8639737 [details] [diff] [review]
> Bug-1044102-Part-1-Implement-ImageBitmap.patch
> 
> If CreateImageFromRawData is supposed to be called only on the main thread,
> could you
> MOZ_ASSERT that we're really on the main thread.
Ok!

> I assume deleting layers::Image on worker thread is still thread safe?
Yes, it is, and it guarantees that its underlying nsCountedRef<nsMainThreadSourceSurfaceRef> (if there is one) releases a SourceSurface at the main thread.
(Assignee)

Comment 250

2 years ago
Created attachment 8641008 [details] [diff] [review]
Bug-1044102-Part-4-update-W3C-web-platform-tests.patch
Attachment #8641008 - Flags: review?(bugs)
(Assignee)

Comment 251

2 years ago
(In reply to Tzuhao Kuo [:kaku] from comment #250)
> Created attachment 8641008 [details] [diff] [review]
> Bug-1044102-Part-4-update-W3C-web-platform-tests.patch

This is for the W3C web platform test.
(Assignee)

Updated

2 years ago
Attachment #8639737 - Flags: superreview?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8639739 - Flags: superreview?(bugs)
(Assignee)

Comment 252

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa6590acf48f
Comment on attachment 8639737 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap.patch

sr isn't often asked, especially when the patch has already been reviewed by two superreviewers ;)
Attachment #8639737 - Flags: superreview?(bugs) → superreview+

Updated

2 years ago
Attachment #8639739 - Flags: superreview?(bugs) → superreview+

Updated

2 years ago
Attachment #8641008 - Flags: review?(bugs) → review+
(Assignee)

Comment 254

2 years ago
Created attachment 8641467 [details] [diff] [review]
Bug-1044102-Part-0-Test-cases.-r-smaug.patch (carry r+:smaug)

Carry r+:smaug.
Attachment #8639730 - Attachment is obsolete: true
Attachment #8639734 - Attachment is obsolete: true
Attachment #8641467 - Flags: review+
(Assignee)

Comment 255

2 years ago
Created attachment 8641468 [details] [diff] [review]
Bug-1044102-Part-1-Implement-ImageBitmap.patch (carry r+:roc,smaug. carry sr+:smaug.)

carry r+:roc,smaug. 
carry sr+:smaug.
Attachment #8639737 - Attachment is obsolete: true
Attachment #8641468 - Flags: superreview+
Attachment #8641468 - Flags: review+
(Assignee)

Comment 256

2 years ago
Created attachment 8641471 [details] [diff] [review]
interdiff-part1.patch

Addredd comment#248.
Attachment #8639738 - Attachment is obsolete: true
(Assignee)

Comment 257

2 years ago
Created attachment 8641472 [details] [diff] [review]
Bug-1044102-Part-2-Support-ImageBitmap-as-CanvasImag.patch (carry r+:smaug. carry sr+:smaug.)

carry r+:smaug. 
carry sr+:smaug.
Attachment #8639739 - Attachment is obsolete: true
Attachment #8641472 - Flags: superreview+
Attachment #8641472 - Flags: review+
(Assignee)

Comment 258

2 years ago
Created attachment 8641473 [details] [diff] [review]
Bug-1044102-Part-3-Support-StructuredClone.patch (carry r+:baku.)

carry r+:baku.
Attachment #8640354 - Attachment is obsolete: true
Attachment #8640355 - Attachment is obsolete: true
Attachment #8641473 - Flags: review+
(Assignee)

Comment 259

2 years ago
Created attachment 8641474 [details] [diff] [review]
Bug-1044102-Part-4-update-W3C-web-platform-tests.patch (carry r+:smaug.)

carry r+:smaug.
Attachment #8641008 - Attachment is obsolete: true
Attachment #8641474 - Flags: review+
(Assignee)

Comment 260

2 years ago
Try looks good.
(Assignee)

Updated

2 years ago
Keywords: dev-doc-needed → checkin-needed
(Assignee)

Comment 261

2 years ago
Thanks for all the reviews.
Congratulations! Kaku's first landing bug! ^o^
(Assignee)

Updated

2 years ago
Blocks: 1189629
(Assignee)

Updated

2 years ago
Blocks: 1189632
(Assignee)

Updated

2 years ago
Blocks: 1189634
Keywords: dev-doc-needed

Comment 263

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8ade9cdad7
https://hg.mozilla.org/integration/mozilla-inbound/rev/af88cf4b922b
https://hg.mozilla.org/integration/mozilla-inbound/rev/075fd774a827
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0af677fae4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5afab84bd62f
Keywords: checkin-needed
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #262)
> Congratulations! Kaku's first landing bug! ^o^

checkin done! Congrats and welcome to mozilla!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8ade9cdad7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/af88cf4b922b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/075fd774a827
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0af677fae4f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5afab84bd62f
https://hg.mozilla.org/mozilla-central/rev/9b8ade9cdad7
https://hg.mozilla.org/mozilla-central/rev/af88cf4b922b
https://hg.mozilla.org/mozilla-central/rev/075fd774a827
https://hg.mozilla.org/mozilla-central/rev/f0af677fae4f
https://hg.mozilla.org/mozilla-central/rev/5afab84bd62f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

2 years ago
Depends on: 1190210
Kaku, do you think this should be part of the 42 release notes?

If yes, could you fill this form? Thanks
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Flags: needinfo?(tkuo)
(Assignee)

Comment 267

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #266)
> Kaku, do you think this should be part of the 42 release notes?
Yes and thanks for reminding! Is this reply enough or do I need to go into other process?

> If yes, could you fill this form? Thanks
> Release Note Request (optional, but appreciated)
> [Why is this notable]:
ImageBitmap is a lightweight representative to lots of image-like sources. What is important is that ImageBitmap could be used in the WebWorker and posting ImageBitmaps between windows/workers is very efficient.
> [Suggested wording]:
Support ImageBitmap and createImageBitmap().
> [Links (documentation, blog post, etc)]:
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap
https://developer.mozilla.org/en-US/docs/Web/API/CreateImageBitmap
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawImage
Flags: needinfo?(tkuo) → needinfo?(sledru)
Thanks, added to the release notes with this wording: "Support ImageBitmap and createImageBitmap()"
relnote-firefox: ? → 42+
Flags: needinfo?(sledru)
Reference docs:
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap/height
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap/width
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapFactories
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapFactories/createImageBitmap
Developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/42#New_APIs
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1224647
(Assignee)

Updated

2 years ago
Blocks: 1235301

Comment 270

2 years ago
Drawing a ImageBitmap created in a webworker to a canvas appears to be very slow.  the actual drawImage call takes 40+ms for a full HD photo, where had the ImageBitmap been created in the Main thread, it takes less than 1ms.

Please see bug:  https://bugzilla.mozilla.org/show_bug.cgi?id=1248392
(Assignee)

Updated

a year ago
No longer blocks: 1235301
You need to log in before you can comment on or make changes to this bug.