Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1962413002:
Implement transferToImageBitmap() in WebGLRenderingContext (Closed)
|
Created:
4 years, 7 months ago by xidachen Modified:
4 years, 7 months ago Reviewers:
Ken Russell (switch to Gerrit), bajones, Justin Novosad, Zhenyao Mo, bsalomon, bsalomon_chromium CC:
chromium-reviews, blink-reviews, haraken Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement transferToImageBitmap() in WebGLRenderingContext
To verify that WebGL Rendering works with OffscreenCanvas, we need to
implement this function first.
This CL also comes with two layout tests does the following things:
1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the
ImageBitmap to another canvas via transferFromImageBitmap.
2. Make sure the above case works on a worker thread.
These two layout tests will eventually be moved to KhronosGroup
as part of its conformance test. However, at this moment, the layout
tests serve the purpose of verifying correctness of the code changes.
BUG=610759
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/d30c45e8878402c605e467ce537897ef38748c98
Cr-Commit-Position: refs/heads/master@{#395065}
Patch Set 1 #
Total comments: 8
Patch Set 2 : layout test fails #
Total comments: 3
Patch Set 3 : works #
Total comments: 3
Patch Set 4 : using mailbox, it works #Patch Set 5 : code clean up, almost identical to PS4 #
Total comments: 8
Patch Set 6 : address kbr@'s and bsalomon@'s comment #
Total comments: 3
Patch Set 7 : add new test case, cache SkIMage #
Total comments: 2
Messages
Total messages: 44 (11 generated)
Description was changed from ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. BUG=610759 ========== to ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. These two layout tests will eventually be moved to KhronosGroup as part of its conformance test. However, at this moment, the layout tests serve the purpose of verifying correctness of the code changes. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
xidachen@chromium.org changed reviewers: + bajones@chromium.org, junov@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL
https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/webgl/OffscreenCanvas-TransferToFromImageBitmap-expected.html (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/webgl/OffscreenCanvas-TransferToFromImageBitmap-expected.html:4: <canvas id='output' width='100' height='100' style='background:red'></canvas> Apologies, I'm unfamiliar with reference tests in the layout test infrastructure -- this does a pixel comparison with OffscreenCanvas-TransferToFromImageBitmap.html? https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:90: attributes.setPremultipliedAlpha(false); I don't think it's legal to just set this all the time for WebGL contexts. It will break rendering of certain applications which assume the compositor and the WebGL app behave in a certain way. Unfortunately, I don't think this is or can be covered by a WebGL conformance test, but we should add a reference test that verifies the behavior of premultipliedAlpha:true and false for WebGL-rendered canvases. Can you help with that or should I write them? https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:638: ImageData* imageData = paintRenderingResultsToImageData(BackBuffer); This needs to be much better optimized in the future. This operation should transfer the DrawingBuffer's texture into a texture mailbox, retrieve that texture into a new texture object (presumably), and create an ImageBitmap from that texture ID. Is this how the 2D canvas context works, or will work? Is there a plan for this? Please, minimally, add a TODO to this that this needs to avoid readbacks from the GPU, which is how the canvas is getting the data into an ImageData object.
By the way, the new tests look good.
https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/webgl/OffscreenCanvas-TransferToFromImageBitmap-expected.html (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/webgl/OffscreenCanvas-TransferToFromImageBitmap-expected.html:4: <canvas id='output' width='100' height='100' style='background:red'></canvas> On 2016/05/10 23:11:41, Ken Russell wrote: > Apologies, I'm unfamiliar with reference tests in the layout test infrastructure > -- this does a pixel comparison with > OffscreenCanvas-TransferToFromImageBitmap.html? Yes, it does per-pixel comparison with the reference html. Even if a pixel value is off by 1, it will report as failure. https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:90: attributes.setPremultipliedAlpha(false); On 2016/05/10 23:11:42, Ken Russell wrote: > I don't think it's legal to just set this all the time for WebGL contexts. It > will break rendering of certain applications which assume the compositor and the > WebGL app behave in a certain way. Unfortunately, I don't think this is or can > be covered by a WebGL conformance test, but we should add a reference test that > verifies the behavior of premultipliedAlpha:true and false for WebGL-rendered > canvases. Can you help with that or should I write them? I did hesitate when I added this statement. The only reason I put it here is that there is a "ASSERT(!m_premultipliedAlpha);" in the method DrawingBuffer::paintRenderingResultsToImageData. So without this line, I am always hitting that ASSERT. Is there a way to avoid hitting that ASSERT without setting false here? I'd like to remove this like we can avoid that ASSERT. https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:638: ImageData* imageData = paintRenderingResultsToImageData(BackBuffer); On 2016/05/10 23:11:42, Ken Russell wrote: > This needs to be much better optimized in the future. This operation should > transfer the DrawingBuffer's texture into a texture mailbox, retrieve that > texture into a new texture object (presumably), and create an ImageBitmap from > that texture ID. Is this how the 2D canvas context works, or will work? Is there > a plan for this? Please, minimally, add a TODO to this that this needs to avoid > readbacks from the GPU, which is how the canvas is getting the data into an > ImageData object. In the case of 2d canvas context, it contains a ImageBuffer private member, and creating an ImageBitmap from a ImageBuffer is easy. I will dig into your suggestion here. If it is not hard to get a new texture object and create an ImageBitmap, then I will do that in this CL. I agree with you, accessing the entire image data and creating from that is really in-efficient.
https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:90: attributes.setPremultipliedAlpha(false); On 2016/05/11 00:17:52, xidachen wrote: > On 2016/05/10 23:11:42, Ken Russell wrote: > > I don't think it's legal to just set this all the time for WebGL contexts. It > > will break rendering of certain applications which assume the compositor and > the > > WebGL app behave in a certain way. Unfortunately, I don't think this is or can > > be covered by a WebGL conformance test, but we should add a reference test > that > > verifies the behavior of premultipliedAlpha:true and false for WebGL-rendered > > canvases. Can you help with that or should I write them? > > I did hesitate when I added this statement. The only reason I put it here is > that there is a "ASSERT(!m_premultipliedAlpha);" in the method > DrawingBuffer::paintRenderingResultsToImageData. So without this line, I am > always hitting that ASSERT. Is there a way to avoid hitting that ASSERT without > setting false here? I'd like to remove this like we can avoid that ASSERT. It's likely that that code path has never been tested, and that the assertion was merely put in place to force re-evaluation of the code if anyone hit it. This code path would probably take effect if someone called toDataURL() on a WebGL-rendered canvas where the context was created with premultiplyAlpha:true. Also, clearly, your new code is hitting it. In order to generalize it, a lossy "unmultiplication" of the alpha channel would have to be done during the readback. Thinking about it, something like this must be being done for 2D canvas when fetching the ImageData for the backing store. Can you talk with junov@ and confirm? In some sense, there's no problem with adding support for AlphaDoUnmultiply in DrawingBuffer::readBackFramebuffer -- but if we do this, we need to add tests. Alternatively, we could work on implementing this the right way, and avoid the readbacks entirely when creating the ImageBitmap.
https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/1962413002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:90: attributes.setPremultipliedAlpha(false); On 2016/05/11 01:00:41, Ken Russell wrote: > On 2016/05/11 00:17:52, xidachen wrote: > > On 2016/05/10 23:11:42, Ken Russell wrote: > > > I don't think it's legal to just set this all the time for WebGL contexts. > It > > > will break rendering of certain applications which assume the compositor and > > the > > > WebGL app behave in a certain way. Unfortunately, I don't think this is or > can > > > be covered by a WebGL conformance test, but we should add a reference test > > that > > > verifies the behavior of premultipliedAlpha:true and false for > WebGL-rendered > > > canvases. Can you help with that or should I write them? > > > > I did hesitate when I added this statement. The only reason I put it here is > > that there is a "ASSERT(!m_premultipliedAlpha);" in the method > > DrawingBuffer::paintRenderingResultsToImageData. So without this line, I am > > always hitting that ASSERT. Is there a way to avoid hitting that ASSERT > without > > setting false here? I'd like to remove this like we can avoid that ASSERT. > > It's likely that that code path has never been tested, and that the assertion > was merely put in place to force re-evaluation of the code if anyone hit it. > > This code path would probably take effect if someone called toDataURL() on a > WebGL-rendered canvas where the context was created with premultiplyAlpha:true. > Also, clearly, your new code is hitting it. > > In order to generalize it, a lossy "unmultiplication" of the alpha channel would > have to be done during the readback. Thinking about it, something like this must > be being done for 2D canvas when fetching the ImageData for the backing store. > Can you talk with junov@ and confirm? > > In some sense, there's no problem with adding support for AlphaDoUnmultiply in > DrawingBuffer::readBackFramebuffer -- but if we do this, we need to add tests. > > Alternatively, we could work on implementing this the right way, and avoid the > readbacks entirely when creating the ImageBitmap. Yeah, it makes more sense to implement this using another approach. I just took another look at: WebGLRenderingContextBase::paintRenderingResultsToImageData Apparent it returns nullptr when the premultipliedAlpha=true.
https://codereview.chromium.org/1962413002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:646: sk_sp<SkSurface> surface = SkSurface::MakeRenderTarget(grContext, SkBudgeted::kNo, info, 0, (m_requestedAttributes.premultipliedAlpha()) ? &disableLCDProps : nullptr); junov@: could you take a look? I am not sure what am I missing here, I looked at the pixels in this surface, and it is all 0s.
https://codereview.chromium.org/1962413002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:646: sk_sp<SkSurface> surface = SkSurface::MakeRenderTarget(grContext, SkBudgeted::kNo, info, 0, (m_requestedAttributes.premultipliedAlpha()) ? &disableLCDProps : nullptr); On 2016/05/13 18:56:08, xidachen wrote: > junov@: could you take a look? I am not sure what am I missing here, I looked at > the pixels in this surface, and it is all 0s. The code above is referencing the Ganesh context for the DrawingBuffer which surely won't do what you want. Skia and Ganesh know nothing about the textures the DrawingBuffer allocates internally. Look at DrawingBuffer::prepareMailbox for the code structure which sends the back buffer's texture over to the compositor's context. Something like that will be needed. I don't know what the "receiving" context will be for the texture. Maybe it doesn't need to be known at the time the ImageBitmap is constructed, only when it's consumed. So the process of creating an ImageBitmap from a WebGLRenderingContext base may be: put the back buffer texture into a texture mailbox, and construct the ImageBitmap from that mailbox. When the ImageBitmap is later transferred into an ImageBitmapRenderingContext, that's when the mailbox is consumed into the target canvas's context.
https://codereview.chromium.org/1962413002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:646: sk_sp<SkSurface> surface = SkSurface::MakeRenderTarget(grContext, SkBudgeted::kNo, info, 0, (m_requestedAttributes.premultipliedAlpha()) ? &disableLCDProps : nullptr); I agree with what Ken wrote. Sorry for the bad path I put you on earlier. The various code paths that consume ImageBitmaps are going to need to provide a GrContext in order to access the mailbox. This might be tricky.
https://codereview.chromium.org/1962413002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:654: if (!(drawingBuffer()->copyToPlatformTexture(gl, textureId, GL_RGBA, GL_UNSIGNED_BYTE, 0, true, false, BackBuffer))) It turns out that we can consume the texture in here. Basically what I did here is to create a new SkSurface, give the surface's texture handle to the copyToPlatformTexture() method. My understanding of copyToPlatformTexture() is that it transfers texture to the surface via a mailbox. With this, the layout tests work fine, on both main thread and a worker thread. Using this approach, we wouldn't have to change anything in the places that consume this ImageBitmap, would this be acceptable?
https://codereview.chromium.org/1962413002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:654: if (!(drawingBuffer()->copyToPlatformTexture(gl, textureId, GL_RGBA, GL_UNSIGNED_BYTE, 0, true, false, BackBuffer))) On 2016/05/17 17:02:00, xidachen wrote: > It turns out that we can consume the texture in here. Basically what I did here > is to create a new SkSurface, give the surface's texture handle to the > copyToPlatformTexture() method. My understanding of copyToPlatformTexture() is > that it transfers texture to the surface via a mailbox. With this, the layout > tests work fine, on both main thread and a worker thread. Using this approach, > we wouldn't have to change anything in the places that consume this ImageBitmap, > would this be acceptable? DrawingBuffer::copyToPlatformTexture, as the name implies, does a copy of the DrawingBuffer's contents into the given texture ID that is owned by the given GLES2Interface. This isn't the semantic that is desired. The goal here is to *give* the texture away, not copy it. Copying, even on the GPU, is expensive. The DrawingBuffer should need to allocate a new front buffer after calling this primitive. Further, when the consumer of this ImageBitmap transfers it into the destination canvas (using ImageBitmapRenderingContext), any texture that was previously being displayed there will need to go into some recycling pool so that in the steady state there is no continuous allocation and deletion of textures. From previous conversations with reed@ and bsalomon@ I think the primitive you may want to use is SkImage::MakeFromTexture. This lets you take an externally-owned texture and feed it into Skia. From there you can use Skia's APIs against it. However, I don't know in which OpenGL context you will need to have the texture's ID, for whoever is going to consume the ImageBitmap later. It might be that which is given by Platform::current()->createSharedOffscreenGraphicsContext3DProvider() (see src/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp). Basically, it's probably the shared OpenGL context used by all canvases using ImageBitmapRenderingContext. However, I'm not sure you can reference that context from the worker thread -- which is why you'd need to transfer the texture through a mailbox, and add a new constructor for ImageBitmap, taking a texture mailbox.
https://codereview.chromium.org/1962413002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:654: if (!(drawingBuffer()->copyToPlatformTexture(gl, textureId, GL_RGBA, GL_UNSIGNED_BYTE, 0, true, false, BackBuffer))) On 2016/05/18 01:20:41, Ken Russell wrote: > On 2016/05/17 17:02:00, xidachen wrote: > > It turns out that we can consume the texture in here. Basically what I did > here > > is to create a new SkSurface, give the surface's texture handle to the > > copyToPlatformTexture() method. My understanding of copyToPlatformTexture() is > > that it transfers texture to the surface via a mailbox. With this, the layout > > tests work fine, on both main thread and a worker thread. Using this approach, > > we wouldn't have to change anything in the places that consume this > ImageBitmap, > > would this be acceptable? > > DrawingBuffer::copyToPlatformTexture, as the name implies, does a copy of the > DrawingBuffer's contents into the given texture ID that is owned by the given > GLES2Interface. This isn't the semantic that is desired. The goal here is to > *give* the texture away, not copy it. Copying, even on the GPU, is expensive. > The DrawingBuffer should need to allocate a new front buffer after calling this > primitive. Further, when the consumer of this ImageBitmap transfers it into the > destination canvas (using ImageBitmapRenderingContext), any texture that was > previously being displayed there will need to go into some recycling pool so > that in the steady state there is no continuous allocation and deletion of > textures. > > From previous conversations with reed@ and bsalomon@ I think the primitive you > may want to use is SkImage::MakeFromTexture. This lets you take an > externally-owned texture and feed it into Skia. From there you can use Skia's > APIs against it. However, I don't know in which OpenGL context you will need to > have the texture's ID, for whoever is going to consume the ImageBitmap later. It > might be that which is given by > Platform::current()->createSharedOffscreenGraphicsContext3DProvider() (see > src/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp). Basically, > it's probably the shared OpenGL context used by all canvases using > ImageBitmapRenderingContext. However, I'm not sure you can reference that > context from the worker thread -- which is why you'd need to transfer the > texture through a mailbox, and add a new constructor for ImageBitmap, taking a > texture mailbox. Sorry for the delay. I should have used mailbox from the beginning. Please take a look at the latest patch, which works under both debug and release mode.
kbr@chromium.org changed reviewers: + bsalomon@chromium.org
Excellent. LGTM overall. Couple of comments. https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:644: return imageBitmap; This is a good start. Please add a TODO like: TODO(xidachen): create a small pool of recycled textures from ImageBitmapRenderingContext's transferFromImageBitmap, and try to use them in DrawingBuffer. This will avoid texture re-allocation in the steady state when repeatedly transferring frames from a WebGLRenderingContext to an ImageBitmapRenderingContext. https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:77: OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); Someone who understands this code better needs to confirm that this is the correct context into which to consume the texture mailbox. https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:97: sk_sp<SkImage> skImage = SkImage::MakeFromTexture(grContext, backendTexture); junov@ or bsalomon@ should review this code for correctness.
bsalomon@google.com changed reviewers: + bsalomon@google.com
https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:97: sk_sp<SkImage> skImage = SkImage::MakeFromTexture(grContext, backendTexture); On 2016/05/19 00:15:11, Ken Russell wrote: > junov@ or bsalomon@ should review this code for correctness. It seems OK to me but want to raise two potential issues: 1) Does gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, ...) return a texture ID of target type GL_TEXTURE_2D or of m_mailbox.textureTarget? 2) This wraps a SkImage around the texture ID but doesn't turn responsibility for deleting the texture over to Skia. If that is not the intention here then you use SkImage::MakeFromAdoptedTexture() so that the Skia will free the texture when it is done with it.
https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:644: return imageBitmap; On 2016/05/19 00:15:11, Ken Russell wrote: > This is a good start. Please add a TODO like: > > TODO(xidachen): create a small pool of recycled textures from > ImageBitmapRenderingContext's transferFromImageBitmap, and try to use them in > DrawingBuffer. This will avoid texture re-allocation in the steady state when > repeatedly transferring frames from a WebGLRenderingContext to an > ImageBitmapRenderingContext. Done. https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:77: OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); On 2016/05/19 00:15:12, Ken Russell wrote: > Someone who understands this code better needs to confirm that this is the > correct context into which to consume the texture mailbox. I believe this can be verified. Please take a look at the updated layout test case in worker. Basically I created 2 webgl contexts in worker, one clear the canvas with red and the other one with green. Then I call transferToImageBitmap on both contexts. If the code here doesn't get the correct context, the ImageBitmap that we got back will have the wrong color. Locally I run the test under debug mode with repeat=20, and it always passes. https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:97: sk_sp<SkImage> skImage = SkImage::MakeFromTexture(grContext, backendTexture); On 2016/05/19 12:57:20, bsalomon wrote: > On 2016/05/19 00:15:11, Ken Russell wrote: > > junov@ or bsalomon@ should review this code for correctness. > > It seems OK to me but want to raise two potential issues: > > 1) Does gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, ...) return a texture > ID of target type GL_TEXTURE_2D or of m_mailbox.textureTarget? > > 2) This wraps a SkImage around the texture ID but doesn't turn responsibility > for deleting the texture over to Skia. If that is not the intention here then > you use SkImage::MakeFromAdoptedTexture() so that the Skia will free the texture > when it is done with it. > Thanks for the comments. 1). In my opinion, it returns the ID of target type GL_TEXTURE_2D. kbr@: could you verify that? 2). Good point. I believe that now the texture is consumed, SkImage should be responsible for freeing the texture. junov@: WDYT?
https://codereview.chromium.org/1962413002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:77: OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); This is an unsafe assumption. You should receive a GrContext as an argument. What if the consumer of the SkImage is a WebGL context (e.g. texImage2D). A texture from the shared context will not work in that case. You need a test for the texImage case BTW. https://codereview.chromium.org/1962413002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:87: GLuint textureId = gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name); This can break if the imageForCurrentFrame() is called more than once. If the SkImage produce by the first call has gone out of scope, the underlying texture object may no longer exist. GL textureIds work work like a reference count on the shared texture object and CreateAndConsumeTextureCHROMIUM is like an adoptRef. I think the solution is to cache the SkImage, the first time imageForCurrentFrame(). You'd need to track the GrContext of the SkImage and do another mailbox round trip if the image is ever requested on a different context. Also you' have re-mailbox the texture if the ImageBitmap were transfered.
https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:97: sk_sp<SkImage> skImage = SkImage::MakeFromTexture(grContext, backendTexture); On 2016/05/19 13:48:29, xidachen wrote: > On 2016/05/19 12:57:20, bsalomon wrote: > > On 2016/05/19 00:15:11, Ken Russell wrote: > > > junov@ or bsalomon@ should review this code for correctness. > > > > It seems OK to me but want to raise two potential issues: > > > > 1) Does gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, ...) return a > texture > > ID of target type GL_TEXTURE_2D or of m_mailbox.textureTarget? > > > > 2) This wraps a SkImage around the texture ID but doesn't turn responsibility > > for deleting the texture over to Skia. If that is not the intention here then > > you use SkImage::MakeFromAdoptedTexture() so that the Skia will free the > texture > > when it is done with it. > > > > Thanks for the comments. > 1). In my opinion, it returns the ID of target type GL_TEXTURE_2D. kbr@: could > you verify that? It returns a texture of the type passed to CreateAndConsumeTextureCHROMIUM, in this case GL_TEXTURE_2D. The texture mailbox does not have an associated target. See DoCreateAndConsumeTextureCHROMIUM in src/gpu/command_buffer/service/gles2_cmd_decoder.cc.
https://codereview.chromium.org/1962413002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:77: OwnPtr<WebGraphicsContext3DProvider> provider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); On 2016/05/19 15:12:30, Justin Novosad wrote: > This is an unsafe assumption. You should receive a GrContext as an argument. > What if the consumer of the SkImage is a WebGL context (e.g. texImage2D). A > texture from the shared context will not work in that case. You need a test for > the texImage case BTW. Could you take a look at the new layout test that I added? In that test, an ImageBitmap is created by transferToImageBitmap on worker, and that ImageBitmap is consumed by texImage2D on the main thread by another WebGL context. Am I just getting lucky there or is it working as intended?
On 2016/05/19 20:08:48, xidachen wrote: > Could you take a look at the new layout test that I added? In that test, an > ImageBitmap is created by transferToImageBitmap on worker, and that ImageBitmap > is consumed by texImage2D on the main thread by another WebGL context. Am I just > getting lucky there or is it working as intended? Hah that is interesting. It be working because texImage2D cause a readback from the SkImage before re-uploading it to the GL texture. Please confirm whether that is what's happening. If that is the case, then the only problem is that this is going to be very slow. We could solve the performance in a separate CL (create a crbug for it). There is still the problem of what happens when you consume the texture more that once.
On 2016/05/19 20:26:35, Justin Novosad wrote: > On 2016/05/19 20:08:48, xidachen wrote: > > Could you take a look at the new layout test that I added? In that test, an > > ImageBitmap is created by transferToImageBitmap on worker, and that > ImageBitmap > > is consumed by texImage2D on the main thread by another WebGL context. Am I > just > > getting lucky there or is it working as intended? > > > Hah that is interesting. It be working because texImage2D cause a readback from > the SkImage before re-uploading it to the GL texture. Please confirm whether > that is what's happening. If that is the case, then the only problem is that > this is going to be very slow. We could solve the performance in a separate CL > (create a crbug for it). > > There is still the problem of what happens when you consume the texture more > that once. Typos: "It might be working because texImage2D causes (..)
In the implementation of texImage2d(ImageBitmap), we do do a readback of the SkImage and give the pointer to the pixels to the caller of texImage2d. The reason is that gl->TexImage2D() requires a parameter which is a const void* to the pixel. What would be the alternative approach to get the pointer to the pixels without readback? https://codereview.chromium.org/1962413002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:98: m_image = fromSkSp(skImage); Here we cache this SkImage, so the next time when it gets to this function, we have a m_image and we just return that m_image at the very beginning of this function. Wouldn't that prevent the problem when we try to consume the texture multiple times?
https://codereview.chromium.org/1962413002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/1962413002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:98: m_image = fromSkSp(skImage); On 2016/05/19 20:43:51, xidachen wrote: > Here we cache this SkImage, so the next time when it gets to this function, we > have a m_image and we just return that m_image at the very beginning of this > function. Wouldn't that prevent the problem when we try to consume the texture > multiple times? I don't think so. The same ImageBitmap might be consumed by different contexts each time.
On 2016/05/19 20:43:51, xidachen wrote: > In the implementation of texImage2d(ImageBitmap), we do do a readback of the > SkImage and give the pointer to the pixels to the caller of texImage2d. The > reason is that gl->TexImage2D() requires a parameter which is a const void* to > the pixel. What would be the alternative approach to get the pointer to the > pixels without readback? The alternative would to a texture to texture copy (gl->CopyTextureCHROMIUM()) instead of gl->TexImage2D() Would be much faster. > > https://codereview.chromium.org/1962413002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): > > https://codereview.chromium.org/1962413002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:98: m_image = > fromSkSp(skImage); > Here we cache this SkImage, so the next time when it gets to this function, we > have a m_image and we just return that m_image at the very beginning of this > function. Wouldn't that prevent the problem when we try to consume the texture > multiple times? That's a start. Now, what happens if you try to transfer an ImageBitmap that is in this state?
On 2016/05/19 20:43:51, xidachen wrote: > In the implementation of texImage2d(ImageBitmap), we do do a readback of the > SkImage and give the pointer to the pixels to the caller of texImage2d. The > reason is that gl->TexImage2D() requires a parameter which is a const void* to > the pixel. What would be the alternative approach to get the pointer to the > pixels without readback? The basic idea to avoid a CPU-side round-trip would be to fetch the texture object out of the SkImage (possibly through a mailbox) and to do a GPU-to-GPU copy using the CopyTextureCHROMIUM extension. See WebGLRenderingContextBase::texImageCanvasByGPU as an example, though the code for this path would be quite different.
FWIW, we don't have to fix all the problems all at once. To keep this CL manageable, you can create crbugs and reference them in comments in the code. You already have a decent chunk of the problem solved in this CL.
On 2016/05/19 21:57:59, Justin Novosad wrote: > FWIW, we don't have to fix all the problems all at once. To keep this CL > manageable, you can create crbugs and reference them in comments in the code. > You already have a decent chunk of the problem solved in this CL. Thank you both for your comments. I see two problems here: 1. crbug.com/613409: ImageBitmap needs to keep track of the GrContext, which comes from the original drawingBuffer when transferToImageBitmap happens. 2. crbug.com/613411: In texImage2D, we should not read back the pixels of the SkImage. Instead, we should pass a texture mailbox to the call site of the texImage2D, and the call site consumes the texture mailbox.
On 2016/05/20 01:22:20, xidachen wrote: > On 2016/05/19 21:57:59, Justin Novosad wrote: > > FWIW, we don't have to fix all the problems all at once. To keep this CL > > manageable, you can create crbugs and reference them in comments in the code. > > You already have a decent chunk of the problem solved in this CL. > > Thank you both for your comments. I see two problems here: > > 1. crbug.com/613409: ImageBitmap needs to keep track of the GrContext, which > comes from the original drawingBuffer when transferToImageBitmap happens. > > 2. crbug.com/613411: In texImage2D, we should not read back the pixels of the > SkImage. Instead, we should pass a texture mailbox to the call site of the > texImage2D, and the call site consumes the texture mailbox. Thanks for filing these. Sounds good to me to proceed with this CL as is.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1962413002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1962413002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/20 12:25:27, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/1962413002/#ps120001 (title: "add new test case, cache SkIMage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1962413002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1962413002/120001
Message was sent while issue was closed.
Description was changed from ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. These two layout tests will eventually be moved to KhronosGroup as part of its conformance test. However, at this moment, the layout tests serve the purpose of verifying correctness of the code changes. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. These two layout tests will eventually be moved to KhronosGroup as part of its conformance test. However, at this moment, the layout tests serve the purpose of verifying correctness of the code changes. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. These two layout tests will eventually be moved to KhronosGroup as part of its conformance test. However, at this moment, the layout tests serve the purpose of verifying correctness of the code changes. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement transferToImageBitmap() in WebGLRenderingContext To verify that WebGL Rendering works with OffscreenCanvas, we need to implement this function first. This CL also comes with two layout tests does the following things: 1. Rendering to OffscreenCanvas, call transferToImageBitmap, and draw the ImageBitmap to another canvas via transferFromImageBitmap. 2. Make sure the above case works on a worker thread. These two layout tests will eventually be moved to KhronosGroup as part of its conformance test. However, at this moment, the layout tests serve the purpose of verifying correctness of the code changes. BUG=610759 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/d30c45e8878402c605e467ce537897ef38748c98 Cr-Commit-Position: refs/heads/master@{#395065} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d30c45e8878402c605e467ce537897ef38748c98 Cr-Commit-Position: refs/heads/master@{#395065} |
