Closed Bug 1172796 Opened 9 years ago Closed 8 years ago

Implement remain feature of OffscreenCanvas.

Categories

(Core :: Graphics, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla46
feature-b2g 2.6?
tracking-b2g +
Tracking Status
firefox46 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

(Keywords: dev-doc-complete, perf, Whiteboard: gfx-noted)

Attachments

(9 files, 2 obsolete files)

40 bytes, text/x-review-board-request
seth
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
mtseng
: review+
Details
40 bytes, text/x-review-board-request
jgilbert
: review+
Details
Bug 709490 only implements a subset of OffscreenCanvas. This bug would implement remain features.
Whiteboard: gfx-noted
Must wait ImageBitmap before implement remaining features.
Depends on: 1044102
Current WIP version.
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Blocks: 1180705
Target Milestone: --- → FxOS-S10 (30Oct)
Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

We need ImageEncoder on worker thread because toBlob may happened on
worker thread due to OffscreenCanvas. So I split EncodingCompleteEvent
to 2 parts.
1. Inform the encoding complete event, just like EncodingCompleteEvent.
But remove the part of shutdown encoding thread. This part is running on
creation thread.
2. Shutdown the encoding thread. This part have to run on main-thread.

Split to those 2 parts, we can run ImageEncoder on worker thread.
Attachment #8672545 - Flags: review?(seth)
Bug 1172796 - Part 2: Implement toBlob and transferToImageBitmap. r=roc
Attachment #8672546 - Flags: review?(roc)
Bug 1172796 - Part 4: Implements ImageBitmapRenderingContext. r=roc
Attachment #8672548 - Flags: review?(roc)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

https://reviewboard.mozilla.org/r/21725/#review19545

::: dom/base/ImageEncoder.cpp:79
(Diff revision 1)
> -  EncodingCompleteEvent(nsIThread* aEncoderThread,
> +  explicit EncodingThreadShutdown(nsIThread* aEncoderThread)

"EncodingThreadShutdown" doesn't seem quite right - it's a little unclear out of context. Maybe "ShutdownEncodingThreadRunnable" or "EncodingThreadShutdownEvent"?

::: dom/base/ImageEncoder.cpp:235
(Diff revision 1)
> +      do_QueryInterface(mEncodingCompleteEvent);

Is it necessary to QI here? It doesn't seem like it should be.

::: dom/base/ImageEncoder.cpp:301
(Diff revision 1)
>    nsresult rv = NS_NewThread(getter_AddRefs(encoderThread), nullptr);

So we create a new OS thread every time someone calls toBlob()? That's not great. Shouldn't we be using a thread pool for this, at a minimum?

You didn't introduce this issue in this patch, of course, so you don't necessarily have to fix it right now. But you wouldn't need EncodingThreadShutdown if you were using a thread pool, so maybe it does make sense to just switch over to a thread pool at this point.
Attachment #8672545 - Flags: review?(seth)
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

https://reviewboard.mozilla.org/r/21727/#review19555

::: dom/canvas/ImageBitmap.cpp:545
(Diff revision 1)
> +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext());

Why do we need to do this on the main thread? It seems to me we cannot and should not. The SourceSurface we create here should belong solely to the worker's thread.

I think we need to add some assertions to SourceSurface to catch cases where it's being used on multiple threads.

::: dom/canvas/OffscreenCanvas.cpp:224
(Diff revision 1)
> +  return ImageBitmap::CreateFromOffscreenCanvas(workerPrivate->GlobalScope(), *this, rv);

This is not a transfer, it is a copy.

transferToImageBitmap should not make a copy. Instead it should detach the underlying canvas buffer, wrap an ImageBitmap object around that, and return that ImageBitmap. The canvas buffer should be replaced with a new clear buffer. (Actually we should just set the flags so that a new buffer is not allocated but will be allocated next time it's drawn into.)

Probably transferToImageBitmap should be implemented in a separate patch.
Attachment #8672546 - Flags: review?(roc)
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

https://reviewboard.mozilla.org/r/21729/#review19559

::: dom/canvas/ImageBitmap.cpp:423
(Diff revision 1)
> +  mSurface = nullptr;

You need to also clear mPictureRect so that width and height return 0.

Please add tests for that. (It looks like you currently have no close() tests at all.) Also add tests for drawing operations with a closed ImageBitmap --- they should do nothing I guess.
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

https://reviewboard.mozilla.org/r/21731/#review19561

::: dom/canvas/ImageBitmapRenderingContext.cpp:42
(Diff revision 1)
> +  aImageBitmap.Close();

I would prefer to have a method on ImageBitmap that closes the ImageBitmap and returns the old source surface in a single operation. Right now this code builds in the assumption that getting the surface and then Close()ing is a safe thing to do, which it might not be.

::: dom/canvas/ImageBitmapRenderingContext.cpp:89
(Diff revision 1)
> +  if (!data || data->GetSize() != IntSize(mWidth, mHeight)) {

toDataURL and toBlob should give the same results as the rendered content, i.e. match the canvas intrinsic size. So if the sizes don't match, render into a temporary surface and use that. This needs testing!

::: dom/canvas/ImageBitmapRenderingContext.cpp:198
(Diff revision 1)
> +  canvasLayer->Updated();

Does this actually do the right thing when the surface size doesn't match the intrinsic size? I have a feeling it might not :-(. Definitely needs testing!

::: gfx/layers/CopyableCanvasLayer.h:57
(Diff revision 1)
> +  RefPtr<gfx::SourceSurface> mDrawSurface;

How is this different from mSurface? Why do we need both? If we do need both, you definitely need to explain in comments here why.
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

https://reviewboard.mozilla.org/r/21733/#review19563

See earlier comments about tests.

I think it would be easier to review if you put the tests in the patches that add the code you're testing --- assuming the tests pass with that prefix of the patch queue.
Attachment #8672549 - Flags: review?(roc)
Attachment #8629870 - Attachment is obsolete: true
Attachment #8629871 - Attachment is obsolete: true
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> ::: dom/canvas/ImageBitmapRenderingContext.cpp:89
> (Diff revision 1)
> > +  if (!data || data->GetSize() != IntSize(mWidth, mHeight)) {
> 
> toDataURL and toBlob should give the same results as the rendered content,
> i.e. match the canvas intrinsic size. So if the sizes don't match, render
> into a temporary surface and use that. This needs testing!
> 

This copy from our canvas2d implementation. Looks like we also bail out if the sizes don't match in canvas2d. I also checked the behavior of webgl. It just return the buffer whether the intrinsic size is equal or not. Why canvas2d and webgl doesn't give the same results as the rendered content?

> ::: dom/canvas/ImageBitmap.cpp:545
> (Diff revision 1)
> > +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext());
> 
> Why do we need to do this on the main thread? It seems to me we cannot and
> should not. The SourceSurface we create here should belong solely to the
> worker's thread.
> 
> I think we need to add some assertions to SourceSurface to catch cases where
> it's being used on multiple threads.
The main reason is we wrap SourceSurface to CairoImage. But CairoImage only be accessed in main thread. I can write a different image type that can be operated in any thread. But there is a problem about SourceSurface lifetime. If the SourceSurface is create by worker then it should be released on worker as well. If we create a ImageBitmap in worker and transfer it to main-thread. Then we shutdown the worker. How do we release this ImageBitmap? Does it make sense to release this ImageBitmap on main-thread if the worker is dead?
Flags: needinfo?(roc)
(In reply to Morris Tseng [:mtseng] from comment #14)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> > ::: dom/canvas/ImageBitmapRenderingContext.cpp:89
> > (Diff revision 1)
> > > +  if (!data || data->GetSize() != IntSize(mWidth, mHeight)) {
> > 
> > toDataURL and toBlob should give the same results as the rendered content,
> > i.e. match the canvas intrinsic size. So if the sizes don't match, render
> > into a temporary surface and use that. This needs testing!
> > 
> 
> This copy from our canvas2d implementation. Looks like we also bail out if
> the sizes don't match in canvas2d. I also checked the behavior of webgl. It
> just return the buffer whether the intrinsic size is equal or not. Why
> canvas2d and webgl doesn't give the same results as the rendered content?

I don't know about WebGL. For canvas2d, the buffer size is always the same size as
the intrinsic size (or maybe it's zero in some special situations), so this isn't 
an issue for canvas2d. ImageBitmapRenderingContext is different, because it allows
the buffer to be any size independent of the intrinsic size.

> > ::: dom/canvas/ImageBitmap.cpp:545
> > (Diff revision 1)
> > > +    task->Dispatch(GetCurrentThreadWorkerPrivate()->GetJSContext());
> > 
> > Why do we need to do this on the main thread? It seems to me we cannot and
> > should not. The SourceSurface we create here should belong solely to the
> > worker's thread.
> > 
> > I think we need to add some assertions to SourceSurface to catch cases where
> > it's being used on multiple threads.
> The main reason is we wrap SourceSurface to CairoImage. But CairoImage only
> be accessed in main thread.
> I can write a different image type that can be
> operated in any thread. But there is a problem about SourceSurface lifetime.
> If the SourceSurface is create by worker then it should be released on
> worker as well. If we create a ImageBitmap in worker and transfer it to
> main-thread. Then we shutdown the worker. How do we release this
> ImageBitmap? Does it make sense to release this ImageBitmap on main-thread
> if the worker is dead?

Transferring arbitrary ImageBitmaps between threads is a tough problem. Some related facts:
1) SourceSurfaces can only belong to a single thread and be touched on that thread.
2) Currently CairoImage is documented to only work on the main thread. We could easily create them on another thread, but that wouldn't be useful since currently you could only draw/upload it (calling GetTextureClient) on the thread you created it on. The only thread that does uploads that you can also have an ImageBitmap on is the main thread.

I think you approach here of trying to create the SourceSurface on the main thread will not work. If worker code tries to draw the ImageBitmap into a canvas, we won't be able to do that since the SourceSurface can only be touched on the main thread, but we need to draw it in the worker thread.

I think the best solution here is to allow SourceSurfaces to be moved between threads (while still disallowing a SourceSurface from being used on more than one thread at a time). Please file a bug about that; we'll need to discuss it with the gfx team. If we get that working, then we can transfer ImageBitmaps between threads efficiently, and we can also change CairoImage so that it can be created and used on any (single) thread. When used in a worker, we would upload the CairoImage via the ImageBridge thread by temporarily passing ownership of the SourceSurface to the ImageBridge thread and waiting for a reply from ImageBridge which implicitly passes ownership back to the worker thread.

We should rename CairoImage to SourceSurfaceImage at some point too...
Flags: needinfo?(roc)
Depends on: 1215005
No longer blocks: 1180705
feature-b2g: --- → 3.0?
tracking-b2g: --- → +
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Target Milestone: FxOS-S10 (30Oct) → ---
Making sure there's some graphics folks CC'ed on this.
https://reviewboard.mozilla.org/r/21725/#review19545

> "EncodingThreadShutdown" doesn't seem quite right - it's a little unclear out of context. Maybe "ShutdownEncodingThreadRunnable" or "EncodingThreadShutdownEvent"?

This problem didn't exist when I using thread pool

> Is it necessary to QI here? It doesn't seem like it should be.

This problem didn't exist when I using thread pool
https://reviewboard.mozilla.org/r/21727/#review19555

> This is not a transfer, it is a copy.
> 
> transferToImageBitmap should not make a copy. Instead it should detach the underlying canvas buffer, wrap an ImageBitmap object around that, and return that ImageBitmap. The canvas buffer should be replaced with a new clear buffer. (Actually we should just set the flags so that a new buffer is not allocated but will be allocated next time it's drawn into.)
> 
> Probably transferToImageBitmap should be implemented in a separate patch.

For WebGL case, we cannot detach the underlying buffer easily. We need readback from GLContext. For Canvas2D case, it might able to do this. But since Canvas2D support for OffscreenCanvas(bug 801176) is not landed yet. Can we create a follow-up bug for 2D case?
Attachment #8672545 - Flags: review?(seth)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/1-2/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/1-2/
Attachment #8672546 - Attachment description: MozReview Request: Bug 1172796 - Part 2: Implement toBlob and transferToImageBitmap. r=roc → MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc
Attachment #8672546 - Flags: review?(roc)
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/1-2/
Attachment #8672547 - Attachment description: MozReview Request: Bug 1172796 - Part 3: Implements ImageBitmap::Close(). r=roc → MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc
Attachment #8672547 - Flags: review?(roc)
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/1-2/
Attachment #8672548 - Attachment description: MozReview Request: Bug 1172796 - Part 4: Implements ImageBitmapRenderingContext. r=roc → MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc
Attachment #8672548 - Flags: review?(roc)
Attachment #8672549 - Attachment description: MozReview Request: Bug 1172796 - Part 5: Add test cases. r=roc → MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc
Attachment #8672549 - Flags: review?(roc)
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/1-2/
Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc
Attachment #8688800 - Flags: review?(roc)
Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=roc
Attachment #8688801 - Flags: review?(roc)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

https://reviewboard.mozilla.org/r/21725/#review22929

::: dom/base/ImageEncoder.h:116
(Diff revision 2)
> +  static nsIThreadPool* sThreadPool;

Use StaticRefPtr.

::: dom/base/ImageEncoder.cpp:274
(Diff revision 2)
> -  nsCOMPtr<nsIThread> encoderThread;
> +  nsresult rv = ImageEncoder::EnsureThreadPool();

You can just write |EnsureThreadPool()|. No need for |ImageEncoder::|.

::: dom/base/ImageEncoder.cpp:310
(Diff revision 2)
> -  nsCOMPtr<nsIThread> encoderThread;
> +  nsresult rv = ImageEncoder::EnsureThreadPool();

Remove |ImageEncoder::| here too.

::: dom/base/ImageEncoder.cpp:499
(Diff revision 2)
> +    threadPool.forget(&sThreadPool);

You won't need to use |forget()| once you switch to StaticRefPtr. Just assign to |sThreadPool| directly.

::: dom/base/ImageEncoder.cpp:531
(Diff revision 2)
> +ImageEncoder::ReleaseThreadPool()

Get rid of this; use ClearOnShutdown() to clear |sThreadPool|.

::: dom/base/ImageEncoder.cpp:533
(Diff revision 2)
> +  NS_IF_RELEASE(sThreadPool);

StaticRefPtr would let you just write |sThreadPool = nullptr;| here, but it's irrelevant since you should get rid of ReleaseThreadPool() anyway.

::: dom/base/nsContentUtils.cpp:4386
(Diff revision 2)
> +  ImageEncoder::ReleaseThreadPool();

If you use ClearOnShutdown(), all the changes in this file can be removed.
Attachment #8672545 - Flags: review?(seth)
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

https://reviewboard.mozilla.org/r/21727/#review23035
Attachment #8672546 - Flags: review?(roc) → review+
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

https://reviewboard.mozilla.org/r/21729/#review23037
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

https://reviewboard.mozilla.org/r/21731/#review23045
Attachment #8672548 - Flags: review?(roc) → review+
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

https://reviewboard.mozilla.org/r/21733/#review23049

::: dom/canvas/OffscreenCanvas.cpp:224
(Diff revision 2)
> +  return ImageBitmap::CreateFromOffscreenCanvas(workerPrivate->GlobalScope(), *this, rv);

This doesn't actually do a transfer. It needs to at least clear the current canvas as if we'd done a transfer.
Attachment #8672549 - Flags: review?(roc)
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

https://reviewboard.mozilla.org/r/25417/#review23051
Attachment #8688799 - Flags: review?(roc) → review+
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review23167

::: dom/canvas/ImageBitmapRenderingContext.h:15
(Diff revision 1)
> + ** ImageBitmapRenderingContext

Say more about what this class is for. At least reference the spec.

::: dom/canvas/ImageBitmapRenderingContext.cpp:148
(Diff revision 1)
> +ImageBitmapRenderingContext::GetSurfaceSnapshot(bool* aPremultAlpha)

Don't we need to do some kind of size adjustment here, too?

::: dom/canvas/ImageBitmapRenderingContext.cpp:226
(Diff revision 1)
> +  return canvasLayer.forget();

I don't see anything here that clips the surface to the canvas size if it's larger. Better test that.

::: gfx/layers/CopyableCanvasLayer.cpp:114
(Diff revision 1)
> +      mAsyncRenderer || 

remove trailing whitespace
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

https://reviewboard.mozilla.org/r/25421/#review23169

You need review from a DOM peer!
feature-b2g: 3.0? → 2.6?
Keywords: perf
Attachment #8672545 - Flags: review?(seth)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/2-3/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/2-3/
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/2-3/
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/2-3/
Attachment #8672549 - Flags: review?(roc)
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/2-3/
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25417/diff/1-2/
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25419/diff/1-2/
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25421/diff/1-2/
Attachment #8688801 - Attachment description: MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=roc → MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug
Attachment #8688801 - Flags: review?(bugs)
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

So we're aiming to let this feature to be enabled in FF45 by default?
Attachment #8688801 - Flags: review?(bugs) → review+
Attachment #8672545 - Flags: review?(seth) → review+
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

https://reviewboard.mozilla.org/r/21725/#review23245

Looks good! One minor nit below, but I don't need to see this again. Thanks for doing this work!

::: dom/base/ImageEncoder.cpp:499
(Diff revisions 2 - 3)
>      nsCOMPtr<nsIThreadPool> threadPool = do_CreateInstance(NS_THREADPOOL_CONTRACTID);

Can't you just assign directly to sThreadPool?
https://reviewboard.mozilla.org/r/21723/#review23249

::: dom/webidl/OffscreenCanvas.webidl:12
(Diff revision 3)
>   * remaining spec.

The comment might need to be revised.
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

https://reviewboard.mozilla.org/r/21733/#review23265

::: dom/canvas/OffscreenCanvas.cpp:228
(Diff revisions 2 - 3)
> +  webGL->ClearScreen();

Please add some code to check the context type so we don't just crash once we support more context types in OffscreenCanvas.
Attachment #8672549 - Flags: review?(roc)
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review23267

::: dom/canvas/ImageBitmapRenderingContext.h:21
(Diff revisions 1 - 2)
> - ** ImageBitmapRenderingContext
> + * The purpose of ImageBitmapRenderingContext is providing a faster and efficient

"is to provide"

::: dom/canvas/ImageBitmapRenderingContext.h:23
(Diff revisions 1 - 2)
> + * the surface of ImageBitmap to this context and then using it to display.

"and then use it"

::: gfx/layers/CopyableCanvasLayer.cpp:110
(Diff revisions 1 - 2)
> -                               IntRect(0, 0, mBounds.width, mBounds.height),
> +                               IntRect(0, 0, copyWidth, copyHeight),

I still think we need something here to handle the case where we need to clip the input surface to the canvas intrinsic size.
(In reply to Olli Pettay [:smaug] from comment #43)
> Comment on attachment 8688801 [details]
> MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to
> test_intefaces.html r=smaug
> 
> So we're aiming to let this feature to be enabled in FF45 by default?

Probably not. There are some stability issues need to be resolved. We'll enable it by default once those issues got resolved.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #45)
> https://reviewboard.mozilla.org/r/21723/#review23249
> 
> ::: dom/webidl/OffscreenCanvas.webidl:12
> (Diff revision 3)
> >   * remaining spec.
> 
> The comment might need to be revised.
Great catch!! I'll fix it. Thanks Ting-Yu.
https://reviewboard.mozilla.org/r/21725/#review23245

> Can't you just assign directly to sThreadPool?

I can't. do_CreateInstance returns a special helper for nsCOMPtr instread of a raw pointer. So I cannot assign it to a StaticRefPtr directly. Can I stick with current version?
ni seth for comment 50.
Flags: needinfo?(seth)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/3-4/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/3-4/
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/3-4/
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/3-4/
Attachment #8672549 - Flags: review?(roc)
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/3-4/
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25417/diff/2-3/
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25419/diff/2-3/
Attachment #8688801 - Flags: review+ → review?(bugs)
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25421/diff/2-3/
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Carry smaug's r+ from comment 43.
Attachment #8688801 - Flags: review?(bugs) → review+
(In reply to Morris Tseng [:mtseng] from comment #50)
> https://reviewboard.mozilla.org/r/21725/#review23245
> 
> > Can't you just assign directly to sThreadPool?
> 
> I can't. do_CreateInstance returns a special helper for nsCOMPtr instread of
> a raw pointer. So I cannot assign it to a StaticRefPtr directly. Can I stick
> with current version?

Yes. That wasn't a required change, or I would've opened an issue for it.
Flags: needinfo?(seth)
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

https://reviewboard.mozilla.org/r/21733/#review23331
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review23335

I should have thought of this earlier, but the whole point of ImageBitmapRenderingContext is to enable rendering an ImageBitmap without copying it, but we do copy it here :-(. At least for the case where the ImageBitmap size matches the canvas size, we should be able to avoid a copy.

So I think a better approach would be to create an ImageLayer for the canvas instead of a CanvasLayer when this context is in use. When the image size is correct, we can put the ImageBitmap's Image directly into an ImageContainer for the ImageLayer. Otherwise we can create the temporary surface and use that.
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/4-5/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/4-5/
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/4-5/
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/4-5/
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/4-5/
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25417/diff/3-4/
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25419/diff/3-4/
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25421/diff/3-4/
Attachment #8688801 - Flags: review+ → review?(bugs)
Attachment #8688801 - Flags: review?(bugs) → review+
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review23685

Needs DOM peer review for the WebIDL change.

::: dom/canvas/ImageBitmapRenderingContext.cpp:41
(Diff revision 4)
> +    mDisplayImage = new layers::SourceSurfaceImage(gfx::IntSize(mWidth, mHeight), surface);

I think you're supposed to pass the surface size here. I don't think passing a different size will necessarily work. Why not call MatchWithIntrinsicSize instead?

::: dom/canvas/ImageBitmapRenderingContext.cpp:214
(Diff revision 4)
> +    return ret.forget();

Don't you need to do something here in case the image changed?

eds t
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/5-6/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/5-6/
Attachment #8672547 - Attachment description: MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc → MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug
Attachment #8672547 - Flags: review?(bugs)
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/5-6/
Attachment #8672548 - Attachment description: MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc → MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug
Attachment #8672548 - Flags: review?(bugs)
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/5-6/
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/5-6/
Attachment #8672549 - Attachment description: MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc → MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug
Attachment #8672549 - Flags: review?(bugs)
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25417/diff/4-5/
Attachment #8688799 - Attachment description: MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc → MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug
Attachment #8688799 - Flags: review?(bugs)
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25419/diff/4-5/
Attachment #8688800 - Attachment description: MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc → MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug
Attachment #8688800 - Flags: review?(roc)
Attachment #8688800 - Flags: review?(bugs)
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25421/diff/4-5/
Attachment #8688801 - Flags: review+ → review?(bugs)
Attachment #8688801 - Flags: review?(bugs) → review+
smaug, would you mind to review webidl changes in part 3-7? Thanks.
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review23829

::: dom/canvas/ImageBitmapRenderingContext.cpp:45
(Diff revisions 4 - 5)
> -    mDisplayImage = new layers::SourceSurfaceImage(gfx::IntSize(mWidth, mHeight), surface);
> +    mImage = new layers::SourceSurfaceImage(gfx::IntSize(mWidth, mHeight), surface);

If someone passes an image to a too-small canvas but later the canvas resizes to a bigger size, we'll continue displaying the clipped mImage. So we need to write a test for that, and create a separate clipped mDisplayImage if necessary ... and update mDisplayImage when the canvas size changes. Might be best to create/update mDisplayImage every time we paint, checking if the size has changed.
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/6-7/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/6-7/
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/6-7/
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/6-7/
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/6-7/
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25417/diff/5-6/
Attachment #8688800 - Flags: review?(roc)
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25419/diff/5-6/
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25421/diff/5-6/
Attachment #8688801 - Flags: review+ → review?(bugs)
Attachment #8688801 - Flags: review?(bugs) → review+
Attachment #8672547 - Flags: review?(bugs) → review+
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

https://reviewboard.mozilla.org/r/21729/#review24041
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

https://reviewboard.mozilla.org/r/21731/#review24049

So this is missing some tests for main thread usage.

::: dom/canvas/OffscreenCanvas.cpp:232
(Diff revision 7)
> +

OffscreenCanvas can be used both in main thread and in workers. This code crashes if used in main thread.
Attachment #8672548 - Flags: review?(bugs)
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

https://reviewboard.mozilla.org/r/21733/#review24067

Need also mainthread tests.

::: dom/canvas/OffscreenCanvas.cpp:224
(Diff revision 7)
> +  RefPtr<ImageBitmap> result = ImageBitmap::CreateFromOffscreenCanvas(workerPrivate->GlobalScope(), *this, rv);

Again, this code assumes OffscreenCanvas is used only in workers, but nothing prevents its use in the main thread (and the spec seems to allow main thread usage too)
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

https://reviewboard.mozilla.org/r/25417/#review24077

::: dom/canvas/ImageBitmap.cpp:416
(Diff revision 6)
> +  mPictureRect.SetEmpty();

So this is safe also in workers, right? Perhaps add a test which is run in workers too.
Attachment #8688799 - Flags: review?(bugs) → review+
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review24079

This really needs tests also for worker usage.
ImageBitmapRenderingContext is exposed to workers, so I assume offscreenCanvas.getContent("bitmaprenderer") should just work. in workers and main thread.

::: dom/canvas/ImageBitmapRenderingContext.h:41
(Diff revision 6)
> +  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;

s/JSContext */JSContext*/
Similar also elsewhere.
Attachment #8688800 - Flags: review?(bugs)
Comment on attachment 8672545 [details]
MozReview Request: Bug 1172796 - Part 1: ImageEncoder can be used on worker thread. r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21725/diff/7-8/
Comment on attachment 8672546 [details]
MozReview Request: Bug 1172796 - Part 2: Add write-only flag for origin-clean check. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21727/diff/7-8/
Comment on attachment 8672547 [details]
MozReview Request: Bug 1172796 - Part 3: Implement OffscreenCanvas constructor. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21729/diff/7-8/
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21731/diff/7-8/
Attachment #8672548 - Flags: review- → review?(bugs)
Attachment #8672549 - Flags: review- → review?(bugs)
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21733/diff/7-8/
Comment on attachment 8688799 [details]
MozReview Request: Bug 1172796 - Part 6: Implements ImageBitmap::Close(). r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25417/diff/6-7/
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25419/diff/6-7/
Attachment #8688800 - Flags: review- → review?(bugs)
Attachment #8688801 - Flags: review+ → review?(bugs)
Comment on attachment 8688801 [details]
MozReview Request: Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25421/diff/6-7/
Bug 1172796 - Part 9: Use gfxPrefs to get webgl.enable-prototype-webgl2. r=jgilbert
Attachment #8694089 - Flags: review?(jgilbert)
Attachment #8688801 - Flags: review?(bugs) → review+
Comment on attachment 8672548 [details]
MozReview Request: Bug 1172796 - Part 4: Implement OffscreenCanvas::ToBlob. r=roc r=smaug

(back to using bugzilla and patches for reviewing and no mozreview for me. Too difficult/slow to use and not really providing useful tools for me. )

   aRv = ImageEncoder::ExtractDataAsync(type,
                                        params,
                                        usingCustomParseOptions,
                                        Move(imageBuffer),
                                        format,
                                        GetWidthHeight(),
                                        callback);
please verify from seth that ExtractDataAsync is threadsafe, or more precisely
thread agnostic.


+OffscreenCanvas::ToBlob(JSContext* aCx,
+                        const nsAString& aType,
+                        JS::Handle<JS::Value> aParams,
+                        ErrorResult& aRv)
+{
+  // do a trust check if this is a write-only canvas
+  if (mIsWriteOnly) {
+    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
+    return nullptr;
+  }
I don't see any tests for this case. We certainly need them, both for main thread and worker case. Feel free to upload a followup patch which includes just those tests.
Attachment #8672548 - Flags: review?(bugs) → review+
Comment on attachment 8672549 [details]
MozReview Request: Bug 1172796 - Part 5: Implements OffscreenCanvas::TransferImageBitmap. r=roc r=smaug

I don't see tests for
+ImageBitmap::CreateFromOffscreenCanvas(nsIGlobalObject* aGlobal,
+                                       OffscreenCanvas& aOffscreenCanvas,
+                                       ErrorResult& aRv)
+{
+  // Check origin-clean.
+  if (aOffscreenCanvas.IsWriteOnly()) {
+    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
+    return nullptr;
+  }
Both mainthread and worker tests, please. But again, a followup patch where toBlob is tested too is fine.

+  if (!mCurrentContext)
+    return nullptr;
always {} with 'if'

+  static SurfaceFromElementResult
+  SurfaceFromOffscreenCanvas(mozilla::dom::OffscreenCanvas *aOffscreenCanvas,
This layout code is something roc is way more familiar with, but I would
document clearly that the method is thread agnostic - safe to call in any thread.

+nsLayoutUtils::SurfaceFromOffscreenCanvas
some part of the method uses 3 spaces for indentation. please use 2 everywhere.
Attachment #8672549 - Flags: review?(bugs) → review+
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

+protected:
+  already_AddRefed<gfx::DataSourceSurface> MatchWithIntrinsicSize();
+  void ClipToIntrinsicSize();
+  int32_t mWidth, mHeight;
nit, I would put mWidth and mHeigth to separate lines.

gfx API usage needs to be reviewed by roc.
Attachment #8688800 - Flags: review?(bugs) → review+
Comment on attachment 8688800 [details]
MozReview Request: Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc r=smaug

https://reviewboard.mozilla.org/r/25419/#review24363
Attachment #8688800 - Flags: review?(roc) → review+
Comment on attachment 8694089 [details]
MozReview Request: Bug 1172796 - Part 9: Use gfxPrefs to get webgl.enable-prototype-webgl2. r=jgilbert

https://reviewboard.mozilla.org/r/26653/#review24909
Attachment #8694089 - Flags: review?(jgilbert) → review+
Depends on: 1244118
Depends on: 1244696
Depends on: 1234531
What do we need to enable OffscreenCanvas by default?
Does this depend on Bug 801176?
Flags: needinfo?(vliu)
Flags: needinfo?(mtseng)
There are some stability issues. We won't enable this by default before we solved those stability issues.
Flags: needinfo?(mtseng)
Clear ni? since Morris has commented.
Flags: needinfo?(vliu)
Depends on: 1486187
Regressions: 1599696
You need to log in before you can comment on or make changes to this bug.