Request context sharing via content::ContextProviderCommandBuffer

Request context sharing via content::ContextProviderCommandBuffer

Current context sharing is done in 1 of 2 ways:
1) Pass share_resources=true to the constructor of
   WebGraphicsContext3DCommandBufferImpl.
2) Pass a WebGraphicsContext3DCommandBufferImpl* to the constructor or
   WebGraphicsContext3DCommandBufferImpl.

FBOFW, this patch is conceptually 3 in 1..

a) Delete |share_resources| and make sharing requests always use option
   2 from above. This is possible now that checking context loss checks
   if any context in the share group has been lost.

b) Make RenderThreadImpl return a ContextProviderCommandBuffer for the
   shared main thread context. Since this context is given to blink in
   a WebGraphicsContext3DProvider, and blink specifies a provider when
   requesting to share contexts, we need a ContextProviderCommandBuffer
   in the provider.

c) Move the client-side share group tracking vector to the
   ContextProviderCommandBuffer from the
   WebGraphicsContext3DCommandBufferImpl class. We simplify the code
   a bit, and just make it a simple struct. The lock ends up being held
   for a larger number of commands for now, until things in
   WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread ends
   up moving out to ContextProviderCommandBuffer::BindToCurrentThread.

At this point the WebGraphicsContext3D base class is unused, and the
WebGraphicsContext3DProvider will return null for it, so we can remove
the class from blink.

R=piman@chromium.org, sievers@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/615aac2acd5f4aa12f42a8174e4636eb7d894de7
Cr-Commit-Position: refs/heads/master@{#390766}

[danakj, 2016-04-27 02:14:57]

Description was changed from

==========
Request context sharing via content::ContextProviderCommandBuffer

Current context sharing is done in 1 of 2 ways:
1) Pass share_resources=true to the constructor of
   WebGraphicsContext3DCommandBufferImpl.
2) Pass a WebGraphicsContext3DCommandBufferImpl* to the constructor or
   WebGraphicsContext3DCommandBufferImpl.

FBOFW, this patch is conceptually 3 in 1..

a) Delete |share_resources| and make sharing requests always use option
   2 from above. This is possible now that checking context loss checks
   if any context in the share group has been lost.

b) Make RenderThreadImpl return a ContextProviderCommandBuffer for the
   shared main thread context. Since this context is given to blink in
   a WebGraphicsContext3DProvider, and blink specifies a provider when
   requesting to share contexts, we need a ContextProviderCommandBuffer
   in the provider.

c) Move the client-side share group tracking vector to the
   ContextProviderCommandBuffer from the
   WebGraphicsContext3DCommandBufferImpl class. We simplify the code
   a bit, and just make it a simple struct. The lock ends up being held
   for a larger number of commands for now, until things in
   WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread ends
   up moving out to ContextProviderCommandBuffer::BindToCurrentThread.

At this point the WebGraphicsContext3D base class is unused, and the
WebGraphicsContext3DProvider will return null for it, so we can remove
the class from blink.

R=piman@chromium.org, sievers@chromium.org
BUG=584497
==========

to

==========
Request context sharing via content::ContextProviderCommandBuffer

Current context sharing is done in 1 of 2 ways:
1) Pass share_resources=true to the constructor of
   WebGraphicsContext3DCommandBufferImpl.
2) Pass a WebGraphicsContext3DCommandBufferImpl* to the constructor or
   WebGraphicsContext3DCommandBufferImpl.

FBOFW, this patch is conceptually 3 in 1..

a) Delete |share_resources| and make sharing requests always use option
   2 from above. This is possible now that checking context loss checks
   if any context in the share group has been lost.

b) Make RenderThreadImpl return a ContextProviderCommandBuffer for the
   shared main thread context. Since this context is given to blink in
   a WebGraphicsContext3DProvider, and blink specifies a provider when
   requesting to share contexts, we need a ContextProviderCommandBuffer
   in the provider.

c) Move the client-side share group tracking vector to the
   ContextProviderCommandBuffer from the
   WebGraphicsContext3DCommandBufferImpl class. We simplify the code
   a bit, and just make it a simple struct. The lock ends up being held
   for a larger number of commands for now, until things in
   WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread ends
   up moving out to ContextProviderCommandBuffer::BindToCurrentThread.

At this point the WebGraphicsContext3D base class is unused, and the
WebGraphicsContext3DProvider will return null for it, so we can remove
the class from blink.

R=piman@chromium.org, sievers@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-04-27 02:22:49]

Patchset #1 (id:1) has been deleted

[danakj, 2016-04-27 02:23:14]

> FBOFW, this patch is conceptually 3 in 1..

I'm happy to do these as 3 patches too. Just ran out of time today and thought
I'd upload.

[piman, 2016-04-27 22:09:34]

https://codereview.chromium.org/1916923004/diff/60001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1916923004/diff/60001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:353: // compositor.
It shares resources with the worker context.
Should we do this only if shared_worker_context_provider_ is non-null?

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:111:
gpu::CommandBufferProxyImpl* shared_command_buffer = nullptr;
nit: move both of those inside the AutoLock scope below (since we're not taking
a reference, they're only valid while the lock is held)

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:92: if
(!CreateContext(memory_limits, shared_command_buffer, share_group)) {
nit: std::move(share_group)

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:196: if
(!MaybeInitializeGL(memory_limits, shared_command_buffer, share_group)) {
nit: std::move(share_group)

https://codereview.chromium.org/1916923004/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1916923004/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:189:
DCHECK(!share_group_->IsLost());
I don't think we want this DCHECK - another thread may have lost the share group
after it's passed to the constructor. The GLES2Implementation maybe DOA, but
such is life?

[danakj, 2016-04-28 00:25:58]

PTAL. Will rebase also.

https://codereview.chromium.org/1916923004/diff/60001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1916923004/diff/60001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:353: // compositor.
It shares resources with the worker context.
On 2016/04/27 22:09:33, piman wrote:
> Should we do this only if shared_worker_context_provider_ is non-null?

Good point, will match RenderWidget and go back to EstablishGpuChannel again.

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:111:
gpu::CommandBufferProxyImpl* shared_command_buffer = nullptr;
On 2016/04/27 22:09:33, piman wrote:
> nit: move both of those inside the AutoLock scope below (since we're not
taking
> a reference, they're only valid while the lock is held)

Done. I forget why I moved these out to start with..

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:92: if
(!CreateContext(memory_limits, shared_command_buffer, share_group)) {
On 2016/04/27 22:09:33, piman wrote:
> nit: std::move(share_group)

Done.

https://codereview.chromium.org/1916923004/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:196: if
(!MaybeInitializeGL(memory_limits, shared_command_buffer, share_group)) {
On 2016/04/27 22:09:33, piman wrote:
> nit: std::move(share_group)

Done.

https://codereview.chromium.org/1916923004/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1916923004/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:189:
DCHECK(!share_group_->IsLost());
On 2016/04/27 22:09:33, piman wrote:
> I don't think we want this DCHECK - another thread may have lost the share
group
> after it's passed to the constructor. The GLES2Implementation maybe DOA, but
> such is life?

Ah.. ya fair point. Removed and moved the check to IsLost in the context
provider to be *after* the GLES2Impl is set up.

[piman, 2016-04-28 02:09:22]

lgtm

[danakj, 2016-04-29 18:33:06]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-04-29 18:33:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916923004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916923004/120001

[commit-bot: I haz the power, 2016-04-29 19:22:17]

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

[commit-bot: I haz the power, 2016-04-29 19:22:18]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[danakj, 2016-04-29 20:18:19]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-29 20:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1916923004/#ps140001
(title: "sharegroup: rebase")

[commit-bot: I haz the power, 2016-04-29 20:18:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916923004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916923004/140001

[danakj, 2016-04-29 20:20:48]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-29 20:20:49]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1916923004/#ps160001
(title: "sharegroup: .")

[commit-bot: I haz the power, 2016-04-29 20:21:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916923004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916923004/160001

[commit-bot: I haz the power, 2016-04-29 21:28:34]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-04-30 17:29:12]

Patchset 8 (id:??) landed as
https://crrev.com/615aac2acd5f4aa12f42a8174e4636eb7d894de7
Cr-Commit-Position: refs/heads/master@{#390766}

[commit-bot: I haz the power, 2016-04-30 18:02:45]

Description was changed from

==========
Request context sharing via content::ContextProviderCommandBuffer

Current context sharing is done in 1 of 2 ways:
1) Pass share_resources=true to the constructor of
   WebGraphicsContext3DCommandBufferImpl.
2) Pass a WebGraphicsContext3DCommandBufferImpl* to the constructor or
   WebGraphicsContext3DCommandBufferImpl.

FBOFW, this patch is conceptually 3 in 1..

a) Delete |share_resources| and make sharing requests always use option
   2 from above. This is possible now that checking context loss checks
   if any context in the share group has been lost.

b) Make RenderThreadImpl return a ContextProviderCommandBuffer for the
   shared main thread context. Since this context is given to blink in
   a WebGraphicsContext3DProvider, and blink specifies a provider when
   requesting to share contexts, we need a ContextProviderCommandBuffer
   in the provider.

c) Move the client-side share group tracking vector to the
   ContextProviderCommandBuffer from the
   WebGraphicsContext3DCommandBufferImpl class. We simplify the code
   a bit, and just make it a simple struct. The lock ends up being held
   for a larger number of commands for now, until things in
   WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread ends
   up moving out to ContextProviderCommandBuffer::BindToCurrentThread.

At this point the WebGraphicsContext3D base class is unused, and the
WebGraphicsContext3DProvider will return null for it, so we can remove
the class from blink.

R=piman@chromium.org, sievers@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Request context sharing via content::ContextProviderCommandBuffer

Current context sharing is done in 1 of 2 ways:
1) Pass share_resources=true to the constructor of
   WebGraphicsContext3DCommandBufferImpl.
2) Pass a WebGraphicsContext3DCommandBufferImpl* to the constructor or
   WebGraphicsContext3DCommandBufferImpl.

FBOFW, this patch is conceptually 3 in 1..

a) Delete |share_resources| and make sharing requests always use option
   2 from above. This is possible now that checking context loss checks
   if any context in the share group has been lost.

b) Make RenderThreadImpl return a ContextProviderCommandBuffer for the
   shared main thread context. Since this context is given to blink in
   a WebGraphicsContext3DProvider, and blink specifies a provider when
   requesting to share contexts, we need a ContextProviderCommandBuffer
   in the provider.

c) Move the client-side share group tracking vector to the
   ContextProviderCommandBuffer from the
   WebGraphicsContext3DCommandBufferImpl class. We simplify the code
   a bit, and just make it a simple struct. The lock ends up being held
   for a larger number of commands for now, until things in
   WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread ends
   up moving out to ContextProviderCommandBuffer::BindToCurrentThread.

At this point the WebGraphicsContext3D base class is unused, and the
WebGraphicsContext3DProvider will return null for it, so we can remove
the class from blink.

R=piman@chromium.org, sievers@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/615aac2acd5f4aa12f42a8174e4636eb7d894de7
Cr-Commit-Position: refs/heads/master@{#390766}
==========