Proxy RtcVideoDecoder calls to a media::VideoDecoder.

Proxy RtcVideoDecoder calls to a media::VideoDecoder.

RtcVideoDecoder currently talks directly to a VideoDecodeAccelerator,
managing all of the state necessary to do hardware decoding with a
remote VDA.

Instead, it should proxy calls to a media::GpuVideoDecoder, which
manages this state properly, simplifying RtcVideoDecoder to be a much-
simpler shim class. This allows us to remove a significant amount of
brittle, dupliucated code, and prepares for the deprecation of VDA in
favor of media::MojoVideoDecoder.

BUG=

[slan, 2016-10-12 20:21:43]

slan@chromium.org changed reviewers:
+ emircan@chromium.org

[slan, 2016-10-12 20:21:43]

This is ready for the first round of review. This change is the first building
block in use MojoVideoDecoder to support HW-decode. With this change (plus
several others) applied, I can do remote decoding with mojo.

Emircan, please add more reviewers as needed. What platforms should I test this
on?

[emircan, 2016-10-12 20:25:53]

Description was changed from

==========
Proxy RtcVideoDecoder calls to a media::VideoDecoder.

RtcVideoDecoder currently talks directly to a VideoDecodeAccelerator,
managing all of the state necessary to do hardware decoding with a
remote VDA.

Instead, it should proxy calls to a media::GpuVideoDecoder, which
manages this state properly, simplifying RtcVideoDecoder to be a much-
simpler shim class. This allows us to remove a significant amount of
brittle, dupliucated code, and prepares for the deprecation of VDA in
favor of media::MojoVideoDecoder.

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

to

==========
Proxy RtcVideoDecoder calls to a media::VideoDecoder.

RtcVideoDecoder currently talks directly to a VideoDecodeAccelerator,
managing all of the state necessary to do hardware decoding with a
remote VDA.

Instead, it should proxy calls to a media::GpuVideoDecoder, which
manages this state properly, simplifying RtcVideoDecoder to be a much-
simpler shim class. This allows us to remove a significant amount of
brittle, dupliucated code, and prepares for the deprecation of VDA in
favor of media::MojoVideoDecoder.

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

[emircan, 2016-10-12 20:25:53]

emircan@chromium.org changed reviewers:
+ wuchengli@chromium.org

[emircan, 2016-10-12 20:30:28]

emircan@chromium.org changed reviewers:
+ posciak@google.com

[emircan, 2016-10-12 20:32:58]

> Emircan, please add more reviewers as needed. What platforms should I test
this
> on?
I added wuchengli@ and posciak@.

Windows and Mac have HW H264 support. CrOS have additional VPX support on some
boards. You can test it via AppRTC loopback, i.e.
https://apprtc.appspot.com/?debug=loopback&vsc=h264

[slan, 2016-10-18 18:58:02]

Friendly ping for review! This patch works well with H264 on Mac. 

One note for review: PS2 blocks InitDecode while the decoder is created, rather
than doing this at creation time. This allows us to pass the real config values
to the VideoDecoder, rather than use dummy values. Please take a look!

[emircan, 2016-10-18 18:59:38]

emircan@chromium.org changed reviewers:
+ posciak@chromium.org
- posciak@google.com

[slan, 2016-10-19 22:02:19]

On 2016/10/18 18:58:02, slan wrote:
> Friendly ping for review! This patch works well with H264 on Mac. 
> 
> One note for review: PS2 blocks InitDecode while the decoder is created,
rather
> than doing this at creation time. This allows us to pass the real config
values
> to the VideoDecoder, rather than use dummy values. Please take a look!

Another friendly 24-hr ping! Reviewers: please let me know if there are others I
should put on this CL instead.

Thank you!

[emircan, 2016-10-20 17:52:25]

First pass. Can you run the trybots please?

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_decoder.cc (right):

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:137:
DCHECK(!decoder_task_runner->BelongsToCurrentThread());
Additionally, can you add a check to make sure that these methods, that are not
on |decoder_task_runner_|, are called in sequence via base::SequenceChecker?

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:152: new RTCVideoDecoder(type,
create_video_decoder_cb, decoder_task_runner));
This simplifies things, but does not take into account that we expect to return
nullptr here if the codec is not supported. WebRTC falls back to SW immediately
for that case.

https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:184: media::VideoDecoderConfig
config =
const

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:285: int32_t decoder_buffer_id =
next_decoder_buffer_id_;
const

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:489:
base::Bind(&RTCVideoDecoder::OnFrameReady, weak_factory_.GetWeakPtr()));
Use media::BindToCurrentLoop to make sure they are posted on this thread.

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_decoder_factory.h (right):

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder_factory.h:42: const
CreateVideoDecoderCB& create_video_decoder_cb,
Can we name this as create_mojo_video_decoder_cb or
create_video_decoder_client_cb? Currently it sounds like the callback to create
the actual decoder implementation in gpu process here:
https://cs.chromium.org/chromium/src/media/gpu/gpu_video_decode_accelerator_f...

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:217:
media::GpuVideoAcceleratorFactories* gpu_factories =
media::GpuVideoAcceleratorFactories* const
Also, const auto below.

[slan, 2016-10-25 21:25:32]

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_decoder.cc (right):

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:137:
DCHECK(!decoder_task_runner->BelongsToCurrentThread());
On 2016/10/20 17:52:25, emircan wrote:
> Additionally, can you add a check to make sure that these methods, that are
not
> on |decoder_task_runner_|, are called in sequence via base::SequenceChecker? 

SequenceChecker can only check one of two things: if calls are made from the
same SequencedTaskRunner, or if calls unassociated to a TaskRunner are run on
the same thread. IFAICT, a SequencedTaskRunner is not used in webrtc to call
this interface, so we are stuck checking if all calls are made on the same
thread.

Create() and Destroy() may be called on any thread, so we don't want to check
them. InitDecode(), Decode(), and RegisterDecodeCompleteCallback() are called on
webrtc's DecodingThread, and Release() is called on
Chrome_libJingle_WorkerThread. 

I added a ThreadChecker on the DecodingThread methods for good measure. Let me
know if this satisfies your concerns.

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:152: new RTCVideoDecoder(type,
create_video_decoder_cb, decoder_task_runner));
On 2016/10/20 17:52:25, emircan wrote:
> This simplifies things, but does not take into account that we expect to
return
> nullptr here if the codec is not supported. WebRTC falls back to SW
immediately
> for that case.
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi...

Talked about this offline. We'll do *two* Initialize calls. One now (to query
whether the codec is supported) and one in InitDecode (to initialize the decoder
with the right params). Both of these will block the calling thread.

When media::VideoDecoder adds the ability to do synchronous querying, we will
update the unnecessary call in Create().

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:184: media::VideoDecoderConfig
config =
On 2016/10/20 17:52:25, emircan wrote:
> const

Done here, and in Create() above.

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:285: int32_t decoder_buffer_id =
next_decoder_buffer_id_;
On 2016/10/20 17:52:25, emircan wrote:
> const

Done.

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder.cc:489:
base::Bind(&RTCVideoDecoder::OnFrameReady, weak_factory_.GetWeakPtr()));
On 2016/10/20 17:52:25, emircan wrote:
> Use media::BindToCurrentLoop to make sure they are posted on this thread.

OnFrameReady has a DCHECK to ensure that the task is posted to this thread. I
would rather CHECK and break than unnecessarily trampoline the call (all
media::VideoDecoders that I know of callback on the calling thread).

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_decoder_factory.h (right):

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_decoder_factory.h:42: const
CreateVideoDecoderCB& create_video_decoder_cb,
On 2016/10/20 17:52:25, emircan wrote:
> Can we name this as create_mojo_video_decoder_cb or
> create_video_decoder_client_cb? Currently it sounds like the callback to
create
> the actual decoder implementation in gpu process here:
>
https://cs.chromium.org/chromium/src/media/gpu/gpu_video_decode_accelerator_f...

Using "mojo" in the name would be inaccurate right now, and using "client" would
be misleading (I think "Client" tends to have different implications in Chrome).
What about create_media_video_decoder_cb? I could also beef up the comments
above the type alias.

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/2418613002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:217:
media::GpuVideoAcceleratorFactories* gpu_factories =
On 2016/10/20 17:52:25, emircan wrote:
> media::GpuVideoAcceleratorFactories* const
> Also, const auto below.

Done.

[slan, 2016-10-25 21:25:45]

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

[commit-bot: I haz the power, 2016-10-25 21:26:35]

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

[commit-bot: I haz the power, 2016-10-25 21:31:00]

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

[commit-bot: I haz the power, 2016-10-25 21:31:01]

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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wuchengli, 2017-08-09 04:20:58]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org