Add ContentVideoViewOverlay to AVDA.

Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950
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/2692863011
Cr-Commit-Position: refs/heads/master@{#456135}
Committed: https://chromium.googlesource.com/chromium/src/+/c58e8f82526098f4ef3dca5f957a46c439540a59

[liberato (no reviews please), 2017-02-16 22:12:29]

Description was changed from

==========
include order


cl format


compiles


trying to figure out destruction path


started cvv overlay


switched to WAITING_FOR_SURFACE


removed incorrect dchecks


Unify UpdateSurface with initial surface creation in AVDA.

AVDA::UpdateSurface now uses StartSurfaceCreation (etc.) to switch
to the incoming surface.  While it still does not support surfaces
which are not able to be allocated immediately, it does reduce some
duplicated code.

BUG=

removed a comment


added BEFORE_SURFACE_ALLOC
==========

to

==========
include order


cl format


compiles


trying to figure out destruction path


started cvv overlay


switched to WAITING_FOR_SURFACE


removed incorrect dchecks


Unify UpdateSurface with initial surface creation in AVDA.

AVDA::UpdateSurface now uses StartSurfaceCreation (etc.) to switch
to the incoming surface.  While it still does not support surfaces
which are not able to be allocated immediately, it does reduce some
duplicated code.

BUG=

removed a comment


added BEFORE_SURFACE_ALLOC
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), 2017-02-17 01:17:30]

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

[commit-bot: I haz the power, 2017-02-17 01:19:05]

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, 2017-02-17 01:56:48]

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

[commit-bot: I haz the power, 2017-02-17 01:56:49]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[liberato (no reviews please), 2017-02-17 05:29:07]

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

[commit-bot: I haz the power, 2017-02-17 05:31:18]

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, 2017-02-17 08:24:58]

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

[commit-bot: I haz the power, 2017-02-17 08:24:59]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-02-17 18:31:06]

Description was changed from

==========
include order


cl format


compiles


trying to figure out destruction path


started cvv overlay


switched to WAITING_FOR_SURFACE


removed incorrect dchecks


Unify UpdateSurface with initial surface creation in AVDA.

AVDA::UpdateSurface now uses StartSurfaceCreation (etc.) to switch
to the incoming surface.  While it still does not support surfaces
which are not able to be allocated immediately, it does reduce some
duplicated code.

BUG=

removed a comment


added BEFORE_SURFACE_ALLOC
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

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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), 2017-02-17 18:31:52]

Description was changed from

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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), 2017-02-17 18:31:52]

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

[liberato (no reviews please), 2017-02-17 18:33:23]

Description was changed from

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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

==========
Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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), 2017-02-17 18:33:48]

Description was changed from

==========
Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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), 2017-02-17 18:34:38]

as far as i can tell, it does everything exactly as before.  just reorganizes it
a little.

i had a few questions about issues that might exist with or without this change.
 curious what you think about them.

thanks
-fl

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:370: // Note that we don't enforce
that for any SurfaceTexture or its Surface,
watk: do you agree with this reasoning?  i believe it was our old behavior --
PictureBufferManager::Initialize did it before.  we now do it at the same place
in InitializePictureBufferManager.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:439: // Note that we might want to
do this after SetSurface, or on return, or
hrm, this also exists in the current code.  i'm pretty sure that it's okay,
since we're not releasing any buffers.

[watk, 2017-02-17 22:40:36]

Looks pretty good. Hopefully it will help us simplify this for MCVD because it's
beyond me at the moment.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
File media/base/android/android_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.cc:13: AndroidOverlay::AndroidOverlay() {}
can write "= default" here too

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
File media/base/android/android_overlay.h (right):

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:21: // When SurfaceDestroyed arrives, you
should stop using the Android Surface and
The contract about what to do when SurfaceDestroyed is called is unclear. Also
s/SurfaceDestroyed arrives/DestroyedCallback is called/.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:28: using ReadyCallback =
base::Callback<void()>;
Use the conventional "cb": ReadyCb?

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:42: // TODO(liberato): add format, etc.
here.
TODO doesn't seem necessary because we'll add things when we need to.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:50: // Schedule a relayout of this overlay.
 If called before the client is
nit: here and elsewhere comments style is not in line with the style guide. This
part: 

These comments should be descriptive ("Opens the file") rather than imperative
("Open the file"); the comment describes the function, it does not tell the
function what to do.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:54: // Return the Java Surface object.
nit: Don't think it needs a comment

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:64: void
SetJavaSurface(gl::ScopedJavaSurface);
Doesn't seem necessary if subclasses can set surface_ themselves? You can just
say j_surface() is only valid after ReadyCb is called.

The closer this is to a pure virtual class, the better IMO.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:370: // Note that we don't enforce
that for any SurfaceTexture or its Surface,
On 2017/02/17 18:34:38, liberato wrote:
> watk: do you agree with this reasoning?  i believe it was our old behavior --
> PictureBufferManager::Initialize did it before.  we now do it at the same
place
> in InitializePictureBufferManager.

I don't know to be honest. I'm pretty confused :(

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:388: // Create the overlay.  Note
that it will never call us back immedaitely.
immediately

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:418: // TODO(liberato): We could
short-circuit the AndroidOverlay callback to call
Remove TODO? I don't think we need a comment here but up to you.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:439: // Note that we might want to
do this after SetSurface, or on return, or
On 2017/02/17 18:34:38, liberato wrote:
> hrm, this also exists in the current code.  i'm pretty sure that it's okay,
> since we're not releasing any buffers.

Yeah +1 to already exists.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:443:
codec_config_->surface_texture = nullptr;
This seems broken? Before this change. I think it was introduced by the
StartSurfaceCreation change. 

This releases the ST before SetSurface succeeded. So the codec has a dead
surface?

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.cc:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.cc:42: // TODO(liberato): should we have a
separate cb for this?
Delete TODO? Or is it just for the code review?

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay.h (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.h:2: // Use of this source code is governed
by a BSD-style license that can be
no (c)

[liberato (no reviews please), 2017-03-07 19:41:26]

Description was changed from

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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-03-07 21:30:25]

amazingly, this still plays video.  it's a simpler change now that surface
bundle handles most of the cleanup.

i'm still testing this, but wanted to push out my replies and most recent code.

-fl

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
File media/base/android/android_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.cc:13: AndroidOverlay::AndroidOverlay() {}
On 2017/02/17 22:40:35, watk wrote:
> can write "= default" here too

true.  these are moved to the header anyway now that |surface_| isn't needed.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
File media/base/android/android_overlay.h (right):

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:21: // When SurfaceDestroyed arrives, you
should stop using the Android Surface and
On 2017/02/17 22:40:36, watk wrote:
> The contract about what to do when SurfaceDestroyed is called is unclear. Also
> s/SurfaceDestroyed arrives/DestroyedCallback is called/.

Done.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:28: using ReadyCallback =
base::Callback<void()>;
On 2017/02/17 22:40:36, watk wrote:
> Use the conventional "cb": ReadyCb?

done, though i used "CB" since it seems more common than "Cb"

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:42: // TODO(liberato): add format, etc.
here.
On 2017/02/17 22:40:36, watk wrote:
> TODO doesn't seem necessary because we'll add things when we need to.

on a CL a while ago, an objection was raised that we coujld replace Config with
just gfx::Rect.  now that we have callbacks, i guess i don't need it anymore.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:50: // Schedule a relayout of this overlay.
 If called before the client is
On 2017/02/17 22:40:36, watk wrote:
> nit: here and elsewhere comments style is not in line with the style guide.
This
> part: 
> 
> These comments should be descriptive ("Opens the file") rather than imperative
> ("Open the file"); the comment describes the function, it does not tell the
> function what to do.

nit++ :)  Done.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:54: // Return the Java Surface object.
On 2017/02/17 22:40:36, watk wrote:
> nit: Don't think it needs a comment

i agree that it adds no value as written.  changed it to reiterate when one may
call it, rather than what it does.

https://codereview.chromium.org/2692863011/diff/100001/media/base/android/and...
media/base/android/android_overlay.h:64: void
SetJavaSurface(gl::ScopedJavaSurface);
On 2017/02/17 22:40:36, watk wrote:
> Doesn't seem necessary if subclasses can set surface_ themselves? You can just
> say j_surface() is only valid after ReadyCb is called.
> 
> The closer this is to a pure virtual class, the better IMO.

true.  i'll just make j_surface() abstract and make it entirely the subclass's
problem.  i agree about pure virtual is better.  will rename from j_surface()
for style guide.  wanted to keep it the same as ScopedJavaSurface, but really
not to many folks will need ScopedJavaSurface directly if this works.

that will let me inline the AndroidOverlay *structors, too.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:370: // Note that we don't enforce
that for any SurfaceTexture or its Surface,
On 2017/02/17 22:40:36, watk wrote:
> On 2017/02/17 18:34:38, liberato wrote:
> > watk: do you agree with this reasoning?  i believe it was our old behavior
--
> > PictureBufferManager::Initialize did it before.  we now do it at the same
> place
> > in InitializePictureBufferManager.
> 
> I don't know to be honest. I'm pretty confused :(

i think that this is now okay, since surface bundles handle it.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:388: // Create the overlay.  Note
that it will never call us back immedaitely.
On 2017/02/17 22:40:36, watk wrote:
> immediately

Done.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:418: // TODO(liberato): We could
short-circuit the AndroidOverlay callback to call
On 2017/02/17 22:40:36, watk wrote:
> Remove TODO? I don't think we need a comment here but up to you.

Done.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:439: // Note that we might want to
do this after SetSurface, or on return, or
On 2017/02/17 22:40:36, watk wrote:
> On 2017/02/17 18:34:38, liberato wrote:
> > hrm, this also exists in the current code.  i'm pretty sure that it's okay,
> > since we're not releasing any buffers.
> 
> Yeah +1 to already exists.

surface bundle should fix this.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:443:
codec_config_->surface_texture = nullptr;
On 2017/02/17 22:40:36, watk wrote:
> This seems broken? Before this change. I think it was introduced by the
> StartSurfaceCreation change. 
> 
> This releases the ST before SetSurface succeeded. So the codec has a dead
> surface?

it did this before, too.  it used to be that AVDA::UpdateSurface would call
PBM::Initialize, then call MCB::SetSurface.

ah, but wait -- in both cases, the outgoing |shared_state_| still has a ref to
it.  that's why it's okay -- we won't drop the ST until all the picture buffers
are re-issued and the last ref to |shared_state_| is dropped.  wow, that's
hacky.  surface owners are so much better at this -- MCVD really, really needs
to use them.

however, |surface_texture_surface| is now broken.  it will auto-release when we
drop it here.

i think that this bug was introduced by me in the Unify CL.  AVDA:UpdateSurface
used to use |new_surface| to hold the new surface, and only std::move() it to
|surface| on success.  we should similarly preserve |surface_texture_surface|
here.  i won't preserve surface_texture, since we don't want it to outlive the
outgoing |shared_state_|.  |shared_state_| will delete the GL objects during
destruction.

note that for overlay surfaces, it doesn't matter, since the surface won't
auto-release anyway.  hacky, but if we're switching to a SurfaceTexture and
SetSurface fails, we've already dropped the overlay.  it might be
OnSurfaceDetroyed anyway (we don't know here).  in any case SetSurfaceFailure =>
NOTIFY_ERROR => UpdateSurface notices ERROR and ReleaseCodec.

you know, a SurfaceTexture wrapper that holds the GL objects (rather than
letting shared_state do it) might make this easier.  then we can post a ref to
it with MC Release, and probably skip all the special ST processing.  i'll think
more about it.  it's quite similar to the SurfaceTextureProducer interface, but
with a lot less work.  could probably be a subclass, in fact.

== weeks later ==

surface bundle + SurfaceTextureGLOwner deal with this case.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.cc:1: // Copyright (c) 2017 The Chromium
Authors. All rights reserved.
On 2017/02/17 22:40:36, watk wrote:
> no (c)

Done.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.cc:42: // TODO(liberato): should we have a
separate cb for this?
On 2017/02/17 22:40:36, watk wrote:
> Delete TODO? Or is it just for the code review?

for review.  on further thought, i think that we should have a separate one. 
the client can always bind it to the same thing as destroyed cb.

i'll do that.

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay.h (right):

https://codereview.chromium.org/2692863011/diff/100001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.h:2: // Use of this source code is governed
by a BSD-style license that can be
On 2017/02/17 22:40:36, watk wrote:
> no (c)

never copy old headers :)

[liberato (no reviews please), 2017-03-08 22:35:44]

i think this works now.  i've tested both UpdateSurface and restart.  it's ready
for review.

note that the AndroidOverlay interface is slightly different here than in the
mojo framework CL (https://codereview.chromium.org/2688193002/).  my plan is to
use the version from here instead of the mojo version.  so, comments on it are
appreciated.

the only bugs i've found are from the rebase onto SurfaceBundle.  it quit
switching to surface view (checked the wrong surface id, rather than
|incoming_bundle_|).

thanks
-fl

[liberato (no reviews please), 2017-03-08 22:48:59]

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (left):

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1239:
codec_allocator_->DeallocateSurface(this, surface_id());
one difference with this CL is that, if codec release hangs, then this won't
happen.

not sure if this is actually a bad thing or not.

[watk, 2017-03-10 02:19:44]

I like this a lot! lgtm

https://codereview.chromium.org/2692863011/diff/180001/media/base/android/and...
File media/base/android/android_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/180001/media/base/android/and...
media/base/android/android_overlay.cc:12: }  // namespace media
super nit: newline before this line?

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.h:53: // TODO(liberato); update comment.
Leaving a reminder for this todo :)

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.cc:54:
gpu::GpuSurfaceLookup::GetInstance()->AcquireJavaSurface(surface_id_);
I wonder if we should check whether this is a non null and non-empty and turn
that into a failed_cb + deallocate surface? Then we can delete the client code
that checks whether the surface is valid.

[liberato (no reviews please), 2017-03-10 17:40:37]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-10 17:40:38]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2692863011/#ps200001
(title: "cl feedback")

[liberato (no reviews please), 2017-03-10 17:40:56]

thanks for the feedback!

-fl

https://codereview.chromium.org/2692863011/diff/180001/media/base/android/and...
File media/base/android/android_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/180001/media/base/android/and...
media/base/android/android_overlay.cc:12: }  // namespace media
On 2017/03/10 02:19:44, watk wrote:
> super nit: newline before this line?

Done.

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.h:53: // TODO(liberato); update comment.
On 2017/03/10 02:19:44, watk wrote:
> Leaving a reminder for this todo :)

thanks, done.

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay.cc (right):

https://codereview.chromium.org/2692863011/diff/180001/media/gpu/content_vide...
media/gpu/content_video_view_overlay.cc:54:
gpu::GpuSurfaceLookup::GetInstance()->AcquireJavaSurface(surface_id_);
On 2017/03/10 02:19:44, watk wrote:
> I wonder if we should check whether this is a non null and non-empty and turn
> that into a failed_cb + deallocate surface? Then we can delete the client code
> that checks whether the surface is valid.

i like that better.  definitely should run the failed cb on IsEmpty().

however, rather than remove the check in AVDA::IPBM, i'll move it to the
SurfaceTexture case.  i'll also make PBM::InitializeForSurfaceTexture return
failure rather than DCHECK if it fails to create one.

[commit-bot: I haz the power, 2017-03-10 17:41:06]

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-03-10 17:50:35]

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

[commit-bot: I haz the power, 2017-03-10 17:50:37]

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), 2017-03-10 18:05:38]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-10 18:05:39]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2692863011/#ps220001
(title: "fixed avda unittest")

[commit-bot: I haz the power, 2017-03-10 18:05:57]

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-03-10 19:45:52]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489169138625060,
"parent_rev": "7da5699334a643bf58a2a9dc7f0104a41c7f4ef5", "commit_rev":
"c58e8f82526098f4ef3dca5f957a46c439540a59"}

[commit-bot: I haz the power, 2017-03-10 19:46:41]

Description was changed from

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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

==========
Add ContentVideoViewOverlay to AVDA.

Wrap usage of ContentVideoView in an AndroidOverlay interface.  This
doesn't change the functionality, but starts to move AVDA towards
using AndroidOverlays rather than hard-coding CVV.

Much of Allocate/DeallocateSurface could be moved from
AVDACodecAllocator to ContentVideoViewOverlay.  However, to keep the
size of this change small, it isn't yet.

There shouldn't be any functional difference with this CL.

BUG=667950

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/2692863011
Cr-Commit-Position: refs/heads/master@{#456135}
Committed:
https://chromium.googlesource.com/chromium/src/+/c58e8f82526098f4ef3dca5f957a...
==========

[commit-bot: I haz the power, 2017-03-10 19:46:44]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/c58e8f82526098f4ef3dca5f957a...