GPU: Keep track of the last real context and real surface made current.

GPU: Keep track of the last real context and real surface made current.

This allows us to set virtual contexts current, which allows us
to restore from a GLStateRestorer in Scoped{Texture,Framebuffer}Binders
with --enable-virtual-gl-contexts.

BUG=241762, 163217
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204500

[jonathan.backer, 2013-05-23 21:00:53]

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc#newcode144
ui/gl/gl_context.cc:144: SetCurrent(virtual_context, surface);
Maybe SetCurrent(NULL, NULL) otherwise?

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_gl_api_implementatio...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_gl_api_implementatio...
ui/gl/gl_gl_api_implementation.cc:273:
DCHECK(real_context_->IsCurrent(GLSurface::GetRealCurrent()));
Switching to a real surface is necessary because GLContextVirtual::IsCurrent is
permissive (as an optimization).

[no sievers, 2013-05-24 17:04:02]

Do we need to track the real context and surface only for various DCHECKS()?

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc#newcode101
ui/gl/gl_context.cc:101: if (!context || !context->IsVirtualContext()) {
Do we allow SetCurrent(NULL, !NULL)?
Or should we DCHECK_EQ(!surface, !context) as it might not make sense?

[jonathan.backer, 2013-05-24 20:36:27]

On 2013/05/24 17:04:02, Daniel Sievers wrote:
> Do we need to track the real context and surface only for various DCHECKS()?

No. We also use it in GLSurfaceOSMesa::Resize(...).

> 
> https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc
> File ui/gl/gl_context.cc (right):
> 
> https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc#newcode101
> ui/gl/gl_context.cc:101: if (!context || !context->IsVirtualContext()) {
> Do we allow SetCurrent(NULL, !NULL)?
> Or should we DCHECK_EQ(!surface, !context) as it might not make sense?

[jonathan.backer, 2013-05-24 20:36:40]

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc#newcode101
ui/gl/gl_context.cc:101: if (!context || !context->IsVirtualContext()) {
On 2013/05/24 17:04:02, Daniel Sievers wrote:
> Do we allow SetCurrent(NULL, !NULL)?
> Or should we DCHECK_EQ(!surface, !context) as it might not make sense?

Yeah. I just looked at the OSX code (which might have been the exception --- see
NoOpSurfaceCGL) and I think that this is always true.

[no sievers, 2013-05-24 21:48:06]

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc
File ui/gl/gl_surface_osmesa.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc#ne...
ui/gl/gl_surface_osmesa.cc:26: GLContext* current_context =
GLContext::GetRealCurrent();
Wait, why does this have to get the real context here then?
And isn't that a problem if a virtual context was current, that we will now
return from here with the real context being set current for both
GLContext::GetCurrent() and GLContext::GetRealCurrent()?

Is this just to keep the virtual API from potentially skipping the context
switch below in line 52? Would it?

[jonathan.backer, 2013-05-27 16:37:36]

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_context.cc#newcode101
ui/gl/gl_context.cc:101: if (!context || !context->IsVirtualContext()) {
On 2013/05/24 20:36:40, jonathan.backer wrote:
> On 2013/05/24 17:04:02, Daniel Sievers wrote:
> > Do we allow SetCurrent(NULL, !NULL)?
> > Or should we DCHECK_EQ(!surface, !context) as it might not make sense?
> 
> Yeah. I just looked at the OSX code (which might have been the exception ---
see
> NoOpSurfaceCGL) and I think that this is always true.

Done.

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc
File ui/gl/gl_surface_osmesa.cc (right):

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc#ne...
ui/gl/gl_surface_osmesa.cc:26: GLContext* current_context =
GLContext::GetRealCurrent();
On 2013/05/24 21:48:06, Daniel Sievers wrote:
> Wait, why does this have to get the real context here then?

AFAICT, this code it about resizing a buffer that may be in use. If it's in use,
we need to release, resize, rebind.

In practice, this surface could be current with a real context or a virtual
context. But it's the real binding that matters (not the virtual binding). Note,
a surface being current virtually does not always mean that it's current for
real (there's a 'lite' path in VirtualGLApi::MakeCurrent).

> And isn't that a problem if a virtual context was current, that we will now
> return from here with the real context being set current for both
> GLContext::GetCurrent() and GLContext::GetRealCurrent()?

Yes. This is a problem. Thank you. I'm going to fix this with a
ScopedMakeCurrent.

> Is this just to keep the virtual API from potentially skipping the context
> switch below in line 52? Would it?

No. The reset in line 52 is a bug (I should restore whatever was previously
current --- including virtual contexts).

[no sievers, 2013-05-28 18:59:49]

https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc#ne...
> ui/gl/gl_surface_osmesa.cc:26: GLContext* current_context =
> GLContext::GetRealCurrent();
> On 2013/05/24 21:48:06, Daniel Sievers wrote:
> > Wait, why does this have to get the real context here then?
> 
> AFAICT, this code it about resizing a buffer that may be in use. If it's in
use,
> we need to release, resize, rebind.
> 
> In practice, this surface could be current with a real context or a virtual
> context. But it's the real binding that matters (not the virtual binding).
Note,
> a surface being current virtually does not always mean that it's current for
> real (there's a 'lite' path in VirtualGLApi::MakeCurrent).
> 
> > And isn't that a problem if a virtual context was current, that we will now
> > return from here with the real context being set current for both
> > GLContext::GetCurrent() and GLContext::GetRealCurrent()?
> 
> Yes. This is a problem. Thank you. I'm going to fix this with a
> ScopedMakeCurrent.
> 

I see. This looks like it would do the right thing now. But shouldn't the code
currently in place just work too? GLContextVirtual::IsCurrent() and
GLContextVirtual::ReleaseCurrent() should definitely be usable for this case,
where you are trying to assure that a surface is not current. Otherwise we would
also potentially be leaking platform surfaces during GLSurface destruction (I
mean we might otherwise accidentally keep a shared context current with a
surface that we are trying to delete. We should really be doing
glMakeCurrent(NO_SURFACE) in that case).

IsCurrent() might be overly permissive, but I think it's a problem if
ReleaseCurrent(surface) would not guarantee that after the call surface is not
current anymore.

The other thing that would be cool about making this work more robustly is that
we might not have to expose the notion of the real context everywhere (we could
maybe make the methods protected if they are otherwise only used for the
dchecks).

[no sievers, 2013-05-28 19:01:36]

On 2013/05/28 18:59:49, Daniel Sievers wrote:
> currently in place just work too? GLContextVirtual::IsCurrent() and
> GLContextVirtual::ReleaseCurrent() should definitely be usable for this case,
> where you are trying to assure that a surface is not current. 

Oh and forgot to mention that I *think* it does:

void GLContextVirtual::ReleaseCurrent(gfx::GLSurface* surface) {
  if (IsCurrent(surface)) // might return true if surface is not actually
current, but should never return false if the surface is really current
    shared_context_->ReleaseCurrent(surface); // calls into the real context
}

[epenner, 2013-05-28 21:33:52]

https://codereview.chromium.org/15928002/diff/10021/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/10021/ui/gl/gl_context.cc#newco...
ui/gl/gl_context.cc:102: if (!context || !context->IsVirtualContext()) {
One drive-by comment:

If we keep keep track of the real context, I think we need to always set it here
(for example by grabbing the real context from the virtual context). For example
if there is several real contexts, each backing several virtual contexts, we
still want to change the real context if it has changed.

I believe Gregg mentioned pepper as a use-case where we would use one real
context for each plugin type, with virtual for each plugin instance.

[jonathan.backer, 2013-05-31 17:21:01]

On 2013/05/28 18:59:49, Daniel Sievers wrote:
>
https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc#ne...
> > ui/gl/gl_surface_osmesa.cc:26: GLContext* current_context =
> > GLContext::GetRealCurrent();
> > On 2013/05/24 21:48:06, Daniel Sievers wrote:
> > > Wait, why does this have to get the real context here then?
> > 
> > AFAICT, this code it about resizing a buffer that may be in use. If it's in
> use,
> > we need to release, resize, rebind.
> > 
> > In practice, this surface could be current with a real context or a virtual
> > context. But it's the real binding that matters (not the virtual binding).
> Note,
> > a surface being current virtually does not always mean that it's current for
> > real (there's a 'lite' path in VirtualGLApi::MakeCurrent).
> > 
> > > And isn't that a problem if a virtual context was current, that we will
now
> > > return from here with the real context being set current for both
> > > GLContext::GetCurrent() and GLContext::GetRealCurrent()?
> > 
> > Yes. This is a problem. Thank you. I'm going to fix this with a
> > ScopedMakeCurrent.
> > 
> 
> I see. This looks like it would do the right thing now. But shouldn't the code
> currently in place just work too? GLContextVirtual::IsCurrent() and
> GLContextVirtual::ReleaseCurrent() should definitely be usable for this case,
> where you are trying to assure that a surface is not current. Otherwise we
would
> also potentially be leaking platform surfaces during GLSurface destruction (I
> mean we might otherwise accidentally keep a shared context current with a
> surface that we are trying to delete. We should really be doing
> glMakeCurrent(NO_SURFACE) in that case).
> 
> IsCurrent() might be overly permissive, but I think it's a problem if
> ReleaseCurrent(surface) would not guarantee that after the call surface is not
> current anymore.
> 
> The other thing that would be cool about making this work more robustly is
that
> we might not have to expose the notion of the real context everywhere (we
could
> maybe make the methods protected if they are otherwise only used for the
> dchecks).

Agreed. I'd like to make these methods protected. AFAICT, they're only critical
for the DCHECKs.

Here's my gripe with GLSurfaceOSMesa::Resize on ToT:

  if (was_current)
    return current_context->MakeCurrent(this);

may not restore the value for the GLSurface::GetCurrent(). I think that in
practice it does. But looking locally at the code (just this method) there is no
guarantee because current_context->IsCurrent(this) is permissive. Perhaps this
is OK. If IsCurrent is going to be permissive, then our code shouldn't rely on
exact state restoration.

ScopedMakeCurrent will restore {GLSurface,GLContext}::GetCurrent.

[jonathan.backer, 2013-05-31 17:21:40]

https://codereview.chromium.org/15928002/diff/10021/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/10021/ui/gl/gl_context.cc#newco...
ui/gl/gl_context.cc:102: if (!context || !context->IsVirtualContext()) {
On 2013/05/28 21:33:53, epenner wrote:
> One drive-by comment:
> 
> If we keep keep track of the real context, I think we need to always set it
here
> (for example by grabbing the real context from the virtual context). For
example
> if there is several real contexts, each backing several virtual contexts, we
> still want to change the real context if it has changed.

By only changing current_real_context_ when the real context is made current,
current_real_context_ reflects a GL call that was made. I think it's easier to
reason if that invariant is maintained.

AFAICT, as part of the making the virtual context current, the real context will
be made current. We've got a DCHECK in VirtualGLApi::MakeCurrent to this affect.

> 
> I believe Gregg mentioned pepper as a use-case where we would use one real
> context for each plugin type, with virtual for each plugin instance.

Sorry. I may be mistaken (I often am), but I don't see how this precludes that.
There may need to be some changes at the gl_gl_api_implementation level.

[no sievers, 2013-05-31 20:38:33]

On 2013/05/31 17:21:01, jonathan.backer wrote:
> On 2013/05/28 18:59:49, Daniel Sievers wrote:
> >
>
https://codereview.chromium.org/15928002/diff/1/ui/gl/gl_surface_osmesa.cc#ne...
> > > ui/gl/gl_surface_osmesa.cc:26: GLContext* current_context =
> > > GLContext::GetRealCurrent();
> > > On 2013/05/24 21:48:06, Daniel Sievers wrote:
> > > > Wait, why does this have to get the real context here then?
> > > 
> > > AFAICT, this code it about resizing a buffer that may be in use. If it's
in
> > use,
> > > we need to release, resize, rebind.
> > > 
> > > In practice, this surface could be current with a real context or a
virtual
> > > context. But it's the real binding that matters (not the virtual binding).
> > Note,
> > > a surface being current virtually does not always mean that it's current
for
> > > real (there's a 'lite' path in VirtualGLApi::MakeCurrent).
> > > 
> > > > And isn't that a problem if a virtual context was current, that we will
> now
> > > > return from here with the real context being set current for both
> > > > GLContext::GetCurrent() and GLContext::GetRealCurrent()?
> > > 
> > > Yes. This is a problem. Thank you. I'm going to fix this with a
> > > ScopedMakeCurrent.
> > > 
> > 
> > I see. This looks like it would do the right thing now. But shouldn't the
code
> > currently in place just work too? GLContextVirtual::IsCurrent() and
> > GLContextVirtual::ReleaseCurrent() should definitely be usable for this
case,
> > where you are trying to assure that a surface is not current. Otherwise we
> would
> > also potentially be leaking platform surfaces during GLSurface destruction
(I
> > mean we might otherwise accidentally keep a shared context current with a
> > surface that we are trying to delete. We should really be doing
> > glMakeCurrent(NO_SURFACE) in that case).
> > 
> > IsCurrent() might be overly permissive, but I think it's a problem if
> > ReleaseCurrent(surface) would not guarantee that after the call surface is
not
> > current anymore.
> > 
> > The other thing that would be cool about making this work more robustly is
> that
> > we might not have to expose the notion of the real context everywhere (we
> could
> > maybe make the methods protected if they are otherwise only used for the
> > dchecks).
> 
> Agreed. I'd like to make these methods protected. AFAICT, they're only
critical
> for the DCHECKs.
> 
> Here's my gripe with GLSurfaceOSMesa::Resize on ToT:
> 
>   if (was_current)
>     return current_context->MakeCurrent(this);
> 
> may not restore the value for the GLSurface::GetCurrent(). I think that in
> practice it does. But looking locally at the code (just this method) there is
no
> guarantee because current_context->IsCurrent(this) is permissive. Perhaps this
> is OK. If IsCurrent is going to be permissive, then our code shouldn't rely on
> exact state restoration.
> 
> ScopedMakeCurrent will restore {GLSurface,GLContext}::GetCurrent.

Ah, sorry now it does make sense to me after all. So LGTM.

One thing we should do (maybe as a follow-up? I'll be happy to do it too) is
remove the permissive logic from IsCurrent(),
so that the existing code in gl_surface_egl/osmesa would actually just work as
expected.

It seems more and more wrong to me. Eric also pointed out it's dangerously wrong
if a caller did this:

if (!context->IsCurrent(this))
  context->MakeCurrent()

We can just move the optimization to gl_gl_api_implementation.cc::MakeCurrent().
That's where it's really needed to fall into MakeCurrent 'lite'.

[no sievers, 2013-05-31 20:41:30]

On 2013/05/31 20:38:33, Daniel Sievers wrote:
> It seems more and more wrong to me. Eric also pointed out it's dangerously
wrong
> if a caller did this:
> 
> if (!context->IsCurrent(this))
>   context->MakeCurrent()


The reason that it's dangerous is because it could bypass state restore where
restoring the state is really necessary.
The permissive IsCurrent() logic really needs to be moved further down where the
optimization to avoid makeCurrent() calls is happening.

[greggman, 2013-05-31 21:13:38]

https://codereview.chromium.org/15928002/diff/47001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/47001/ui/gl/gl_context.cc#newco...
ui/gl/gl_context.cc:102: if (!context || !context->IsVirtualContext()) {
I feel like the suggestion of GLRealContext vs GLVirtualContext vs GLContext
(the abstract class) would solve this without the need for IsVirtualContext

GLVirtualContext would call SetCurrent which would only set current_context_. 

It would also call shared_context_->SetAsRealContext()

GLRealContext would call SetCurrent and call

   GLContext::SetRealContext(this);

I don't know if I'm making any sense but you shouldn't have to know if this is
virtual. The virtual should delegate to its parent which might be real or
virtual. If virtual it would delegate to it's parent. etc.. Eventually you reach
a real context which would set current_real_context_

No need for "IsVirtualContext". No one should need to know that.

thoughts?

[no sievers, 2013-05-31 21:34:03]

On 2013/05/31 21:13:38, greggman wrote:
> https://codereview.chromium.org/15928002/diff/47001/ui/gl/gl_context.cc
> File ui/gl/gl_context.cc (right):
> 
>
https://codereview.chromium.org/15928002/diff/47001/ui/gl/gl_context.cc#newco...
> ui/gl/gl_context.cc:102: if (!context || !context->IsVirtualContext()) {
> I feel like the suggestion of GLRealContext vs GLVirtualContext vs GLContext
> (the abstract class) would solve this without the need for IsVirtualContext
> 
> GLVirtualContext would call SetCurrent which would only set current_context_. 
> 
> It would also call shared_context_->SetAsRealContext()
> 
> GLRealContext would call SetCurrent and call
> 
>    GLContext::SetRealContext(this);
> 
> I don't know if I'm making any sense but you shouldn't have to know if this is
> virtual. The virtual should delegate to its parent which might be real or
> virtual. If virtual it would delegate to it's parent. etc.. Eventually you
reach
> a real context which would set current_real_context_
> 
> No need for "IsVirtualContext". No one should need to know that.
> 
> thoughts?

This class hierarchy?

GLContextEGL/GLContextGLX/etc. -> GLRealContext -> GLContext
GLContextVirtual -> GLContext

(I was also thinking about some context adapter class between virtual and shared
context instead. But not sure if that can solve the SetCurrent() problem nicely.
It would allow us to pull virtual context logic out of the GLContext base class
though.)

[jonathan.backer, 2013-06-03 14:57:23]

https://codereview.chromium.org/15928002/diff/47001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/47001/ui/gl/gl_context.cc#newco...
ui/gl/gl_context.cc:102: if (!context || !context->IsVirtualContext()) {
On 2013/05/31 21:13:38, greggman wrote:
> I feel like the suggestion of GLRealContext vs GLVirtualContext vs GLContext
> (the abstract class) would solve this without the need for IsVirtualContext
> 
> GLVirtualContext would call SetCurrent which would only set current_context_. 
> 
> It would also call shared_context_->SetAsRealContext()
> 
> GLRealContext would call SetCurrent and call
> 
>    GLContext::SetRealContext(this);
> 
> I don't know if I'm making any sense but you shouldn't have to know if this is
> virtual. The virtual should delegate to its parent which might be real or
> virtual. If virtual it would delegate to it's parent. etc.. Eventually you
reach
> a real context which would set current_real_context_
> 
> No need for "IsVirtualContext". No one should need to know that.
> 
> thoughts?
> 

Done with some slight modifications. Mostly used GLContextReal (vs
GLRealContext) to follow the naming GLContextVirtual.

[jonathan.backer, 2013-06-04 19:14:40]

Yeah trybots.

There was a failing DCHECK in ImageTransportSurfaceMac::UnrefIOSurface().  I've
remedied this by calling SetCurrent appropriately before any calls to
OnMakeCurrent.  I examined all the implementations of OnMakeCurrent, and it is
usually caching the the context that is passed to it. My change helps ensure
that GetCurrent returns the context passed to OnMakeCurrent.

[greggman, 2013-06-04 22:58:50]

lgtm

[commit-bot: I haz the power, 2013-06-04 23:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/15928002/66001

[no sievers, 2013-06-04 23:47:19]

lgtm with one question.

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.cc#newco...
ui/gl/gl_context.cc:141: surface->OnMakeCurrent(virtual_context);
Wait, why add a call to surface->OnMakeCurrent() here?

[jonathan.backer, 2013-06-05 14:02:09]

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.cc#newco...
ui/gl/gl_context.cc:141: surface->OnMakeCurrent(virtual_context);
On 2013/06/04 23:47:20, Daniel Sievers wrote:
> Wait, why add a call to surface->OnMakeCurrent() here?

Most (all?) of the implementations of GLSurface::OnMakeCurrent are caching the
context passed into the call for use later on. They don't do much else
(sometimes set swap interval, sometimes allocation some buffer backing an FBO).

What was happening on OSX was that we were caching the real context underlying
the virtual context. This caused a failing DCHECK(cached_context_ ==
GLContext::GetCurrent) later on once the virtual context was set current.

[commit-bot: I haz the power, 2013-06-05 14:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/15928002/66001

[boliu, 2013-06-05 15:43:56]

Flyby comment:

The android test failures are probably not flakes. I am seeing this DCHECK
failing a lot:
[FATAL:gl_gl_api_implementation.cc(274)] Check failed:
real_context_->IsCurrent(GLSurface::GetRealCurrent()).

I can produce a backtrace locally if that helps?

[jonathan.backer, 2013-06-05 15:48:46]

On 2013/06/05 15:43:56, boliu wrote:
> Flyby comment:
> 
> The android test failures are probably not flakes. I am seeing this DCHECK
> failing a lot:
> [FATAL:gl_gl_api_implementation.cc(274)] Check failed:
> real_context_->IsCurrent(GLSurface::GetRealCurrent()).
> 
> I can produce a backtrace locally if that helps?

Thanks boliu@. Unchecked the commit box. I can build locally. (I'll pull you in
later if I need help.)

[no sievers, 2013-06-05 16:38:06]

On 2013/06/05 14:02:09, jonathan.backer wrote:
> https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.cc
> File ui/gl/gl_context.cc (right):
> 
>
https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.cc#newco...
> ui/gl/gl_context.cc:141: surface->OnMakeCurrent(virtual_context);
> On 2013/06/04 23:47:20, Daniel Sievers wrote:
> > Wait, why add a call to surface->OnMakeCurrent() here?
> 
> Most (all?) of the implementations of GLSurface::OnMakeCurrent are caching the
> context passed into the call for use later on. They don't do much else
> (sometimes set swap interval, sometimes allocation some buffer backing an
FBO).
> 

Could you change it in gl_gl_api_implementation instead to pass in the
virtual_context rather than real_context so we don't have to call
surface->OnMakeCurrent() twice?
In https://codereview.chromium.org/15925007/ I'm changing it to not allow being
made current without a decoder anymore, but I guess the surface should not be
attempting to make the virtual context current after the decoder went away.

> What was happening on OSX was that we were caching the real context underlying
> the virtual context. This caused a failing DCHECK(cached_context_ ==
> GLContext::GetCurrent) later on once the virtual context was set current.

[no sievers, 2013-06-05 16:43:37]

Few more thoughts:

I think you need to add SetCurrent(NULL) to GLContextVirtual::ReleaseCurrent()
now that we are updating the current context for virtual ones.

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.h#newcod...
ui/gl/gl_context.h:119: static GLContext* GetRealCurrent();
Acutally, can we do this now instead:

GLContext::GetCurrent()->GetRealContext() with GetRealContext() being virtual.

GLContext* GLContextReal::GetRealContext() {
  return this;
}

GLContext* GLContextVirtual::GetRealContext() {
  return shared_context_;
}

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_surface.h#newcod...
ui/gl/gl_surface.h:124: static GLSurface* GetRealCurrent();
...and do we then really need to track this separately?

[jonathan.backer, 2013-06-05 20:40:10]

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_context.h#newcod...
ui/gl/gl_context.h:119: static GLContext* GetRealCurrent();
On 2013/06/05 16:43:37, Daniel Sievers wrote:
> Acutally, can we do this now instead:
> 
> GLContext::GetCurrent()->GetRealContext() with GetRealContext() being virtual.
> 
> GLContext* GLContextReal::GetRealContext() {
>   return this;
> }
> 
> GLContext* GLContextVirtual::GetRealContext() {
>   return shared_context_;
> }

This is fundamentally the same as IsVirtualContext() no? Specifically,
IsVirtualContext() is equivalent to (this == GetRealContext()).

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/15928002/diff/66001/ui/gl/gl_surface.h#newcod...
ui/gl/gl_surface.h:124: static GLSurface* GetRealCurrent();
On 2013/06/05 16:43:37, Daniel Sievers wrote:
> ...and do we then really need to track this separately?

Nope. I think that the DCHECK in gl_gl_api_implementation.cc was too
restrictive. Nuked it.

[jonathan.backer, 2013-06-05 20:42:26]

On 2013/06/05 16:43:37, Daniel Sievers wrote:
> Few more thoughts:
> 
> I think you need to add SetCurrent(NULL) to GLContextVirtual::ReleaseCurrent()
> now that we are updating the current context for virtual ones.

I think that shared_context_->ReleaseCurrent() has us covered. I can add the
call
if you think it serves as documentation.

[no sievers, 2013-06-05 20:49:31]

On 2013/06/05 20:42:26, jonathan.backer wrote:
> On 2013/06/05 16:43:37, Daniel Sievers wrote:
> > Few more thoughts:
> > 
> > I think you need to add SetCurrent(NULL) to
GLContextVirtual::ReleaseCurrent()
> > now that we are updating the current context for virtual ones.
> 
> I think that shared_context_->ReleaseCurrent() has us covered. I can add the
> call
> if you think it serves as documentation.

Yep, sorry you are right. Don't think we need to add anything then.
Let me know if you want me to take a look at the Android failures.

There's something wrong with patch set 8 though, it keeps saying it doesn't
exist when I try to look at files.

[jonathan.backer, 2013-06-05 20:53:07]

On 2013/06/05 20:49:31, Daniel Sievers wrote:
> On 2013/06/05 20:42:26, jonathan.backer wrote:
> > On 2013/06/05 16:43:37, Daniel Sievers wrote:
> > > Few more thoughts:
> > > 
> > > I think you need to add SetCurrent(NULL) to
> GLContextVirtual::ReleaseCurrent()
> > > now that we are updating the current context for virtual ones.
> > 
> > I think that shared_context_->ReleaseCurrent() has us covered. I can add the
> > call
> > if you think it serves as documentation.
> 
> Yep, sorry you are right. Don't think we need to add anything then.
> Let me know if you want me to take a look at the Android failures.
> 
> There's something wrong with patch set 8 though, it keeps saying it doesn't
> exist when I try to look at files.

Hopefully I fixed the errors. My repo got in a bad state.

This fixes the android errors.

[no sievers, 2013-06-05 21:20:53]

still lgtm. thanks!

[commit-bot: I haz the power, 2013-06-06 12:37:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/15928002/119005

[commit-bot: I haz the power, 2013-06-06 14:59:25]

Change committed as 204500