IOSurface CGL context current lifetime cleanup

IOSurface CGL context current lifetime cleanup

Make the CGL context current while setting up the IOSurface-backed
texture be the context owned by the IOSurfaceTexture, not the
context owned by the CAOpenGLLayer.

Clean up some lifetime management of the CGL contexts.

BUG=420253
Committed: https://crrev.com/e77478355df808ac988a2bc9d2db84b7f2a85fd3
Cr-Commit-Position: refs/heads/master@{#298601}

[ccameron, 2014-10-03 00:57:24]

ccameron@chromium.org changed reviewers:
+ andresantoso@chromium.org

[ccameron, 2014-10-03 00:58:36]

ccameron@chromium.org changed reviewers:
+ kbr@chromium.org
- andresantoso@chromium.org

[ccameron, 2014-10-03 00:58:37]

This is, I suspect, the last change that I'll be able to make to the IOSurface
code (the second to last I'll try to make -- the next will probably need to be
reverted).

The change after this makes it so that we just use 1 CGL context, and I think
that is what triggered the crashes that you fixed for me.

[Ken Russell (switch to Gerrit), 2014-10-03 21:54:17]

Please file a bug about this in case it needs to be reverted and re-landed
multiple times.

[ccameron, 2014-10-03 21:59:34]

On 2014/10/03 21:54:17, Ken Russell wrote:
> Please file a bug about this in case it needs to be reverted and re-landed
> multiple times.

Filed crbug.com/420253

[Ken Russell (switch to Gerrit), 2014-10-04 00:33:39]

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_texture_mac.mm (right):

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_texture_mac.mm:181:
offscreen_context_->cgl_context());
I'm sure I'm being dense, but am finding it difficult to prove to myself that
offscreen_context_ is in the same share group as the context which is current
when IOSurfaceTexture::DrawIOSurface is called. IOSurfaceLayer initializes its
context from IOSurfaceContext::Get(IOSurfaceContext::kCALayerContext), and
that's the context current when drawing this texture_ to the screen in
IOSurfaceTexture::DrawIOSurface. However, offscreen_context_ is initialized from
IOSurfaceContext::Get(IOSurfaceContext::kOffscreenContext), and as far as I can
tell, IOSurfaceContext deliberately puts those contexts in disjoint share
groups. Am I missing something fundamental?

[ccameron, 2014-10-04 00:49:38]

Lmk if this clears things up.

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_texture_mac.mm (right):

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_texture_mac.mm:181:
offscreen_context_->cgl_context());
On 2014/10/04 00:33:39, Ken Russell wrote:
> I'm sure I'm being dense, but am finding it difficult to prove to myself that
> offscreen_context_ is in the same share group as the context which is current
> when IOSurfaceTexture::DrawIOSurface is called. IOSurfaceLayer initializes its
> context from IOSurfaceContext::Get(IOSurfaceContext::kCALayerContext), and
> that's the context current when drawing this texture_ to the screen in
> IOSurfaceTexture::DrawIOSurface. However, offscreen_context_ is initialized
from
> IOSurfaceContext::Get(IOSurfaceContext::kOffscreenContext), and as far as I
can
> tell, IOSurfaceContext deliberately puts those contexts in disjoint share
> groups. Am I missing something fundamental?

The types (kCALayerContext v kOffscreenContext) actually identify specific
contexts -- not parameters for creating the context. This is the part of the
guesswork where it seems that I need to keep the 2 types of contexts separate to
avoid crashes (the follow-on CL will show if that is the case or not).

The bit that puts the contexts in the same group is in IOSurfaceContext::Get,
where all contexts retrieved by Get are put in the same sharegroup (the
share_context there just pulls an arbitrary context out of the type->context
map).

When an error is encountered (or the GPU is changed), all of the contexts in the
sharegroup are poisoned and the map is cleared, and from then on a new
sharegroup is created.

[Ken Russell (switch to Gerrit), 2014-10-07 22:09:45]

LGTM per our over-the-shoulder review; thanks for sitting down and discussing
this with me.

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_texture_mac.mm (right):

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_texture_mac.mm:62:
offscreen_context_->cgl_context());
Per our over-the-shoulder review, this ScopedCGLSetCurrentContext is redundant
since this is done inside ReleaseIOSurfaceAndTexture.

[ccameron, 2014-10-07 22:33:45]

Thanks!

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_texture_mac.mm (right):

https://codereview.chromium.org/625753002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_texture_mac.mm:62:
offscreen_context_->cgl_context());
On 2014/10/07 22:09:44, Ken Russell wrote:
> Per our over-the-shoulder review, this ScopedCGLSetCurrentContext is redundant
> since this is done inside ReleaseIOSurfaceAndTexture.

Done.

[ccameron, 2014-10-07 22:33:49]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-10-07 22:34:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625753002/40001

[commit-bot: I haz the power, 2014-10-07 23:11:45]

Committed patchset #3 (id:40001) as c7445917e8d5492dd4a6c4bfe30ea41d679535ec

[commit-bot: I haz the power, 2014-10-07 23:13:18]

Patchset 3 (id:??) landed as
https://crrev.com/e77478355df808ac988a2bc9d2db84b7f2a85fd3
Cr-Commit-Position: refs/heads/master@{#298601}