AndroidVideoDecodeAccelerator can now render to a SurfaceView

AndroidVideoDecodeAccelerator can now render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
Committed: https://crrev.com/cd5d9e95ca27d3a71ef31c21ba82b2f55fa4f49e
Cr-Commit-Position: refs/heads/master@{#373365}

[watk, 2016-01-26 21:24:04]

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

[watk, 2016-01-26 21:24:04]

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

[watk, 2016-01-26 21:28:31]

Description was changed from

==========
AndroidVideoDecodeAccelerator can render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

to

==========
AndroidVideoDecodeAccelerator can render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

[DaleCurtis, 2016-01-26 21:37:14]

Drive by test request! Can we start a content_unittest which spawns up the gpu
video decoder yet with SurfaceView? Or well... just calls the VDA at all :)
Also, pass scoped_refptr by const& to avoid unnecessary atomic add/remove.

[DaleCurtis, 2016-01-26 21:37:25]

(nice work!)

[liberato (no reviews please), 2016-01-26 21:48:45]

i'm happy that the required changes here are pretty small.  i've made
suggestions on how to make them even smaller, if you're inclined.

thanks
-fl

https://codereview.chromium.org/1639963002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1639963002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:28: void
AndroidDeferredRenderingBackingStrategy::Initialize(
Initialize(state_provider, surface_id (which may be kNullSurfaceID) would end up
requiring only very small changes to the existing code.

https://codereview.chromium.org/1639963002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1639963002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:702: if
(surface_texture_) {
it might be simpler to hide all of this inside the deferred strategy, since the
copying strategy won't work with SurfaceViews anyway.

in other words, replace strategy::GetSurfaceTexture with GetScopedJavaSurface,
and let the strategy decide what it wants to do.  the copying strategy will
create an ST like it does now, then wrap it in a scoped java surface.  the
deferred strategy can either create/wrap an ST, or look up and wrap the SV if it
was given a non-null ID in Initialize().

as part of strategy::Initialize, it can be given the surface id directly to use
(or not) as it likes.

all that's left is whether to set allow_overlay, above.  the strategy can just
expose a getter for avda to use.

https://codereview.chromium.org/1639963002/diff/1/content/common/gpu/media/av...
File content/common/gpu/media/avda_codec_image.h (right):

https://codereview.chromium.org/1639963002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_codec_image.h:64: enum { kInvalidCodecBufferIndex
= -1 };
good idea.

[watk, 2016-01-27 00:05:36]

Patchset #4 (id:60001) has been deleted

[watk, 2016-01-27 00:06:14]

Description was changed from

==========
AndroidVideoDecodeAccelerator can render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

to

==========
AndroidVideoDecodeAccelerator can now render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

[watk, 2016-01-27 00:08:25]

liberato: I implemented your suggested refactor. There's a bit of awkwardness
related to the OnFrameAvailableListener.

dalecurtis: I'll take a look at how hard that would be. BTW, I've tested this
with a local change that creates a SurfaceView and configures AVDA with it.

[liberato (no reviews please), 2016-01-27 16:15:33]

nice.

-fl

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_copying_backing_strategy.cc:29:
gfx::ScopedJavaSurface AndroidCopyingBackingStrategy::Initialize(
that's a good idea.

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:230:
scoped_refptr<gfx::SurfaceTexture> surface_texture =
i forgot about the onFrameAvailable listener.  not sure that we'll want to move
it into the strategy (both of them should use it).  comment is fine, though. 
sounds better than "TODO: do something", and there is work to be done here
later.

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:52: //
|kNoSurfaceID| it refers to a SurfaceView to render to.
please make a note that it is required to use the surface if it is provided.

this is somewhat unfortunate, since it makes it hard to switch back to copying +
WMPI for testing.  can you add a flag similar to
NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE, that controls whether the fullscreen SV
path happens at runtime?

it would be eventually be set if and only if NEEDS_ALL... is set, but we want to
keep it separate.  it would let us use the copy strategy with WMPI, and also try
deferred without SV.  that's going to help with perf / power testing.

plus, it gives you a flag to turn off SV by default, but still land the code.

the answer might be 'no', since i don't remember how far up the stack one needs
to go to turn off the SV optimization.  the flags that i'm talking about don't
leave GVD.  if it's a lot of plumbing, then just be sure to document how to turn
SV off.  this suggestion was mostly "in case it's easy".

[watk, 2016-01-27 20:45:29]

On 2016/01/27 16:15:33, liberato wrote:
> nice.
> 
> -fl
> 
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> File content/common/gpu/media/android_copying_backing_strategy.cc (right):
> 
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> content/common/gpu/media/android_copying_backing_strategy.cc:29:
> gfx::ScopedJavaSurface AndroidCopyingBackingStrategy::Initialize(
> that's a good idea.
> 
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> File content/common/gpu/media/android_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> content/common/gpu/media/android_video_decode_accelerator.cc:230:
> scoped_refptr<gfx::SurfaceTexture> surface_texture =
> i forgot about the onFrameAvailable listener.  not sure that we'll want to
move
> it into the strategy (both of them should use it).  comment is fine, though. 
> sounds better than "TODO: do something", and there is work to be done here
> later.
> 
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> File content/common/gpu/media/android_video_decode_accelerator.h (right):
> 
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> content/common/gpu/media/android_video_decode_accelerator.h:52: //
> |kNoSurfaceID| it refers to a SurfaceView to render to.
> please make a note that it is required to use the surface if it is provided.
> 
> this is somewhat unfortunate, since it makes it hard to switch back to copying
+
> WMPI for testing.  can you add a flag similar to
> NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE, that controls whether the fullscreen SV
> path happens at runtime?
> 
> it would be eventually be set if and only if NEEDS_ALL... is set, but we want
to
> keep it separate.  it would let us use the copy strategy with WMPI, and also
try
> deferred without SV.  that's going to help with perf / power testing.
> 
> plus, it gives you a flag to turn off SV by default, but still land the code.
> 
> the answer might be 'no', since i don't remember how far up the stack one
needs
> to go to turn off the SV optimization.  the flags that i'm talking about don't
> leave GVD.  if it's a lot of plumbing, then just be sure to document how to
turn
> SV off.  this suggestion was mostly "in case it's easy".

I was thinking I'd land the changes to use the SV with a flag like
--enable-surface-view-fullscreen or whatever. So without the flag both copying
and deferred would work as they do now. With the flag copying would break and
deferred would use SV. Sound good?

[liberato (no reviews please), 2016-01-27 20:47:51]

On 2016/01/27 20:45:29, watk wrote:
> On 2016/01/27 16:15:33, liberato wrote:
> > nice.
> > 
> > -fl
> > 
> >
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> > File content/common/gpu/media/android_copying_backing_strategy.cc (right):
> > 
> >
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> > content/common/gpu/media/android_copying_backing_strategy.cc:29:
> > gfx::ScopedJavaSurface AndroidCopyingBackingStrategy::Initialize(
> > that's a good idea.
> > 
> >
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> > File content/common/gpu/media/android_video_decode_accelerator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> > content/common/gpu/media/android_video_decode_accelerator.cc:230:
> > scoped_refptr<gfx::SurfaceTexture> surface_texture =
> > i forgot about the onFrameAvailable listener.  not sure that we'll want to
> move
> > it into the strategy (both of them should use it).  comment is fine, though.

> > sounds better than "TODO: do something", and there is work to be done here
> > later.
> > 
> >
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> > File content/common/gpu/media/android_video_decode_accelerator.h (right):
> > 
> >
>
https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
> > content/common/gpu/media/android_video_decode_accelerator.h:52: //
> > |kNoSurfaceID| it refers to a SurfaceView to render to.
> > please make a note that it is required to use the surface if it is provided.
> > 
> > this is somewhat unfortunate, since it makes it hard to switch back to
copying
> +
> > WMPI for testing.  can you add a flag similar to
> > NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE, that controls whether the fullscreen SV
> > path happens at runtime?
> > 
> > it would be eventually be set if and only if NEEDS_ALL... is set, but we
want
> to
> > keep it separate.  it would let us use the copy strategy with WMPI, and also
> try
> > deferred without SV.  that's going to help with perf / power testing.
> > 
> > plus, it gives you a flag to turn off SV by default, but still land the
code.
> > 
> > the answer might be 'no', since i don't remember how far up the stack one
> needs
> > to go to turn off the SV optimization.  the flags that i'm talking about
don't
> > leave GVD.  if it's a lot of plumbing, then just be sure to document how to
> turn
> > SV off.  this suggestion was mostly "in case it's easy".
> 
> I was thinking I'd land the changes to use the SV with a flag like
> --enable-surface-view-fullscreen or whatever. So without the flag both copying
> and deferred would work as they do now. With the flag copying would break and
> deferred would use SV. Sound good?

lgtm.

[DaleCurtis, 2016-01-27 21:48:13]

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1639963002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_copying_backing_strategy.cc:32:
DCHECK(surface_view_id ==
DCHECK_EQ

[watk, 2016-01-29 02:30:58]

Patchset #5 (id:100001) has been deleted

[watk, 2016-01-29 02:42:19]

Testing was discussed offline. The plan is to add a content_browsertest once the
renderer side changes for SurfaceView fullscreen land, because at that point we
will only need to write a test that plays a fullscreen video to test this path,
rather than implementing test infrastructure to create the SurfaceView and other
dependencies manually.

https://codereview.chromium.org/1639963002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1639963002/diff/140001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:31:
AndroidCopyingBackingStrategy(AVDAStateProvider* state_provider);
I moved this because Debug builds dereference state_provider in Cleanup(). Which
is called regardless of whether Initialize() was called.

[watk, 2016-01-29 20:56:59]

This is ready to submit, so I'll need an owner lgtm from sandersd and another
lgtm from liberato for the changes since the last one.

[liberato (no reviews please), 2016-01-29 23:28:46]

lgtm % nit

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:47:
GLuint service_id = 0;
can this be moved into the surface texture case? should be okay, though maybe
something is using it in the surface view case without checking to see if it is
nonzero. ii don't think that anything should be using it in the SV case.

[sandersd (OOO until July 31), 2016-02-01 19:58:26]

LGTM % nits.

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:46: bool
PicturesAreOverlayable() override;
Nit: ArePicturesOverlayable()

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:236: "Java surface
is empty.";
Handle this error somehow?

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.cc (right):

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:177: DCHECK(surface_texture_);
Nit: blank line before comment.

[watk, 2016-02-02 02:19:01]

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

[watk, 2016-02-02 02:19:02]

dcheng@chromium.org: Please review changes in gpu_messages.h

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1639963002/diff/160001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:47:
GLuint service_id = 0;
On 2016/01/29 23:28:46, liberato wrote:
> can this be moved into the surface texture case? should be okay, though maybe
> something is using it in the surface view case without checking to see if it
is
> nonzero. ii don't think that anything should be using it in the SV case.

Unfortunately we DCHECK that it's non-zero in SetImageForPicture. I guess I'll
leave it for now.

[dcheng, 2016-02-02 07:37:26]

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:35:
DCHECK_EQ(surface_view_id,
Ultimately, this is invoked by a message sent by the renderer, right? Do we
really want to DCHECK here? Seems like we should fail gracefully
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-i...)

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:31:
AndroidCopyingBackingStrategy(AVDAStateProvider* state_provider);
Make this explicit.

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.h
(right):

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.h:36:
AndroidDeferredRenderingBackingStrategy(AVDAStateProvider* state_provider);
Ditto: explicit

[watk, 2016-02-02 19:56:45]

Thanks, done

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:35:
DCHECK_EQ(surface_view_id,
On 2016/02/02 07:37:26, dcheng wrote:
> Ultimately, this is invoked by a message sent by the renderer, right? Do we
> really want to DCHECK here? Seems like we should fail gracefully
>
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-i...)

Done.

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:31:
AndroidCopyingBackingStrategy(AVDAStateProvider* state_provider);
On 2016/02/02 07:37:26, dcheng wrote:
> Make this explicit.

Done.

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.h
(right):

https://codereview.chromium.org/1639963002/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.h:36:
AndroidDeferredRenderingBackingStrategy(AVDAStateProvider* state_provider);
On 2016/02/02 07:37:26, dcheng wrote:
> Ditto: explicit

Done.

[dcheng, 2016-02-02 21:20:50]

lgtm

[watk, 2016-02-02 21:23:40]

The CQ bit was checked by watk@chromium.org

[watk, 2016-02-02 21:23:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1639963002/#ps220001
(title: "rebase only")

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

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

[commit-bot: I haz the power, 2016-02-02 23:44:49]

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

[commit-bot: I haz the power, 2016-02-02 23:44:50]

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

[watk, 2016-02-03 21:04:29]

The CQ bit was checked by watk@chromium.org

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

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

[commit-bot: I haz the power, 2016-02-03 22:37:11]

Description was changed from

==========
AndroidVideoDecodeAccelerator can now render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

to

==========
AndroidVideoDecodeAccelerator can now render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

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

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-02-03 22:38:04]

Description was changed from

==========
AndroidVideoDecodeAccelerator can now render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630
==========

to

==========
AndroidVideoDecodeAccelerator can now render to a SurfaceView

Previously AVDA would always configure MediaCodec with a SurfaceTexture.
Now, to support efficient fullscreen video, it can render to a
SurfaceView too.

BUG=533630

Committed: https://crrev.com/cd5d9e95ca27d3a71ef31c21ba82b2f55fa4f49e
Cr-Commit-Position: refs/heads/master@{#373365}
==========

[commit-bot: I haz the power, 2016-02-03 22:38:05]

Patchset 10 (id:??) landed as
https://crrev.com/cd5d9e95ca27d3a71ef31c21ba82b2f55fa4f49e
Cr-Commit-Position: refs/heads/master@{#373365}