Right now when we structured clone a ImageBitmap, we actually transfer the underlying Image to another thread which is not right. Because thus we have multiple active threads access same SourceSurface which is not thread-safe. According the spec [1], we should copy the underlying surface when we structured clone the ImageBitmap. And for the transferring of ImageBitmap, [2] says we should neuter the ImageBitmap on the sending thread. That is, we have only one active ImageBitmap when we transferring. I think this is right behavior. This bug should re-implement structured-clone and transfer part of ImageBitmap. Although we need bug 1215005 to move SourceSurface to another thread. Set this depend on bug 1215005. [1]: https://html.spec.whatwg.org/#structured-clone [2]: https://wiki.whatwg.org/wiki/WorkerCanvas

Hi Morris, This is kind of optimization and the reason why we implemented it in this way is that the ImageBitmap is immutable, so even thought a single underlying Image is accessible via multiple active threads, those threads can only read it without writing. We can still support transferring to the ImageBitmap with neutering behavior, and keep the current StructuredClone implementation, what do you think?

Hi kaku, I think access ImageBitmap in multiple active threads at the same time is no problem. The problem is the life cycle of SourceSurface. The SourceSurface is owned by its creation thread and we need release it on that thread, too. Considering following example: 1. Create ImageBitmap on worker via OffscreenCanvas. 2. Structured clone this ImageBitmap to main-thread. 3. Shutdown the worker. In this example, On the step2, we have same SourceSurface on 2 active threads and worker owns this SourceSurface. On the step3, we close the worker which means we cannot release SourceSurface on the worker which causes problem. That's why I think current implementation is wrong. We should either copy the underlying buffer for structured clone or transfer ownership of SourceSurface explicitly for transferring ImageBitmap to ensure the problem I mentioned above doesn't happened. I think the optimization is good once the lifetime issue is resolved. Or we should copy underlying buffer. At least we also provided transferring mechanism.

I got your idea, however it is not really a problem in current implementation. For now, only creating ImageBitmap from ImageData or Blob needs to create a SoruceSurface, however, the tasks of creating a SourceSurface are all dispatched to the main thread and I assume that the main thread is always there, right? Is it possible to implement the "step 1" through the same way described above? (You might refer to CreateImageFromRawDataInMainThreadSyncTask() method.) If it is not possible, then I agree that we need to fall back to the standard StructuredClone implementation.

(In reply to Tzuhao Kuo [:kaku] (Out of office from 9/26 to 10/11) from comment #3) > I got your idea, however it is not really a problem in current > implementation. > > For now, only creating ImageBitmap from ImageData or Blob needs to create a > SoruceSurface, however, the tasks of creating a SourceSurface are all > dispatched to the main thread and I assume that the main thread is always > there, right? > > Is it possible to implement the "step 1" through the same way described > above? (You might refer to CreateImageFromRawDataInMainThreadSyncTask() > method.) If it is not possible, then I agree that we need to fall back to > the standard StructuredClone implementation. We can't. Please refer bug 1172796 comment 15.

I totally understand now. BTW, will bug1215002 helps to keep the current transferring-like StructuredClone behavior?

I guess you mean bug 1215005? It will help if SourceSurface is keep by one thread at the same time. If neuter the ImageBitmap once it is transferred, it should be fine. But if you want to keep current behavior which mean two threads keep SourceSurface at the same time, it wouldn't help.

Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc

Bug 1215438 - Part 2: Add transfer support for ImageBitmap. r=baku

Bug 1215438 - Part 3: CairoImage can use in any thread. r=roc

Bug 1215438 - Part 4: Rename CairoImage to SourceSurfaceImage. r=roc

Bug 1215438 - Part 5: Add test for imagebitmap transfer. r=roc

Comment on attachment 8678740 [details] MozReview Request: Bug 1215438 - Part 2: Add transfer support for ImageBitmap. r=baku https://reviewboard.mozilla.org/r/23229/#review20709

Comment on attachment 8678740 [details] MozReview Request: Bug 1215438 - Part 2: Add transfer support for ImageBitmap. r=baku https://reviewboard.mozilla.org/r/23229/#review20817

Comment on attachment 8678741 [details] MozReview Request: Bug 1215438 - Part 3: CairoImage can use in any thread. r=roc https://reviewboard.mozilla.org/r/23231/#review20819

Comment on attachment 8678742 [details] MozReview Request: Bug 1215438 - Part 4: Rename CairoImage to SourceSurfaceImage. r=roc https://reviewboard.mozilla.org/r/23233/#review20821 Nice!

Comment on attachment 8678743 [details] MozReview Request: Bug 1215438 - Part 5: Add test for imagebitmap transfer. r=roc https://reviewboard.mozilla.org/r/23235/#review20823

Comment on attachment 8678739 [details] MozReview Request: Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc https://reviewboard.mozilla.org/r/23227/#review20825 ::: dom/canvas/ImageBitmap.cpp:1279 (Diff revision 1) > + RefPtr<ThreadSafeSurfaceSnapshot> snapshot = dstDataSurface->TransferToAnotherThread(); Actually I think we should add SourceSurface::CopyToAnotherThread, which creates a ThreadSafeSurfaceSnapshot by copying instead of transferring. And we should call that on 'surface', and it should never fail since it can fall back to the data-surface code you have here. On some platforms we can do a faster copy without readback; and on some platforms they can perhaps just share the data instead of actually making a copy. ::: gfx/gl/GLReadTexImageHelper.h:42 (Diff revision 1) > + gfx::DataSourceSurface* aDest); This should be moved out of GL since it's not GL specific. Put it somewhere in gfx/2d?

Comment on attachment 8678739 [details] MozReview Request: Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc

Comment on attachment 8678740 [details] MozReview Request: Bug 1215438 - Part 2: Add transfer support for ImageBitmap. r=baku Bug 1215438 - Part 2: Add transfer support for ImageBitmap. r=baku

Comment on attachment 8678741 [details] MozReview Request: Bug 1215438 - Part 3: CairoImage can use in any thread. r=roc Bug 1215438 - Part 3: CairoImage can use in any thread. r=roc

Comment on attachment 8678742 [details] MozReview Request: Bug 1215438 - Part 4: Rename CairoImage to SourceSurfaceImage. r=roc Bug 1215438 - Part 4: Rename CairoImage to SourceSurfaceImage. r=roc

Comment on attachment 8678743 [details] MozReview Request: Bug 1215438 - Part 5: Add test for imagebitmap transfer. r=roc Bug 1215438 - Part 5: Add test for imagebitmap transfer. r=roc

Comment on attachment 8678739 [details] MozReview Request: Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc https://reviewboard.mozilla.org/r/23227/#review21001

Comment on attachment 8678739 [details] MozReview Request: Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc https://reviewboard.mozilla.org/r/23227/#review21003 You should get a gfx person to review the CopyToAnotherThread API.

Comment on attachment 8678739 [details] MozReview Request: Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23227/diff/2-3/

Comment on attachment 8678740 [details] MozReview Request: Bug 1215438 - Part 2: Add transfer support for ImageBitmap. r=baku Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23229/diff/2-3/

Comment on attachment 8678741 [details] MozReview Request: Bug 1215438 - Part 3: CairoImage can use in any thread. r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23231/diff/2-3/

Comment on attachment 8678742 [details] MozReview Request: Bug 1215438 - Part 4: Rename CairoImage to SourceSurfaceImage. r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23233/diff/2-3/

Comment on attachment 8678743 [details] MozReview Request: Bug 1215438 - Part 5: Add test for imagebitmap transfer. r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23235/diff/2-3/

https://reviewboard.mozilla.org/r/23227/#review21003 Since we don't have ThreadSafeSurfaceSnapshot now. I think we can remove CopyToAnotherThread as well.

Comment on attachment 8678739 [details] MozReview Request: Bug 1215438 - Part 1: Add utility functions to support transferring ImageBitmap. r=roc https://reviewboard.mozilla.org/r/23227/#review22393

Any reason why these haven't landed yet?

I'm looking into a try failure on windows platform. Here is try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcad7191d51c And another reason is this bug use a function which is implemented in bug 1172796. So my plan is land with bug 1172796.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a24c8cab934c

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6709eb307f https://hg.mozilla.org/integration/mozilla-inbound/rev/384e0084e85f https://hg.mozilla.org/integration/mozilla-inbound/rev/dd4d0619b391 https://hg.mozilla.org/integration/mozilla-inbound/rev/c07ad4a4f4fd https://hg.mozilla.org/integration/mozilla-inbound/rev/265b7d0980e2

https://hg.mozilla.org/mozilla-central/rev/3b6709eb307f https://hg.mozilla.org/mozilla-central/rev/384e0084e85f https://hg.mozilla.org/mozilla-central/rev/dd4d0619b391 https://hg.mozilla.org/mozilla-central/rev/c07ad4a4f4fd https://hg.mozilla.org/mozilla-central/rev/265b7d0980e2