Add hints for potential overlay promotion on android.

Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=671354
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/304dc19797fefea519996ef33919b61513716af8
Cr-Commit-Position: refs/heads/master@{#437643}

[liberato (no reviews please), 2016-11-18 23:03:03]

Description was changed from

==========
first pass at feedback from overlay processor => GLImage

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

to

==========
first pass at feedback from overlay processor => GLImage

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-11-18 23:03:03]

Description was changed from

==========
first pass at feedback from overlay processor => GLImage

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
first pass at feedback from overlay processor => GLImage

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[liberato (no reviews please), 2016-11-19 18:17:20]

Description was changed from

==========
first pass at feedback from overlay processor => GLImage

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
first pass at feedback from overlay processor => GLImage

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[liberato (no reviews please), 2016-11-19 18:17:20]

liberato@chromium.org changed reviewers:
+ piman@chromium.org

[liberato (no reviews please), 2016-11-19 18:25:52]

Hi

Here's a first pass at notifying GLStreamTextureImage about suitability for
promotion to SurfaceView.  From my manual tests, it does correctly identify all
the cases that i'd care about for SV that i could simulate.  forgot to try
chrome://inspect.

It can stand a little clean up, and the mojo path is entirely untested.  also
needs some additional test coverage.

I wanted to see if you believe that it captures our mtg from a few weeks ago
before doing that.  mtg doc is
https://docs.google.com/a/google.com/document/d/1nN0d24UCY92d8Rg5xQofBZWw_2IM...
.

thanks
-fl

https://codereview.chromium.org/2508203004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2508203004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2475: bool is_promotable =
IsResourcePromotable(quad->resource_id());
this is where i picture sending the message to the GLStreamTextureImage via
gl_->SomeCustomCommandCHROMIUM().

i tried to encapsulate this inside the overlay logic also, but it was a little
weird having to lock the resource, send in the context, etc.   getting all of
the 'is / is not promotable' logic into the overlay processor seemed like the
key idea from our mtg, so i didn't worry too much about moving notification
elsewhere.

please let me know if you'd prefer this to be moved.

https://codereview.chromium.org/2508203004/diff/1/components/display_composit...
File
components/display_compositor/compositor_overlay_candidate_validator_android.cc
(right):

https://codereview.chromium.org/2508203004/diff/1/components/display_composit...
components/display_compositor/compositor_overlay_candidate_validator_android.cc:40:
// TODO(liberato): maybe we should notify by resource, not by quad.  while
this comment block is a little stale, sorry.  we now do report per-resource via
IsResourcePromotable.  It does not check if multiple quads refer to the same
resource, but that seems to be not too important.

https://codereview.chromium.org/2508203004/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2508203004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:834:
picture.set_surface_texture(!surface_view);
this is where the "backed by surface texture" flag is set initially.

the flag follows roughly this: picture (AVDA) => VideoFrameMetaData
(GpuVideoDecoder) => TextureMailbox (VideoResourceUpdater) => Resource
(ResourceUpdater) => TransferableResource => Resource => overlay_processor.

[liberato (no reviews please), 2016-11-19 18:26:08]

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

[commit-bot: I haz the power, 2016-11-19 18:26:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-11-23 00:37:25]

Sorry for the review delay... At a high level, this seems reasonable. A few
comments.

https://codereview.chromium.org/2508203004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2508203004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2475: bool is_promotable =
IsResourcePromotable(quad->resource_id());
On 2016/11/19 18:25:52, liberato wrote:
> this is where i picture sending the message to the GLStreamTextureImage via
> gl_->SomeCustomCommandCHROMIUM().
> 
> i tried to encapsulate this inside the overlay logic also, but it was a little
> weird having to lock the resource, send in the context, etc.   getting all of
> the 'is / is not promotable' logic into the overlay processor seemed like the
> key idea from our mtg, so i didn't worry too much about moving notification
> elsewhere.
> 
> please let me know if you'd prefer this to be moved.

I guess it feels a bit odd to do here...
How about:
- ResourceProvider::SetPromotionHint(ResourceId resource, bool promote) which
internally can do the GL call (which should be straightforward after a few
DCHECKs, don't necessarily need the whole ScopedReadLockGL)
- pass the ResourceProvider to the
OverlayCandidateValidator::CheckOverlaySupport and have it issue the hints on
the resources you want to promote

https://codereview.chromium.org/2508203004/diff/1/components/display_composit...
File
components/display_compositor/compositor_overlay_candidate_validator_android.cc
(right):

https://codereview.chromium.org/2508203004/diff/1/components/display_composit...
components/display_compositor/compositor_overlay_candidate_validator_android.cc:40:
// TODO(liberato): maybe we should notify by resource, not by quad.  while
On 2016/11/19 18:25:52, liberato wrote:
> this comment block is a little stale, sorry.  we now do report per-resource
via
> IsResourcePromotable.  It does not check if multiple quads refer to the same
> resource, but that seems to be not too important.

As long as we gracefully handle the case, it's ok I think - keeping in mind that
we can't trust the renderer not to send 2 quads referring to the same resource.

https://codereview.chromium.org/2508203004/diff/20001/cc/ipc/cc_param_traits_...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2508203004/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:146:
IPC_STRUCT_TRAITS_MEMBER(is_overlay_candidate)
nit: is_overlay_candidate is already on l.141

https://codereview.chromium.org/2508203004/diff/20001/cc/output/overlay_candi...
File cc/output/overlay_candidate_validator.h (right):

https://codereview.chromium.org/2508203004/diff/20001/cc/output/overlay_candi...
cc/output/overlay_candidate_validator.h:40: virtual bool
IsResourcePromotable(ResourceId resource_id);
Super nit: in the context of OverlayCandidateValidator, IsResourcePromotable
suggests "this resource can be put in an overlay" as in "right now", whereas I
think you mean "this resource would be ok to put in an overlay, but only if it's
switched to SurfaceView, but it isn't right now". Not sure what is the best name
to convey the concept...

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

https://codereview.chromium.org/2508203004/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1558: resource->is_backed_by_surface_texture =
it->is_backed_by_surface_texture;
note, however we will use this, we can't trust that the renderer gave us the
correct value. If it didn't, we don't have to display properly, but we can't
crash. I think at the end of the day it means that we need to handle the fact
that we may end up calling ScheduleOverlayPlane with a SurfaceTexture, or try to
draw with a SurfaceView, and we just need to maybe ignore the
ScheduleOverlayPlane in the first case and draw with a default texture in the
second case.

[liberato (no reviews please), 2016-12-01 20:59:11]

sorry for the delay on this, and thanks for your previous feedback.

here are the major changes:

- ResourceProvider now notifies resources of the promotability hint, based on
your feedback.

- the resource now has to request the hint.  this just makes it easier for
ResourceProvider to know what to send it to.  also makes it less likely that we
waste time calling back with hints on things that don't want them, even if that
must still work gracefully.  i could also remove this in favor of
"IsResourceOverlayable && external texture target" or similar, but an explicit
flag seemed clearer.

- the state of whether a resource id is currently promotable or not is stored in
OverlayCandidateList instead of in the validator.  the validator became weirdly
stateful in my previous CL.

- OverlayProcessor::ProcessForOverlays is responsible for asking the resource
provider to send notifications.  i don't believe that this can be done directly
in the validator, since the answer isn't known until we learn if any quad is
actually going to be promoted.  if so, then we must answer 'no' to all the
others due the 'one overlay' limit, even if they would otherwise be promotable.

i'm WFH today, so I can't test this until tomorrow.  however, your feedback on
the structure is what i'm concerned about, so i'm sending it now anyway.  :)

thanks
-fl

https://codereview.chromium.org/2508203004/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2508203004/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2475: bool is_promotable =
IsResourcePromotable(quad->resource_id());
On 2016/11/23 00:37:25, piman wrote:
> On 2016/11/19 18:25:52, liberato wrote:
> > this is where i picture sending the message to the GLStreamTextureImage via
> > gl_->SomeCustomCommandCHROMIUM().
> > 
> > i tried to encapsulate this inside the overlay logic also, but it was a
little
> > weird having to lock the resource, send in the context, etc.   getting all
of
> > the 'is / is not promotable' logic into the overlay processor seemed like
the
> > key idea from our mtg, so i didn't worry too much about moving notification
> > elsewhere.
> > 
> > please let me know if you'd prefer this to be moved.
> 
> I guess it feels a bit odd to do here...
> How about:
> - ResourceProvider::SetPromotionHint(ResourceId resource, bool promote) which
> internally can do the GL call (which should be straightforward after a few
> DCHECKs, don't necessarily need the whole ScopedReadLockGL)
> - pass the ResourceProvider to the
> OverlayCandidateValidator::CheckOverlaySupport and have it issue the hints on
> the resources you want to promote

i'll try to do that.

the trick is that OverlayCandidateValidator::CheckOverlaySupport doesn't know
what the correct hint is for the resource by the time it returns.  It has to
wait until all resources are processed to see if any are promoted, since we'll
promote at most one.  promotable resources become unpromotable if some other
resource is actually promoted.

i really don't like having CheckOverlaySupport have this stateful set of
resources that i added.  i'll try to get rid of that too, since it really goes
with the candidate list.

https://codereview.chromium.org/2508203004/diff/1/components/display_composit...
File
components/display_compositor/compositor_overlay_candidate_validator_android.cc
(right):

https://codereview.chromium.org/2508203004/diff/1/components/display_composit...
components/display_compositor/compositor_overlay_candidate_validator_android.cc:40:
// TODO(liberato): maybe we should notify by resource, not by quad.  while
On 2016/11/23 00:37:25, piman wrote:
> On 2016/11/19 18:25:52, liberato wrote:
> > this comment block is a little stale, sorry.  we now do report per-resource
> via
> > IsResourcePromotable.  It does not check if multiple quads refer to the same
> > resource, but that seems to be not too important.
> 
> As long as we gracefully handle the case, it's ok I think - keeping in mind
that
> we can't trust the renderer not to send 2 quads referring to the same
resource.

Acknowledged.

https://codereview.chromium.org/2508203004/diff/20001/cc/ipc/cc_param_traits_...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2508203004/diff/20001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_macros.h:146:
IPC_STRUCT_TRAITS_MEMBER(is_overlay_candidate)
On 2016/11/23 00:37:25, piman wrote:
> nit: is_overlay_candidate is already on l.141

Done.

https://codereview.chromium.org/2508203004/diff/20001/cc/output/overlay_candi...
File cc/output/overlay_candidate_validator.h (right):

https://codereview.chromium.org/2508203004/diff/20001/cc/output/overlay_candi...
cc/output/overlay_candidate_validator.h:40: virtual bool
IsResourcePromotable(ResourceId resource_id);
On 2016/11/23 00:37:25, piman wrote:
> Super nit: in the context of OverlayCandidateValidator, IsResourcePromotable
> suggests "this resource can be put in an overlay" as in "right now", whereas I
> think you mean "this resource would be ok to put in an overlay, but only if
it's
> switched to SurfaceView, but it isn't right now". Not sure what is the best
name
> to convey the concept...

i've since moved the set into OverlayCandidateList, since it was a little weird
to have this statefulness here.

named the set 'promotable_resource_hints_'

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

https://codereview.chromium.org/2508203004/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1558: resource->is_backed_by_surface_texture =
it->is_backed_by_surface_texture;
On 2016/11/23 00:37:25, piman wrote:
> note, however we will use this, we can't trust that the renderer gave us the
> correct value. If it didn't, we don't have to display properly, but we can't
> crash. I think at the end of the day it means that we need to handle the fact
> that we may end up calling ScheduleOverlayPlane with a SurfaceTexture, or try
to
> draw with a SurfaceView, and we just need to maybe ignore the
> ScheduleOverlayPlane in the first case and draw with a default texture in the
> second case.

currently, the renderer can always mark the mailbox as an overlay candidate,
whether it is backed by ST or SV or anything else.  i think that it's handled
properly now, and that the behavior is unchanged here.

for AVDA in particular, there's still a texture + AVDACodecImage that
understands whether an overlay is okay or not.  attempting to draw a SV will use
an external texture with no glimage attached.  promoting a ST will return false
from ScheduleOverlayPlane.

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

https://codereview.chromium.org/2508203004/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1674: // TODO(liberato): I'm unsure why we can
assume that |gl_id| is set yet
i did this based on previous CL feedback, but i'm unsure i understood correctly
after looking at ScopedReadLockGL.

[piman, 2016-12-01 21:55:25]

Yeah, the approach looks fine. Some comments, but nothing major.

https://codereview.chromium.org/2508203004/diff/80001/cc/ipc/cc_param_traits_...
File cc/ipc/cc_param_traits_unittest.cc (right):

https://codereview.chromium.org/2508203004/diff/80001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_unittest.cc:211:
EXPECT_EQ(a.is_backed_by_surface_texture, b.is_backed_by_surface_texture);
nit (here and below): also add wants_promotion_hint
You'll need to put them in #if defined(OS_ANDROID)

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
cc/output/overlay_candidate.cc:325:
promotable_resource_hints_(other.promotable_resource_hints_) {}
nit: '= default' should be good.

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
File cc/output/overlay_candidate.h (right):

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
cc/output/overlay_candidate.h:110: OverlayCandidateList(const
OverlayCandidateList&);
nit: rule-of-3? (i.e. add a copy-assignment operator)
maybe make it moveable too (i.e rule-of-5, add move constructor and
move-assignment operator)?

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

https://codereview.chromium.org/2508203004/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1664: for (const auto& it : resources_) {
This sounds mildly expensive to go over all resources (including tiles etc.). If
it is, maybe we can keep the wants_promotion_hint resources into a separate set.

https://codereview.chromium.org/2508203004/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1675: // without using a read lock.
I don't think you can, actually, gl_id is set (and the mailbox bound) on the
first LockForRead.
I think you want to Lock/UnlockForRead() around the GL call.

https://codereview.chromium.org/2508203004/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1677: //
gl->SetStreamTexturePromotionHintCHROMIUM(resource.gl_id,
Do you expect we'll be doing a lot of these every frame? If so, I would
recommend adding a "multi" call where you would pass an array of gl textures.

[liberato (no reviews please), 2016-12-02 20:56:14]

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

[commit-bot: I haz the power, 2016-12-02 20:57:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 21:10:25]

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

[commit-bot: I haz the power, 2016-12-02 21:10:26]

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

[liberato (no reviews please), 2016-12-03 20:48:30]

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

[commit-bot: I haz the power, 2016-12-03 20:49:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 21:02:03]

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

[commit-bot: I haz the power, 2016-12-03 21:02:04]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[liberato (no reviews please), 2016-12-04 05:31:24]

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

[commit-bot: I haz the power, 2016-12-04 05:31:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-04 05:46:46]

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

[commit-bot: I haz the power, 2016-12-04 05:46:47]

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

[liberato (no reviews please), 2016-12-04 06:09:08]

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

[commit-bot: I haz the power, 2016-12-04 06:09:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-04 06:34:56]

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

[commit-bot: I haz the power, 2016-12-04 06:34:57]

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

[liberato (no reviews please), 2016-12-05 18:07:46]

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

[commit-bot: I haz the power, 2016-12-05 18:08:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2016-12-05 18:14:01]

added a new unit test for |ResourceProvider::wants_promotion_hints_set_|, which
is the cache of resources that would like hints.

to keep this CL more manageable, i'd prefer not to include the new GL command /
GLStreamTextureImage changes.  GL boilerplate is kinda big.  that would,
unfortunately, leave this code unused except for unit tests.  WDYT?

i can pull out the media changes too, if you like, but they're pretty small.  my
current plan to revert android_video_decode_accelerator only, so that it doesn't
request a hint, but leave the flags otherwise intact in this CL.

thanks
-fl

https://codereview.chromium.org/2508203004/diff/80001/cc/ipc/cc_param_traits_...
File cc/ipc/cc_param_traits_unittest.cc (right):

https://codereview.chromium.org/2508203004/diff/80001/cc/ipc/cc_param_traits_...
cc/ipc/cc_param_traits_unittest.cc:211:
EXPECT_EQ(a.is_backed_by_surface_texture, b.is_backed_by_surface_texture);
On 2016/12/01 21:55:25, piman wrote:
> nit (here and below): also add wants_promotion_hint
> You'll need to put them in #if defined(OS_ANDROID)

Done.

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
cc/output/overlay_candidate.cc:325:
promotable_resource_hints_(other.promotable_resource_hints_) {}
On 2016/12/01 21:55:25, piman wrote:
> nit: '= default' should be good.

Done.

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
File cc/output/overlay_candidate.h (right):

https://codereview.chromium.org/2508203004/diff/80001/cc/output/overlay_candi...
cc/output/overlay_candidate.h:110: OverlayCandidateList(const
OverlayCandidateList&);
On 2016/12/01 21:55:25, piman wrote:
> nit: rule-of-3? (i.e. add a copy-assignment operator)
> maybe make it moveable too (i.e rule-of-5, add move constructor and
> move-assignment operator)?

Done.

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

https://codereview.chromium.org/2508203004/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1664: for (const auto& it : resources_) {
On 2016/12/01 21:55:25, piman wrote:
> This sounds mildly expensive to go over all resources (including tiles etc.).
If
> it is, maybe we can keep the wants_promotion_hint resources into a separate
set.

Done.

https://codereview.chromium.org/2508203004/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1675: // without using a read lock.
On 2016/12/01 21:55:25, piman wrote:
> I don't think you can, actually, gl_id is set (and the mailbox bound) on the
> first LockForRead.
> I think you want to Lock/UnlockForRead() around the GL call.

Done.

https://codereview.chromium.org/2508203004/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1677: //
gl->SetStreamTexturePromotionHintCHROMIUM(resource.gl_id,
On 2016/12/01 21:55:25, piman wrote:
> Do you expect we'll be doing a lot of these every frame? If so, I would
> recommend adding a "multi" call where you would pass an array of gl textures.

good question.  it'll be one per video element that gets to the compositor,
which think requires it to be on screen or near it.  at least, i see hints stop
flowing by when i scroll an element off screen.

so, it's probably not too many.

[piman, 2016-12-05 20:19:06]

LGTM + couple nits
Let's land this and do the GL bits right afterwards.

https://codereview.chromium.org/2508203004/diff/180001/cc/output/overlay_cand...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2508203004/diff/180001/cc/output/overlay_cand...
cc/output/overlay_candidate.cc:333: const OverlayCandidateList& other) {
nit: here and below you can do '= default' as well!

https://codereview.chromium.org/2508203004/diff/180001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2508203004/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1681: for (const auto& it :
wants_promotion_hints_set_) {
nit: "id" or "resource_id" instead of "it"? This is not an iterator...

https://codereview.chromium.org/2508203004/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1690: // promtable_hints.count(it->first) > 0
.
nit: add a TODO(liberato), maybe link to a bug?

[piman, 2016-12-05 20:20:31]

Oh, also, could you fix the CL description?

[liberato (no reviews please), 2016-12-05 21:08:22]

Description was changed from

==========
first pass at feedback from overlay processor => GLImage

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Overlay promotion on android is tricky, in that one must know in advance whether
to use a SurfaceView for the overlay or not.  If the overlay turns out to be
skipped for promotion by chrome's overlay processor, then it simply won't be
visible.

This CL adds functionality for a resource to request a hint about whether it
would be promoted or not, even though it's not backed by a SurfaceView.  This
allows the producer of the resource to decide whether SurfaceView is appropriate
or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes to promote
to SV as much as possible for power savings.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[liberato (no reviews please), 2016-12-05 21:08:22]

liberato@chromium.org changed reviewers:
+ watk@chromium.org

[liberato (no reviews please), 2016-12-05 21:09:28]

Description was changed from

==========
Overlay promotion on android is tricky, in that one must know in advance whether
to use a SurfaceView for the overlay or not.  If the overlay turns out to be
skipped for promotion by chrome's overlay processor, then it simply won't be
visible.

This CL adds functionality for a resource to request a hint about whether it
would be promoted or not, even though it's not backed by a SurfaceView.  This
allows the producer of the resource to decide whether SurfaceView is appropriate
or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes to promote
to SV as much as possible for power savings.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[liberato (no reviews please), 2016-12-05 21:16:45]

Description was changed from

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=671354
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[liberato (no reviews please), 2016-12-05 21:50:46]

liberato@chromium.org changed reviewers:
+ dcheng@chromium.org

[liberato (no reviews please), 2016-12-05 21:50:48]

piman: thanks for the feedback!

watk: PTAL at media bits.  TL;DR: just plumbs some extra flags that will be set
in Picture by AVDA to the CC resource in VideoResourceUpdater.  the flags aren't
set by AVDA this CL.  PS11 has the code that should go there in a separate CL.

dcheng: please review cc/ipc and media/gpu/ipc changes.

thanks
-fl

https://codereview.chromium.org/2508203004/diff/180001/cc/output/overlay_cand...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2508203004/diff/180001/cc/output/overlay_cand...
cc/output/overlay_candidate.cc:333: const OverlayCandidateList& other) {
On 2016/12/05 20:19:06, piman wrote:
> nit: here and below you can do '= default' as well!

Done.

https://codereview.chromium.org/2508203004/diff/180001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2508203004/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1681: for (const auto& it :
wants_promotion_hints_set_) {
On 2016/12/05 20:19:06, piman wrote:
> nit: "id" or "resource_id" instead of "it"? This is not an iterator...

Done.

https://codereview.chromium.org/2508203004/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1690: // promtable_hints.count(it->first) > 0
.
On 2016/12/05 20:19:06, piman wrote:
> nit: add a TODO(liberato), maybe link to a bug?

Done.

[watk, 2016-12-06 23:36:18]

media/ lgtm

[dcheng, 2016-12-07 03:34:50]

ipc lgtm (also leta => let?)

https://codereview.chromium.org/2508203004/diff/220001/cc/ipc/transferable_re...
File cc/ipc/transferable_resource_struct_traits.h (right):

https://codereview.chromium.org/2508203004/diff/220001/cc/ipc/transferable_re...
cc/ipc/transferable_resource_struct_traits.h:54: // TransferableResource has
this in an #ifdef, but mojo doesn't leta us.
Mind marking this with TODO(https://crbug.com/671901) (and the below one as
well)?

[liberato (no reviews please), 2016-12-07 17:38:29]

thanks for the help, everybody!

-fl

https://codereview.chromium.org/2508203004/diff/220001/cc/ipc/transferable_re...
File cc/ipc/transferable_resource_struct_traits.h (right):

https://codereview.chromium.org/2508203004/diff/220001/cc/ipc/transferable_re...
cc/ipc/transferable_resource_struct_traits.h:54: // TransferableResource has
this in an #ifdef, but mojo doesn't leta us.
On 2016/12/07 03:34:50, dcheng wrote:
> Mind marking this with TODO(https://crbug.com/671901) (and the below one as
> well)?

done, + leta.

[liberato (no reviews please), 2016-12-07 17:38:37]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-12-07 17:38:38]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
watk@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2508203004/#ps240001
(title: "added TODO, fixed comments")

[commit-bot: I haz the power, 2016-12-07 17:39:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 20:13:33]

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

[commit-bot: I haz the power, 2016-12-07 20:13:37]

Failed to apply patch for cc/ipc/cc_param_traits_unittest.cc:
While running git apply --index -p1;
  error: patch failed: cc/ipc/cc_param_traits_unittest.cc:581
  error: cc/ipc/cc_param_traits_unittest.cc: patch does not apply

Patch:       cc/ipc/cc_param_traits_unittest.cc
Index: cc/ipc/cc_param_traits_unittest.cc
diff --git a/cc/ipc/cc_param_traits_unittest.cc
b/cc/ipc/cc_param_traits_unittest.cc
index
1b663fdc2912b6c4f1f232eee820954fb7535f98..ebd698ff235f75d94a65510d92162f37033b121e
100644
--- a/cc/ipc/cc_param_traits_unittest.cc
+++ b/cc/ipc/cc_param_traits_unittest.cc
@@ -208,6 +208,10 @@ class CCParamTraitsTest : public testing::Test {
     EXPECT_EQ(a.mailbox_holder.texture_target,
b.mailbox_holder.texture_target);
     EXPECT_EQ(a.mailbox_holder.sync_token, b.mailbox_holder.sync_token);
     EXPECT_EQ(a.is_overlay_candidate, b.is_overlay_candidate);
+#if defined(OS_ANDROID)
+    EXPECT_EQ(a.is_backed_by_surface_texture, b.is_backed_by_surface_texture);
+    EXPECT_EQ(a.wants_promotion_hint, b.wants_promotion_hint);
+#endif
   }
 };
 
@@ -571,6 +575,10 @@ TEST_F(CCParamTraitsTest, Resources) {
   arbitrary_resource1.mailbox_holder.texture_target = GL_TEXTURE_2D;
   arbitrary_resource1.mailbox_holder.sync_token = arbitrary_token1;
   arbitrary_resource1.is_overlay_candidate = true;
+#if defined(OS_ANDROID)
+  arbitrary_resource1.is_backed_by_surface_texture = true;
+  arbitrary_resource1.wants_promotion_hint = true;
+#endif
 
   TransferableResource arbitrary_resource2;
   arbitrary_resource2.id = 789132;
@@ -581,6 +589,10 @@ TEST_F(CCParamTraitsTest, Resources) {
   arbitrary_resource2.mailbox_holder.texture_target = GL_TEXTURE_EXTERNAL_OES;
   arbitrary_resource2.mailbox_holder.sync_token = arbitrary_token2;
   arbitrary_resource2.is_overlay_candidate = false;
+#if defined(OS_ANDROID)
+  arbitrary_resource2.is_backed_by_surface_texture = false;
+  arbitrary_resource2.wants_promotion_hint = false;
+#endif
 
   std::unique_ptr<RenderPass> renderpass_in = RenderPass::Create();
   renderpass_in->SetNew(RenderPassId(1, 1), gfx::Rect(), gfx::Rect(),

[liberato (no reviews please), 2016-12-07 21:12:05]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-12-07 21:12:05]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2508203004/#ps260001
(title: "rebased")

[commit-bot: I haz the power, 2016-12-07 21:12:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 23:03:11]

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

[commit-bot: I haz the power, 2016-12-07 23:03:12]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[liberato (no reviews please), 2016-12-08 16:57:54]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-12-08 16:58:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 18:19:48]

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

[commit-bot: I haz the power, 2016-12-08 18:19:50]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[liberato (no reviews please), 2016-12-08 23:17:59]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:18:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 23:36:39]

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

[commit-bot: I haz the power, 2016-12-08 23:36:41]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[liberato (no reviews please), 2016-12-09 05:59:55]

On 2016/12/08 23:36:41, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
>   mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

this is a compile error in blink that has nothing to do with this CL.  earlier,
there were problems on the 5X that look to be unrelated (couldn't repro locally,
and by inspection didn't seem related to this change at all).

[liberato (no reviews please), 2016-12-09 06:00:07]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-12-09 06:00:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 06:47:19]

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

[commit-bot: I haz the power, 2016-12-09 06:47:20]

Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[liberato (no reviews please), 2016-12-09 19:20:20]

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

[commit-bot: I haz the power, 2016-12-09 19:20:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 20:04:24]

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

[commit-bot: I haz the power, 2016-12-09 20:04:26]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2016-12-09 20:09:10]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-12-09 20:10:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 20:55:32]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481314150553960,
"parent_rev": "405f9cf62d69f49572d470ab14b5af3c07ced45e", "commit_rev":
"73387076f2eca1caaefbe5f98e99563e59d3ec2b"}

[commit-bot: I haz the power, 2016-12-09 20:56:08]

Description was changed from

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=671354
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=671354
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2508203004
==========

[commit-bot: I haz the power, 2016-12-09 20:56:09]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-12-12 14:40:29]

Description was changed from

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=671354
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2508203004
==========

to

==========
Overlay promotion on android is tricky, in that one must know in
advance whether to use a SurfaceView for the overlay or not.  If the
overlay turns out to be skipped for promotion by chrome's overlay
processor, then it simply won't be visible.

This CL adds functionality for a resource to request a hint about
whether it would be promoted or not, even though it's not backed by
a SurfaceView.  This allows the producer of the resource to decide
whether SurfaceView is appropriate or not.

Initial use case will be AndroidVideoDecodeAccelerator, which likes
to promote to SV as much as possible for power savings.

BUG=671354
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/304dc19797fefea519996ef33919b61513716af8
Cr-Commit-Position: refs/heads/master@{#437643}
==========

[commit-bot: I haz the power, 2016-12-12 14:40:31]

Patchset 14 (id:??) landed as
https://crrev.com/304dc19797fefea519996ef33919b61513716af8
Cr-Commit-Position: refs/heads/master@{#437643}