Bug 709490 only implements a subset of OffscreenCanvas. This bug would implement remain features.

Must wait ImageBitmap before implement remaining features.

Current WIP version.

Current WIP version.

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.

Bug 1172796 - Part 2: Implement toBlob and transferToImageBitmap. r=roc

Bug 1172796 - Part 3: Implements ImageBitmap::Close(). r=roc

Bug 1172796 - Part 4: Implements ImageBitmapRenderingContext. r=roc

Bug 1172796 - Part 5: Add test cases. r=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.

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.

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.

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

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

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?

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/

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/

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/

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 6: Implements ImageBitmap::Close(). r=roc

Bug 1172796 - Part 7: Implements ImageBitmapRenderingContext. r=roc

Bug 1172796 - Part 8: Add ImageBitmapRenderingContext to test_intefaces.html r=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.

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

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

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.

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

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!

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/

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/

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/

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?

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.

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.

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/

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/

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/

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.

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

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.

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/

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/

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

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/

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/

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/

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/

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/

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/

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.

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/

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/

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.

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.

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.

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/

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/

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

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.

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.

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.

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

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff06ac4e4c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/09a6fc31e52d https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc0f234fbc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/802a42308109 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8792ea5d624 https://hg.mozilla.org/integration/mozilla-inbound/rev/f28299005e54 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a971c8065b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfe2aa6c8c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/c347dd2df979

https://hg.mozilla.org/mozilla-central/rev/7ff06ac4e4c0 https://hg.mozilla.org/mozilla-central/rev/09a6fc31e52d https://hg.mozilla.org/mozilla-central/rev/2bc0f234fbc5 https://hg.mozilla.org/mozilla-central/rev/802a42308109 https://hg.mozilla.org/mozilla-central/rev/c8792ea5d624 https://hg.mozilla.org/mozilla-central/rev/f28299005e54 https://hg.mozilla.org/mozilla-central/rev/6a971c8065b0 https://hg.mozilla.org/mozilla-central/rev/3bfe2aa6c8c4 https://hg.mozilla.org/mozilla-central/rev/c347dd2df979

Documentation updates: OffscreenCanvas constructor (behind pref) https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/OffscreenCanvas OffscreenCanvas.toBlob (behind pref) https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/toBlob OffscreenCanvas.transferToImageBitmap (behind pref) https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/transferToImageBitmap ImageBitmap.close (no pref) https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap/close ImageBitmapRenderingContext (no pref) https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapRenderingContext ImageBitmapRenderingContext.transferImageBitmap (no pref) https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapRenderingContext/transferImageBitmap Added "bitmaprenderer" getContext methods (no pref) https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/getContext https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/getContext Developer release notes https://developer.mozilla.org/en-US/Firefox/Releases/46#Canvas Any review to the MDN wiki pages is appreciated (as always).

What do we need to enable OffscreenCanvas by default? Does this depend on Bug 801176?

There are some stability issues. We won't enable this by default before we solved those stability issues.

Clear ni? since Morris has commented.