Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
179973004:
Share Group plumbing in Blink; Remove WebGL from default share group (Closed)
|
Created:
6 years, 10 months ago by bajones Modified:
6 years, 9 months ago CC:
blink-reviews, jamesr, krit, dsinclair, jbroman, abarth-chromium, danakj, dglazkov+blink, Rik, adamk+blink_chromium.org, Stephen Chennney, aandrey+blink_chromium.org, pdr., rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionShare Group plumbing in Blink; Remove WebGL from default share group
BUG=127940
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169525
Patch Set 1 #Patch Set 2 : Changed nullptr to 0 #
Total comments: 4
Patch Set 3 : Fixed some failing conformance tests on OSX #
Total comments: 4
Patch Set 4 : Refactored syncing based on piman@'s feedback #
Total comments: 6
Patch Set 5 : Addressed Ken's feedback #
Total comments: 8
Patch Set 6 : Fixed issue Ken described. #
Total comments: 1
Patch Set 7 : Addressed jamesr@'s feedback #
Total comments: 1
Patch Set 8 : Style nit and comment #Messages
Total messages: 31 (0 generated)
Something to consider regarding this CL: As was mentioned in the Chrome-side CL for this change (https://codereview.chromium.org/101223005/) Blink will need to do some explicit handling of the case where a WebGL context (or any context, really) is added to a Share Group that is already lost. The current plan is to track share groups within Blink such that: * If a context is added to or restored with a share group who's contexts have been lost a new context in a new share group is created and added to the Blink-level share group tracker. * When other contexts within that Blink-level share group are recreated they will use the new context to specify their share group. This allows all contexts within a share group to be lost and restored and still be shared with the same contexts as before. This CL *DOES NOT* implement the above behavior, primarily because WebGL has no way to specify a share group. This patch does change all WebGL contexts to be created in their own share groups, but since they are the only context within the group further management is not yet necessary. This would allow us to start working on GPU-process-level context loss sooner, and defer the high level share group management until such time as the API actually supports it. If you'd rather see that high-level management implemented now, however, I understand.
Review Ping
LGTM
https://codereview.chromium.org/179973004/diff/20001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/179973004/diff/20001/public/platform/Platform... public/platform/Platform.h:558: virtual WebGraphicsContext3D* createOffscreenGraphicsContext3D(const WebGraphicsContext3D::Attributes&) { return 0; } I think getting rid of the other signature (the one without share_context) is better, but don't feel it's a big deal.
https://codereview.chromium.org/179973004/diff/20001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/179973004/diff/20001/public/platform/Platform... public/platform/Platform.h:558: virtual WebGraphicsContext3D* createOffscreenGraphicsContext3D(const WebGraphicsContext3D::Attributes&) { return 0; } On 2014/03/03 18:39:49, Zhenyao Mo wrote: > I think getting rid of the other signature (the one without share_context) is > better, but don't feel it's a big deal. The idea is to get rid of it, but it can't be removed until everything is landed and enabled. I'll be doing it in a follow-up patch.
https://codereview.chromium.org/179973004/diff/20001/Source/platform/graphics... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/20001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:339: // Context may be in a different share group. Must copy texture first Please clarify the comment. Something like "Must transfer it through a mailbox first." https://codereview.chromium.org/179973004/diff/20001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:341: m_context->genMailboxCHROMIUM(bufferMailbox->mailbox.name); There's an attempt elsewhere in the DrawingBuffer to re-use mailboxes. Will this mailbox get cleaned up properly? Or will it be leaked for an indeterminate amount of time?
Fixed some conformance test failures that were manifesting on OSX due to ImageBuffer::copyToPlatformTexture needing mailbox sync logic as well. I actually don't understand how this worked on Linux at all, since a similar situation (DrawingBuffer) was clearly and consistently failing. On 2014/03/04 03:55:02, Ken Russell wrote: > https://codereview.chromium.org/179973004/diff/20001/Source/platform/graphics... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/179973004/diff/20001/Source/platform/graphics... > Source/platform/graphics/gpu/DrawingBuffer.cpp:339: // Context may be in a > different share group. Must copy texture first > Please clarify the comment. Something like "Must transfer it through a mailbox > first." Done. > https://codereview.chromium.org/179973004/diff/20001/Source/platform/graphics... > Source/platform/graphics/gpu/DrawingBuffer.cpp:341: > m_context->genMailboxCHROMIUM(bufferMailbox->mailbox.name); > There's an attempt elsewhere in the DrawingBuffer to re-use mailboxes. Will this > mailbox get cleaned up properly? Or will it be leaked for an indeterminate > amount of time? This took a bit of digging, but I believe that in this case the mailbox dies when the texture it produced is deleted. You can follow the code here: https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... genMailbox just returns a series of cryptographically-secure bytes, but those byes aren't tracked within the GPU process outside of produce/consume. As such, when the Mailbox object goes out of scope here we have effectively "deleted" it, pending the consumption of any textures produced with it. https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:164: sharedContext->waitSyncPoint(mailbox->syncPoint); Same comments as in DrawingBuffer, you want to do the wait on |context| https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:190: context->flush(); same comments as in DrawingBuffer, you can add sharedContext->waitSyncPoint(context->insertSyncPoint()) for future-proofing the ordering. https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:347: m_context->waitSyncPoint(bufferMailbox->mailbox.syncPoint); This is essentially a noop. You insert a sync point in one context, and wait on the same context. What you should do instead is move the wait onto |context| before the consumeTextureCHROMIUM. https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:378: context->flush(); If you wanted to be thorough, you would want to insert a sync point on |context|, and wait on |m_context| here. This would ensure |m_context| can't delete the texture before |context| had a chance to consume it. (since everything is in the same channel currently, you won't see it as a race today, but we might in the future).
On 2014/03/11 00:02:24, piman wrote: > https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... > File Source/platform/graphics/ImageBuffer.cpp (right): > > https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... > Source/platform/graphics/ImageBuffer.cpp:164: > sharedContext->waitSyncPoint(mailbox->syncPoint); > Same comments as in DrawingBuffer, you want to do the wait on |context| > > https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... > Source/platform/graphics/ImageBuffer.cpp:190: context->flush(); > same comments as in DrawingBuffer, you can add > sharedContext->waitSyncPoint(context->insertSyncPoint()) for future-proofing the > ordering. > > https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... > Source/platform/graphics/gpu/DrawingBuffer.cpp:347: > m_context->waitSyncPoint(bufferMailbox->mailbox.syncPoint); > This is essentially a noop. You insert a sync point in one context, and wait on > the same context. What you should do instead is move the wait onto |context| > before the consumeTextureCHROMIUM. > > https://codereview.chromium.org/179973004/diff/40001/Source/platform/graphics... > Source/platform/graphics/gpu/DrawingBuffer.cpp:378: context->flush(); > If you wanted to be thorough, you would want to insert a sync point on > |context|, and wait on |m_context| here. This would ensure |m_context| can't > delete the texture before |context| had a chance to consume it. > > (since everything is in the same channel currently, you won't see it as a race > today, but we might in the future). Thanks for the feedback, and for improving my unfortunately poor understanding of how they syncPoints work. (Makes a lot more sense now.) CL updates.
Sorry for the long delay reviewing this. It looks good overall. It's simple and easy to understand. However, some gets should be optimized away. See below. https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:153: sharedContext->getIntegerv(GL_TEXTURE_BINDING_2D, &boundTexture); Get calls are expensive in the command buffer. Is it really necessary to preserve the texture bound to the current active texture unit in the shared context? If it's used in a stateless fashion by the calling code (probably Blink and Ganesh?) then this save/restore can be skipped. In that case, ImageBuffer::copyToPlatformTexture should document that it destroys the TEXTURE_2D binding of the active texture unit of the context provided by Blink's SharedOffscreenGraphicsContext3DProvider. https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:171: context->getIntegerv(GL_TEXTURE_BINDING_2D, &boundTexture); The only caller of ImageBuffer::copyToPlatformTexture is WebGLRenderingContextBase, and it should have enough information to restore the TEXTURE_2D binding for the current texture unit. I think the documentation for ImageBuffer::copyToPlatformTexture should clearly state that it destroys the TEXTURE_2D binding for the active texture unit of the passed context, and then the calling code should restore it. That will avoid the get query here. https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:187: context->bindTexture(GL_TEXTURE_2D, boundTexture); In order to effectively delete sourceTexture below, this could bind NULL. https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:337: m_context->getIntegerv(GL_TEXTURE_BINDING_2D, &boundTexture); One of the two callers of this method is ImageBuffer::copyRenderingResultsFromDrawingBuffer, called by WebGLRenderingContextBase::paintRenderingResultsToCanvas. The latter method has enough information to restore the bound texture, which would avoid this save/restore. The other caller is WebGLRenderingContextBase::texImage2D, which again can do the save/restore. I think DrawingBuffer::copyToPlatformTexture should be documented as destroying the TEXTURE_2D binding of the active texture unit of its owned context. https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:354: context->getIntegerv(GL_TEXTURE_BINDING_2D, &boundTexture); There are only two callers of DrawingBuffer::copyToPlatformTexture: ImageBuffer::copyRenderingResultsFromDrawingBuffer and WebGLRenderingContextBase::texImage2D. In the former case the shared offscreen graphics context is passed, and in the latter case the WebGLRenderingContext's WebGraphicsContext3D is passed. In the latter case the WebGLRenderingContext can always restore the TEXTURE_2D binding, and in the former I'm not sure it's necessary to restore it. I think DrawingBuffer::copyToPlatformTexture should be documented as destroying the TEXTURE_2D binding of the active texture unit of the passed context, and leave it up to the caller to restore it. https://codereview.chromium.org/179973004/diff/60001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:375: context->bindTexture(GL_TEXTURE_2D, boundTexture); Again, this can bind NULL in order to delete sourceTexture in a timely fashion.
Thanks for the optimization tips! Was able to remove 3 out of 4 gets, the last on (DrawingBuffer, changing passed context) breaks WebGL conformance tests if I don't reset it. Added a TODO in the code. Trying to run some try jobs via "git cl try", but it's not being very cooperative...
LGTM for my parts
Thanks, this looks better, but I think there's a bug. https://codereview.chromium.org/179973004/diff/80001/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/179973004/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContextBase.cpp:3425: if (gl && gl->m_drawingBuffer->copyToPlatformTexture(m_context.get(), texture->object(), internalformat, type, If the save/restore in DrawingBuffer::copyToPlatformTexture were removed, then it would also be necessary to restore the TEXTURE_2D binding on the context "gl", not just "this". Actually, I think it'll always be destroying the TEXTURE_2D binding against "gl" as it's currently written. Please double-check, and try to write a regression test if there's a bug here. https://codereview.chromium.org/179973004/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContextBase.cpp:3432: restoreCurrentTexture2D(); It might be worth creating a class which makes the restoreCurrentTexture2D() call in its destructor, and stack-allocating an instance higher up in this block so that all code paths implicitly execute it before exit. https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:156: sharedContext->bindTexture(GL_TEXTURE_2D, getBackingTexture()); I recall you mentioned that it was always necessary to preserve the TEXTURE_2D binding on the shared context. Was that not actually necessary here? https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:350: // TODO: Should be able to change the texture bindings here without reverting but TODO(bajones) https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:352: // tests to fail. We should find out why and fix it. The reason this was failing is probably that an additional restore call is needed higher up. DrawingBuffer::copyToPlatformTexture would need to be documented as destroying the TEXTURE_2D binding for the active texture unit both for its owned context as well as the passed WebGraphicsContext3D.
Will this break WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() and WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() since that passes in a texture id which the video hw texture is then copied into? Because if the source texture was not created in the same share group (on Android we use the main thread context, on CrOS I think it has its own context), then the copy would fail. There are conformance tests for video which exercise the path of copying the hw video into a webgl texture. However, for Android there is no trybot.
Thank you for great improvement. I'm curious about perf difference of PerformanceTests/Canvas/* https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... Source/platform/graphics/ImageBuffer.cpp:182: context->bindTexture(GL_TEXTURE_2D, 0); If changing context->bindTexture(GL_TEXTURE_2D, texture), ImageBuffer::copyToPlatformTexture() seems to be not changed from client's perspective. So clients don't need to call restoreCurrentTexture2D(). So imo, we can remove to query bound texture in DrawingBuffer. https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:374: context->bindTexture(GL_TEXTURE_2D, boundTexture); If changing context->bindTexture(GL_TEXTURE_2D, texture), this method is also not changed from clients' perspective. https://codereview.chromium.org/179973004/diff/80001/Source/platform/graphics... Source/platform/graphics/gpu/DrawingBuffer.cpp:378: m_context->waitSyncPoint(context->insertSyncPoint()); before this line, looks like m_context->makeContextCurrent() is needed. after this line, drawingBuffer should bind boundFBO.
On 2014/03/14 01:50:29, sievers wrote: > Will this break WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() and > WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() since that passes in > a texture id which the video hw texture is then copied into? > > Because if the source texture was not created in the same share group (on > Android we use the main thread context, on CrOS I think it has its own context), > then the copy would fail. > > There are conformance tests for video which exercise the path of copying the hw > video into a webgl texture. However, for Android there is no trybot. imo, this CL doesn't affect WMPI and WMPA, because we pass context3d of WebGL to them when calling those methods, and they don't use the main thread context.
On 2014/03/14 21:51:22, dshwang wrote: > On 2014/03/14 01:50:29, sievers wrote: > > Will this break WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() and > > WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() since that passes > in > > a texture id which the video hw texture is then copied into? > > > > Because if the source texture was not created in the same share group (on > > Android we use the main thread context, on CrOS I think it has its own > context), > > then the copy would fail. > > > > There are conformance tests for video which exercise the path of copying the > hw > > video into a webgl texture. However, for Android there is no trybot. > > imo, this CL doesn't affect WMPI and WMPA, because we pass context3d of WebGL to > them when calling those methods, and they don't use the main thread context. The problem would be the source texture, not the destination texture that you pass in. Either way, it can easily be made to work by using a mailbox in WMP::copyVideoTextureToPlatformTexture() which i wanted to do anyhow.
Fixed issue kbr@ mentioned. A test has been added to the WebGL conformance suite to cover that case. (on GitHub) On 2014/03/14 01:50:29, sievers wrote: > Will this break WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() and > WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() since that passes in > a texture id which the video hw texture is then copied into? > > Because if the source texture was not created in the same share group (on > Android we use the main thread context, on CrOS I think it has its own context), > then the copy would fail. WebMediaPlayerImpl::copyVideoTextureToPlatformTexture appears to already be using mailboxes. It's not failing the related WebGL conformance tests, in any case.
Excellent work. LGTM as long as it passes tests. You might want to be prepared to revert this if it breaks stuff on the Android GPU bot since there aren't any try servers for that configuration yet.
jamesr@ or abarth@: public/platform review please?
https://codereview.chromium.org/179973004/diff/120001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/179973004/diff/120001/public/platform/Platfor... public/platform/Platform.h:562: #ifdef ENABLE_EXPLICIT_GL_SHARE_GROUPS i don't see any need for an #ifdef here. just add both entry points - they have default impls so no harm in having them - and then when you don't need one of them any more just delete it
On 2014/03/15 00:05:32, jamesr wrote: > https://codereview.chromium.org/179973004/diff/120001/public/platform/Platform.h > File public/platform/Platform.h (right): > > https://codereview.chromium.org/179973004/diff/120001/public/platform/Platfor... > public/platform/Platform.h:562: #ifdef ENABLE_EXPLICIT_GL_SHARE_GROUPS > i don't see any need for an #ifdef here. just add both entry points - they have > default impls so no harm in having them - and then when you don't need one of > them any more just delete it Updated to address jamesr@'s feedback. New patch will require Blink changes from crrev.com/202543002/ before it can land.
public/ lgtm https://codereview.chromium.org/179973004/diff/140001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/179973004/diff/140001/public/platform/Platfor... public/platform/Platform.h:561: virtual WebGraphicsContext3D* createOffscreenGraphicsContext3D(const WebGraphicsContext3D::Attributes&, WebGraphicsContext3D* share_context) { return 0; } this is blink, so shareContext. probably worth explaining what this parameter does in a brief comment
On 2014/03/14 23:19:30, bajones wrote: > Fixed issue kbr@ mentioned. A test has been added to the WebGL conformance suite > to cover that case. (on GitHub) > > On 2014/03/14 01:50:29, sievers wrote: > > Will this break WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() and > > WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() since that passes > in > > a texture id which the video hw texture is then copied into? > > > > Because if the source texture was not created in the same share group (on > > Android we use the main thread context, on CrOS I think it has its own > context), > > then the copy would fail. > > WebMediaPlayerImpl::copyVideoTextureToPlatformTexture appears to already be > using mailboxes. It's not failing the related WebGL conformance tests, in any > case. https://codereview.chromium.org/198923007/ for Android.
The CQ bit was checked by bajones@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bajones@chromium.org/179973004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_compile_dbg
The CQ bit was checked by bajones@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bajones@chromium.org/179973004/160001
Message was sent while issue was closed.
Change committed as 169525 |
