Position overlays in AVDACodecImage

Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2889603005
Cr-Commit-Position: refs/heads/master@{#472590}
Committed: https://chromium.googlesource.com/chromium/src/+/95c9282a9c8b5773f4f25f78fe829479d40304fa

[liberato (no reviews please), 2017-05-16 18:22:37]

Description was changed from

==========
Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
==========

to

==========
Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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), 2017-05-16 18:22:53]

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

[commit-bot: I haz the power, 2017-05-16 18:24:15]

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), 2017-05-16 18:29:17]

Description was changed from

==========
Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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), 2017-05-16 18:29:17]

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

[liberato (no reviews please), 2017-05-16 18:31:32]

this is the follow-up CL that i mentioned.  it makes the surface bundle
available to AVDACodecImage via AVDASharedState.  ScheduleOverlayPlane calls
ScheduleLayout, which is a no-op for CVV.

the shared state will keep the overlay around (unless it receives
OnSurfaceDestroyed), so the overlay can outlast the codec if a VideoFrame is
still using it.  of course, CVV gets destroyed when exiting fullscreen, but
DialogSurface won't.

thanks
-fl

[commit-bot: I haz the power, 2017-05-16 19:44:26]

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

[commit-bot: I haz the power, 2017-05-16 19:44:27]

Dry run: Try jobs failed on following builders:
  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), 2017-05-16 20:09:23]

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

[commit-bot: I haz the power, 2017-05-16 20:10:26]

Dry run: CQ is trying da patch.

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

[tguilbert, 2017-05-16 20:41:59]

LGTM

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state.cc
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state...
media/gpu/avda_shared_state.cc:143: surface_texture()->UpdateTexImage();
Is there ever a risk of a nullptr deref?

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state...
media/gpu/avda_shared_state.cc:153: if (surface_bundle_ &&
surface_bundle_->overlay.get() == overlay)
Equivalent to overlay() == overlay?

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:80: on_frame_available_handler_ =
Curiosity: Do you know why this is a scoped ptr and not a unique ptr?

[liberato (no reviews please), 2017-05-16 20:59:53]

thanks for the feedback.
-fl

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state.cc
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state...
media/gpu/avda_shared_state.cc:143: surface_texture()->UpdateTexImage();
On 2017/05/16 20:41:59, tguilbert wrote:
> Is there ever a risk of a nullptr deref?

shouldn't be.  i think the only path here (UpdateSurfaceInternal) already
dchecks if it's told to UpdateTexImage without a surface texture.

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state...
media/gpu/avda_shared_state.cc:153: if (surface_bundle_ &&
surface_bundle_->overlay.get() == overlay)
On 2017/05/16 20:41:59, tguilbert wrote:
> Equivalent to overlay() == overlay?

was my first choice too.  didn't compile, with "no == for unique_ptr<> vs
AndroidOverlay*" .  weird that there's no operator== for it.

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:80: on_frame_available_handler_ =
On 2017/05/16 20:41:59, tguilbert wrote:
> Curiosity: Do you know why this is a scoped ptr and not a unique ptr?

both |this| and the callback own a reference to it.  |this| uses the reference
to be able to turn it off later.

one might be able to do something with a wp to the shared state, but would be
tricky since the callbacks happen on random threads.

[commit-bot: I haz the power, 2017-05-16 23:14:32]

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

[commit-bot: I haz the power, 2017-05-16 23:14:33]

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

[watk, 2017-05-17 19:45:14]

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.cc:77: shared_state_ = nullptr;
delete now?

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.cc:92: // Only do this one the surface
texture is filled in, since the constructor
s/one/once

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.h:58: // Returns true on success, false
otherwise.
nit: Reminds me of this from the styleguide :P

However, do not be unnecessarily verbose or state the completely obvious. Notice
below that it is not necessary to say "returns false otherwise" because this is
implied.

// Returns true if the table cannot hold any more entries.
bool IsTableFull();

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:86: if (surface_bundle_ &&
surface_bundle_->overlay) {
surface_bundle_ can't be null here right?

if (overlay()) {
  overlay()->AddSurfaceDestro...

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:93: if (!surface_texture())
this is bizarre looking, haha

We should have written:

if (on_frame_available_handler_)
  on_frame_available_handler_->ClearListener();

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:153: if (surface_bundle_ &&
surface_bundle_->overlay.get() == overlay)
if (overlay() == overlay)?

[watk, 2017-05-17 19:46:40]

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state.cc
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/1/media/gpu/avda_shared_state...
media/gpu/avda_shared_state.cc:153: if (surface_bundle_ &&
surface_bundle_->overlay.get() == overlay)
On 2017/05/16 20:59:52, liberato wrote:
> On 2017/05/16 20:41:59, tguilbert wrote:
> > Equivalent to overlay() == overlay?
> 
> was my first choice too.  didn't compile, with "no == for unique_ptr<> vs
> AndroidOverlay*" .  weird that there's no operator== for it.

Oh, I commented on this as well.

overlay() == overlay is comparing two AndroidOverlay*'s. Are you thinking of
something else?

[liberato (no reviews please), 2017-05-17 20:19:33]

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.cc:77: shared_state_ = nullptr;
On 2017/05/17 19:45:14, watk wrote:
> delete now?

can't, in case of the early out @85.

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.cc:92: // Only do this one the surface
texture is filled in, since the constructor
On 2017/05/17 19:45:14, watk wrote:
> s/one/once

Done.

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.h:58: // Returns true on success, false
otherwise.
On 2017/05/17 19:45:14, watk wrote:
> nit: Reminds me of this from the styleguide :P
> 
> However, do not be unnecessarily verbose or state the completely obvious.
Notice
> below that it is not necessary to say "returns false otherwise" because this
is
> implied.
> 
> // Returns true if the table cannot hold any more entries.
> bool IsTableFull();

done, though i'm not sure that i agree with the style guide's use of the word
"implied".

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:86: if (surface_bundle_ &&
surface_bundle_->overlay) {
On 2017/05/17 19:45:14, watk wrote:
> surface_bundle_ can't be null here right?
> 
> if (overlay()) {
>   overlay()->AddSurfaceDestro...

good point, done.

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:93: if (!surface_texture())
On 2017/05/17 19:45:14, watk wrote:
> this is bizarre looking, haha
> 
> We should have written:
> 
> if (on_frame_available_handler_)
>   on_frame_available_handler_->ClearListener();

good point!

https://codereview.chromium.org/2889603005/diff/20001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:153: if (surface_bundle_ &&
surface_bundle_->overlay.get() == overlay)
On 2017/05/17 19:45:14, watk wrote:
> if (overlay() == overlay)?

oooohhhhh.  done.  yes, i originally just compared 'surface_bundle_->overlay ==
overlay'.

[liberato (no reviews please), 2017-05-17 20:19:40]

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

[commit-bot: I haz the power, 2017-05-17 20:20:56]

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), 2017-05-17 20:23:30]

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

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

Dry run: CQ is trying da patch.

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

[watk, 2017-05-17 20:35:58]

nice lgtm

[commit-bot: I haz the power, 2017-05-17 22:06:36]

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

[commit-bot: I haz the power, 2017-05-17 22:06:37]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-17 22:18:55]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-17 22:18:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2889603005/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2017-05-17 22:20: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, 2017-05-17 22:36:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495059535211900,
"parent_rev": "6a1792097d320077386c51680171d0fd66108072", "commit_rev":
"95c9282a9c8b5773f4f25f78fe829479d40304fa"}

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

Description was changed from

==========
Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Position overlays in AVDACodecImage

This CL adds a call to ScheduleLayout from AVDACodecImage.

To get the overlay, AVDASharedState now holds a reference to the
surface bundle.  It registers for DestroyedCB notification from the
overlay, and drops the reference if this happens.  This will have
the side-effect of keeping the overlay around even if the decoder
is destroyed.  For CVV, that's a no-op, since leaving fullscreen
destroys the surface anyway, and the decoder is only destroyed when
not in fullscreen.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2889603005
Cr-Commit-Position: refs/heads/master@{#472590}
Committed:
https://chromium.googlesource.com/chromium/src/+/95c9282a9c8b5773f4f25f78fe82...
==========

[commit-bot: I haz the power, 2017-05-17 22:36:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/95c9282a9c8b5773f4f25f78fe82...