gpu: Bind dummy GL API when no context is current

gpu: Bind dummy GL API when no context is current

Also make platform behavior consistent in always releasing
any previously current context when MakeCurrent() fails.

This catches GL call sites with no context current.
It also avoids problems with GL implementations potentially not liking
this (and crashing) or even us ending up calling into the wrong context
(for example accidentally deleting a resource in the wrong context).

BUG=355275
R=piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261153

[no sievers, 2014-04-01 22:50:00]

ptal. 

Quite possible that the IOSurface image transport code also tries to make GL
calls when the context gets lost.

[no sievers, 2014-04-01 22:51:32]

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc#...
ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
I guess you could argue whether we should really ReleaseCurrent() here. wdyt?

[piman, 2014-04-01 23:00:02]

lgtm

https://codereview.chromium.org/221433004/diff/20001/content/common/gpu/textu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/221433004/diff/20001/content/common/gpu/textu...
content/common/gpu/texture_image_transport_surface.cc:185: } else {
It's not specific to this change, but this makes me think we may be leaking
resources if we lose contexts for non-GL reasons. Could you file a bug for this,
referencing this CL?

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

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context.h#newco...
ui/gl/gl_context.h:130: GLContext* context_;
nit: scoped_refptr? (below too)

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc#...
ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
On 2014/04/01 22:51:32, sievers wrote:
> I guess you could argue whether we should really ReleaseCurrent() here. wdyt?

yes.

[no sievers, 2014-04-01 23:39:37]

https://codereview.chromium.org/221433004/diff/20001/content/common/gpu/textu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/221433004/diff/20001/content/common/gpu/textu...
content/common/gpu/texture_image_transport_surface.cc:185: } else {
On 2014/04/01 23:00:02, piman wrote:
> It's not specific to this change, but this makes me think we may be leaking
> resources if we lose contexts for non-GL reasons. Could you file a bug for
this,
> referencing this CL?

In the stack from 355275 we are not leaking because it's called from
GpuChannelManager::OnLoseAllContexts().

We also wouldn't be leaking textures in case the context is lost because of a
parse error, because GpuCommandBufferStub::Destroy() wil still try to make the
decoder's context + surface current before calling OnWillDestroyStub().

So we could only leak if
- we don't lose the whole share group when losing contexts
- we cannot make the lost context current at all during cleanup
- there is still another context around that references the share group

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc#...
ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
On 2014/04/01 23:00:02, piman wrote:
> On 2014/04/01 22:51:32, sievers wrote:
> > I guess you could argue whether we should really ReleaseCurrent() here.
wdyt?
> 
> yes.

Or should this be stronger and release any context that was previously current?
With my current patch we will take the early-out in ReleaseCurrent() below if
either |this| is not current, or |this| is not current with |surface|.

wglMakeCurrent() and eglMakeCurrent() seem to say that a previously current GL
context is released even if MakeCurrent() fails.

Unfortunately, glxMakeCurrent() says "If False is returned, the previously
current rendering context and drawable (if any) remain unchanged."

However we could make the behavior compatible here.

[no sievers, 2014-04-01 23:47:23]

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc#...
ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
On 2014/04/01 23:39:37, sievers wrote:
> On 2014/04/01 23:00:02, piman wrote:
> > On 2014/04/01 22:51:32, sievers wrote:
> > > I guess you could argue whether we should really ReleaseCurrent() here.
> wdyt?
> > 
> > yes.
> 
> Or should this be stronger and release any context that was previously
current?
> With my current patch we will take the early-out in ReleaseCurrent() below if
> either |this| is not current, or |this| is not current with |surface|.
> 
> wglMakeCurrent() and eglMakeCurrent() seem to say that a previously current GL
> context is released even if MakeCurrent() fails.
> 
> Unfortunately, glxMakeCurrent() says "If False is returned, the previously
> current rendering context and drawable (if any) remain unchanged."
> 
> However we could make the behavior compatible here.

Actually, I just realized that this comment doesn't make sense.
However, I think I can still make the release logic simpler/static.

[piman, 2014-04-01 23:50:58]

https://codereview.chromium.org/221433004/diff/20001/content/common/gpu/textu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/221433004/diff/20001/content/common/gpu/textu...
content/common/gpu/texture_image_transport_surface.cc:185: } else {
On 2014/04/01 23:39:37, sievers wrote:
> On 2014/04/01 23:00:02, piman wrote:
> > It's not specific to this change, but this makes me think we may be leaking
> > resources if we lose contexts for non-GL reasons. Could you file a bug for
> this,
> > referencing this CL?
> 
> In the stack from 355275 we are not leaking because it's called from
> GpuChannelManager::OnLoseAllContexts().
> 
> We also wouldn't be leaking textures in case the context is lost because of a
> parse error, because GpuCommandBufferStub::Destroy() wil still try to make the
> decoder's context + surface current before calling OnWillDestroyStub().
> 


Only if the context isn't know to be lost, though. E.g. if the client calls
LoseContextCHROMIUM (e.g on GL_OUT_OF_MEMORY with Dana's patch), we'd set the
kLostContext state even though GL isn't lost.
That loses the whole ContextGroup, but not the whole GLShareGroup


> So we could only leak if
> - we don't lose the whole share group when losing contexts
> - we cannot make the lost context current at all during cleanup
> - there is still another context around that references the share group
>

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc#...
ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
On 2014/04/01 23:47:23, sievers wrote:
> On 2014/04/01 23:39:37, sievers wrote:
> > On 2014/04/01 23:00:02, piman wrote:
> > > On 2014/04/01 22:51:32, sievers wrote:
> > > > I guess you could argue whether we should really ReleaseCurrent() here.
> > wdyt?
> > > 
> > > yes.
> > 
> > Or should this be stronger and release any context that was previously
> current?
> > With my current patch we will take the early-out in ReleaseCurrent() below
if
> > either |this| is not current, or |this| is not current with |surface|.
> > 
> > wglMakeCurrent() and eglMakeCurrent() seem to say that a previously current
GL
> > context is released even if MakeCurrent() fails.
> > 
> > Unfortunately, glxMakeCurrent() says "If False is returned, the previously
> > current rendering context and drawable (if any) remain unchanged."
> > 
> > However we could make the behavior compatible here.
> 
> Actually, I just realized that this comment doesn't make sense.
> However, I think I can still make the release logic simpler/static.

I think it's best if we can have consistent semantics. If GLContext::MakeCurrent
fails, then the semantics is that no context is current (I think that's the only
semantics we can guarantee).

[no sievers, 2014-04-01 23:51:55]

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc
File ui/gl/gl_context_egl.cc (right):

https://codereview.chromium.org/221433004/diff/20001/ui/gl/gl_context_egl.cc#...
ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
On 2014/04/01 23:47:23, sievers wrote:
> On 2014/04/01 23:39:37, sievers wrote:
> > On 2014/04/01 23:00:02, piman wrote:
> > > On 2014/04/01 22:51:32, sievers wrote:
> > > > I guess you could argue whether we should really ReleaseCurrent() here.
> > wdyt?
> > > 
> > > yes.
> > 
> > Or should this be stronger and release any context that was previously
> current?
> > With my current patch we will take the early-out in ReleaseCurrent() below
if
> > either |this| is not current, or |this| is not current with |surface|.
> > 
> > wglMakeCurrent() and eglMakeCurrent() seem to say that a previously current
GL
> > context is released even if MakeCurrent() fails.
> > 
> > Unfortunately, glxMakeCurrent() says "If False is returned, the previously
> > current rendering context and drawable (if any) remain unchanged."
> > 
> > However we could make the behavior compatible here.
> 
> Actually, I just realized that this comment doesn't make sense.
> However, I think I can still make the release logic simpler/static.

So what I'll do is make this consistent. Essentially making sure that even the
failure paths like line 111 above guarantee SetCurrent(NULL) (plus releasing the
current context where necessary).

Sound good?

One caveat: I noticed a while ago that GLContextOSMesa::ReleaseCurrent() is
actually broken because OSMesaMakeCurrent(NULL, NULL..) does not work.

You can only make current a valid context. We'd have to create a dummy one, or
fix mesa. It definitely makes things flaky because it keeps it referenced on the
thread.

[piman, 2014-04-01 23:56:19]

On Tue, Apr 1, 2014 at 4:51 PM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.org/221433004/diff/20001/ui/
> gl/gl_context_egl.cc
> File ui/gl/gl_context_egl.cc (right):
>
> https://codereview.chromium.org/221433004/diff/20001/ui/
> gl/gl_context_egl.cc#newcode124
> ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
> On 2014/04/01 23:47:23, sievers wrote:
>
>> On 2014/04/01 23:39:37, sievers wrote:
>> > On 2014/04/01 23:00:02, piman wrote:
>> > > On 2014/04/01 22:51:32, sievers wrote:
>> > > > I guess you could argue whether we should really
>>
> ReleaseCurrent() here.
>
>> > wdyt?
>> > >
>> > > yes.
>> >
>> > Or should this be stronger and release any context that was
>>
> previously
>
>> current?
>> > With my current patch we will take the early-out in ReleaseCurrent()
>>
> below if
>
>> > either |this| is not current, or |this| is not current with
>>
> |surface|.
>
>> >
>> > wglMakeCurrent() and eglMakeCurrent() seem to say that a previously
>>
> current GL
>
>> > context is released even if MakeCurrent() fails.
>> >
>> > Unfortunately, glxMakeCurrent() says "If False is returned, the
>>
> previously
>
>> > current rendering context and drawable (if any) remain unchanged."
>> >
>> > However we could make the behavior compatible here.
>>
>
>  Actually, I just realized that this comment doesn't make sense.
>> However, I think I can still make the release logic simpler/static.
>>
>
> So what I'll do is make this consistent. Essentially making sure that
> even the failure paths like line 111 above guarantee SetCurrent(NULL)
> (plus releasing the current context where necessary).
>
> Sound good?
>

Yep.


>
> One caveat: I noticed a while ago that GLContextOSMesa::ReleaseCurrent()
> is actually broken because OSMesaMakeCurrent(NULL, NULL..) does not
> work.
>
> You can only make current a valid context. We'd have to create a dummy
> one, or fix mesa. It definitely makes things flaky because it keeps it
> referenced on the thread.
>

Let's not worry too much about it, maybe add a note somewhere?


>
> https://codereview.chromium.org/221433004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-01 23:56:20]

On Tue, Apr 1, 2014 at 4:51 PM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.org/221433004/diff/20001/ui/
> gl/gl_context_egl.cc
> File ui/gl/gl_context_egl.cc (right):
>
> https://codereview.chromium.org/221433004/diff/20001/ui/
> gl/gl_context_egl.cc#newcode124
> ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
> On 2014/04/01 23:47:23, sievers wrote:
>
>> On 2014/04/01 23:39:37, sievers wrote:
>> > On 2014/04/01 23:00:02, piman wrote:
>> > > On 2014/04/01 22:51:32, sievers wrote:
>> > > > I guess you could argue whether we should really
>>
> ReleaseCurrent() here.
>
>> > wdyt?
>> > >
>> > > yes.
>> >
>> > Or should this be stronger and release any context that was
>>
> previously
>
>> current?
>> > With my current patch we will take the early-out in ReleaseCurrent()
>>
> below if
>
>> > either |this| is not current, or |this| is not current with
>>
> |surface|.
>
>> >
>> > wglMakeCurrent() and eglMakeCurrent() seem to say that a previously
>>
> current GL
>
>> > context is released even if MakeCurrent() fails.
>> >
>> > Unfortunately, glxMakeCurrent() says "If False is returned, the
>>
> previously
>
>> > current rendering context and drawable (if any) remain unchanged."
>> >
>> > However we could make the behavior compatible here.
>>
>
>  Actually, I just realized that this comment doesn't make sense.
>> However, I think I can still make the release logic simpler/static.
>>
>
> So what I'll do is make this consistent. Essentially making sure that
> even the failure paths like line 111 above guarantee SetCurrent(NULL)
> (plus releasing the current context where necessary).
>
> Sound good?
>

Yep.


>
> One caveat: I noticed a while ago that GLContextOSMesa::ReleaseCurrent()
> is actually broken because OSMesaMakeCurrent(NULL, NULL..) does not
> work.
>
> You can only make current a valid context. We'd have to create a dummy
> one, or fix mesa. It definitely makes things flaky because it keeps it
> referenced on the thread.
>

Let's not worry too much about it, maybe add a note somewhere?


>
> https://codereview.chromium.org/221433004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-01 23:59:31]

On Tue, Apr 1, 2014 at 4:51 PM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.org/221433004/diff/20001/ui/
> gl/gl_context_egl.cc
> File ui/gl/gl_context_egl.cc (right):
>
> https://codereview.chromium.org/221433004/diff/20001/ui/
> gl/gl_context_egl.cc#newcode124
> ui/gl/gl_context_egl.cc:124: if (!surface->OnMakeCurrent(this)) {
> On 2014/04/01 23:47:23, sievers wrote:
>
>> On 2014/04/01 23:39:37, sievers wrote:
>> > On 2014/04/01 23:00:02, piman wrote:
>> > > On 2014/04/01 22:51:32, sievers wrote:
>> > > > I guess you could argue whether we should really
>>
> ReleaseCurrent() here.
>
>> > wdyt?
>> > >
>> > > yes.
>> >
>> > Or should this be stronger and release any context that was
>>
> previously
>
>> current?
>> > With my current patch we will take the early-out in ReleaseCurrent()
>>
> below if
>
>> > either |this| is not current, or |this| is not current with
>>
> |surface|.
>
>> >
>> > wglMakeCurrent() and eglMakeCurrent() seem to say that a previously
>>
> current GL
>
>> > context is released even if MakeCurrent() fails.
>> >
>> > Unfortunately, glxMakeCurrent() says "If False is returned, the
>>
> previously
>
>> > current rendering context and drawable (if any) remain unchanged."
>> >
>> > However we could make the behavior compatible here.
>>
>
>  Actually, I just realized that this comment doesn't make sense.
>> However, I think I can still make the release logic simpler/static.
>>
>
> So what I'll do is make this consistent. Essentially making sure that
> even the failure paths like line 111 above guarantee SetCurrent(NULL)
> (plus releasing the current context where necessary).
>
> Sound good?
>

Yep.


>
> One caveat: I noticed a while ago that GLContextOSMesa::ReleaseCurrent()
> is actually broken because OSMesaMakeCurrent(NULL, NULL..) does not
> work.
>
> You can only make current a valid context. We'd have to create a dummy
> one, or fix mesa. It definitely makes things flaky because it keeps it
> referenced on the thread.
>

Let's not worry too much about it, maybe add a note somewhere?


>
> https://codereview.chromium.org/221433004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2014-04-02 00:39:52]

Updated.

[piman, 2014-04-02 00:54:35]

lgtm

[no sievers, 2014-04-02 18:00:52]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-04-02 18:02:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/221433004/80001

[commit-bot: I haz the power, 2014-04-02 18:27:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 18:27:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[no sievers, 2014-04-02 18:37:17]

Committed patchset #4 manually as r261153 (presubmit successful).

[bungeman-skia, 2014-04-02 19:33:50]

On 2014/04/02 18:37:17, sievers wrote:
> Committed patchset #4 manually as r261153 (presubmit successful).

It looks like this might require a suppression for the new static
g_no_context_gl? See
http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%...

[no sievers, 2014-04-02 19:39:28]

On 2014/04/02 19:33:50, bungeman1 wrote:
> On 2014/04/02 18:37:17, sievers wrote:
> > Committed patchset #4 manually as r261153 (presubmit successful).
> 
> It looks like this might require a suppression for the new static
> g_no_context_gl? See
>
http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%...

This should fix it: https://codereview.chromium.org/220913004/