Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2026803002:
Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) (Closed)
|
Created:
4 years, 6 months ago by xidachen Modified:
4 years, 6 months ago Reviewers:
Ken Russell (switch to Gerrit), bajones, Justin Novosad, Zhenyao Mo, bsalomon, bsalomon_chromium CC:
chromium-reviews, blink-reviews-platform-graphics_chromium.org, dshwang, eae+blinkwatch, fs, dcheng, kouhei+svg_chromium.org, rwlbuis, krit, drott+blinkwatch_chromium.org, szager+layoutwatch_chromium.org, Rik, jchaffraix+rendering, blink-reviews, gyuyoung2, pdr+svgwatchlist_chromium.org, ajuma+watch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, haraken, danakj+watch_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, f(malita), Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid GPU readback in tex(Sub)Image2D(ImageBitmap)
At this moment, if an ImageBitmap is texture backed, texImage2D will do
a GPU read back which is expensive. The solution is to prepare a mailbox
for the ImageBitmap, and use the WebGL context to consume it and then do
a GPU-GPU texture copy.
BUG=613411, 613409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/0bed6ac0177c01b7aa2719ecbb5f80143ccaa4ef
Cr-Commit-Position: refs/heads/master@{#401903}
Patch Set 1 #Patch Set 2 : should work #Patch Set 3 : fix compile error #Patch Set 4 : layout tests and conformances tests pass #Patch Set 5 : should work #
Total comments: 12
Patch Set 6 : address comments #
Total comments: 4
Patch Set 7 : rebase #
Total comments: 23
Patch Set 8 : address comments #Patch Set 9 : staticbitmapimage does not keep provider anymore #
Total comments: 25
Patch Set 10 : address comments #Patch Set 11 : change function name, layout still broken, DONOT COMMIT THIS PS #
Total comments: 3
Patch Set 12 : layout test timeout in a strange way #
Total comments: 2
Patch Set 13 : address fmalita's comments, an extra function for WebGL #Patch Set 14 : rebase #Patch Set 15 : layout tests pass, needs a bit more clean up #Patch Set 16 : almost identical to 15 #Patch Set 17 : takes care of TexSubImage2D(3D) #Patch Set 18 : should work #
Total comments: 1
Patch Set 19 : address junov@'s comments #
Total comments: 10
Patch Set 20 : address kbr@'s comments #Messages
Total messages: 49 (14 generated)
Description was changed from ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 ========== to ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 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, kbr@chromium.org, senorblanco@chromium.org, zmo@chromium.org
PTAL. Most of the changes are in WebGL and StaticBitmapImage class. Changes to the other classes are very small.
The high-level direction is good, but it looks to me like there are some issues. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4170: GLint level, GLint internalformat, GLenum type, GLint xoffset, GLint yoffset, GLint zoffset, HTMLCanvasElement* canvas, ImageBitmap* bitmap) Please add DCHECK(!canvas || !bitmap) and DCHECK(canvas || bitmap). https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4180: possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(target, internalformat, type, level); At some point this function should be expanded to support CopySubTextureCHROMIUM, which is relatively new, and which should allow a direct copy to be made in more cases for TexSubImage calls. As long as you're touching this code would you mind adding a TODO about this? https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4337: // In the case where we cannot use texImageByGPU, peekPixels() fails. This comment seems to indicate that peekPixels() will always fail here. Is that true? If so, why does the code covered by "peekSucceed" still exist? If not, the comment should be adjusted. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4617: || extensionEnabled(EXTsRGBName); This looks wrong. Whether the readback path is used should depend on the format of the texture, not on whether these extensions are enabled. If these extensions are enabled at all then this will destroy performance for texSubImage2D calls taking ImageBitmaps. Why is this not using canUseTexImageByGPU like the texImage2D case? Will generalizing copyTextureChromium (http://crbug.com/612542) avoid the need for these special cases? https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4626: // In the case where we cannot use texImageByGPU, peekPixels() fails. Same comment as above here. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:83: m_textureId = gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name); Is it legal to cache this value in StaticBitmapImage? What happens if the same ImageBitmap is uploaded to two different WebGL contexts as textures? The m_textureId will be unknown in the second context. If there is a bug here, could you please add a WebGL conformance test for it?
https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4170: GLint level, GLint internalformat, GLenum type, GLint xoffset, GLint yoffset, GLint zoffset, HTMLCanvasElement* canvas, ImageBitmap* bitmap) On 2016/06/01 22:55:09, Ken Russell wrote: > Please add DCHECK(!canvas || !bitmap) and DCHECK(canvas || bitmap). Done. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4180: possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(target, internalformat, type, level); On 2016/06/01 22:55:09, Ken Russell wrote: > At some point this function should be expanded to support > CopySubTextureCHROMIUM, which is relatively new, and which should allow a direct > copy to be made in more cases for TexSubImage calls. As long as you're touching > this code would you mind adding a TODO about this? Done. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4337: // In the case where we cannot use texImageByGPU, peekPixels() fails. On 2016/06/01 22:55:09, Ken Russell wrote: > This comment seems to indicate that peekPixels() will always fail here. Is that > true? If so, why does the code covered by "peekSucceed" still exist? If not, the > comment should be adjusted. The comment is bad, it doesn't convey what I want it to say. I have changed it. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4617: || extensionEnabled(EXTsRGBName); On 2016/06/01 22:55:09, Ken Russell wrote: > This looks wrong. Whether the readback path is used should depend on the format > of the texture, not on whether these extensions are enabled. If these extensions > are enabled at all then this will destroy performance for texSubImage2D calls > taking ImageBitmaps. Why is this not using canUseTexImageByGPU like the > texImage2D case? Will generalizing copyTextureChromium (http://crbug.com/612542) > avoid the need for these special cases? This part of code is actually copied from line 4549--4553, which is from texSubImage2D(canvas). My understanding of that code is that when a canvas is texture backed, useReadBackPath indicates that it cannot do a GPU-GPU texture copy? Is that correct? https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4626: // In the case where we cannot use texImageByGPU, peekPixels() fails. On 2016/06/01 22:55:09, Ken Russell wrote: > Same comment as above here. Done. https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:83: m_textureId = gl->CreateAndConsumeTextureCHROMIUM(GL_TEXTURE_2D, m_mailbox.name); On 2016/06/01 22:55:09, Ken Russell wrote: > Is it legal to cache this value in StaticBitmapImage? What happens if the same > ImageBitmap is uploaded to two different WebGL contexts as textures? The > m_textureId will be unknown in the second context. > > If there is a bug here, could you please add a WebGL conformance test for it? I re-organized the structure of the imageForCurrentFrame() in this class in the new patch. In the case when an ImageBitmap is created by WebGL's transferToImageBitmap, it has a mailbox, and no cached SkImage. Now when this ImageBitmap is consumed by texImage, it will call this consumeTextureMailbox() function which cache this textureId, this textureId is used in copyTexture() only. When this ImageBitmap is consumed by a different WebGL context, it will again go through the code path of consumeTextureMailbox()-->copyTexture(). Notice that the consumeTextureMailbox() will change the cached textureId. Also, here is all the cases in the imageForCurrentFrame() function. Please let me know if I misunderstand anything. 1. If there is no cached SkImage, which means just a mailbox, and the consumer of this ImageBitmap is null (2D and bitmaprenderer), use the shared context3D provider to consume the mailbox which will generate a SkImage, cache that shared context3D provider and cache the generated SkImage. 2. If no cached SkImage (just a mailbox), and the consumer is a WebGL context, use the WebGL's context provider to consume the mailbox which generates a SkImage, cache the SkImage but don't cache the context provider. 3. If there is a cached SkImage and it is not texture backed, return that SkImage right away. 4. If there is a cached SkImage and the consumer is null (2D or bitmaprenderer), return that SkImage right away. 5. If there is a cached SkImage and the consumer is a WebGL context, it has 2 sub-cases. If there is no cached context3D provider, then consume the mailbox which generates a new SkImage, cache the SkImage and don't cache the provider. The second case is that there is a cached context3D provider, then prepare a new mailbox, and consume the new mailbox. Does the above categorization make sense?
Justin should definitely take a look at this when he gets back. In general, I'm not fond of the dependencies on WebGraphicsContext3DProvider all over the Image hierarchy (even if on StaticBitmapImage uses it). I wonder if there's a cleaner way to do this. I also share Ken's concern about persisting texture IDs inside StaticBitmapImage. For one thing, do we have to worry about the lost context case here? I think it also needs more tests (if these cases aren't covered already). https://codereview.chromium.org/2026803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (left): https://codereview.chromium.org/2026803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4317: // TODO(crbug.com/613411): peekPixels fails if the SkImage is texture-backed Is the mailbox case now handled by the copyTexture() case above? I.e., is this bug fixed? https://codereview.chromium.org/2026803002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4617: bool useReadBackPath = isWebGL2OrHigher() Seems to be copied from line 4570. Could you refactor this into a helper function, and include the FIXME above? I was a little confused as to where these restrictions came from. https://codereview.chromium.org/2026803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4621: if (bitmap->isTextureBacked() && !useReadBackPath) { Are there tests which cover all these possible conditions? https://codereview.chromium.org/2026803002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4645: if (type == GL_UNSIGNED_INT_10F_11F_11F_REV) { Is there a 10_11_11 test somewhere?
xidachen@chromium.org changed reviewers: + junov@chromium.org - senorblanco@chromium.org
Please ping when this is ready for re-review. I'm guessing it's waiting for the refactoring in https://codereview.chromium.org/2025703002/ .
junov@: PTAL
https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:471: return (!m_image->hasSkImage() || m_image->hasContext3DProvider()); This is hard to read. The "!m_image->hasSkImage()" condition implies that m_image has a mailbox, which is what we really want to know here. You should put an isTextureBacked() method directly on StaticBitmapImage and call that here. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:347: return frameAtIndex(currentFrame()); If this method is not going to look at contextProvider, it should at least check that the SkImage is never texture backed. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:35: ASSERT(m_image); Should have: DCHECK(!m_image->isTextureBacked()); because having a texture backed SkImage without having a context provider would break stuff. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:97: void StaticBitmapImage::copyTexture(WebGraphicsContext3DProvider* provider, GLuint texture, GLenum internalFormat, GLenum destType) Readability nit: From the names of the function and args, it is not clear whether we this function is meant to copy to or from 'texture'. copyTexture -> copyToTexture texture -> destinationTexture https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:104: gl->Flush(); Why is this flush needed? the fence should be enough. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:109: bool StaticBitmapImage::prepareMailboxForSkImage(WebGraphicsContext3DProvider* provider) Remove the unused 'provider' argument https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:125: if (m_image->getTexture()->getCustomData()) { I believe skia team wants to deprecate custom data. Did you check with bsalomon before using this? Also packaging the mailbox into skia seems unnecessary. Why not just use m_mailbox? Having the texture referenced by both an SkImage_Gpu and a mailbox was a complexity I was hoping to avoid, to make resource management easier to reason about. With the current design there are several risks of introducing resource leaks in the future, though I don't think there are leaks right now. For example, if StaticBitmapImage is destroyed with an SkImage that has a mailbox stored in m_image's custom data, the texture is leaked because we'll be discarding the mailbox name without having consumed it. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:135: gl->Flush(); Why is this flush necessary? fence should be enough https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:146: if (!m_image->isTextureBacked() || !contextProvider) What is expected to happen if contextProvider is null when m_image is texture backed? Seems like we should have DCHECK(!m_image->isTextureBacked() || contextProvider)? https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:149: if (!prepareMailboxForSkImage(contextProvider)) This should only be necessary if m_provider != contextPRovider https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:34: bool hasContext3DProvider() const { return !!m_provider; } !! is not necessary You should only use that when coercion to bool is necessary. Pointers convert implicitly to bool just fine. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:35: bool hasSkImage() const { return !!m_image; } same here
https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:471: return (!m_image->hasSkImage() || m_image->hasContext3DProvider()); On 2016/06/07 15:11:07, Justin Novosad wrote: > This is hard to read. The "!m_image->hasSkImage()" condition implies that > m_image has a mailbox, which is what we really want to know here. You should > put an isTextureBacked() method directly on StaticBitmapImage and call that > here. I actually tried that before and it is a bit complicated... When texImage2D consumes the ImageBitmap, this method tells texImage2D that the ImageBitmap is texture-backed either: 1. it doesn't have a SkImage, which means it has a mailbox 2. or it has a contextProvider. But if I put this method in StaticBitmapImage, it should look like this, this StaticBitmapImage is texture-backed if either: 1. it doesn't have a SkImage, which means it has a mailbox 2. or it has a contextProvider 3. or the SkImage is texture-backed. Now if put this method in StaticBitmapImage, and texImage2D calls it, all those texImage-ImageBitmap layout tests will fail under virtual/gpu/. In those layout tests, ImageBitmap is created from different sources, so it has an SkImage and that SkImage is texture-backed. But it doesn't have a mailbox, nor does it have a context provider. In this case, isTextureBacked() should tell texImage2D that this ImageBitmap is not texture-backed. I understand that this will trigger a read-back, how should I solve this case? https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:347: return frameAtIndex(currentFrame()); On 2016/06/07 15:11:07, Justin Novosad wrote: > If this method is not going to look at contextProvider, it should at least check > that the SkImage is never texture backed. In my opinion, if this method is called in a test that is running under virtual/gpu, then the SkImage can be texture-backed. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:35: ASSERT(m_image); On 2016/06/07 15:11:07, Justin Novosad wrote: > Should have: DCHECK(!m_image->isTextureBacked()); because having a texture > backed SkImage without having a context provider would break stuff. Not necessary. When I run the layout tests under virtual/gpu/, in those texImage2D tests for ImageBitmap, ImageBitmaps are texture backed but no contextProvider. Also, I think in our previous discussion, we mentioned that when a context provider consumes the mailbox inside the ImageBitmap, we should cache that context provider in the ImageBitmap. I found that this won't always work. For example, when it is a WebGL context that is consuming the ImageBitmap, the context provider is owned by the drawing buffer, so ImageBitmap cannot just take the ownership of that context provider. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:97: void StaticBitmapImage::copyTexture(WebGraphicsContext3DProvider* provider, GLuint texture, GLenum internalFormat, GLenum destType) On 2016/06/07 15:11:07, Justin Novosad wrote: > Readability nit: From the names of the function and args, it is not clear > whether we this function is meant to copy to or from 'texture'. > copyTexture -> copyToTexture > texture -> destinationTexture Done. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:104: gl->Flush(); On 2016/06/07 15:11:07, Justin Novosad wrote: > Why is this flush needed? the fence should be enough. Done. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:109: bool StaticBitmapImage::prepareMailboxForSkImage(WebGraphicsContext3DProvider* provider) On 2016/06/07 15:11:07, Justin Novosad wrote: > Remove the unused 'provider' argument Done. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:125: if (m_image->getTexture()->getCustomData()) { On 2016/06/07 15:11:07, Justin Novosad wrote: > I believe skia team wants to deprecate custom data. Did you check with bsalomon > before using this? Also packaging the mailbox into skia seems unnecessary. Why > not just use m_mailbox? Having the texture referenced by both an SkImage_Gpu and > a mailbox was a complexity I was hoping to avoid, to make resource management > easier to reason about. With the current design there are several risks of > introducing resource leaks in the future, though I don't think there are leaks > right now. For example, if StaticBitmapImage is destroyed with an SkImage that > has a mailbox stored in m_image's custom data, the texture is leaked because > we'll be discarding the mailbox name without having consumed it. change it to use m_mailbox directly. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:135: gl->Flush(); On 2016/06/07 15:11:07, Justin Novosad wrote: > Why is this flush necessary? fence should be enough Layout test fails when I comment out this line. Also, the DrawingBuffer::prepareMailbox has a flush. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:146: if (!m_image->isTextureBacked() || !contextProvider) On 2016/06/07 15:11:07, Justin Novosad wrote: > What is expected to happen if contextProvider is null when m_image is texture > backed? > Seems like we should have DCHECK(!m_image->isTextureBacked() || > contextProvider)? In the case when m_image is not null and is texture backed, then there is always a m_provider, so it goes through the code path of prepareMailboxForSkImage-->consumeTextureMailbox. This DCHECK could cause problem in the above case, isn't it? https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:149: if (!prepareMailboxForSkImage(contextProvider)) On 2016/06/07 15:11:07, Justin Novosad wrote: > This should only be necessary if m_provider != contextPRovider Done. https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:34: bool hasContext3DProvider() const { return !!m_provider; } On 2016/06/07 15:11:07, Justin Novosad wrote: > !! is not necessary You should only use that when coercion to bool is necessary. > Pointers convert implicitly to bool just fine. I am not sure how windows bots will react to this, so removing the !! and will have win bots to try.
On 2016/06/07 17:49:54, xidachen wrote: > https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): > > https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/ImageBitmap.cpp:471: return > (!m_image->hasSkImage() || m_image->hasContext3DProvider()); > On 2016/06/07 15:11:07, Justin Novosad wrote: > > This is hard to read. The "!m_image->hasSkImage()" condition implies that > > m_image has a mailbox, which is what we really want to know here. You should > > put an isTextureBacked() method directly on StaticBitmapImage and call that > > here. > > I actually tried that before and it is a bit complicated... > > When texImage2D consumes the ImageBitmap, this method tells texImage2D that the > ImageBitmap is texture-backed either: > 1. it doesn't have a SkImage, which means it has a mailbox > 2. or it has a contextProvider. > > But if I put this method in StaticBitmapImage, it should look like this, this > StaticBitmapImage is texture-backed if either: > 1. it doesn't have a SkImage, which means it has a mailbox > 2. or it has a contextProvider > 3. or the SkImage is texture-backed. I think checking condition 2 is unnecessary since 2 implies 3. You should just check conditions 1 and 3. > > https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): > > https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:347: return > frameAtIndex(currentFrame()); > On 2016/06/07 15:11:07, Justin Novosad wrote: > > If this method is not going to look at contextProvider, it should at least > check > > that the SkImage is never texture backed. > > In my opinion, if this method is called in a test that is running under > virtual/gpu, then the SkImage can be texture-backed. Right. Okay. > > https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): > > https://codereview.chromium.org/2026803002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:35: > ASSERT(m_image); > On 2016/06/07 15:11:07, Justin Novosad wrote: > > Should have: DCHECK(!m_image->isTextureBacked()); because having a texture > > backed SkImage without having a context provider would break stuff. > > Not necessary. When I run the layout tests under virtual/gpu/, in those > texImage2D tests for ImageBitmap, ImageBitmaps are texture backed but no > contextProvider. Understood. > > Also, I think in our previous discussion, we mentioned that when a context > provider consumes the mailbox inside the ImageBitmap, we should cache that > context provider in the ImageBitmap. I found that this won't always work. For > example, when it is a WebGL context that is consuming the ImageBitmap, the > context provider is owned by the drawing buffer, so ImageBitmap cannot just take > the ownership of that context provider. Hmmm... Perhaps we could create a new context provider object that references the same underlying GLES interface object as the WebGLRenderingContext's context provider?
> > > > Also, I think in our previous discussion, we mentioned that when a context > > provider consumes the mailbox inside the ImageBitmap, we should cache that > > context provider in the ImageBitmap. I found that this won't always work. For > > example, when it is a WebGL context that is consuming the ImageBitmap, the > > context provider is owned by the drawing buffer, so ImageBitmap cannot just > take > > the ownership of that context provider. > > Hmmm... Perhaps we could create a new context provider object that references > the same underlying GLES interface object as the WebGLRenderingContext's context > provider? I am not sure there is a way to do this cleanly. Maybe for now, the solution is to put the texture back into a new mailbox.
> > But if I put this method in StaticBitmapImage, it should look like this, this > > StaticBitmapImage is texture-backed if either: > > 1. it doesn't have a SkImage, which means it has a mailbox > > 2. or it has a contextProvider > > 3. or the SkImage is texture-backed. > > I think checking condition 2 is unnecessary since 2 implies 3. You should just > check conditions 1 and 3. > There will still be problems. If we change that to check either: 1. it doesn't have a SkImage (just a mailbox) 2. or the SkImage is texture-backed. The problem comes in those texImage-ImageBitmap* layout tests. When these layout tests are run under virtual/gpu/, the SkImage is texture-backed, and it doesn't have a mailbox nor does it have a context provider. In this case, texImage2D will have to do a GPU readback, because it cannot consume any mailbox. And we cannot not generate a mailbox for a SkImage in that state because it doesn't have a context provider.
On 2016/06/07 19:04:34, Justin Novosad wrote: > > > > > > Also, I think in our previous discussion, we mentioned that when a context > > > provider consumes the mailbox inside the ImageBitmap, we should cache that > > > context provider in the ImageBitmap. I found that this won't always work. > For > > > example, when it is a WebGL context that is consuming the ImageBitmap, the > > > context provider is owned by the drawing buffer, so ImageBitmap cannot just > > take > > > the ownership of that context provider. > > > > Hmmm... Perhaps we could create a new context provider object that references > > the same underlying GLES interface object as the WebGLRenderingContext's > context > > provider? > > I am not sure there is a way to do this cleanly. Maybe for now, the solution is > to put the texture back into a new mailbox. So this is how it works at this moment: Say that we have an ImageBitmap that is generated from transferToImageBitmap. That ImageBitmap doesn't have a SkImage or a contextProvider, it has just a mailbox. If it is consumed by a 2D context, we cache the shared offscreen contextProvider. After that, if it is consumed by texImage2D, we then generate a new mailbox using the cached contextProvider, and this new texture mailbox is consumed by the WebGL context, but we cannot cache the WebGL's contextProvider. If a different WebGL context is trying to consume this ImageBitmap, we still need to ask the cached contextProvider to generate a new mailbox, and then this new mailbox is consumed by the WebGL context. Does this workflow make sense?
On 2016/06/07 19:16:13, xidachen wrote: > On 2016/06/07 19:04:34, Justin Novosad wrote: > > > > > > > > Also, I think in our previous discussion, we mentioned that when a context > > > > provider consumes the mailbox inside the ImageBitmap, we should cache that > > > > context provider in the ImageBitmap. I found that this won't always work. > > For > > > > example, when it is a WebGL context that is consuming the ImageBitmap, the > > > > context provider is owned by the drawing buffer, so ImageBitmap cannot > just > > > take > > > > the ownership of that context provider. > > > > > > Hmmm... Perhaps we could create a new context provider object that > references > > > the same underlying GLES interface object as the WebGLRenderingContext's > > context > > > provider? > > > > I am not sure there is a way to do this cleanly. Maybe for now, the solution > is > > to put the texture back into a new mailbox. > > So this is how it works at this moment: > Say that we have an ImageBitmap that is generated from transferToImageBitmap. > That ImageBitmap doesn't have a SkImage or a contextProvider, it has just a > mailbox. If it is consumed by a 2D context, we cache the shared offscreen > contextProvider. After that, if it is consumed by texImage2D, we then generate a > new mailbox using the cached contextProvider, and this new texture mailbox is > consumed by the WebGL context, but we cannot cache the WebGL's contextProvider. > If a different WebGL context is trying to consume this ImageBitmap, we still > need to ask the cached contextProvider to generate a new mailbox, and then this > new mailbox is consumed by the WebGL context. > > Does this workflow make sense? That does not quite work. After the original mailbox has been consumed, you have to re-create the new mailbox using the context where original mailbox was consumed. That's why I was saying you should texImage* should put the texture back into a mailbox immediately when it is done. You know what? We should make this simpler. since we cannot reference WebGL's contextProvider, let's get rid of StaticBitmapImage::m_provider, and just assume that if the SkImage is accelerated, it is in the shared context. Any accelerated SkImage in any other context is just going to be transient anyways.
On 2016/06/07 19:34:41, Justin Novosad wrote: > On 2016/06/07 19:16:13, xidachen wrote: > > On 2016/06/07 19:04:34, Justin Novosad wrote: > > > > > > > > > > Also, I think in our previous discussion, we mentioned that when a > context > > > > > provider consumes the mailbox inside the ImageBitmap, we should cache > that > > > > > context provider in the ImageBitmap. I found that this won't always > work. > > > For > > > > > example, when it is a WebGL context that is consuming the ImageBitmap, > the > > > > > context provider is owned by the drawing buffer, so ImageBitmap cannot > > just > > > > take > > > > > the ownership of that context provider. > > > > > > > > Hmmm... Perhaps we could create a new context provider object that > > references > > > > the same underlying GLES interface object as the WebGLRenderingContext's > > > context > > > > provider? > > > > > > I am not sure there is a way to do this cleanly. Maybe for now, the > solution > > is > > > to put the texture back into a new mailbox. > > > > So this is how it works at this moment: > > Say that we have an ImageBitmap that is generated from transferToImageBitmap. > > That ImageBitmap doesn't have a SkImage or a contextProvider, it has just a > > mailbox. If it is consumed by a 2D context, we cache the shared offscreen > > contextProvider. After that, if it is consumed by texImage2D, we then generate > a > > new mailbox using the cached contextProvider, and this new texture mailbox is > > consumed by the WebGL context, but we cannot cache the WebGL's > contextProvider. > > If a different WebGL context is trying to consume this ImageBitmap, we still > > need to ask the cached contextProvider to generate a new mailbox, and then > this > > new mailbox is consumed by the WebGL context. > > > > Does this workflow make sense? > > That does not quite work. After the original mailbox has been consumed, you have > to re-create the new mailbox using the context where original mailbox was > consumed. That's why I was saying you should texImage* should put the texture > back into a mailbox immediately when it is done. > > You know what? We should make this simpler. since we cannot reference WebGL's > contextProvider, let's get rid of StaticBitmapImage::m_provider, and just assume > that if the SkImage is accelerated, it is in the shared context. Any accelerated > SkImage in any other context is just going to be transient anyways. Yes, I think that would work. Just to make sure I understand this correctly: A StaticBitmapImage should just keep a mailbox and a SkImage. If any context (2d or 3d) consumes this StaticBitmapImage, we update the mailbox right after that such that next if there is a different context consuming it, the mailbox can be used right away. Is that correct?
On 2016/06/07 19:41:19, xidachen wrote: > On 2016/06/07 19:34:41, Justin Novosad wrote: > > On 2016/06/07 19:16:13, xidachen wrote: > > > On 2016/06/07 19:04:34, Justin Novosad wrote: > > > > > > > > > > > > Also, I think in our previous discussion, we mentioned that when a > > context > > > > > > provider consumes the mailbox inside the ImageBitmap, we should cache > > that > > > > > > context provider in the ImageBitmap. I found that this won't always > > work. > > > > For > > > > > > example, when it is a WebGL context that is consuming the ImageBitmap, > > the > > > > > > context provider is owned by the drawing buffer, so ImageBitmap cannot > > > just > > > > > take > > > > > > the ownership of that context provider. > > > > > > > > > > Hmmm... Perhaps we could create a new context provider object that > > > references > > > > > the same underlying GLES interface object as the WebGLRenderingContext's > > > > context > > > > > provider? > > > > > > > > I am not sure there is a way to do this cleanly. Maybe for now, the > > solution > > > is > > > > to put the texture back into a new mailbox. > > > > > > So this is how it works at this moment: > > > Say that we have an ImageBitmap that is generated from > transferToImageBitmap. > > > That ImageBitmap doesn't have a SkImage or a contextProvider, it has just a > > > mailbox. If it is consumed by a 2D context, we cache the shared offscreen > > > contextProvider. After that, if it is consumed by texImage2D, we then > generate > > a > > > new mailbox using the cached contextProvider, and this new texture mailbox > is > > > consumed by the WebGL context, but we cannot cache the WebGL's > > contextProvider. > > > If a different WebGL context is trying to consume this ImageBitmap, we still > > > need to ask the cached contextProvider to generate a new mailbox, and then > > this > > > new mailbox is consumed by the WebGL context. > > > > > > Does this workflow make sense? > > > > That does not quite work. After the original mailbox has been consumed, you > have > > to re-create the new mailbox using the context where original mailbox was > > consumed. That's why I was saying you should texImage* should put the texture > > back into a mailbox immediately when it is done. > > > > You know what? We should make this simpler. since we cannot reference WebGL's > > contextProvider, let's get rid of StaticBitmapImage::m_provider, and just > assume > > that if the SkImage is accelerated, it is in the shared context. Any > accelerated > > SkImage in any other context is just going to be transient anyways. > > Yes, I think that would work. Just to make sure I understand this correctly: > A StaticBitmapImage should just keep a mailbox and a SkImage. If any context (2d > or 3d) consumes this StaticBitmapImage, we update the mailbox right after that > such that next if there is a different context consuming it, the mailbox can be > used right away. > Is that correct? Actually, creating a new mailbox is not necessary when a 2d context uses the image bitmap. In that case, we can just keep a texture backed SkImage (since it belongs to the shared context). This will avoid repetitive mailbox overhead when drawing to a 2D canvas.
PTAL https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:142: { The reason I put a lot of if statement here is that each if stands for one case. I can certainly merge some of them to be one if statement if that is preferable.
https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:469: // This function is used in WebGL's texImage, if it returns true, WebGL will a "will do a" https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:473: return m_image && m_image->hasMailbox(); What about the case where we have no mailbox, but the image is from an accelerated 2D canvas (and thus has a texture backed SkImage)? If it is your intent to ignore that case, then the method should be called something else (hasMailbox?) to prevent it from being misused or misinterpreted by others. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:141: virtual PassRefPtr<SkImage> imageForCurrentFrame(WebGraphicsContext3DProvider* = nullptr) = 0; The usage of contextProvider argument is non-trivial. You should add comments to explain it. Something like: The returned image may or may not be texture backed, if the caller needs texture backed images to be in a context other that the shared 3D context (e.g. a WebGL context), the it must specify the desired context. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:120: m_image->getTexture()->textureParamsModified(); I am pretty sure this call is unnecessary https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:129: RefPtr<SkData> mailboxNameData = adoptRef(SkData::NewWithCopy(&m_mailbox.name[0], sizeof(m_mailbox.name))); No need for this. consumeTextureMailbox gets all it needs from m_mailbox. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:137: grContext->resetContext(kTextureBinding_GrGLBackendState); You should discard m_image here IMHO, to make sure it does not get confused for a texture-backed image that is in the shared context. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:142: { On 2016/06/09 15:09:53, xidachen wrote: > The reason I put a lot of if statement here is that each if stands for one case. > I can certainly merge some of them to be one if statement if that is preferable. +1 this it is very readable. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:148: if (!hasMailbox()) This does not seem correct to me. If you have an image bitmap that was from an accelerated 2D canvas, and pass it to texSubImage, you need to go through a mailbox to bring it into the right context. I think you need something like this: if (!hasMailbox()) { // SkImage is texture backed on the shared context if (contextProvider) { OwnPtr<WebGraphicsContext3DProvider> sharedProvider = adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); prepareMailboxForSkImage(sharedProvider); consumeTextureMailbox(contextProvider); prepareMailboxForSkImage(contextProvider); // Because we can't retain a texture in the WebGL context. } return m_image; } To test this, make sure you have a layout test that covers the following case: Draw to a 2D canvas, createImageBitmap from the 2d canvas, upload the ImageBitmap to a webgl texture. under the virtual/gpu suite, that test should exercise the case I am concerned about here. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:161: return m_image; might as well make this nullptr
https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:469: // This function is used in WebGL's texImage, if it returns true, WebGL will a On 2016/06/09 20:21:29, Justin Novosad wrote: > "will do a" Done. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:473: return m_image && m_image->hasMailbox(); On 2016/06/09 20:21:29, Justin Novosad wrote: > What about the case where we have no mailbox, but the image is from an > accelerated 2D canvas (and thus has a texture backed SkImage)? If it is your > intent to ignore that case, then the method should be called something else > (hasMailbox?) to prevent it from being misused or misinterpreted by others. Changed the implementation to indicate whether bitmap is actually texture backed or not. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:141: virtual PassRefPtr<SkImage> imageForCurrentFrame(WebGraphicsContext3DProvider* = nullptr) = 0; On 2016/06/09 20:21:29, Justin Novosad wrote: > The usage of contextProvider argument is non-trivial. You should add comments to > explain it. Something like: The returned image may or may not be texture > backed, if the caller needs texture backed images to be in a context other that > the shared 3D context (e.g. a WebGL context), the it must specify the desired > context. Done. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:120: m_image->getTexture()->textureParamsModified(); On 2016/06/09 20:21:29, Justin Novosad wrote: > I am pretty sure this call is unnecessary Done. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:129: RefPtr<SkData> mailboxNameData = adoptRef(SkData::NewWithCopy(&m_mailbox.name[0], sizeof(m_mailbox.name))); On 2016/06/09 20:21:29, Justin Novosad wrote: > No need for this. consumeTextureMailbox gets all it needs from m_mailbox. Done. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:137: grContext->resetContext(kTextureBinding_GrGLBackendState); On 2016/06/09 20:21:29, Justin Novosad wrote: > You should discard m_image here IMHO, to make sure it does not get confused for > a texture-backed image that is in the shared context. I am a little confused. In the code path of imageForCurrentFrame(), we return m_image after calling this function in some cases. If we discard m_image, then we are returning nullptr? https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:148: if (!hasMailbox()) On 2016/06/09 20:21:29, Justin Novosad wrote: > This does not seem correct to me. If you have an image bitmap that was from an > accelerated 2D canvas, and pass it to texSubImage, you need to go through a > mailbox to bring it into the right context. I think you need something like > this: > if (!hasMailbox()) { > // SkImage is texture backed on the shared context > if (contextProvider) { > OwnPtr<WebGraphicsContext3DProvider> sharedProvider = > adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); > prepareMailboxForSkImage(sharedProvider); > consumeTextureMailbox(contextProvider); > prepareMailboxForSkImage(contextProvider); // Because we can't retain a > texture in the WebGL context. > } > return m_image; > } > > To test this, make sure you have a layout test that covers the following case: > Draw to a 2D canvas, createImageBitmap from the 2d canvas, upload the > ImageBitmap to a webgl texture. under the virtual/gpu suite, that test should > exercise the case I am concerned about here. We actually have a layout test for that: fast/canvas/webgl/texImage-imageBitmap-from-canvas.html Before your suggested change, the way it works is to do a GPU-CPU read back and then upload the pixelData as texture. Now after your suggested change, I use fprintf and I can see that it goes through the code path of GPU-GPU texture copy. Unfortunately that test is broken. I tried to debug and what I see is this: When the expected color is [255, 0, 0], the actual color that got back is [0, 255, 0]. When the expected color is [26, 0, 0], the actual color is [0, 26, 0]. Any idea why red & green channel gets shifted? https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:161: return m_image; On 2016/06/09 20:21:29, Justin Novosad wrote: > might as well make this nullptr Done.
https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:113: bool StaticBitmapImage::prepareMailboxForSkImage(WebGraphicsContext3DProvider* provider) Could we call this switchStorageToMailbox, and the "consume" method could be switchStorageToSkImage. This naming would make it clear that only one form of storage is current at a given time and that the code is meant to toggle between the two. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:137: grContext->resetContext(kTextureBinding_GrGLBackendState); On 2016/06/10 15:36:08, xidachen wrote: > On 2016/06/09 20:21:29, Justin Novosad wrote: > > You should discard m_image here IMHO, to make sure it does not get confused > for > > a texture-backed image that is in the shared context. > > I am a little confused. In the code path of imageForCurrentFrame(), we return > m_image after calling this function in some cases. If we discard m_image, then > we are returning nullptr? Right, in that case you should capture a local ref before calling this method. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:148: if (!hasMailbox()) On 2016/06/10 15:36:08, xidachen wrote: > On 2016/06/09 20:21:29, Justin Novosad wrote: > > This does not seem correct to me. If you have an image bitmap that was from an > > accelerated 2D canvas, and pass it to texSubImage, you need to go through a > > mailbox to bring it into the right context. I think you need something like > > this: > > if (!hasMailbox()) { > > // SkImage is texture backed on the shared context > > if (contextProvider) { > > OwnPtr<WebGraphicsContext3DProvider> sharedProvider = > > > adoptPtr(Platform::current()->createSharedOffscreenGraphicsContext3DProvider()); > > prepareMailboxForSkImage(sharedProvider); > > consumeTextureMailbox(contextProvider); > > prepareMailboxForSkImage(contextProvider); // Because we can't retain a > > texture in the WebGL context. > > } > > return m_image; > > } > > > > To test this, make sure you have a layout test that covers the following case: > > Draw to a 2D canvas, createImageBitmap from the 2d canvas, upload the > > ImageBitmap to a webgl texture. under the virtual/gpu suite, that test should > > exercise the case I am concerned about here. > > We actually have a layout test for that: > fast/canvas/webgl/texImage-imageBitmap-from-canvas.html > > Before your suggested change, the way it works is to do a GPU-CPU read back and > then upload the pixelData as texture. > > Now after your suggested change, I use fprintf and I can see that it goes > through the code path of GPU-GPU texture copy. Unfortunately that test is > broken. I tried to debug and what I see is this: > When the expected color is [255, 0, 0], the actual color that got back is [0, > 255, 0]. When the expected color is [26, 0, 0], the actual color is [0, 26, 0]. > Any idea why red & green channel gets shifted? Would be worth investigating what happens to the other component to determine if we are dealing with a format or endian-ness issue or a buffer offset issue.
xidachen@chromium.org changed reviewers: + bsalomon@chromium.org
https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:113: bool StaticBitmapImage::prepareMailboxForSkImage(WebGraphicsContext3DProvider* provider) On 2016/06/10 17:50:40, Justin Novosad wrote: > Could we call this switchStorageToMailbox, and the "consume" method could be > switchStorageToSkImage. This naming would make it clear that only one form of > storage is current at a given time and that the code is meant to toggle between > the two. Done. https://codereview.chromium.org/2026803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:150: if (contextProvider) { bsalomon@: in a layout test that runs on GPU, it creates a SkImage by calling Blink::Image::imageForCurrentFrame(), and that SkImage is texture-backed. When WebGL's texImage2D tries to consume this SkImage, it goes through this code path. What I have found is that if I change: backendTexture.fOrigin = kBottomLeft_GrSurfaceOrigin; to backendTexture.fOrigin = kTopLeft_GrSurfaceOrigin; in the function switchStorageToSkImage(), the result of copyToTexture() doesn't change at all. https://codereview.chromium.org/2026803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:156: // Get a new mailbox because we cannot retain a texture in the WebGL context. junov@: I tried to inject a readPixels() call to the m_image and I can see that all the pixels are as expected. But after calling the copyToTexture(), the result that drawing to canvas is flipY version of expected result. I don't know what is going on...
bsalomon@google.com changed reviewers: + bsalomon@google.com
https://codereview.chromium.org/2026803002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:150: if (contextProvider) { On 2016/06/14 18:09:40, xidachen wrote: > bsalomon@: in a layout test that runs on GPU, it creates a SkImage by calling > Blink::Image::imageForCurrentFrame(), and that SkImage is texture-backed. When > WebGL's texImage2D tries to consume this SkImage, it goes through this code > path. What I have found is that if I change: > backendTexture.fOrigin = kBottomLeft_GrSurfaceOrigin; > to backendTexture.fOrigin = kTopLeft_GrSurfaceOrigin; > in the function switchStorageToSkImage(), the result of copyToTexture() doesn't > change at all. From what I can tell copyToTexture doesn't look at the SkImage's texture's origin. Skia doesn't copy the the texture to flip it. The GrTexture's origin is only seen by Skia and is used when Skia reads the image. It affects the texture coordinates that we use to access the texture, but only in draws issued by Skia. Changing the texture origin won't affect copyToTexture's behavior.
https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4283: GLint level, GLint internalformat, GLenum type, GLint xoffset, GLint yoffset, GLint zoffset, HTMLCanvasElement* canvas, ImageBitmap* bitmap) Instead of having these two arguments (canvas + bitmap), you should just use use a single argument of a common base class, like CanvasImageSource. Then, move any source-type-specific code into virtual methods that are implemented in both HTMLCanvasElement and ImageBitmap. This is a more extensible design that will make it easy to later go through this path for things like laoding accelerated video decode and SVG rendered on the GPU and who knows what else. https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4478: bitmap->bitmapImage()->copyToTexture(drawingBuffer()->contextProvider(), targetTexture, targetInternalformat, targetType); I think this is where your bug is. When copying a GPU-backed ImageBitmap to a WebGL texture, you are ignoring the WebGL context's m_unpackFlipY state. See texImageCanvasByGPU which does the right thing.
https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4283: GLint level, GLint internalformat, GLenum type, GLint xoffset, GLint yoffset, GLint zoffset, HTMLCanvasElement* canvas, ImageBitmap* bitmap) On 2016/06/14 19:30:53, Justin Novosad wrote: > Instead of having these two arguments (canvas + bitmap), you should just use use > a single argument of a common base class, like CanvasImageSource. Then, move any > source-type-specific code into virtual methods that are implemented in both > HTMLCanvasElement and ImageBitmap. This is a more extensible design that will > make it easy to later go through this path for things like laoding accelerated > video decode and SVG rendered on the GPU and who knows what else. I do agree that it is a better design to use the base class such as CanvasImageSource. The only problem is that the width() and height() methods for derived class of CanvasImageSource has different return types. For example, ImageBitmap is const, while HTMLImageElement is not const. I don't think we can make a virtual width() and height() non-const and its derived class const. https://codereview.chromium.org/2026803002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-image-bitmap-utils.js (right): https://codereview.chromium.org/2026803002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-image-bitmap-utils.js:137: // runTestOnBindingTarget(gl.TEXTURE_CUBE_MAP, program, bitmaps, retVal); kbr@: could you give some suggestions? This js file is used in many layout tests. Right now this particular one: texImage-imageBitmap-from-canvas.html is timed out when running under virtual/gpu/ mode, without commenting out these two lines. What surprises me is that the test runs fine by either: 1. commenting out these two lines 2. or commenting out the two lines above. Basically running either gl.Texture_2D or gl.Texture_CUBE_MAP is fine, but running both will time out this test. I tried to debug, and it seems that the test timed out at the: tiu.setupTexturedQuadWithCubeMap() line. I don't know what could cause timeout in that line?
https://codereview.chromium.org/2026803002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2026803002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:143: virtual PassRefPtr<SkImage> imageForCurrentFrame(WebGraphicsContext3DProvider* = nullptr) = 0; Looks like we only ever pass a WebGraphicsContext3DProvider argument when calling on a StaticBitmapImage object (in WebGLRenderingContextBase::texImageBitmapByGPU). Rather than exposing this on the Image interface (encumbers all subclasses), why not make it a StaticBitmapImage method? Then you would also have more freedom in naming - maybe StaticBitmapImage::imageForContext3DProvider(...)?
On 2016/06/15 12:18:48, xidachen wrote: > On 2016/06/14 19:30:53, Justin Novosad wrote: > > Instead of having these two arguments (canvas + bitmap), you should just use > use > > a single argument of a common base class, like CanvasImageSource. Then, move > any > > source-type-specific code into virtual methods that are implemented in both > > HTMLCanvasElement and ImageBitmap. This is a more extensible design that will > > make it easy to later go through this path for things like laoding accelerated > > video decode and SVG rendered on the GPU and who knows what else. > > I do agree that it is a better design to use the base class such as > CanvasImageSource. The only problem is that the width() and height() methods for > derived class of CanvasImageSource has different return types. For example, > ImageBitmap is const, while HTMLImageElement is not const. I don't think we can > make a virtual width() and height() non-const and its derived class const. > HTMLImageElement::width() does not return the value you want anyways (it returns the width of the element's computed style). You'd want to get your image dimensions from the Image that is returned by CanvasImageSource::getSourceImageForCanvas
ping, this CL is ready for re-review now.
lgtm with nit. Wait for lgtm from a WebGL reviewer. https://codereview.chromium.org/2026803002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/canvas/CanvasImageSource.h (right): https://codereview.chromium.org/2026803002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/canvas/CanvasImageSource.h:71: virtual int sourceWidth() { return 0; } This should be pure virtual IMHO. The default behavior of returning 0 is not the correct result for any of the subclasses
I'm not so clear on the management of the various objects in StaticBitmapImage, but if junov's carefully reviewed that then lgtm. Basically relying on the tests to verify the correctness of the code. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4265: // FIXME: Implement GPU-to-GPU path for WebGL 2 and more internal formats. TODO(xidachen). Also, please reference a bug ID for the TODO. WebGL 2.0 is almost out, and it would be unfortunate to have a major performance penalty in key texture uploading use cases compared to WebGL 1.0. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4303: // TODO: Make this function support more cases in texSubImage calls. TODO(xidachen) https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4475: bitmap->bitmapImage()->copyToTexture(drawingBuffer()->contextProvider(), textureId, targetTexture, targetInternalformat, targetType, flipY); It seems a little strange that this work is split across two calls. Why not have StaticBitmapImage::copyToTexture not take the textureId argument, and have it call texureIdForWebGL internally? Does anything need to be done to the textureId after the copy is done in order to release the reference to the texture in the WebGL context? I don't remember all of the semantics of mailboxes and their consumed textures. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4506: && internalformat != GL_R8 && internalformat != GL_RG8 && internalformat != GL_RGB8 && internalformat != GL_RGBA8) { That is very strange -- at least that a copy of a GL_RGBA8 internal format texture would fail. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:178: switchStorageToSkImage(sharedProvider.get()); What happens if switchStorageToSkImage returns 0? Should this still return m_image?
https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4265: // FIXME: Implement GPU-to-GPU path for WebGL 2 and more internal formats. On 2016/06/24 01:25:10, Ken Russell wrote: > TODO(xidachen). Also, please reference a bug ID for the TODO. WebGL 2.0 is > almost out, and it would be unfortunate to have a major performance penalty in > key texture uploading use cases compared to WebGL 1.0. Bug filed here: https://bugs.chromium.org/p/chromium/issues/detail?id=622958 Notice that this function is also called in tex(Sub)Image when the source is an HTMLCanvasElement. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4303: // TODO: Make this function support more cases in texSubImage calls. On 2016/06/24 01:25:10, Ken Russell wrote: > TODO(xidachen) This comment is duplicate of the above one which says implement GPU-GPU texture copy for WebGL 2.0 and more internal formats. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4475: bitmap->bitmapImage()->copyToTexture(drawingBuffer()->contextProvider(), textureId, targetTexture, targetInternalformat, targetType, flipY); On 2016/06/24 01:25:10, Ken Russell wrote: > It seems a little strange that this work is split across two calls. Why not have > StaticBitmapImage::copyToTexture not take the textureId argument, and have it > call texureIdForWebGL internally? > > Does anything need to be done to the textureId after the copy is done in order > to release the reference to the texture in the WebGL context? I don't remember > all of the semantics of mailboxes and their consumed textures. There is no need to split the function calls indeed. Thanks for pointing this out. I have merge these two calls into one. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4506: && internalformat != GL_R8 && internalformat != GL_RG8 && internalformat != GL_RGB8 && internalformat != GL_RGBA8) { On 2016/06/24 01:25:10, Ken Russell wrote: > That is very strange -- at least that a copy of a GL_RGBA8 internal format > texture would fail. I have not yet debug deeply. But it is strange that all the other 29 tests works perfectly while these 4 formats failed. https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2026803002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:178: switchStorageToSkImage(sharedProvider.get()); On 2016/06/24 01:25:10, Ken Russell wrote: > What happens if switchStorageToSkImage returns 0? Should this still return > m_image? Change it to return nullptr in that case.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2026803002/#ps380001 (title: "address kbr@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 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 #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Avoid GPU readback in tex(Sub)Image2D(ImageBitmap) At this moment, if an ImageBitmap is texture backed, texImage2D will do a GPU read back which is expensive. The solution is to prepare a mailbox for the ImageBitmap, and use the WebGL context to consume it and then do a GPU-GPU texture copy. BUG=613411, 613409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/0bed6ac0177c01b7aa2719ecbb5f80143ccaa4ef Cr-Commit-Position: refs/heads/master@{#401903} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/0bed6ac0177c01b7aa2719ecbb5f80143ccaa4ef Cr-Commit-Position: refs/heads/master@{#401903} |
