Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/87aa976eacd8ed0c530d32a7c85d6ba706dcabf6
Cr-Commit-Position: refs/heads/master@{#395150}

[erikchen, 2016-05-16 23:27:18]

Description was changed from

==========
Create a new class ScopedReadLockGpuMemoryBuffer.

The GLRenderer uses this class to hold on to GpuMemoryBuffer resources until
they are no longer in use by the Window Server.

BUG=
==========

to

==========
Create a new class ScopedReadLockGpuMemoryBuffer.

The GLRenderer uses this class to hold on to GpuMemoryBuffer resources until
they are no longer in use by the Window Server.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-16 23:32:06]

Description was changed from

==========
Create a new class ScopedReadLockGpuMemoryBuffer.

The GLRenderer uses this class to hold on to GpuMemoryBuffer resources until
they are no longer in use by the Window Server.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Create a new class ScopedReadLockGpuMemoryBuffer.

The GLRenderer uses this class to hold on to GpuMemoryBuffer resources until
they are no longer in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-16 23:32:12]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 23:32:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984873002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984873002/20001

[erikchen, 2016-05-16 23:36:33]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 23:36:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984873002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984873002/40001

[commit-bot: I haz the power, 2016-05-16 23:55:01]

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

[commit-bot: I haz the power, 2016-05-16 23:55:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[erikchen, 2016-05-17 00:46:27]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 00:46:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984873002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984873002/60001

[erikchen, 2016-05-17 01:04:11]

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

[erikchen, 2016-05-17 01:04:12]

ccameron: Please review.

[commit-bot: I haz the power, 2016-05-17 01:59:28]

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

[commit-bot: I haz the power, 2016-05-17 01:59:29]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-05-17 03:36:38]

Couple structural suggestions.

IMO the cc::ResourceProvider part of this doesn't have enough of the dependent
work done yet to land (soon though).

The front that you can make the most independent progress on right now is
populating WebExternalTextureMailbox::gpuMemoryBufferId (and the related work of
getting the id out of GL ... there are probably lots of bikesheds to paint along
the way there).

https://codereview.chromium.org/1984873002/diff/60001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/1984873002/diff/60001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:277: std::deque<GmbResourceLockList>
in_use_gmb_resources_;
Couple of things:

1. This logic should be shared with the OverlayResourceLockList. The platforms
that use ScheduleOverlayPlane should actually be using a
ScopedReadLockGpuMemoryBuffer as well, since they're not actually using the
resource in OpenGL (they're doing some equivalent GMB thing on their platforms
-- they just don't have an 'in use' parameter).

2. This could potentially grow without bound -- if you're watching a video then,
ideally, most of the resources are getting re-used for thousands of frames on
end.

We should have a
  map<ResourceId, unique_ptr<ScopedReadLockGpuMemoryBuffer>>
instead.

I think the groups would be
- pending_overlay_resources (vector)
- swapped_overlay_resources (deque of vectors)
- swapped_and_acked_overlay_resources (map)
When we get a swap-ack, we merge the oldest of swapped into swapped_and_acked
(instead of removing it).

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1051:
DCHECK(IsGpuResourceType(resource_->type));
This will need to create the GpuMemoryBuffer from the (not yet existing)
CreateFromClientId method added in https://codereview.chromium.org/1975023002/.

We already have the plumbing for the GpuMemoryBufferId, but I am still working
on plumbing the gpu_memory_buffer_factor_client_id through.

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:325: bool IsInUseByMacOSWindowServer() const;
This should have a GetGpuMemoryBuffer accessor, like
ScopedWriteLockGpuMemoryBuffer -- no need to have a IsInUse function here as
well.

[erikchen, 2016-05-17 05:13:06]

On 2016/05/17 03:36:38, ccameron wrote:
> Couple structural suggestions.
> 
> IMO the cc::ResourceProvider part of this doesn't have enough of the dependent
> work done yet to land (soon though).
> 
> The front that you can make the most independent progress on right now is
> populating WebExternalTextureMailbox::gpuMemoryBufferId (and the related work
of
> getting the id out of GL ... there are probably lots of bikesheds to paint
along
> the way there).
That's waiting for reveman@ review...
https://codereview.chromium.org/1974163003/

[ccameron, 2016-05-17 14:09:26]

On 2016/05/17 05:13:06, erikchen wrote:
> On 2016/05/17 03:36:38, ccameron wrote:
> > Couple structural suggestions.
> > 
> > IMO the cc::ResourceProvider part of this doesn't have enough of the
dependent
> > work done yet to land (soon though).
> > 
> > The front that you can make the most independent progress on right now is
> > populating WebExternalTextureMailbox::gpuMemoryBufferId (and the related
work
> of
> > getting the id out of GL ... there are probably lots of bikesheds to paint
> along
> > the way there).
> That's waiting for reveman@ review...
> https://codereview.chromium.org/1974163003/

Ok, then go ahead with the changes I suggested here. I'll try to get the
remaining dependencies out for review today (it's just the 1 patch to get the
gpu memory buffer manager client id through).

[ccameron, 2016-05-17 16:39:37]

On 2016/05/17 14:09:26, ccameron wrote:
> On 2016/05/17 05:13:06, erikchen wrote:
> > On 2016/05/17 03:36:38, ccameron wrote:
> > > Couple structural suggestions.
> > > 
> > > IMO the cc::ResourceProvider part of this doesn't have enough of the
> dependent
> > > work done yet to land (soon though).
> > > 
> > > The front that you can make the most independent progress on right now is
> > > populating WebExternalTextureMailbox::gpuMemoryBufferId (and the related
> work
> > of
> > > getting the id out of GL ... there are probably lots of bikesheds to paint
> > along
> > > the way there).
> > That's waiting for reveman@ review...
> > https://codereview.chromium.org/1974163003/
> 
> Ok, then go ahead with the changes I suggested here. I'll try to get the
> remaining dependencies out for review today (it's just the 1 patch to get the
> gpu memory buffer manager client id through).

Hmm, as I'm adding the argument for GpuMemoryBuffer client Id, the only way to
test it is to add the ScopedGpuMemoryBufferReadLock -- I may grab that part of
this patch and put it in one I'm working on (stepping on each others' toes ...).

[erikchen, 2016-05-17 22:08:16]

Description was changed from

==========
Create a new class ScopedReadLockGpuMemoryBuffer.

The GLRenderer uses this class to hold on to GpuMemoryBuffer resources until
they are no longer in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-17 22:08:25]

Description was changed from

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-17 22:12:55]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2016-05-17 22:13:21]

ccameron: PTAL

https://codereview.chromium.org/1984873002/diff/60001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/1984873002/diff/60001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:277: std::deque<GmbResourceLockList>
in_use_gmb_resources_;
On 2016/05/17 03:36:38, ccameron wrote:
> Couple of things:
> 
> 1. This logic should be shared with the OverlayResourceLockList. The platforms
> that use ScheduleOverlayPlane should actually be using a
> ScopedReadLockGpuMemoryBuffer as well, since they're not actually using the
> resource in OpenGL (they're doing some equivalent GMB thing on their platforms
> -- they just don't have an 'in use' parameter).
Done.

> 
> 2. This could potentially grow without bound -- if you're watching a video
then,
> ideally, most of the resources are getting re-used for thousands of frames on
> end.
Right. I added a comment in the implementation indicating that we should have a
DCHECK, but currently don't, since some compositor consumers incorrectly reuse
resources while they may still be in use by the Window Server. I've also updated
the logic to be more resilient to this.

> 
> We should have a
>   map<ResourceId, unique_ptr<ScopedReadLockGpuMemoryBuffer>>
> instead.
> 
> I think the groups would be
> - pending_overlay_resources (vector)
> - swapped_overlay_resources (deque of vectors)
> - swapped_and_acked_overlay_resources (map)
> When we get a swap-ack, we merge the oldest of swapped into swapped_and_acked
> (instead of removing it).
Done.

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1051:
DCHECK(IsGpuResourceType(resource_->type));
On 2016/05/17 03:36:38, ccameron wrote:
> This will need to create the GpuMemoryBuffer from the (not yet existing)
> CreateFromClientId method added in
https://codereview.chromium.org/1975023002/.
> 
> We already have the plumbing for the GpuMemoryBufferId, but I am still working
> on plumbing the gpu_memory_buffer_factor_client_id through.

N/A

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1984873002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:325: bool IsInUseByMacOSWindowServer() const;
On 2016/05/17 03:36:38, ccameron wrote:
> This should have a GetGpuMemoryBuffer accessor, like
> ScopedWriteLockGpuMemoryBuffer -- no need to have a IsInUse function here as
> well.

N/A

[commit-bot: I haz the power, 2016-05-17 22:14:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984873002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984873002/100001

[ccameron, 2016-05-17 22:40:25]

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (left):

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2726: if
(!settings_->release_overlay_resources_on_swap_complete &&
This "!settings_->release_overlay_resources_on_swap_complete" logic is used by
Chrome OS ... GLRendererWithOverlaysTest.ResourcesExportedAndReturnedWithDelay
should be testing this.

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2730: ReleaseOverlayResources();
This is easier to read without the early-out.

   if (settings_->release_overlay_resources_on_swap_complete &&
+    for (OverlayResourceLock& lock : swapped_overlay_resources_.front()) {
+      swapped_and_acked_overlay_resources_[lock->GetResourceId()] =
+          std::move(lock);
+    }
     swapped_overlay_resources_.pop_front();
   }

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2741: // them to be returned.
This comment isn't right -- the renderer is not allowed to write to the resource
while the read lock is out, but it is expected to specify the same resource
frame-after-frame.

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2746: ReleaseOverlayResources();
I'd just move the ReleaseOverlayResources code here inline.

[commit-bot: I haz the power, 2016-05-17 22:56:36]

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

[commit-bot: I haz the power, 2016-05-17 22:56:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-05-17 23:13:32]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 23:14:17]

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

[erikchen, 2016-05-17 23:22:33]

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (left):

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2726: if
(!settings_->release_overlay_resources_on_swap_complete &&
On 2016/05/17 22:40:25, ccameron wrote:
> This "!settings_->release_overlay_resources_on_swap_complete" logic is used by
> Chrome OS ... GLRendererWithOverlaysTest.ResourcesExportedAndReturnedWithDelay
> should be testing this.

I get it. You wanted me to reuse the *overlay_resources data structures, but not
necessarily the logic.

The logic *does* turn out to be reusable, but it causes the phrase
"release_overlay_resources_on_swap_complete" to not be obviously accurate.  I've
chosen to take this route.

The alternative is to introduce another setting, or perhaps make this setting an
enum.

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2730: ReleaseOverlayResources();
On 2016/05/17 22:40:25, ccameron wrote:
> This is easier to read without the early-out.
> 
>    if (settings_->release_overlay_resources_on_swap_complete &&
> +    for (OverlayResourceLock& lock : swapped_overlay_resources_.front()) {
> +      swapped_and_acked_overlay_resources_[lock->GetResourceId()] =
> +          std::move(lock);
> +    }
>      swapped_overlay_resources_.pop_front();
>    }

Done.

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2741: // them to be returned.
On 2016/05/17 22:40:25, ccameron wrote:
> This comment isn't right -- the renderer is not allowed to write to the
resource
> while the read lock is out, but it is expected to specify the same resource
> frame-after-frame.

got it.

https://codereview.chromium.org/1984873002/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2746: ReleaseOverlayResources();
On 2016/05/17 22:40:25, ccameron wrote:
> I'd just move the ReleaseOverlayResources code here inline.

Done.

[ccameron, 2016-05-17 23:32:18]

lgtm

[erikchen, 2016-05-17 23:32:50]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 23:34:08]

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

[commit-bot: I haz the power, 2016-05-17 23:47:16]

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

[commit-bot: I haz the power, 2016-05-17 23:47:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-05-17 23:55:40]

On 2016/05/17 23:47:17, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

MD:
"""
../../chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1047:15:
error: no member named 'MaterialDesignController' in namespace 'ui'
      if (ui::MaterialDesignController::IsModeMaterial() && isRetina) {
"""

[erikchen, 2016-05-18 17:37:57]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-18 17:38:13]

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

[commit-bot: I haz the power, 2016-05-18 18:09:24]

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

[commit-bot: I haz the power, 2016-05-18 18:09:25]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-05-20 19:05:18]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-05-20 19:05:32]

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

[commit-bot: I haz the power, 2016-05-20 20:13:58]

Description was changed from

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-20 20:13:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-20 20:17:01]

Description was changed from

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add logic to ResourceProvider to correctly lock GpuMemoryBuffer Resources.

The new logic will hold onto GpuMemoryBuffer Resources until they are no longer
in use by the Window Server.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/87aa976eacd8ed0c530d32a7c85d6ba706dcabf6
Cr-Commit-Position: refs/heads/master@{#395150}
==========

[commit-bot: I haz the power, 2016-05-20 20:17:02]

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

[erikchen, 2016-05-23 23:44:35]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2006153002/ by erikchen@chromium.org.

The reason for reverting is: Depending on an implementation still in review.
Results in flooding of LOG messages.

https://bugs.chromium.org/p/chromium/issues/detail?id=614155
https://codereview.chromium.org/1993333002/.