Add support for external video renderers in mojo.

Add support for external video renderers in mojo.

- Extends RendererConfig to handle VideoDecoders.
- Extends the MOJO interfaces for DemuxerStreams with VIDEO.
- Adds the VideoFrame type plus converters to media types.
- Extends DemuxerStreamProviderShim to handle multiple streams.

BUG=410451
TEST=YouTube videos play with MOJO renderer!

Committed: https://crrev.com/f1dc7cd8b00ee69c5a2b3b5c14435ca1f96464de
Cr-Commit-Position: refs/heads/master@{#302728}

[DaleCurtis, 2014-10-28 23:36:55]

dalecurtis@chromium.org changed reviewers:
+ pwestin@chromium.org, xhwang@chromium.org

[DaleCurtis, 2014-10-28 23:36:56]

Please provide only high level commentary, this needs lots of cleanup :)

[xhwang, 2014-10-29 00:22:14]

Thanks for the fast CL!

Looking pretty good. Here's my first round of comments.

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/demuxe...
File media/mojo/interfaces/demuxer_stream.mojom (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/demuxe...
media/mojo/interfaces/demuxer_stream.mojom:53: // A new AudioDecoderConfig is
available. Will be sent by the DemuxerStream
s/Audio/Video

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/media_...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/media_...
media/mojo/interfaces/media_types.mojom:117: // gpu::VideoCodecProfile.
Update: needs to match media::VideoCodecProfile.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:17: DemuxerStream::Type type)
Can we drop this |type| param and populate |type_| in
OnAudioDecoderConfigChanged() and OnVideoDecoderConfigChanged()?

We won't return the stream_ready_cb before we have a valid config (thus a type),
so it should be safe.

I like this better because this way we don't need to pass in the type in
MojoDemuxerStreamAdapter's ctor. This will make DemuxerStreamProviderShim::ctor
cleaner, especially in the future when we use Array(DemuxerStreamPtr).

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:76:
DCHECK(!video_config_queue_.empty());
Some clean up idea for the future. We have both audio and video code in this
class which results in a lot of duplicate code. Maybe we can make
MojoDemuxerStreamAdapter a templated class based on Type, as we did for
DecoderStream.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_impl.cc (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.cc:44:
mojo::VideoDecoderConfig::From(stream_->video_decoder_config()));
ditto about templatize MojoDemuxerStreamImpl.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:15: #include
"media/base/serial_runner.h"
not used?

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:41: static void
PaintNothingDropEverything(const scoped_refptr<VideoFrame>& frame) {
PaintNothing == DropEverything ? Pick one of them?

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:73: }
see above comments

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:89: // TODO(dalecurtis): What to do
here?? Probably we need an API for this.
See https://code.google.com/p/chromium/issues/detail?id=420025#c34

I think I can move the liveness into the DemuxerStream itself. Then we don't
need to worry about it here any more!

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:95: if (++streams_ready_ ==
streams_.size()) {
I like this!

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:97: demuxer_ready_cb_.Reset();
base::ResetAndReturn(&demuxer_ready_cb).Run()?

[DaleCurtis, 2014-10-30 23:13:57]

Patchset #2 (id:20001) has been deleted

[DaleCurtis, 2014-10-30 23:16:56]

dalecurtis@chromium.org changed reviewers:
+ jamesr@chromium.org

[DaleCurtis, 2014-10-30 23:16:57]

PTAL! Comments addressed. Code rebased and cleaned up for final review.

+jamesr for mojo/converters DEPS entry.

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/demuxe...
File media/mojo/interfaces/demuxer_stream.mojom (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/demuxe...
media/mojo/interfaces/demuxer_stream.mojom:53: // A new AudioDecoderConfig is
available. Will be sent by the DemuxerStream
On 2014/10/29 00:22:13, xhwang wrote:
> s/Audio/Video

Done.

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/media_...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/interfaces/media_...
media/mojo/interfaces/media_types.mojom:117: // gpu::VideoCodecProfile.
On 2014/10/29 00:22:13, xhwang wrote:
> Update: needs to match media::VideoCodecProfile.

Done.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:17: DemuxerStream::Type type)
On 2014/10/29 00:22:13, xhwang wrote:
> Can we drop this |type| param and populate |type_| in
> OnAudioDecoderConfigChanged() and OnVideoDecoderConfigChanged()?
> 
> We won't return the stream_ready_cb before we have a valid config (thus a
type),
> so it should be safe.
> 
> I like this better because this way we don't need to pass in the type in
> MojoDemuxerStreamAdapter's ctor. This will make
DemuxerStreamProviderShim::ctor
> cleaner, especially in the future when we use Array(DemuxerStreamPtr).

Done.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:76:
DCHECK(!video_config_queue_.empty());
On 2014/10/29 00:22:13, xhwang wrote:
> Some clean up idea for the future. We have both audio and video code in this
> class which results in a lot of duplicate code. Maybe we can make
> MojoDemuxerStreamAdapter a templated class based on Type, as we did for
> DecoderStream.

Maybe, that's the opposite of what you're suggesting above though... at least
without having some sort of inner class which is templated appropriately for
each.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:15: #include
"media/base/serial_runner.h"
On 2014/10/29 00:22:13, xhwang wrote:
> not used?

Done.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:41: static void
PaintNothingDropEverything(const scoped_refptr<VideoFrame>& frame) {
On 2014/10/29 00:22:13, xhwang wrote:
> PaintNothing == DropEverything ? Pick one of them?

Done.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:73: }
On 2014/10/29 00:22:13, xhwang wrote:
> see above comments

Moved out.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:89: // TODO(dalecurtis): What to do
here?? Probably we need an API for this.
On 2014/10/29 00:22:13, xhwang wrote:
> See https://code.google.com/p/chromium/issues/detail?id=420025#c34
> 
> I think I can move the liveness into the DemuxerStream itself. Then we don't
> need to worry about it here any more!

Acknowledged.

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:97: demuxer_ready_cb_.Reset();
On 2014/10/29 00:22:13, xhwang wrote:
> base::ResetAndReturn(&demuxer_ready_cb).Run()?

Done.

[jamesr, 2014-10-31 02:58:24]

lgtm

[xhwang, 2014-10-31 03:25:57]

Yay for Youtube plays! LGTM!

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/684963003/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:76:
DCHECK(!video_config_queue_.empty());
On 2014/10/30 23:16:56, DaleCurtis wrote:
> On 2014/10/29 00:22:13, xhwang wrote:
> > Some clean up idea for the future. We have both audio and video code in this
> > class which results in a lot of duplicate code. Maybe we can make
> > MojoDemuxerStreamAdapter a templated class based on Type, as we did for
> > DecoderStream.
> 
> Maybe, that's the opposite of what you're suggesting above though... at least
> without having some sort of inner class which is templated appropriately for
> each.

Yeah, I just felt that'd be something nice. But honestly I don't know how to do
that yet :)

[DaleCurtis, 2014-10-31 18:07:35]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-10-31 18:08:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/40001

[commit-bot: I haz the power, 2014-10-31 18:14:40]

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

[commit-bot: I haz the power, 2014-10-31 18:14:41]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/76425)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2014-11-01 05:04:54]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-11-01 05:05:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/60001

[commit-bot: I haz the power, 2014-11-01 05:10:48]

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

[commit-bot: I haz the power, 2014-11-01 05:10:49]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2014-11-03 22:47:04]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-11-03 22:47:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/120001

[commit-bot: I haz the power, 2014-11-04 02:12:54]

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

[commit-bot: I haz the power, 2014-11-04 02:12:55]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[DaleCurtis, 2014-11-04 06:22:40]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-11-04 06:23:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/140001

[commit-bot: I haz the power, 2014-11-04 14:52:00]

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

[commit-bot: I haz the power, 2014-11-04 14:52:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[DaleCurtis, 2014-11-04 18:37:01]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-11-04 18:37:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/160001

[DaleCurtis, 2014-11-04 20:50:11]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-11-04 20:51:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/180001

[commit-bot: I haz the power, 2014-11-04 22:14:52]

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

[commit-bot: I haz the power, 2014-11-04 22:14:54]

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

[DaleCurtis, 2014-11-05 00:42:34]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:43:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684963003/180001

[commit-bot: I haz the power, 2014-11-05 01:15:58]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2014-11-05 01:16:31]

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