1215438 - StructuedClone of ImageBitmap is actually transfer underlying Image which is not right.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 1

[Tzuhao Kuo [:kaku]]

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?

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 3

[Tzuhao Kuo [:kaku]]

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.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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.

Comment 5

[Tzuhao Kuo [:kaku]]

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

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

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

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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 12

[Andrea Marchesini [:baku]]

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 13

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

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 14

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

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 15

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

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 16

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

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 17

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

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 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 21

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 23

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

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 24

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

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 25

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 28

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 29

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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/

Comment 30

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://reviewboard.mozilla.org/r/23227/#review21003

Since we don't have ThreadSafeSurfaceSnapshot now. I think we can remove CopyToAnotherThread as well.

Comment 31

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

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

Comment 32

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

Any reason why these haven't landed yet?

Comment 33

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 34

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 35

[Pulsebot]

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

Comment 36

[Carsten Book [:Tomcat]]

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