Add AndroidVideoSurfaceChooser to manage overlays.

Add AndroidVideoSurfaceChooser to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AndroidVideoSurfaceChooser.  It also handles talking to overlay
factories that will be provided by the renderer.

AndroidVideoSurfaceChooser notifies AVDA about when to transition to
/ from an overlay.  Currently, this is based only on whether the
renderer has provided an overlay factory or not.  In the future,
this will look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AndroidVideoSurfaceChooser
abstracts the logic, and AVDA no longer directly deals with any of
those hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AndroidVideoSurfaceChooserImplTest
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/2813303003
Cr-Commit-Position: refs/heads/master@{#468916}
Committed: https://chromium.googlesource.com/chromium/src/+/9432ee01aaa7821c3ea5cbff746a969a0725a34b

[liberato (no reviews please), 2017-04-12 22:44:39]

Description was changed from

==========
works, i think


kinda works, lots of debug


started overlay manager

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

to

==========
works, i think


kinda works, lots of debug


started overlay manager

BUG=
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-04-18 22:04:50]

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

[commit-bot: I haz the power, 2017-04-18 22:05:26]

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-04-18 22:10:43]

Description was changed from

==========
works, i think


kinda works, lots of debug


started overlay manager

BUG=
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 AVDAOverlayHelper to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AVDAOverlayHelperImpl.  It also handles talking to overlay
factories that will be provided by the renderer.

AVDAOverlayHelper notifies AVDA about when to transition to / from
an overlay.  Currently, this is based only on whether the renderer
has provided an overlay factory or not.  In the future, this will
look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AVDAOverlayHelper abstracts
the logic, and AVDA no longer directly deals with any of those
hard-to-mock objects.

BUG=
TEST=AndroidVideoDecodeAcceleratorTest, AVDAOverlayHelperImplTest
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
==========

[commit-bot: I haz the power, 2017-04-18 22:11:22]

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

[commit-bot: I haz the power, 2017-04-18 22:11:23]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  android_optional_gpu_tests_rel on master.tryserver.chromium.android
(JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[liberato (no reviews please), 2017-04-18 22:17:21]

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

[commit-bot: I haz the power, 2017-04-18 22:17:55]

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-04-18 22:19:58]

Description was changed from

==========
Add AVDAOverlayHelper to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AVDAOverlayHelperImpl.  It also handles talking to overlay
factories that will be provided by the renderer.

AVDAOverlayHelper notifies AVDA about when to transition to / from
an overlay.  Currently, this is based only on whether the renderer
has provided an overlay factory or not.  In the future, this will
look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AVDAOverlayHelper abstracts
the logic, and AVDA no longer directly deals with any of those
hard-to-mock objects.

BUG=
TEST=AndroidVideoDecodeAcceleratorTest, AVDAOverlayHelperImplTest
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 AVDAOverlayHelper to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AVDAOverlayHelperImpl.  It also handles talking to overlay
factories that will be provided by the renderer.

AVDAOverlayHelper notifies AVDA about when to transition to / from
an overlay.  Currently, this is based only on whether the renderer
has provided an overlay factory or not.  In the future, this will
look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AVDAOverlayHelper abstracts
the logic, and AVDA no longer directly deals with any of those
hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AVDAOverlayHelperImplTest
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-04-18 22:21:08]

Description was changed from

==========
Add AVDAOverlayHelper to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AVDAOverlayHelperImpl.  It also handles talking to overlay
factories that will be provided by the renderer.

AVDAOverlayHelper notifies AVDA about when to transition to / from
an overlay.  Currently, this is based only on whether the renderer
has provided an overlay factory or not.  In the future, this will
look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AVDAOverlayHelper abstracts
the logic, and AVDA no longer directly deals with any of those
hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AVDAOverlayHelperImplTest
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 AVDAOverlayHelper to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AVDAOverlayHelperImpl.  It also handles talking to overlay
factories that will be provided by the renderer.

AVDAOverlayHelper notifies AVDA about when to transition to / from
an overlay.  Currently, this is based only on whether the renderer
has provided an overlay factory or not.  In the future, this will
look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AVDAOverlayHelper abstracts
the logic, and AVDA no longer directly deals with any of those
hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AVDAOverlayHelperImplTest
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
==========

[commit-bot: I haz the power, 2017-04-18 22:24:21]

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

[commit-bot: I haz the power, 2017-04-18 22:24:22]

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

[liberato (no reviews please), 2017-04-18 22:50:06]

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

[commit-bot: I haz the power, 2017-04-18 22:50:45]

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-04-19 01:37:18]

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

[commit-bot: I haz the power, 2017-04-19 01:37:19]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-04-20 18:20:28]

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

[liberato (no reviews please), 2017-04-20 18:22:29]

sorry that this is a big review.  of the 1600 lines, ~1000 are tests and quite a
few of the rest are comments.  so, hopefully, it's not as bad.

thanks
-fl

[DaleCurtis, 2017-04-20 19:10:43]

Just a bunch of style nits and WeakPtr complaints, didn't have time for a deep
review yet.

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:5: #ifndef
MEDIA_BASE_ANDROID_MOCK_ANDROID_OVERLAY_H_
Hmm, seems like this should be in media/gpu/android?

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:16: class MockAndroidOverlay : public
::testing::NiceMock<AndroidOverlay> {
Not StrictMock?

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:34: base::WeakPtr<MockAndroidOverlay>
GetWeakPtr() {
Vending of WeakPtrs externally is generally a very bad smell. Can you avoid
this?

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:43: };
DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.h:63:
base::WeakPtr<MockMediaCodecBridge> GetWeakPtr();
Ditto, highly frowned upon since it's unclear what external callers may do with
this and WeakPtrs are quite subtle.

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

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:254: if (platform_config_for_test)
{
This will probably trip presubmit scripts; if so, you'll need to drop the
for_test name here.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:393: // ::Decode will call us
(after clearing |defer_surface_creation_|), but
Decode() w/o :: is the preferred style.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:395: // Also note that we might
choose to defer surface creation for the sync path,
Line break?

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:397: // will fall through to the
below when ::Decode calls us back.  That's okay.
Ditto on naming.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:724: // TODO(liberato): can this
happen now?  We won't be notified about the
Switch to DCHECK then?

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:920: // We should still ne in
BEFORE_OVERLAY_INIT, since we've deferred doing it
s/ne/be/

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1284: if (surface_id !=
SurfaceManager::kNoSurfaceID) {
Drop {} for single-line if.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1347: if
(incoming_overlay_.has_value() && (*incoming_overlay_).get() == overlay) {
just if (incoming_overlay_ && incoming_overlay_.get() == overlay) ?

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1392: incoming_overlay_ =
std::unique_ptr<AndroidOverlay>();
.reset() ?

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:65: const PlatformConfig*
platform_config_for_test = nullptr);
Only a few instances of constructions, should we just update the call sites
instead of default arg?

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:265:
base::WeakPtr<AndroidVideoDecodeAccelerator> GetWeakPtr();
Again, avoid.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:269: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:51: protected:
Per style guide this should be public IIRC.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl.h:55: };
DISALLOW...

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

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/content_vide...
media/gpu/content_video_view_overlay_factory.cc:16: DCHECK(surface_id_ !=
SurfaceManager::kNoSurfaceID);
DCHECK_NE.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay_factory.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/content_vide...
media/gpu/content_video_view_overlay_factory.h:25: };
DISALLOWL...

[liberato (no reviews please), 2017-04-20 22:01:39]

thanks for the feedback.

thanks
-fl

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:5: #ifndef
MEDIA_BASE_ANDROID_MOCK_ANDROID_OVERLAY_H_
On 2017/04/20 19:10:42, DaleCurtis wrote:
> Hmm, seems like this should be in media/gpu/android?

seems more generally useful than that.  i kept it with android_overlay.h, like
mock_media_codec_bridge.h .

i'll move it if you feel strongly.

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:16: class MockAndroidOverlay : public
::testing::NiceMock<AndroidOverlay> {
On 2017/04/20 19:10:42, DaleCurtis wrote:
> Not StrictMock?

good point.  i'll try it and see if anything fails, and decide based on that. 
probably nothing does.

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:34: base::WeakPtr<MockAndroidOverlay>
GetWeakPtr() {
On 2017/04/20 19:10:42, DaleCurtis wrote:
> Vending of WeakPtrs externally is generally a very bad smell. Can you avoid
> this?

that's very hard to do, since they're always unique_ptr'd around.  when the test
wants to poke at them after handing off ownership (e.g. 'send SurfaceDestroyed
now'), i'm not sure how else to do it.

suggestions welcome.

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_android_overlay.h:43: };
On 2017/04/20 19:10:42, DaleCurtis wrote:
> DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.h:63:
base::WeakPtr<MockMediaCodecBridge> GetWeakPtr();
On 2017/04/20 19:10:42, DaleCurtis wrote:
> Ditto, highly frowned upon since it's unclear what external callers may do
with
> this and WeakPtrs are quite subtle.

not sure how to handle it.  in this case, the test wants to be able to set
expectations on a codec that's owned by AVDA.  i can think of some elaborate
ways to avoid the wp, but nothing that's simple.

kinda hoping that it's okay, since it's only mocks.

again, suggestions welcome.

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

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:254: if (platform_config_for_test)
{
On 2017/04/20 19:10:43, DaleCurtis wrote:
> This will probably trip presubmit scripts; if so, you'll need to drop the
> for_test name here.

renamed to |optional_platform_config|.  goes better after removing the '=
nullptr' anyway.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:393: // ::Decode will call us
(after clearing |defer_surface_creation_|), but
On 2017/04/20 19:10:43, DaleCurtis wrote:
> Decode() w/o :: is the preferred style.

done, and elsewhere.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:395: // Also note that we might
choose to defer surface creation for the sync path,
On 2017/04/20 19:10:42, DaleCurtis wrote:
> Line break?

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:397: // will fall through to the
below when ::Decode calls us back.  That's okay.
On 2017/04/20 19:10:43, DaleCurtis wrote:
> Ditto on naming.

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:724: // TODO(liberato): can this
happen now?  We won't be notified about the
On 2017/04/20 19:10:42, DaleCurtis wrote:
> Switch to DCHECK then?

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:920: // We should still ne in
BEFORE_OVERLAY_INIT, since we've deferred doing it
On 2017/04/20 19:10:43, DaleCurtis wrote:
> s/ne/be/

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1284: if (surface_id !=
SurfaceManager::kNoSurfaceID) {
On 2017/04/20 19:10:43, DaleCurtis wrote:
> Drop {} for single-line if.

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1347: if
(incoming_overlay_.has_value() && (*incoming_overlay_).get() == overlay) {
On 2017/04/20 19:10:43, DaleCurtis wrote:
> just if (incoming_overlay_ && incoming_overlay_.get() == overlay) ?

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1392: incoming_overlay_ =
std::unique_ptr<AndroidOverlay>();
On 2017/04/20 19:10:43, DaleCurtis wrote:
> .reset() ?

we want it to has_value() of a nullptr -- i.e., transition to surface texture. 
i'll add a comment to make this clearer.

as an aside, i tried it as reset() and ran the tests.  nothing failed.  turns
out that base::Optional just returns nullptr anyway when dchecks are turned off.
 so, i didn't try to add a separate test to catch it, since
SwitchesToSurfaceTextureWhenSurfaceDestroyed would trigger the dcheck.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:65: const PlatformConfig*
platform_config_for_test = nullptr);
On 2017/04/20 19:10:43, DaleCurtis wrote:
> Only a few instances of constructions, should we just update the call sites
> instead of default arg?

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:265:
base::WeakPtr<AndroidVideoDecodeAccelerator> GetWeakPtr();
On 2017/04/20 19:10:43, DaleCurtis wrote:
> Again, avoid.

it has private access -- does that make it okay?.  i just got tired of typing
"weak_this_factory_.GetWeakPtr()" everywhere. :)

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:269: };
On 2017/04/20 19:10:43, DaleCurtis wrote:
> DISALLOW_COPY_AND_ASSIGN

Done, and elsewhere.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:51: protected:
On 2017/04/20 19:10:43, DaleCurtis wrote:
> Per style guide this should be public IIRC.

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl.h:55: };
On 2017/04/20 19:10:43, DaleCurtis wrote:
> DISALLOW...

Done.  i didn't think i needed it if the base class did.

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

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/content_vide...
media/gpu/content_video_view_overlay_factory.cc:16: DCHECK(surface_id_ !=
SurfaceManager::kNoSurfaceID);
On 2017/04/20 19:10:43, DaleCurtis wrote:
> DCHECK_NE.

Done.

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay_factory.h (right):

https://codereview.chromium.org/2813303003/diff/180001/media/gpu/content_vide...
media/gpu/content_video_view_overlay_factory.h:25: };
On 2017/04/20 19:10:43, DaleCurtis wrote:
> DISALLOWL...

Done.

[liberato (no reviews please), 2017-04-20 22:01:44]

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

[commit-bot: I haz the power, 2017-04-20 22:02:51]

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

[DaleCurtis, 2017-04-20 22:32:26]

Will take a closer look tomorrow to provide suggestions, but the WeakPtr stuff
is probably a blocker. Consider things like allowing internal binding only but
vending some pre-bound callback for test fixtures, etc.

[commit-bot: I haz the power, 2017-04-20 23:57:49]

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

[commit-bot: I haz the power, 2017-04-20 23:57:49]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-04-21 22:02:57]

I think a lot of your WeakPtr usage can be switched out in favor of a subclass
which flips a bit when destructed. I don't see a lot of cases where you're using
it for anything besides destruction testing. Especially around codec destruction
we already have a mock which can be extended to do this.

[liberato (no reviews please), 2017-04-21 22:15:19]

On 2017/04/21 22:02:57, DaleCurtis wrote:
> I think a lot of your WeakPtr usage can be switched out in favor of a subclass
> which flips a bit when destructed. I don't see a lot of cases where you're
using
> it for anything besides destruction testing. Especially around codec
destruction
> we already have a mock which can be extended to do this.

i started work on something similar today.  the effect on the various cases has
been mixed.

case 1: for watching for destruction, i've added a ScopedDestructionObserver
that sounds similar to the thing you're describing.  seems fine.

case 2: the test wants to control the overlay, such as "send OnSurfaceDestroyed
now", are a little weird.  the overlay provides callbacks protected by weak
ptrs.  what i don't like is that the same restrictions about weak ptrs apply as
before, except that now the caller doesn't know about them.

case 3: setting test expectations.  for example, sometimes the test wants to
EXPECT_CALL(OnOverlayReady(overlay_weak.get())).  i can use raw ptrs here, but
that seems bad.

i'll put the new version together and see what you think.

[tguilbert, 2017-04-21 22:55:46]

Tiny nits. Will keep ingesting this whenever you send the next version.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:38: // encded input stream using
Android's MediaCodec. It handles the work of
s/encded/encoded

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:21: // the surface immediatly.
s/immediatly/immediately

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:28: // TODO(liberato): document the differences
between this an 'Immediately'
s/an/and

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl.cc (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl.cc:54: // won't be visible soon, so maybe
that's the wrong thing to do.  For DS,
DS is not clear in this context. Can you use the un-abbreviated form?

[watk, 2017-04-22 00:29:06]

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

[watk, 2017-04-22 00:29:08]

Will continue going through this tomorrow, but wanted to post my comments so
far.

Generally the tests seemed noisy and looked like they had a decent amount of
overlap. I'd prefer if we:

a) used NiceMock and set the default behavior with ON_CALL. Too much EXPECT_CALL
makes the tests noisy because you have to write them even if you don't care, and
brittle because they scatter implementation details through every test. There's
a discussion of it here:
https://groups.google.com/forum/#!topic/googlemock/pRyZwyWmrRE

b) Rely on assertions made in earlier tests in later tests, i.e., don't
duplicate assertions. We only need one test to fail if a given behavior is
broken.

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

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:260:
MediaCodecUtil::IsSetOutputSurfaceSupported();
Could we construct a default one where we construct AVDA and make this
non-optional?

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:429:
EXPECT_CALL(codec_allocator_, MockCreateMediaCodecSync(nullptr, NotNull()));
This looks like noise caused by StrictMock.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:457:
EXPECT_CALL(*helper_, MockStartup());
Seems like noise.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:41: virtual void Startup(
nit: startup is a noun. Start() or Initialize()? I prefer Initialize().

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:45: // If |overlay| is non-null, then |overlay|
owns |surface|.
surface out of date

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:48: // The SurfaceTexture attached to
|surface()|, or nullptr if |surface()| is
There's no surface() function.

[liberato (no reviews please), 2017-04-25 22:13:11]

this PS only tries to remove some weak factories.  it doesn't address other
feedback.  i'm interested in feedback on whether this is better, worse, or if
there's a third option for it.

MockAndroidOverlay now doesn't vend them, in favor of callbacks and an
DestructionObserver.

i left the MockOverlayFactory weak ptrs, since they're internal to the test.

AVDA's GetWeakPtr function is now adjacent to the WeakPtrFactory, so that it
clearly is in the same visibility.

before i address the specific test suggestions from watk@, i want to make sure
that we agree on the general form of the tests.

thanks
-fl

[DaleCurtis, 2017-04-25 23:25:33]

As indicated, I'm still a bit confused on exactly why you need the WeakPtrs. If
you take my suggestion you would just modify tests to have something like

EXPECT_CALL(OnOverlayDestroyed())

With a StrictMock a missed expectation would trigger failure.

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_android_overlay.h:38: 
Why not just MOCK_METHOD0(OnOverlayDestroyed)) that is always called from
~MockAndroidOverlay? I'm a bit confused on why the full destruction observer is
necessary.

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_android_overlay.h:98: base::WeakPtr<MockAndroidOverlay>
GetWeakPtr();
No point if private?

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.h:63:
base::WeakPtr<MockMediaCodecBridge> GetWeakPtr();
Ditto, why not a MOCK_METHOD0(OnCodecDestroyed()) and a strictmock?

https://codereview.chromium.org/2813303003/diff/220001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:396: // Private helper for
|weak_this_factory_|.
Seems unnecessary since internal callers can just use the factory directly?

[liberato (no reviews please), 2017-04-26 17:46:40]

Patchset #13 (id:240001) has been deleted

[liberato (no reviews please), 2017-04-26 17:53:58]

inlined GetWeakPtr() on avda and mock_android_overlay.  i don't really feel
strongly either way.

for the remaining items, will discuss with you offline.

thanks
-fl

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_android_overlay.h:38: 
On 2017/04/25 23:25:33, DaleCurtis wrote:
> Why not just MOCK_METHOD0(OnOverlayDestroyed)) that is always called from
> ~MockAndroidOverlay? I'm a bit confused on why the full destruction observer
is
> necessary.

how would one set the expectation in the test?  we give up ownership of the
object as part of the API.

i guess i could keep a raw ptr, but that seems like exactly the problem that a
weakref is supposed to solve.

same for MockOverlayFactory.

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_android_overlay.h:98: base::WeakPtr<MockAndroidOverlay>
GetWeakPtr();
On 2017/04/25 23:25:33, DaleCurtis wrote:
> No point if private?

removed, see comment in avda.h

https://codereview.chromium.org/2813303003/diff/220001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:396: // Private helper for
|weak_this_factory_|.
On 2017/04/25 23:25:33, DaleCurtis wrote:
> Seems unnecessary since internal callers can just use the factory directly?

it just reads better to me as a utility method than the existing
|weak_this_factory_.GetWeakPtr()| -- less text to parse, fewer wrapped lines. 
plus, ironically in retrospect, less typing.  :)

i'll put it back.

[DaleCurtis, 2017-04-26 18:28:10]

RawPtr+clear lifetime documentation is preferred to WeakPtr.

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_android_overlay.h:38: 
On 2017/04/26 at 17:53:57, liberato wrote:
> On 2017/04/25 23:25:33, DaleCurtis wrote:
> > Why not just MOCK_METHOD0(OnOverlayDestroyed)) that is always called from
> > ~MockAndroidOverlay? I'm a bit confused on why the full destruction observer
is
> > necessary.
> 
> how would one set the expectation in the test?  we give up ownership of the
object as part of the API.
> 
> i guess i could keep a raw ptr, but that seems like exactly the problem that a
weakref is supposed to solve.
> 
> same for MockOverlayFactory.

You have to set expectations before destruction anyways with a strict mock, so
if things are working correctly the raw pointer is always alive.

[liberato (no reviews please), 2017-04-27 18:30:13]

here's a bunch of changes:

 - most exposed weak refs are gone.  there's one or two that are defined in
classes inside unit tests themselves that would have been cumbersome to remove,
so i commented and left them.  they're in unit test source files themselves, not
the mocks.

- DestructionObserver has been split out, since MockMediaCodecBridge and
MockAndroidOverlay both use it.

- removed / truncated tests for AVDA for clarity.  made those mocks nice rather
than strict.

- misc renaming.

thanks
-fl

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

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:260:
MediaCodecUtil::IsSetOutputSurfaceSupported();
On 2017/04/22 00:29:07, watk wrote:
> Could we construct a default one where we construct AVDA and make this
> non-optional?

Done.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:38: // encded input stream using
Android's MediaCodec. It handles the work of
On 2017/04/21 22:55:45, tguilbert wrote:
> s/encded/encoded

Done.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:429:
EXPECT_CALL(codec_allocator_, MockCreateMediaCodecSync(nullptr, NotNull()));
On 2017/04/22 00:29:07, watk wrote:
> This looks like noise caused by StrictMock.

i want to be sure that it uses the sync path for codec construction if we're not
allowed to defer.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:457:
EXPECT_CALL(*helper_, MockStartup());
On 2017/04/22 00:29:07, watk wrote:
> Seems like noise.

Done.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:21: // the surface immediatly.
On 2017/04/21 22:55:46, tguilbert wrote:
> s/immediatly/immediately

Done.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:28: // TODO(liberato): document the differences
between this an 'Immediately'
On 2017/04/21 22:55:46, tguilbert wrote:
> s/an/and

Done.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:41: virtual void Startup(
On 2017/04/22 00:29:07, watk wrote:
> nit: startup is a noun. Start() or Initialize()? I prefer Initialize().

Done, Initialize.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl.cc (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl.cc:54: // won't be visible soon, so maybe
that's the wrong thing to do.  For DS,
On 2017/04/21 22:55:46, tguilbert wrote:
> DS is not clear in this context. Can you use the un-abbreviated form?

Done.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:45: // If |overlay| is non-null, then |overlay|
owns |surface|.
On 2017/04/22 00:29:07, watk wrote:
> surface out of date

Done, and elsewhere.

https://codereview.chromium.org/2813303003/diff/200001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:48: // The SurfaceTexture attached to
|surface()|, or nullptr if |surface()| is
On 2017/04/22 00:29:07, watk wrote:
> There's no surface() function.

Done.

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_android_overlay.h:38: 
On 2017/04/26 18:28:10, DaleCurtis wrote:
> On 2017/04/26 at 17:53:57, liberato wrote:
> > On 2017/04/25 23:25:33, DaleCurtis wrote:
> > > Why not just MOCK_METHOD0(OnOverlayDestroyed)) that is always called from
> > > ~MockAndroidOverlay? I'm a bit confused on why the full destruction
observer
> is
> > > necessary.
> > 
> > how would one set the expectation in the test?  we give up ownership of the
> object as part of the API.
> > 
> > i guess i could keep a raw ptr, but that seems like exactly the problem that
a
> weakref is supposed to solve.
> > 
> > same for MockOverlayFactory.
> 
> You have to set expectations before destruction anyways with a strict mock, so
> if things are working correctly the raw pointer is always alive.

(per offline discussion)

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2813303003/diff/220001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.h:63:
base::WeakPtr<MockMediaCodecBridge> GetWeakPtr();
On 2017/04/25 23:25:33, DaleCurtis wrote:
> Ditto, why not a MOCK_METHOD0(OnCodecDestroyed()) and a strictmock?

(per offline discussion)

[DaleCurtis, 2017-04-27 18:57:38]

Just skimmed to get some Qs out. Will finish review today.

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/BUI...
File media/base/android/BUILD.gn (right):

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/BUI...
media/base/android/BUILD.gn:102: static_library("test_support") {
source_set ?

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
File media/base/android/mock_android_overlay.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.cc:26: c.OverlayReady =
base::Bind(&MockAndroidOverlay::CallbackTrampoline,
Why do you need the trampoline? Can't you just bind directly using the WeakPtr?

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.h:18: class MockAndroidOverlay : public
::testing::StrictMock<AndroidOverlay>,
No ::

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.h:34: using ControlCallback =
base::Callback<bool()>;
base::OnceCallback or RepeatingCallback ?

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.h:50: static bool
CallbackTrampoline(void (MockAndroidOverlay::*cb)(),
Use a base callback signature?

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

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:434: void
AndroidVideoDecodeAccelerator::OnTransitionToOrFromOverlay(
Maybe just OnOverlayTransition?

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1348: if (incoming_overlay_ &&
incoming_overlay_->get() == overlay) {
I think you cna just write incoming_overlay_ == overlay? I see operator==
overrides for this anyways in base::Optional

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1356: if
(!codec_config_->surface_bundle ||
Ditto.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1394: if (!incoming_overlay_)
Isn't this a noop?

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl.cc:14: const UseOverlayCB& use_overlay_cb,
Callback are passed by value now and used with std::move.

[liberato (no reviews please), 2017-04-27 20:05:02]

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/BUI...
File media/base/android/BUILD.gn (right):

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/BUI...
media/base/android/BUILD.gn:102: static_library("test_support") {
On 2017/04/27 18:57:37, DaleCurtis wrote:
> source_set ?

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
File media/base/android/mock_android_overlay.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.cc:26: c.OverlayReady =
base::Bind(&MockAndroidOverlay::CallbackTrampoline,
On 2017/04/27 18:57:37, DaleCurtis wrote:
> Why do you need the trampoline? Can't you just bind directly using the
WeakPtr? 

so the caller knows if the callback succeeded.  that said, i'm not sure that i
actually use that property anywhere anymore.  will delete if so.

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.h:18: class MockAndroidOverlay : public
::testing::StrictMock<AndroidOverlay>,
On 2017/04/27 18:57:37, DaleCurtis wrote:
> No ::

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.h:34: using ControlCallback =
base::Callback<bool()>;
On 2017/04/27 18:57:37, DaleCurtis wrote:
> base::OnceCallback or RepeatingCallback ?

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/base/android/moc...
media/base/android/mock_android_overlay.h:50: static bool
CallbackTrampoline(void (MockAndroidOverlay::*cb)(),
On 2017/04/27 18:57:37, DaleCurtis wrote:
> Use a base callback signature?

?

anyway, doesn't matter.  ended up deleting CallbackTrampoline since nobody ever
checked the return value anyway.

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

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:434: void
AndroidVideoDecodeAccelerator::OnTransitionToOrFromOverlay(
On 2017/04/27 18:57:38, DaleCurtis wrote:
> Maybe just OnOverlayTransition?

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1348: if (incoming_overlay_ &&
incoming_overlay_->get() == overlay) {
On 2017/04/27 18:57:37, DaleCurtis wrote:
> I think you cna just write incoming_overlay_ == overlay? I see operator==
> overrides for this anyways in base::Optional

yeah, they do handle !has_value.  nifty.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1356: if
(!codec_config_->surface_bundle ||
On 2017/04/27 18:57:37, DaleCurtis wrote:
> Ditto.

there's no base::Optional here.  this is making sure that we don't dereference a
null |surface_bundle|.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1394: if (!incoming_overlay_)
On 2017/04/27 18:57:37, DaleCurtis wrote:
> Isn't this a noop? 

it's not.  it sets the base::Optional |incoming_overlay_| to have a value of
nullptr if it currently has no value.  i.e., has_value() 0 => 1.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl.cc:14: const UseOverlayCB& use_overlay_cb,
On 2017/04/27 18:57:38, DaleCurtis wrote:
> Callback are passed by value now and used with std::move.

Done.

[watk, 2017-04-27 20:33:10]

Another incomplete review round :)

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

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:434: void
AndroidVideoDecodeAccelerator::OnTransitionToOrFromOverlay(
On 2017/04/27 18:57:38, DaleCurtis wrote:
> Maybe just OnOverlayTransition?

Or OnSurfaceTransition. To me Surface should be the main concept that the codec
deals with. And Surface seems like a fine way to refer to both STs and Overlays.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1033: DCHECK(!media_codec_);
+1

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:128: // |overlay_helper_| with our
inital factory from VDA::Config.
initial

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:63: class MockVDAClient :
public VideoDecodeAccelerator::Client {
Is there a reason you prefer this and the ones below to be inner classes? I
don't like how this and the other two inner classes make it harder to find the
body of AVDATest, which is why I created them above. Maybe it's just me though.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:445:
EXPECT_CALL(codec_allocator_, MockCreateMediaCodecSync(nullptr, NotNull()));
Doesn't seem like parameters matter, we just need to assert that it was called.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:18: class MEDIA_GPU_EXPORT AVDAOverlayHelper {
Can we replace Helper with a word that indicates the purpose of this thing?
OverlayCoordinator, OverlayScheduler, OverlayTransitionScheduler,
OverlayTransitionCoordinator, OverlayChooser

Actually I kinda like using Surface instead of overlay. Because both STs and
Overlays are Surfaces: SurfaceSelector, SurfaceCoordinator, etc.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:48: virtual void OnOverlayFactory(
Name sounds weird to me. What about just SetOverlayFactory()? Or if you want the
"On", I would prefer OnOverlayFactoryChanged().

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:44: // hand off ownership to the
caller.
I feel like you can delete the "One might argue" comment.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:156:
StartHelper(std::move(factory_));
Seems like these two lines are a separate test. And then they can be removed
from the rest. (along with the verifyandclearexpectations probably)

InitializingWithFactoryCreatesOverlay

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:159: ASSERT_TRUE(factory_weak_ !=
nullptr);
Not clear why this matters in this test

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:176: // Replacing the overlay
will null will destroy it.
s/will/with

[liberato (no reviews please), 2017-04-27 21:31:44]

thanks for the feedback.

-fl

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

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:434: void
AndroidVideoDecodeAccelerator::OnTransitionToOrFromOverlay(
On 2017/04/27 20:33:09, watk wrote:
> On 2017/04/27 18:57:38, DaleCurtis wrote:
> > Maybe just OnOverlayTransition?
> 
> Or OnSurfaceTransition. To me Surface should be the main concept that the
codec
> deals with. And Surface seems like a fine way to refer to both STs and
Overlays.
> 

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1033: DCHECK(!media_codec_);
On 2017/04/27 20:33:09, watk wrote:
> +1

Acknowledged.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:128: // |overlay_helper_| with our
inital factory from VDA::Config.
On 2017/04/27 20:33:09, watk wrote:
> initial

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:63: class MockVDAClient :
public VideoDecodeAccelerator::Client {
On 2017/04/27 20:33:10, watk wrote:
> Is there a reason you prefer this and the ones below to be inner classes? I
> don't like how this and the other two inner classes make it harder to find the
> body of AVDATest, which is why I created them above. Maybe it's just me
though.

because i recently lost a day and half tracking down very weird test failures
that were due to duplicate class names in unrelated tests.  "MockClient" was the
culprit.

i suppose that i could make it an outer class and enclose everything in an
anonymous namespace.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:445:
EXPECT_CALL(codec_allocator_, MockCreateMediaCodecSync(nullptr, NotNull()));
On 2017/04/27 20:33:10, watk wrote:
> Doesn't seem like parameters matter, we just need to assert that it was
called.

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper.h (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper.h:18: class MEDIA_GPU_EXPORT AVDAOverlayHelper {
On 2017/04/27 20:33:10, watk wrote:
> Can we replace Helper with a word that indicates the purpose of this thing?
> OverlayCoordinator, OverlayScheduler, OverlayTransitionScheduler,
> OverlayTransitionCoordinator, OverlayChooser
> 
> Actually I kinda like using Surface instead of overlay. Because both STs and
> Overlays are Surfaces: SurfaceSelector, SurfaceCoordinator, etc.

AndroidVideoSurfaceChooser .  Also gets rid of AVDA, since hopefully this is
useful for MCVD.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
File media/gpu/avda_overlay_helper_impl_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:44: // hand off ownership to the
caller.
On 2017/04/27 20:33:10, watk wrote:
> I feel like you can delete the "One might argue" comment.

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:156:
StartHelper(std::move(factory_));
On 2017/04/27 20:33:10, watk wrote:
> Seems like these two lines are a separate test. And then they can be removed
> from the rest. (along with the verifyandclearexpectations probably)
> 
> InitializingWithFactoryCreatesOverlay

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:159: ASSERT_TRUE(factory_weak_ !=
nullptr);
On 2017/04/27 20:33:10, watk wrote:
> Not clear why this matters in this test

Done.

https://codereview.chromium.org/2813303003/diff/340001/media/gpu/avda_overlay...
media/gpu/avda_overlay_helper_impl_unittest.cc:176: // Replacing the overlay
will null will destroy it.
On 2017/04/27 20:33:10, watk wrote:
> s/will/with

Done.

[DaleCurtis, 2017-04-27 22:01:12]

lgtm % nits. I contend you could still do w/o the destruction observer +
WeakPtrs and the code would be more readable, but this is fine as is.

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.h:8: #include
"base/memory/weak_ptr.h"
Drop.

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/tes...
File media/base/android/test_destruction_observable.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/tes...
media/base/android/test_destruction_observable.h:93: };
DISALLOW...

https://codereview.chromium.org/2813303003/diff/400001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:46: std::unique_ptr<AndroidOverlay> overlay;
These should have accessors and be private members or this needs to be a struct.

[liberato (no reviews please), 2017-04-27 23:17:05]

thanks for the feedback.

-fl

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.h:8: #include
"base/memory/weak_ptr.h"
On 2017/04/27 22:01:11, DaleCurtis wrote:
> Drop.

Done.

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/tes...
File media/base/android/test_destruction_observable.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/tes...
media/base/android/test_destruction_observable.h:93: };
On 2017/04/27 22:01:11, DaleCurtis wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/2813303003/diff/400001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:46: std::unique_ptr<AndroidOverlay> overlay;
On 2017/04/27 22:01:11, DaleCurtis wrote:
> These should have accessors and be private members or this needs to be a
struct.

Done, struct.

[liberato (no reviews please), 2017-04-27 23:19:20]

Description was changed from

==========
Add AVDAOverlayHelper to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AVDAOverlayHelperImpl.  It also handles talking to overlay
factories that will be provided by the renderer.

AVDAOverlayHelper notifies AVDA about when to transition to / from
an overlay.  Currently, this is based only on whether the renderer
has provided an overlay factory or not.  In the future, this will
look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AVDAOverlayHelper abstracts
the logic, and AVDA no longer directly deals with any of those
hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AVDAOverlayHelperImplTest
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 AndroidVideoSurfaceChooser to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AndroidVideoSurfaceChooser.  It also handles talking to overlay
factories that will be provided by the renderer.

AndroidVideoSurfaceChooser notifies AVDA about when to transition to
/ from an overlay.  Currently, this is based only on whether the
renderer has provided an overlay factory or not.  In the future,
this will look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AndroidVideoSurfaceChooser
abstracts the logic, and AVDA no longer directly deals with any of
those hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AndroidVideoSurfaceChooserImplTest
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
==========

[watk, 2017-05-02 21:00:01]

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/moc...
File media/base/android/mock_android_overlay.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/moc...
media/base/android/mock_android_overlay.h:17: // AndroidOverlay implementation
that supports weak ptrs.
The weak ptr comment could use clarification. It's not clear how that's relevant
here.

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/tes...
File media/base/android/test_destruction_observable.h (right):

https://codereview.chromium.org/2813303003/diff/400001/media/base/android/tes...
media/base/android/test_destruction_observable.h:67: // Return a callback gthat
will notify us about destruction.
"gthat"

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

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:366: StartOverlayHelper();
s/OverlayHelper/SurfaceChooser/
s/overlay helper/surface chooser/

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:63: class MockVDAClient :
public VideoDecodeAccelerator::Client {
I'd prefer if these classes were in an anonymous namespace instead. I really
think it makes all of these classes harder to read. (It's 260 lines until you
get to the body of AVDATest and the indentation is harder to follow). But if you
disagree, that's fine as well.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser.h (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser.h:17: // Manage details of which overlay
to use for video playback, if any.
"which surface"
delete "if any".

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser.h:47: // WMPI has revoked the most
recent factory.
I don't think it's really helpful to mention WMPI here.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl.cc (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl.cc:92: // We |overlay_| is the only
overlay for which we haven't gotten a ready
The "We" needs deleting?

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl.h (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl.h:45: // Current overlay that we've
consructed but haven't received ready / failed
"constructed"

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl_unittest.cc:31: class MockClient {
Same comment as above about these inner classes.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay_factory.h (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/content_vide...
media/gpu/content_video_view_overlay_factory.h:14: class
ContentVideoViewOverlayFactory : public AndroidOverlayFactory {
I may be missing something, but it looks like this can be replaced with a single
callback that we bind the surface_id into.

I see that you're observing its destruction in the tests. You could do that by
binding an object that you can observe into the callback.

[liberato (no reviews please), 2017-05-02 21:49:38]

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

[commit-bot: I haz the power, 2017-05-02 21:50:04]

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-02 21:50:05]

thanks for the feedback.

-fl

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

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:366: StartOverlayHelper();
On 2017/05/02 21:00:01, watk wrote:
> s/OverlayHelper/SurfaceChooser/
> s/overlay helper/surface chooser/

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:63: class MockVDAClient :
public VideoDecodeAccelerator::Client {
On 2017/05/02 21:00:01, watk wrote:
> I'd prefer if these classes were in an anonymous namespace instead. I really
> think it makes all of these classes harder to read. (It's 260 lines until you
> get to the body of AVDATest and the indentation is harder to follow). But if
you
> disagree, that's fine as well.

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser.h (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser.h:17: // Manage details of which overlay
to use for video playback, if any.
On 2017/05/02 21:00:01, watk wrote:
> "which surface"
> delete "if any".

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser.h:47: // WMPI has revoked the most
recent factory.
On 2017/05/02 21:00:01, watk wrote:
> I don't think it's really helpful to mention WMPI here.

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl.cc (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl.cc:92: // We |overlay_| is the only
overlay for which we haven't gotten a ready
On 2017/05/02 21:00:01, watk wrote:
> The "We" needs deleting?

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl.h (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl.h:45: // Current overlay that we've
consructed but haven't received ready / failed
On 2017/05/02 21:00:01, watk wrote:
> "constructed"

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl_unittest.cc:31: class MockClient {
On 2017/05/02 21:00:01, watk wrote:
> Same comment as above about these inner classes.

Done.

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/content_vide...
File media/gpu/content_video_view_overlay_factory.h (right):

https://codereview.chromium.org/2813303003/diff/440001/media/gpu/content_vide...
media/gpu/content_video_view_overlay_factory.h:14: class
ContentVideoViewOverlayFactory : public AndroidOverlayFactory {
On 2017/05/02 21:00:01, watk wrote:
> I may be missing something, but it looks like this can be replaced with a
single
> callback that we bind the surface_id into.
> 
> I see that you're observing its destruction in the tests. You could do that by
> binding an object that you can observe into the callback. 

per offline discussion, this is an interesting idea.  i'm going to leave it out
of this cl since it would change other, unrelated things (mojo factory impl).

will give it more consideration and do a follow-up cl if i think it'll work.

[watk, 2017-05-02 22:04:31]

lgtm with remaining naming nits

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

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:376: // If we're trying to defer
surface creation, then don't notify the helper
helper

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1269: // overlay helper.  For
example, if we defer surface creation, then we'll
overlay helper

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:85: // implementatin of
their respective functions.  This allows tests to set
"implementation"

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:234: // Overlay helper
that calls mocked functions to allow counting calls, but
many instances of helper in this file

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl_unittest.cc:122: void
StartHelper(std::unique_ptr<MockAndroidOverlayFactory> factory) {
many instances of helper

[liberato (no reviews please), 2017-05-02 22:31:16]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-02 22:31:17]

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

[liberato (no reviews please), 2017-05-02 22:31:38]

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

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:376: // If we're trying to defer
surface creation, then don't notify the helper
On 2017/05/02 22:04:31, watk wrote:
> helper

Done.

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1269: // overlay helper.  For
example, if we defer surface creation, then we'll
On 2017/05/02 22:04:31, watk wrote:
> overlay helper

Done.

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:85: // implementatin of
their respective functions.  This allows tests to set
On 2017/05/02 22:04:31, watk wrote:
> "implementation"

Done.

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator_unittest.cc:234: // Overlay helper
that calls mocked functions to allow counting calls, but
On 2017/05/02 22:04:31, watk wrote:
> many instances of helper in this file

Done.

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
File media/gpu/android_video_surface_chooser_impl_unittest.cc (right):

https://codereview.chromium.org/2813303003/diff/460001/media/gpu/android_vide...
media/gpu/android_video_surface_chooser_impl_unittest.cc:122: void
StartHelper(std::unique_ptr<MockAndroidOverlayFactory> factory) {
On 2017/05/02 22:04:31, watk wrote:
> many instances of helper

Done.

[commit-bot: I haz the power, 2017-05-02 22:32:30]

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-02 23:23:44]

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

[commit-bot: I haz the power, 2017-05-02 23:23:46]

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

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

The CQ bit was checked by liberato@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2813303003/#ps500001
(title: "fixed dereference of base::Optional")

[commit-bot: I haz the power, 2017-05-03 05:11:01]

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-03 07:18:19]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1493788245380400,
"parent_rev": "636a9d194006073485edfa89533c1076dc1b0206", "commit_rev":
"9432ee01aaa7821c3ea5cbff746a969a0725a34b"}

[commit-bot: I haz the power, 2017-05-03 07:18:30]

Description was changed from

==========
Add AndroidVideoSurfaceChooser to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AndroidVideoSurfaceChooser.  It also handles talking to overlay
factories that will be provided by the renderer.

AndroidVideoSurfaceChooser notifies AVDA about when to transition to
/ from an overlay.  Currently, this is based only on whether the
renderer has provided an overlay factory or not.  In the future,
this will look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AndroidVideoSurfaceChooser
abstracts the logic, and AVDA no longer directly deals with any of
those hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AndroidVideoSurfaceChooserImplTest
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 AndroidVideoSurfaceChooser to manage overlays.

This CL abstracts out much of the surface-switching logic from AVDA
into AndroidVideoSurfaceChooser.  It also handles talking to overlay
factories that will be provided by the renderer.

AndroidVideoSurfaceChooser notifies AVDA about when to transition to
/ from an overlay.  Currently, this is based only on whether the
renderer has provided an overlay factory or not.  In the future,
this will look at compositor feedback and other hints to decide.

This CL also adds lots of tests for this behavior.  Previously, it
wasn't easy to test surface transitions since (a) they were bundled
into AVDA itself, and (b) they directly depended on hard-to-mock
classes like ScopedJavaSurface.  Now, AndroidVideoSurfaceChooser
abstracts the logic, and AVDA no longer directly deals with any of
those hard-to-mock objects.

BUG=710204
TEST=AndroidVideoDecodeAcceleratorTest, AndroidVideoSurfaceChooserImplTest
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/2813303003
Cr-Commit-Position: refs/heads/master@{#468916}
Committed:
https://chromium.googlesource.com/chromium/src/+/9432ee01aaa7821c3ea5cbff746a...
==========

[commit-bot: I haz the power, 2017-05-03 07:18:32]

Committed patchset #25 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/9432ee01aaa7821c3ea5cbff746a...