Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15925007:
Virtual context MakeCurrent tweaks. (Closed)
|
Created:
7 years, 6 months ago by no sievers Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, jonathan.backer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionVirtual context MakeCurrent tweaks.
Do not allow GLContextVirtual::MakeCurrent() without a decoder (state
restorer), since this only leads to state bugs.
For this reason remove the necessity of having a current context during surface destruction (it's ugly anyway, since GLSurface is RefCounted).
BUG=248395
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205481
Patch Set 1 #
Total comments: 3
Patch Set 2 : release surface before context in GLESCmdDecoder::Destroy() #
Total comments: 1
Patch Set 3 : do not require current context during ~GLSurface() #Patch Set 4 : rebase #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 3
Patch Set 7 : #Patch Set 8 : rebase #Patch Set 9 : keep explicit surface ref release #
Messages
Total messages: 30 (0 generated)
ptal
On 2013/05/28 21:47:39, Daniel Sievers wrote: > ptal LGTM if you are sure that is the last surface ref (see comment). It just occurred to me that this usage of context (from outside) would not restore its state when it should. if (!myContext->IsCurrent(s)) myContext->MakeCurrent(s); So I think we should move the MakeCurrent avoidance logic out of IsCurrent.
Missing comments from my last post. https://codereview.chromium.org/15925007/diff/1/content/common/gpu/gpu_comman... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/15925007/diff/1/content/common/gpu/gpu_comman... content/common/gpu/gpu_command_buffer_stub.cc:367: surface_ = NULL; Are you sure this is the last valid reference to the surface? Could decoder_ have one until it is destroyed? It would be nice if we could DCHECK that this is the last suface reference if we need that to be the case. https://codereview.chromium.org/15925007/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (right): https://codereview.chromium.org/15925007/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:60: LOG(ERROR) << "Trying to make virtual context current without decoder."; Me like.
https://codereview.chromium.org/15925007/diff/1/content/common/gpu/gpu_comman... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/15925007/diff/1/content/common/gpu/gpu_comman... content/common/gpu/gpu_command_buffer_stub.cc:367: surface_ = NULL; On 2013/05/28 22:57:21, epenner wrote: > Are you sure this is the last valid reference to the surface? Could decoder_ > have one until it is destroyed? It would be nice if we could DCHECK that this is > the last suface reference if we need that to be the case. Ah good point, I forgot to release the decoder's ref - see updated patch. Otherwise, I think it's fine if multiple decoders share the surface, because the last one going away would still make it current. In fact, that shouldn't have behaved better before my patch. I added the error log now to catch bad usage cases.
https://codereview.chromium.org/15925007/diff/12001/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/15925007/diff/12001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:3200: surface_ = NULL; This looks good but see have_context above. It looks like we don't handle !have_context (although I don't know the details of when that occurs or how the other objects get cleaned up in that case). Perhaps we could do decoder_->SetSurface(NULL) back in the other code location where we know we have the context? It would really help if we could do surface->Destroy(), but it's probably reference-counting faux pas to determine if we have the last reference.
On 2013/05/28 23:36:56, epenner wrote: > https://codereview.chromium.org/15925007/diff/12001/gpu/command_buffer/servic... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/15925007/diff/12001/gpu/command_buffer/servic... > gpu/command_buffer/service/gles2_cmd_decoder.cc:3200: surface_ = NULL; > This looks good but see have_context above. It looks like we don't handle > !have_context (although I don't know the details of when that occurs or how the > other objects get cleaned up in that case). > > Perhaps we could do decoder_->SetSurface(NULL) back in the other code location > where we know we have the context? > > It would really help if we could do surface->Destroy(), but it's probably > reference-counting faux pas to determine if we have the last reference. I think if we don't have a context, there is nothing we can do. It's the caller (stub) anyway that tries to make the context current through the decoder.
> I think if we don't have a context, there is nothing we can do. > It's the caller (stub) anyway that tries to make the context current through the > decoder. Okay, fair enough. Could we just LOG that case then since we can't handle it? Then LGTM.
On 2013/05/29 00:29:37, epenner wrote: > > I think if we don't have a context, there is nothing we can do. > > It's the caller (stub) anyway that tries to make the context current through > the > > decoder. > > Okay, fair enough. Could we just LOG that case then since we can't handle it? > Then LGTM. It's legit though if the context was lost. In that case it's probably ok since we would delete all contexts in the share gorup. But like you said, it's a bit funky when you have RefCounted without explicit destruction where you cold pass flags in (like |have_context|).
@Al/gman: could you take a look? The win_gpu failure seems bogus, my whitespace change is getting those failures too (https://codereview.chromium.org/11636048/).
lgtm I wonder if we should have GLRealContext : A context that talks to the driver directly GLVirutalContext: A context that saves and restores state GLRealContextForVirtualContext: A context that talks directly to the driver for a GLVirtualContext. And basically make it illegal to have a GLVirtualContext have GLRealContext has its parent I'm just wondering if GLContext is being overloaded with 2 roles. The role when it's being used directly and the role when it's used by a virtual context. Note: I'm just thinking out loud. I have no idea if that solves any problems.
Since both Eric and you pointed out that the notion of trying to guarantee a current context during ~Surface() is funky, I'm wondering if anything really relies on it or we should support such a thing. I'll take a look if anything really relies on this. I thought that before textures were shared that TextureImageTransportSurface might actually delete textures during destruction. But that's not the case (or at least anymore), since it has the stub's OnWillDestroyStub() hook.
On 2013/05/30 21:10:39, Daniel Sievers wrote: > Since both Eric and you pointed out that the notion of trying to guarantee a > current context during ~Surface() is funky, I'm wondering if anything really > relies on it or we should support such a thing. > > I'll take a look if anything really relies on this. I thought that before > textures were shared that TextureImageTransportSurface might actually delete > textures during destruction. But that's not the case (or at least anymore), > since it has the stub's OnWillDestroyStub() hook. I think the IOSurfaceImageTransportSurface (for mac) also destroys its GL resources from the destructor. We may want to make this happen in OnWillDestroyStub instead.
On 2013/05/30 21:30:01, piman wrote: > On 2013/05/30 21:10:39, Daniel Sievers wrote: > > Since both Eric and you pointed out that the notion of trying to guarantee a > > current context during ~Surface() is funky, I'm wondering if anything really > > relies on it or we should support such a thing. > > > > I'll take a look if anything really relies on this. I thought that before > > textures were shared that TextureImageTransportSurface might actually delete > > textures during destruction. But that's not the case (or at least anymore), > > since it has the stub's OnWillDestroyStub() hook. > > I think the IOSurfaceImageTransportSurface (for mac) also destroys its GL > resources from the destructor. We may want to make this happen in > OnWillDestroyStub instead. Like this? I noticed that ~GLSurfaceOSMesa() deletes buffer_ which from looking at GLSurfaceOSMesa::Resize() looks like it might actually require the context to be *released*. True?
On 2013/05/31 22:31:07, Daniel Sievers wrote: > On 2013/05/30 21:30:01, piman wrote: > > On 2013/05/30 21:10:39, Daniel Sievers wrote: > > > Since both Eric and you pointed out that the notion of trying to guarantee a > > > current context during ~Surface() is funky, I'm wondering if anything really > > > relies on it or we should support such a thing. > > > > > > I'll take a look if anything really relies on this. I thought that before > > > textures were shared that TextureImageTransportSurface might actually delete > > > textures during destruction. But that's not the case (or at least anymore), > > > since it has the stub's OnWillDestroyStub() hook. > > > > I think the IOSurfaceImageTransportSurface (for mac) also destroys its GL > > resources from the destructor. We may want to make this happen in > > OnWillDestroyStub instead. > > Like this? > > I noticed that ~GLSurfaceOSMesa() deletes buffer_ which from looking at > GLSurfaceOSMesa::Resize() looks like it might actually require the context to be > *released*. True? +piman for content/common/gpu/image_transport_surface_mac.cc
https://codereview.chromium.org/15925007/diff/31001/content/common/gpu/image_... File content/common/gpu/image_transport_surface_mac.cc (right): https://codereview.chromium.org/15925007/diff/31001/content/common/gpu/image_... content/common/gpu/image_transport_surface_mac.cc:357: helper_->stub()->AddDestructionObserver(this); Why not do that always in Initialize? Destroy seems to be doing more than just destroying the surface, and also that would remove the need for the extra bool.
https://codereview.chromium.org/15925007/diff/31001/content/common/gpu/image_... File content/common/gpu/image_transport_surface_mac.cc (right): https://codereview.chromium.org/15925007/diff/31001/content/common/gpu/image_... content/common/gpu/image_transport_surface_mac.cc:357: helper_->stub()->AddDestructionObserver(this); On 2013/06/05 00:05:33, piman wrote: > Why not do that always in Initialize? > Destroy seems to be doing more than just destroying the surface, and also that > would remove the need for the extra bool. Done. I think I was under the false impression that you cannot get to the stub yet during Init(). But looking at the code that does not seem to be a problem at all.
https://codereview.chromium.org/15925007/diff/34003/content/common/gpu/image_... File content/common/gpu/image_transport_surface_mac.cc (right): https://codereview.chromium.org/15925007/diff/34003/content/common/gpu/image_... content/common/gpu/image_transport_surface_mac.cc:160: if (!NoOpGLSurfaceCGL::Initialize()) I guess for correctness it should call helper_->Destroy() in the early return here.
lgtm https://codereview.chromium.org/15925007/diff/34003/content/common/gpu/image_... File content/common/gpu/image_transport_surface_mac.cc (right): https://codereview.chromium.org/15925007/diff/34003/content/common/gpu/image_... content/common/gpu/image_transport_surface_mac.cc:160: if (!NoOpGLSurfaceCGL::Initialize()) On 2013/06/05 00:21:16, Daniel Sievers wrote: > I guess for correctness it should call helper_->Destroy() in the early return > here. You're probably right, though it's a NOOP anyway. Not sure why we have it at all!
https://codereview.chromium.org/15925007/diff/34003/content/common/gpu/image_... File content/common/gpu/image_transport_surface_mac.cc (right): https://codereview.chromium.org/15925007/diff/34003/content/common/gpu/image_... content/common/gpu/image_transport_surface_mac.cc:160: if (!NoOpGLSurfaceCGL::Initialize()) On 2013/06/05 00:33:32, piman wrote: > On 2013/06/05 00:21:16, Daniel Sievers wrote: > > I guess for correctness it should call helper_->Destroy() in the early return > > here. > > You're probably right, though it's a NOOP anyway. Not sure why we have it at > all! Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/15925007/41001
Failed to apply patch for content/common/gpu/gpu_command_buffer_stub.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/common/gpu/gpu_command_buffer_stub.cc
Hunk #1 FAILED at 358.
1 out of 1 hunk FAILED -- saving rejects to file
content/common/gpu/gpu_command_buffer_stub.cc.rej
Patch: content/common/gpu/gpu_command_buffer_stub.cc
Index: content/common/gpu/gpu_command_buffer_stub.cc
diff --git a/content/common/gpu/gpu_command_buffer_stub.cc
b/content/common/gpu/gpu_command_buffer_stub.cc
index
582802d55b28af7429fafeb49974d41a13cbed90..a1c12172ee88c1e474950ea6e06b9c5069d001f7
100644
--- a/content/common/gpu/gpu_command_buffer_stub.cc
+++ b/content/common/gpu/gpu_command_buffer_stub.cc
@@ -358,23 +358,12 @@ void GpuCommandBufferStub::Destroy() {
destruction_observers_,
OnWillDestroyStub());
- scoped_refptr<gfx::GLContext> context;
if (decoder_) {
- context = decoder_->GetGLContext();
decoder_->Destroy(have_context);
decoder_.reset();
}
command_buffer_.reset();
-
- // Make sure that context_ is current while we destroy surface_, because
- // surface_ may have GL resources that it needs to destroy, and will need
- // context_ to be current in order to not leak these resources.
- if (context)
- context->MakeCurrent(surface_.get());
- surface_ = NULL;
- if (context)
- context->ReleaseCurrent(NULL);
}
void GpuCommandBufferStub::OnInitializeFailed(IPC::Message* reply_message) {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/15925007/44001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/15925007/44001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
I had to keep the 'surface_ = NULL' in GpuCommandBufferStub::Destroy() but filed crbug.com/248395 to figure out how to be able to get rid of it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/15925007/81001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/15925007/81001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/15925007/81001
Message was sent while issue was closed.
Change committed as 205481 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
