MediaResource refactoring to support multiple streams

MediaResource refactoring to support multiple streams

This CL changes the old GetStream method returning a single stream into
a new GetAllStreams returning all available demuxer streams.

BUG=487288
Review-Url: https://codereview.chromium.org/2491043003
Cr-Commit-Position: refs/heads/master@{#450819}
Committed: https://chromium.googlesource.com/chromium/src/+/8acf5dcbc4e83fad7f23eb0380e0635058a636bc

[servolk, 2016-11-09 19:39:38]

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

[commit-bot: I haz the power, 2016-11-09 19:40:18]

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

[commit-bot: I haz the power, 2016-11-09 19:54:32]

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

[commit-bot: I haz the power, 2016-11-09 19:54:33]

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

[servolk, 2016-11-10 02:41:05]

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

[commit-bot: I haz the power, 2016-11-10 02:41:31]

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-11-10 02:54:18]

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

[commit-bot: I haz the power, 2016-11-10 02:54:19]

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

[servolk, 2016-12-02 23:03:55]

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

[commit-bot: I haz the power, 2016-12-02 23:04:32]

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-12-02 23:08:00]

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

[commit-bot: I haz the power, 2016-12-02 23:08:01]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[servolk, 2016-12-02 23:28:26]

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

[commit-bot: I haz the power, 2016-12-02 23:28:54]

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-12-02 23:41:50]

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

[commit-bot: I haz the power, 2016-12-02 23:41:51]

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

[servolk, 2016-12-02 23:54:38]

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

[commit-bot: I haz the power, 2016-12-02 23:55:11]

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-12-03 00:12:04]

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

[commit-bot: I haz the power, 2016-12-03 00:12:04]

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

[servolk, 2016-12-03 00:24:02]

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

[commit-bot: I haz the power, 2016-12-03 00:24:43]

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-12-03 00:40:05]

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

[commit-bot: I haz the power, 2016-12-03 00:40:06]

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

[servolk, 2016-12-03 01:51:36]

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

[commit-bot: I haz the power, 2016-12-03 01:52:08]

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-12-03 02:09:42]

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

[commit-bot: I haz the power, 2016-12-03 02:09:43]

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

[servolk, 2016-12-03 02:13:20]

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

[commit-bot: I haz the power, 2016-12-03 02:13:47]

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-12-03 03:52:14]

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

[commit-bot: I haz the power, 2016-12-03 03:52:15]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-12-07 17:15:17]

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

[commit-bot: I haz the power, 2016-12-07 17:15:48]

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

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

[commit-bot: I haz the power, 2016-12-07 18:48:04]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-12-14 01:25:31]

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

[commit-bot: I haz the power, 2016-12-14 01:26:20]

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-12-14 01:29:03]

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

[commit-bot: I haz the power, 2016-12-14 01:29:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[servolk, 2016-12-14 02:22:33]

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

[commit-bot: I haz the power, 2016-12-14 02:23:04]

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-12-14 03:53:21]

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

[commit-bot: I haz the power, 2016-12-14 03:53:22]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-03 23:18:43]

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

[commit-bot: I haz the power, 2017-01-03 23:19:02]

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-01-04 01:25:29]

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

[commit-bot: I haz the power, 2017-01-04 01:25:30]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-12 17:31:35]

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

[commit-bot: I haz the power, 2017-01-12 17:32:16]

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

[servolk, 2017-01-12 19:42:01]

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

[commit-bot: I haz the power, 2017-01-12 19:42:59]

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-01-12 22:20:23]

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

[commit-bot: I haz the power, 2017-01-12 22:20:24]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-23 23:52:35]

Description was changed from

==========
Add DemuxerStreamProvider::GetStreams function returning all streams

BUG=487288
==========

to

==========
DemuxerStreamProvider refactoring

This CL renames DemuxerStreamProvider into MediaResource and changes
the old GetStream method returning a single stream into a new
GetStreams returning all available demuxer streams, both enabled and
disabled.

BUG=487288
==========

[servolk, 2017-01-23 23:52:35]

servolk@chromium.org changed reviewers:
+ xhwang@chromium.org

[servolk, 2017-01-23 23:52:56]

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

[servolk, 2017-01-24 00:07:54]

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

[commit-bot: I haz the power, 2017-01-24 00:08:44]

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-01-24 00:12:16]

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

[commit-bot: I haz the power, 2017-01-24 00:12:17]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[servolk, 2017-01-24 01:47:37]

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

[commit-bot: I haz the power, 2017-01-24 01:48:25]

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-01-24 04:22:56]

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

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

Dry run: This issue passed the CQ dry run.

[tguilbert, 2017-01-24 23:24:41]

LGTM % nits.

https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/t...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/t...
chromecast/media/cma/test/frame_segmenter_for_test.cc:329: if (audio &&
s->type() == ::media::DemuxerStream::AUDIO) {
NIT: Can you remove one of the ifs, and use

::media::DemuxerStream::Type target_type = audio ? ::media::DemuxerStream::AUDIO
: ::media::DemuxerStream::VIDEO
[...]
if(s->type() == target_type)

https://codereview.chromium.org/2491043003/diff/320001/media/base/demuxer_per...
File media/base/demuxer_perftest.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/base/demuxer_per...
media/base/demuxer_perftest.cc:112: stream->EnableBitstreamConverter();
Could there be any problems with enabling this on non Video streams?

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1033: if (stream)
Can there ever be null streams?

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1039: FFmpegDemuxerStream*
FFmpegDemuxer::GetFFmpegStream(
Should this be renamed to "GetFirstEnabledStream"? There are only 3 callsites,
so this might not be necessary.

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.h (right):

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:89: // DemuxerSteamProvider::Type::STREAM.
MediaResource::Type

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:93: // DemuxerSteamProvider::Type::URL.
Same thing here.

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/services/me...
File media/mojo/services/media_resource_shim.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/services/me...
media/mojo/services/media_resource_shim.cc:37: result.push_back(stream.get());
Is there a difference between this code using the vector copy constructor?

https://codereview.chromium.org/2491043003/diff/320001/media/renderers/render...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/renderers/render...
media/renderers/renderer_impl_unittest.cc:143: if (video_stream_)
Is there a way to make CreateAudioStream and CreateVideoStream cleaner? I do
understand this is a big CL and this might be out of scope.

[servolk, 2017-01-25 00:55:14]

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

[commit-bot: I haz the power, 2017-01-25 00:56:08]

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

[servolk, 2017-01-25 00:57:32]

https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/t...
File chromecast/media/cma/test/frame_segmenter_for_test.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/chromecast/media/cma/t...
chromecast/media/cma/test/frame_segmenter_for_test.cc:329: if (audio &&
s->type() == ::media::DemuxerStream::AUDIO) {
On 2017/01/24 23:24:40, tguilbert wrote:
> NIT: Can you remove one of the ifs, and use
> 
> ::media::DemuxerStream::Type target_type = audio ?
::media::DemuxerStream::AUDIO
> : ::media::DemuxerStream::VIDEO
> [...]
> if(s->type() == target_type)

Done.

https://codereview.chromium.org/2491043003/diff/320001/media/base/demuxer_per...
File media/base/demuxer_perftest.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/base/demuxer_per...
media/base/demuxer_perftest.cc:112: stream->EnableBitstreamConverter();
On 2017/01/24 23:24:40, tguilbert wrote:
> Could there be any problems with enabling this on non Video streams?

I believe there shouldn't be any problems enabling this for all streams. But to
be safe I guess I can add an extra check to do this only for video streams, to
preserve the old behavior.

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1033: if (stream)
On 2017/01/24 23:24:40, tguilbert wrote:
> Can there ever be null streams?

Yes. Streams with unsupported audio/video codecs or unsupported stream types
will be null in the |FFmpegDemuxer::streams_| array, see
FFmpegDemuxer::OnFindStreamInfoDone.

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1039: FFmpegDemuxerStream*
FFmpegDemuxer::GetFFmpegStream(
On 2017/01/24 23:24:40, tguilbert wrote:
> Should this be renamed to "GetFirstEnabledStream"? There are only 3 callsites,
> so this might not be necessary.

Thanks for drawing my attention to this. I've fixed the GetBufferedRanges
implementation to handle multi-track cases correctly and that removed two of
those 3 call sites. As to the remaining one, it's not immediately clear to me if
we also need to change logic there to take into account that we might have
multiple audio tracks.
How about renaming this into GetFirstEnabledFFStream to emphasize the fact that
this returns FFmpeg stream pointer, and not a generic DemuxerStream pointer?

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.h (right):

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:89: // DemuxerSteamProvider::Type::STREAM.
On 2017/01/24 23:24:41, tguilbert wrote:
> MediaResource::Type

Done.

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:93: // DemuxerSteamProvider::Type::URL.
On 2017/01/24 23:24:40, tguilbert wrote:
> Same thing here.

Done.

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/services/me...
File media/mojo/services/media_resource_shim.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/services/me...
media/mojo/services/media_resource_shim.cc:37: result.push_back(stream.get());
On 2017/01/24 23:24:41, tguilbert wrote:
> Is there a difference between this code using the vector copy constructor?

We can't use a copy constructor, since |streams_| is a vector of
std::unique_ptrs which can't be implicitly converted to a vector of
DemuxerStream*, so we have to do this explicitly.

https://codereview.chromium.org/2491043003/diff/320001/media/renderers/render...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/renderers/render...
media/renderers/renderer_impl_unittest.cc:143: if (video_stream_)
On 2017/01/24 23:24:41, tguilbert wrote:
> Is there a way to make CreateAudioStream and CreateVideoStream cleaner? I do
> understand this is a big CL and this might be out of scope.

TBH I don't have any ideas how to improve this. These methods get called
individually from different tests, so this is the best solution that I could
come up so far.

[commit-bot: I haz the power, 2017-01-25 00:59:59]

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

[commit-bot: I haz the power, 2017-01-25 01:00:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[servolk, 2017-01-25 01:58:13]

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

[commit-bot: I haz the power, 2017-01-25 01:59:10]

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

[tguilbert, 2017-01-25 02:36:46]

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1039: FFmpegDemuxerStream*
FFmpegDemuxer::GetFFmpegStream(
On 2017/01/25 00:57:32, servolk wrote:
> On 2017/01/24 23:24:40, tguilbert wrote:
> > Should this be renamed to "GetFirstEnabledStream"? There are only 3
callsites,
> > so this might not be necessary.
> 
> Thanks for drawing my attention to this. I've fixed the GetBufferedRanges
> implementation to handle multi-track cases correctly and that removed two of
> those 3 call sites. As to the remaining one, it's not immediately clear to me
if
> we also need to change logic there to take into account that we might have
> multiple audio tracks.
> How about renaming this into GetFirstEnabledFFStream to emphasize the fact
that
> this returns FFmpeg stream pointer, and not a generic DemuxerStream pointer?

That name seems good to me.

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/services/me...
File media/mojo/services/media_resource_shim.cc (right):

https://codereview.chromium.org/2491043003/diff/320001/media/mojo/services/me...
media/mojo/services/media_resource_shim.cc:37: result.push_back(stream.get());
On 2017/01/25 00:57:32, servolk wrote:
> On 2017/01/24 23:24:41, tguilbert wrote:
> > Is there a difference between this code using the vector copy constructor?
> 
> We can't use a copy constructor, since |streams_| is a vector of
> std::unique_ptrs which can't be implicitly converted to a vector of
> DemuxerStream*, so we have to do this explicitly.

Oops, you are right, I missed |streams_|'s type.

[commit-bot: I haz the power, 2017-01-25 02:50:59]

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

[commit-bot: I haz the power, 2017-01-25 02:51:01]

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

[servolk, 2017-01-25 20:04:32]

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

[commit-bot: I haz the power, 2017-01-25 20:05:29]

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-01-25 22:48:09]

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

[commit-bot: I haz the power, 2017-01-25 22:48:11]

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

[servolk, 2017-01-28 03:50:08]

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

[commit-bot: I haz the power, 2017-01-28 03:50:22]

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-01-28 03:52:40]

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

[commit-bot: I haz the power, 2017-01-28 03:52:42]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[servolk, 2017-01-30 17:37:21]

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

[commit-bot: I haz the power, 2017-01-30 17:37:50]

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-01-30 18:08:17]

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

[commit-bot: I haz the power, 2017-01-30 18:08:18]

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

[servolk, 2017-01-30 18:32:53]

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

[commit-bot: I haz the power, 2017-01-30 18:33:20]

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-01-30 19:06:20]

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

[commit-bot: I haz the power, 2017-01-30 19:06:21]

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

[servolk, 2017-01-30 19:09:20]

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

[commit-bot: I haz the power, 2017-01-30 19:09:56]

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-01-30 21:53:29]

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

[commit-bot: I haz the power, 2017-01-30 21:53:31]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-31 21:08:31]

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

[commit-bot: I haz the power, 2017-01-31 21:09:22]

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

[commit-bot: I haz the power, 2017-02-01 00:46:22]

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

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

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-02-01 18:26:04]

Thanks for the CL!

Could you please do a rename-only CL first, which should be very easy to do from
your side, and will only take < 3 minutes to review. Then make other
non-renaming changes in a separate CL. That would do the reviewers a huge favor.
For the current CL, it's really hard to tell rename changes from other changes,
so I have to look closely on every line.

I do have a few comments and a lot of nits.

https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
File chromecast/media/service/cast_renderer.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
chromecast/media/service/cast_renderer.cc:94: // Obtain the first enabled audio
and video streams.
Here and everywhere else, add a TODO to enable multiple streams.

https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
chromecast/media/service/cast_renderer.cc:98: for (const auto& s : streams) {
tiny nit: This is a local variable in the loop so it won't cause any issue. But
in general we are still against abbreviation names:

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

s/s/stream/?

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
File media/base/media_resource.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:19: base::Callback<void(DemuxerStream*, bool,
base::TimeDelta)>;
Add include for base::Callback and TimeDelta

Please add a comment about what the |bool| is.

nit: This probably should be a RepeatingCallback, but it's up to you.

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:21: // Abstract class that defines how to retrieve
"media sources" in DemuxerStream
nit: Does it make sense to s/sources/resource?

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:29: STREAM,  // Indicates GetStream() should be used
GetStreams

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:36: // Returns a collection of available
DemuxerStream objects. Note that some of
Do you want to keep the  

For Type::STREAM:

part given line 47?

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:37: // those streams might be in disabled state
(check DemuxerStream::enabled() ).
I thought we agreed that disabled streams should not be exposed?

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:58: // and the second parameter is the playback
position where the change occured.
Are these callback parameters? If so the comment should go with the callback
definition. Also, the callback takes 3 params, only two are documented here.

nit: Is it okay for a client to set this multiple times, or multiple clients set
this?

https://codereview.chromium.org/2491043003/diff/480001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:922: }
This will set the meta data according to the last stream in the vector. But in
other cases we only handle the first stream. Will this cause problems? Should we
be consistent?

https://codereview.chromium.org/2491043003/diff/480001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/base/renderer.h#...
media/base/renderer.h:30: // |init_cb| upon completion. |media_resource| must be
valid for
nit: please re-flow the comment

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:502: result.push_back(s.get());
ditto: Are we also pushing disabled streams here?

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:117: bool enabled() const override;
From code search, this is not used by the media pipeline (e.g. Renderer). Do we
really need it on the DemuxerStream interface?

This is also related to the discussion on whether we should expose disabled
streams.

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:118: void set_enabled(bool enabled,
base::TimeDelta timestamp) override;
add an empty line here

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:1013: DemuxerStream*
GetStream(DemuxerStream::Type type) {
ditto about TODO

Do we have tests for multiple streams?

https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.h:119: void SetStreamStatusChangeCB(const
StreamStatusChangeCB& cb);
ditto about enabled() and add an empty line

https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.h:281: // FFmpegDemuxerStream and is enabled.
Add a TODO to support multiple streams? Or a comment if we don't plan to support
it.

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:135: // destroyed. The local demuxer streams
returned by MediaResource
nit: reflow comments

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:113: EXPECT_CALL(demuxer_,
GetStreams()).WillRepeatedly(Return(streams));
It's odd that CreateAudioStream() needs to worry about video. I think you should
refactor this part a bit more, e.g. keep a |streams_| member.

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:122: if (audio_stream_)
ditto

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
media/renderers/renderer_impl.cc:362: }
Do we care about disabled streams? This is another example exposing disabled
streams would make a lot of downstream code harder.

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
media/renderers/renderer_impl.cc:385: std::vector<DemuxerStream*> streams =
media_resource_->GetStreams();
ditto about comment and TODO

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
media/renderers/renderer_impl_unittest.cc:144:
streams.push_back(video_stream_.get());
ditto about refactor this test, e.g. maybe just
s/CreateAudioStream/AddAudioStream

[servolk, 2017-02-01 19:09:25]

On 2017/02/01 18:26:04, xhwang_slow wrote:
> Thanks for the CL!
> 
> Could you please do a rename-only CL first, which should be very easy to do
from
> your side, and will only take < 3 minutes to review. Then make other
> non-renaming changes in a separate CL. That would do the reviewers a huge
favor.
> For the current CL, it's really hard to tell rename changes from other
changes,
> so I have to look closely on every line.
> 
> I do have a few comments and a lot of nits.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
> File chromecast/media/service/cast_renderer.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
> chromecast/media/service/cast_renderer.cc:94: // Obtain the first enabled
audio
> and video streams.
> Here and everywhere else, add a TODO to enable multiple streams.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
> chromecast/media/service/cast_renderer.cc:98: for (const auto& s : streams) {
> tiny nit: This is a local variable in the loop so it won't cause any issue.
But
> in general we are still against abbreviation names:
> 
> https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 
> s/s/stream/?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> File media/base/media_resource.h (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> media/base/media_resource.h:19: base::Callback<void(DemuxerStream*, bool,
> base::TimeDelta)>;
> Add include for base::Callback and TimeDelta
> 
> Please add a comment about what the |bool| is.
> 
> nit: This probably should be a RepeatingCallback, but it's up to you.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> media/base/media_resource.h:21: // Abstract class that defines how to retrieve
> "media sources" in DemuxerStream
> nit: Does it make sense to s/sources/resource?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> media/base/media_resource.h:29: STREAM,  // Indicates GetStream() should be
used
> GetStreams
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> media/base/media_resource.h:36: // Returns a collection of available
> DemuxerStream objects. Note that some of
> Do you want to keep the  
> 
> For Type::STREAM:
> 
> part given line 47?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> media/base/media_resource.h:37: // those streams might be in disabled state
> (check DemuxerStream::enabled() ).
> I thought we agreed that disabled streams should not be exposed?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
> media/base/media_resource.h:58: // and the second parameter is the playback
> position where the change occured.
> Are these callback parameters? If so the comment should go with the callback
> definition. Also, the callback takes 3 params, only two are documented here.
> 
> nit: Is it okay for a client to set this multiple times, or multiple clients
set
> this?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/pipeline_im...
> File media/base/pipeline_impl.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:922: }
> This will set the meta data according to the last stream in the vector. But in
> other cases we only handle the first stream. Will this cause problems? Should
we
> be consistent?
> 
> https://codereview.chromium.org/2491043003/diff/480001/media/base/renderer.h
> File media/base/renderer.h (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/base/renderer.h#...
> media/base/renderer.h:30: // |init_cb| upon completion. |media_resource| must
be
> valid for
> nit: please re-flow the comment
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> media/filters/chunk_demuxer.cc:502: result.push_back(s.get());
> ditto: Are we also pushing disabled streams here?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> File media/filters/chunk_demuxer.h (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> media/filters/chunk_demuxer.h:117: bool enabled() const override;
> From code search, this is not used by the media pipeline (e.g. Renderer). Do
we
> really need it on the DemuxerStream interface?
> 
> This is also related to the discussion on whether we should expose disabled
> streams.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> media/filters/chunk_demuxer.h:118: void set_enabled(bool enabled,
> base::TimeDelta timestamp) override;
> add an empty line here
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> File media/filters/chunk_demuxer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
> media/filters/chunk_demuxer_unittest.cc:1013: DemuxerStream*
> GetStream(DemuxerStream::Type type) {
> ditto about TODO
> 
> Do we have tests for multiple streams?
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
> File media/filters/ffmpeg_demuxer.h (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.h:119: void SetStreamStatusChangeCB(const
> StreamStatusChangeCB& cb);
> ditto about enabled() and add an empty line
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.h:281: // FFmpegDemuxerStream and is enabled.
> Add a TODO to support multiple streams? Or a comment if we don't plan to
support
> it.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
> File media/mojo/clients/mojo_renderer.h (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer.h:135: // destroyed. The local demuxer
streams
> returned by MediaResource
> nit: reflow comments
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
> File media/mojo/clients/mojo_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer_unittest.cc:113: EXPECT_CALL(demuxer_,
> GetStreams()).WillRepeatedly(Return(streams));
> It's odd that CreateAudioStream() needs to worry about video. I think you
should
> refactor this part a bit more, e.g. keep a |streams_| member.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer_unittest.cc:122: if (audio_stream_)
> ditto
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
> File media/renderers/renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
> media/renderers/renderer_impl.cc:362: }
> Do we care about disabled streams? This is another example exposing disabled
> streams would make a lot of downstream code harder.
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
> media/renderers/renderer_impl.cc:385: std::vector<DemuxerStream*> streams =
> media_resource_->GetStreams();
> ditto about comment and TODO
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
> File media/renderers/renderer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
> media/renderers/renderer_impl_unittest.cc:144:
> streams.push_back(video_stream_.get());
> ditto about refactor this test, e.g. maybe just
> s/CreateAudioStream/AddAudioStream

Thanks for your comments Xiaohan! I'll try to address them all ASAP.
I agree that it would probably much easier to review this CL if it was split
into several separate changes. There are 3 big changes in here:
1. Replacing the DemuxerStreamProvider::GetStream function returning a single
DemuxerStream with the GetStreams function returning all streams at once.
2. Renaming of DemuxerStreamProvider into MediaResource.
3. Moving stream status notifications (SetStreamStatusChangeCB) from individual
DemuxerStreams to MediaResource.

In fact I do keep those 3 changes as 3 separate commits in my local source tree
and so I can easily upload them as 3 separate CLs. But I've done those changes
in the order described above and changing the order of CLs is not going to be
easy. Hopefully the ordering of those CLs doesn't make much difference for the
review, so long as those changes are separated from each other.
I'll keep only the change #1 in this CL now and will upload #2 and #3 above as
separate CLs soon.

[servolk, 2017-02-01 22:15:23]

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

[commit-bot: I haz the power, 2017-02-01 22:15:58]

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

[servolk, 2017-02-01 22:29:21]

https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
File chromecast/media/service/cast_renderer.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
chromecast/media/service/cast_renderer.cc:94: // Obtain the first enabled audio
and video streams.
On 2017/02/01 18:26:03, xhwang_slow wrote:
> Here and everywhere else, add a TODO to enable multiple streams.

Done.

https://codereview.chromium.org/2491043003/diff/480001/chromecast/media/servi...
chromecast/media/service/cast_renderer.cc:98: for (const auto& s : streams) {
On 2017/02/01 18:26:03, xhwang_slow wrote:
> tiny nit: This is a local variable in the loop so it won't cause any issue.
But
> in general we are still against abbreviation names:
> 
> https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 
> s/s/stream/?

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
File media/base/media_resource.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:19: base::Callback<void(DemuxerStream*, bool,
base::TimeDelta)>;
On 2017/02/01 18:26:03, xhwang_slow wrote:
> Add include for base::Callback and TimeDelta
> 
> Please add a comment about what the |bool| is.
> 
> nit: This probably should be a RepeatingCallback, but it's up to you.

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:21: // Abstract class that defines how to retrieve
"media sources" in DemuxerStream
On 2017/02/01 18:26:03, xhwang_slow wrote:
> nit: Does it make sense to s/sources/resource?

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:29: STREAM,  // Indicates GetStream() should be used
On 2017/02/01 18:26:03, xhwang_slow wrote:
> GetStreams

Done (in the next CL)

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:36: // Returns a collection of available
DemuxerStream objects. Note that some of
On 2017/02/01 18:26:03, xhwang_slow wrote:
> Do you want to keep the  
> 
> For Type::STREAM:
> 
> part given line 47?

Done (in the next CL)

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:37: // those streams might be in disabled state
(check DemuxerStream::enabled() ).
On 2017/02/01 18:26:03, xhwang_slow wrote:
> I thought we agreed that disabled streams should not be exposed?

Yes. Although I haven't done that yet in this CL, and your comment about whether
HasEncryptedStreams actually makes me wonder if we'll need to reconsider this.
Let's continue this discussion in a follow up CL that moves around the
SetStreamStatusChangeCB.

https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou...
media/base/media_resource.h:58: // and the second parameter is the playback
position where the change occured.
On 2017/02/01 18:26:03, xhwang_slow wrote:
> Are these callback parameters? If so the comment should go with the callback
> definition. Also, the callback takes 3 params, only two are documented here.
> 
> nit: Is it okay for a client to set this multiple times, or multiple clients
set
> this?

Moved the comment about params to the callback definition.

Re multiple clients - no, currently only one client is supported. But that's
probably ok, because the subscriber to those callbacks is the renderer, and IIUC
we have only one renderer at a time.

https://codereview.chromium.org/2491043003/diff/480001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:922: }
On 2017/02/01 18:26:03, xhwang_slow wrote:
> This will set the meta data according to the last stream in the vector. But in
> other cases we only handle the first stream. Will this cause problems? Should
we
> be consistent?

No, this will set metadata according to the first stream of each type, because
only for the first stream the metadata.has_video / metadata.has_audio in the if
condition will be false. So we are gonna be consistent.

https://codereview.chromium.org/2491043003/diff/480001/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/base/renderer.h#...
media/base/renderer.h:30: // |init_cb| upon completion. |media_resource| must be
valid for
On 2017/02/01 18:26:03, xhwang_slow wrote:
> nit: please re-flow the comment

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:117: bool enabled() const override;
On 2017/02/01 18:26:03, xhwang_slow wrote:
> From code search, this is not used by the media pipeline (e.g. Renderer). Do
we
> really need it on the DemuxerStream interface?
> 
> This is also related to the discussion on whether we should expose disabled
> streams.

We definitely need this in the DemuxerStream interface since either the renderer
or demuxer needs to check this to figure out if the stream needs to be
decoded/rendered or not. Also this is necessary to properly reflect the
enabled/disabled status of the DecryptingDemuxerStream, which wraps a generic
DemuxerStream* object.

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:118: void set_enabled(bool enabled,
base::TimeDelta timestamp) override;
On 2017/02/01 18:26:03, xhwang_slow wrote:
> add an empty line here

Done (in the next CL)

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:1013: DemuxerStream*
GetStream(DemuxerStream::Type type) {
On 2017/02/01 18:26:04, xhwang_slow wrote:
> ditto about TODO
> 
> Do we have tests for multiple streams?

In this CL I have modified a FFmpegDemuxerTest::Initialize_Multitrack to verify
multiple streams being returned by the FFmpegDemuxer. And we already had a test
for ChunkDemuxer (ChunkDemuxerTest::ChunkDemuxerTest) that tests having multiple
SourceBuffer (and thus implicitly tests having multiple streams).

https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.h:119: void SetStreamStatusChangeCB(const
StreamStatusChangeCB& cb);
On 2017/02/01 18:26:04, xhwang_slow wrote:
> ditto about enabled() and add an empty line

Done (in later CL where SetStreamStatusChangeCB is moved to MediaResource)

https://codereview.chromium.org/2491043003/diff/480001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.h:281: // FFmpegDemuxerStream and is enabled.
On 2017/02/01 18:26:04, xhwang_slow wrote:
> Add a TODO to support multiple streams? Or a comment if we don't plan to
support
> it.

This is an internal function that is only used for determining which ffmpeg
stream to use for seeking. It doesn't need to support multiple streams, the
change to support multiple streams is actually done above at line 221.

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.h (right):

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:135: // destroyed. The local demuxer streams
returned by MediaResource
On 2017/02/01 18:26:04, xhwang_slow wrote:
> nit: reflow comments

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:113: EXPECT_CALL(demuxer_,
GetStreams()).WillRepeatedly(Return(streams));
On 2017/02/01 18:26:04, xhwang_slow wrote:
> It's odd that CreateAudioStream() needs to worry about video. I think you
should
> refactor this part a bit more, e.g. keep a |streams_| member.

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer_unittest.cc:122: if (audio_stream_)
On 2017/02/01 18:26:04, xhwang_slow wrote:
> ditto

Done.

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
media/renderers/renderer_impl.cc:362: }
On 2017/02/01 18:26:04, xhwang_slow wrote:
> Do we care about disabled streams? This is another example exposing disabled
> streams would make a lot of downstream code harder.

Actually I think this, and the unit tests make a good case for reconsidering
whether we want to include disabled streams into GetStreams result or not.
IIUC if at least one of the streams is encrypted, then we want to set up and
initialize the CDM, so that later, if the encrypted stream is enabled we can
start playing it back without tearing down and reinitializing the whole
rendering pipeline. Or can we set up a renderer without CDM and initialize the
CDM later if the encrypted stream gets enabled? This is tricky. Let's move this
discussion to the CL that moves SetStreamStatusChangeCB to the MediaResource.

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/2491043003/diff/480001/media/renderers/render...
media/renderers/renderer_impl_unittest.cc:144:
streams.push_back(video_stream_.get());
On 2017/02/01 18:26:04, xhwang_slow wrote:
> ditto about refactor this test, e.g. maybe just
> s/CreateAudioStream/AddAudioStream

Done.

[servolk, 2017-02-01 22:30:42]

Description was changed from

==========
DemuxerStreamProvider refactoring

This CL renames DemuxerStreamProvider into MediaResource and changes
the old GetStream method returning a single stream into a new
GetStreams returning all available demuxer streams, both enabled and
disabled.

BUG=487288
==========

to

==========
DemuxerStreamProvider refactoring to support multiple streams

This CL changes the old GetStream method returning a single stream into
a new GetStreams returning all available demuxer streams.

BUG=487288
==========

[servolk, 2017-02-02 00:47:25]

On 2017/02/01 19:09:25, servolk wrote:
> On 2017/02/01 18:26:04, xhwang_slow wrote:
> > Thanks for the CL!
> > 
> > Could you please do a rename-only CL first, which should be very easy to do
> from
> > your side, and will only take < 3 minutes to review. Then make other
> > non-renaming changes in a separate CL. That would do the reviewers a huge
> favor.
> > For the current CL, it's really hard to tell rename changes from other
> changes,
> > so I have to look closely on every line.
> > 
> > I do have a few comments and a lot of nits.
> > 
>
> 
> Thanks for your comments Xiaohan! I'll try to address them all ASAP.
> I agree that it would probably much easier to review this CL if it was split
> into several separate changes. There are 3 big changes in here:
> 1. Replacing the DemuxerStreamProvider::GetStream function returning a single
> DemuxerStream with the GetStreams function returning all streams at once.
> 2. Renaming of DemuxerStreamProvider into MediaResource.
> 3. Moving stream status notifications (SetStreamStatusChangeCB) from
individual
> DemuxerStreams to MediaResource.
> 
> In fact I do keep those 3 changes as 3 separate commits in my local source
tree
> and so I can easily upload them as 3 separate CLs. But I've done those changes
> in the order described above and changing the order of CLs is not going to be
> easy. Hopefully the ordering of those CLs doesn't make much difference for the
> review, so long as those changes are separated from each other.
> I'll keep only the change #1 in this CL now and will upload #2 and #3 above as
> separate CLs soon.

Well, disregard this previous comment. It turns out that making changes to
address various CR comments is kind of painful, so I've done some more
refactoring/restructuring on the local CLs and I did reorder it such that the CL
that does the DemuxStreamProvider -> MediaResource (#2 above) is now #1 and no
longer depends on previous changes. The rename has now been extracted into
https://codereview.chromium.org/2668393002/.
And the rest of the changes (GetStream -> GetStreams and moving
SetStreamStatusChangeCB to MediaResource) are now rebased on top of
https://codereview.chromium.org/2668393002/ and are in this CL, to preserve old
CR feedback comments.
I have not made the change to return only enabled streams from
MediaResource::GetStreams yet, because we might need to take even disabled
streams into account when deciding whether or not we need to init CDM, see
https://codereview.chromium.org/2491043003/diff/480001/media/base/media_resou....

[xhwang, 2017-02-02 19:24:28]

Thank you so much for splitting the CL!

Before we get into details again, I'd like to take a step back and discuss some
high level ideas.

dalecurtis / tguilbert: comments welcome!

1. MediaResource API around GetStreams()

I found having all clients dealing with std::vector<DemuxerStream*> is a bit
annoying (check how may cases we use "for (auto& stream : streams)" in this CL).
Also, std::vector isn't very flexible to deal with. I wonder whether it makes
sense to introduce an intermediate interface between MediaResource and
DemuxerStreams. For the sake of discussion, let's name it DemuxerStreamProvider
:) Sorry for bringing this name back, but it seems not a bad name for what it
does.

Another benefit of an interface is that you can easily mock it for tests. I am a
bit sad to see that we have to create empty vectors in multiple tests.

interface DemuxerStreamProvider {
  // Clients that don't support multiple streams don't have to go through all
streams now.
  virtual DemuxerStream* GetFirstAudioStream() = 0;
  virtual DemuxerStream* GetFirstVideoStream() = 0;

  // Only returns enabled streams. In the implementation, you can store
  // both enabled and disabled streams internally.
  virtual vector<DemuxerStream*> GetAllStreams(DemuxerStream::Type type) = 0;
  // or
  virtual vector<DemuxerStream*> GetAllAudioStreams() = 0;
  virtual vector<DemuxerStream*> GetAllVideoStreams() = 0;
  virtual vector<DemuxerStream*> GetAllTextStreams() = 0;

  // Note this is not on MediaResource anymore so MediaUrlDemuxer won't
  // need to worry about it.
  virtual void SetStreamStatusChangeCB(const StreamStatusChangeCB& cb) = 0;

  // This could be limited to enabled streams, or for all streams.
  virtual bool HasEncryptedStreams() = 0;
}

interface MediaResource {
  virtual DemuxerStreamProvider* GetDemuxerStreamProvider() = 0;
  virtual MediaUrlParams GetMediaUrlParams() const;
  ...
}

2. Encrypted playback

I am working on supporting switching between clear (unencrypted) and encrypted
streams. There are definitely complicated but rare cases that we won't support
(by choice). I am writing some docs to make this clear:

https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...

But overall, I don't think this should affect our decision on whether to expose
disabled streams.

[servolk, 2017-02-10 03:40:19]

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

[commit-bot: I haz the power, 2017-02-10 03:40:46]

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

[commit-bot: I haz the power, 2017-02-10 05:26:15]

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

[commit-bot: I haz the power, 2017-02-10 05:26:16]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-02-11 00:32:57]

On 2017/02/02 19:24:28, xhwang_slow wrote:
> Thank you so much for splitting the CL!
> 
> Before we get into details again, I'd like to take a step back and discuss
some
> high level ideas.
> 
> dalecurtis / tguilbert: comments welcome!
> 
> 1. MediaResource API around GetStreams()
> 
> I found having all clients dealing with std::vector<DemuxerStream*> is a bit
> annoying (check how may cases we use "for (auto& stream : streams)" in this
CL).
> Also, std::vector isn't very flexible to deal with. I wonder whether it makes
> sense to introduce an intermediate interface between MediaResource and
> DemuxerStreams. For the sake of discussion, let's name it
DemuxerStreamProvider
> :) Sorry for bringing this name back, but it seems not a bad name for what it
> does.
> 
> Another benefit of an interface is that you can easily mock it for tests. I am
a
> bit sad to see that we have to create empty vectors in multiple tests.
> 
> interface DemuxerStreamProvider {
>   // Clients that don't support multiple streams don't have to go through all
> streams now.
>   virtual DemuxerStream* GetFirstAudioStream() = 0;
>   virtual DemuxerStream* GetFirstVideoStream() = 0;
> 
>   // Only returns enabled streams. In the implementation, you can store
>   // both enabled and disabled streams internally.
>   virtual vector<DemuxerStream*> GetAllStreams(DemuxerStream::Type type) = 0;
>   // or
>   virtual vector<DemuxerStream*> GetAllAudioStreams() = 0;
>   virtual vector<DemuxerStream*> GetAllVideoStreams() = 0;
>   virtual vector<DemuxerStream*> GetAllTextStreams() = 0;
> 
>   // Note this is not on MediaResource anymore so MediaUrlDemuxer won't
>   // need to worry about it.
>   virtual void SetStreamStatusChangeCB(const StreamStatusChangeCB& cb) = 0;
> 
>   // This could be limited to enabled streams, or for all streams.
>   virtual bool HasEncryptedStreams() = 0;
> }
> 
> interface MediaResource {
>   virtual DemuxerStreamProvider* GetDemuxerStreamProvider() = 0;
>   virtual MediaUrlParams GetMediaUrlParams() const;
>   ...
> }
> 
> 2. Encrypted playback
> 
> I am working on supporting switching between clear (unencrypted) and encrypted
> streams. There are definitely complicated but rare cases that we won't support
> (by choice). I am writing some docs to make this clear:
> 
>
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...
> 
> But overall, I don't think this should affect our decision on whether to
expose
> disabled streams.

Sorry for the delay on this, it was a branch cut week for Chromecast. I'm
getting back to media track work now.

Re #1: The primary reason why I proposed refactoring the MediaResource interface
like this, is because having a single GetStreams method returning a collection
of streams is IMHO the simplest API that covers the widest range of scenarios,
including multiple media tracks. The new API is more generic and that makes it
slightly less convenient to use at times. But don't overlook also the fact that
it brings some advantages too! Many places don't really need to care about the
stream type and actually benefit from being able to process a collection of
streams. The change in the mojo_renderer.cc in this CL is a good example -
instead of explicitly dealing with separate audio and video streams, now we can
just use a for loop to avoid duplicating code. The code there is now simpler,
smaller and more generic. We can now add even more demuxer stream types (for
example text streams, or secondary video streams that are used for certain HDR
formats) and this code will still work and won't need to be changed - I think
it's actually a big advantage of this approach. So I'd say that having more "for
(auto& stream : streams)" in the code is not necessarily bad!
As for extracting various GetStream* methods into a new separate interface - TBH
I'm not sure what problems it would solve, can you elaborate please?

Re #2: My main question is still this: if we have multiple audio/video streams
and the initially selected audio and video streams are not encrypted, but some
other streams (that are initially disabled) are encrypted, then will we have an
opportunity to initialize CDM later, when one of the encrypted streams becomes
enabled? Or does CDM need to be initialized before playback starts?
At least in case of Chromecast we need to know upfront, before initializing
media pipeline, whether we are gonna have clear or protected playback, since
media pipeline is set up differently (e.g. protected playback uses secure
memory), and switching between clear and encrypted playback will require tearing
down and reinitializing the whole media pipeline. I suspect that other platforms
might have similar limitations.
If that's the case, then we will need to set up the media pipeline for encrypted
playback, even if only currently disabled streams are encrypted. And in that
case we'll need to have some way to access/inspect disabled stream properties.

[xhwang, 2017-02-11 09:44:19]

On 2017/02/11 00:32:57, servolk wrote:
> On 2017/02/02 19:24:28, xhwang_slow wrote:
> > Thank you so much for splitting the CL!
> > 
> > Before we get into details again, I'd like to take a step back and discuss
> some
> > high level ideas.
> > 
> > dalecurtis / tguilbert: comments welcome!
> > 
> > 1. MediaResource API around GetStreams()
> > 
> > I found having all clients dealing with std::vector<DemuxerStream*> is a bit
> > annoying (check how may cases we use "for (auto& stream : streams)" in this
> CL).
> > Also, std::vector isn't very flexible to deal with. I wonder whether it
makes
> > sense to introduce an intermediate interface between MediaResource and
> > DemuxerStreams. For the sake of discussion, let's name it
> DemuxerStreamProvider
> > :) Sorry for bringing this name back, but it seems not a bad name for what
it
> > does.
> > 
> > Another benefit of an interface is that you can easily mock it for tests. I
am
> a
> > bit sad to see that we have to create empty vectors in multiple tests.
> > 
> > interface DemuxerStreamProvider {
> >   // Clients that don't support multiple streams don't have to go through
all
> > streams now.
> >   virtual DemuxerStream* GetFirstAudioStream() = 0;
> >   virtual DemuxerStream* GetFirstVideoStream() = 0;
> > 
> >   // Only returns enabled streams. In the implementation, you can store
> >   // both enabled and disabled streams internally.
> >   virtual vector<DemuxerStream*> GetAllStreams(DemuxerStream::Type type) =
0;
> >   // or
> >   virtual vector<DemuxerStream*> GetAllAudioStreams() = 0;
> >   virtual vector<DemuxerStream*> GetAllVideoStreams() = 0;
> >   virtual vector<DemuxerStream*> GetAllTextStreams() = 0;
> > 
> >   // Note this is not on MediaResource anymore so MediaUrlDemuxer won't
> >   // need to worry about it.
> >   virtual void SetStreamStatusChangeCB(const StreamStatusChangeCB& cb) = 0;
> > 
> >   // This could be limited to enabled streams, or for all streams.
> >   virtual bool HasEncryptedStreams() = 0;
> > }
> > 
> > interface MediaResource {
> >   virtual DemuxerStreamProvider* GetDemuxerStreamProvider() = 0;
> >   virtual MediaUrlParams GetMediaUrlParams() const;
> >   ...
> > }
> > 
> > 2. Encrypted playback
> > 
> > I am working on supporting switching between clear (unencrypted) and
encrypted
> > streams. There are definitely complicated but rare cases that we won't
support
> > (by choice). I am writing some docs to make this clear:
> > 
> >
>
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...
> > 
> > But overall, I don't think this should affect our decision on whether to
> expose
> > disabled streams.
> 
> Sorry for the delay on this, it was a branch cut week for Chromecast. I'm
> getting back to media track work now.
> 
> Re #1: The primary reason why I proposed refactoring the MediaResource
interface
> like this, is because having a single GetStreams method returning a collection
> of streams is IMHO the simplest API that covers the widest range of scenarios,
> including multiple media tracks. The new API is more generic and that makes it
> slightly less convenient to use at times. But don't overlook also the fact
that
> it brings some advantages too! Many places don't really need to care about the
> stream type and actually benefit from being able to process a collection of
> streams. The change in the mojo_renderer.cc in this CL is a good example -
> instead of explicitly dealing with separate audio and video streams, now we
can
> just use a for loop to avoid duplicating code. The code there is now simpler,
> smaller and more generic. We can now add even more demuxer stream types (for
> example text streams, or secondary video streams that are used for certain HDR
> formats) and this code will still work and won't need to be changed - I think
> it's actually a big advantage of this approach. So I'd say that having more
"for
> (auto& stream : streams)" in the code is not necessarily bad!
> As for extracting various GetStream* methods into a new separate interface -
TBH
> I'm not sure what problems it would solve, can you elaborate please?

I don't see anything you can do now but can't in the new interface.

In this case, you could just have:

class DemuxerStreamProvider {

virtual DemuxerStream* GetFirstAudioStream() = 0;
virtual DemuxerStream* GetFirstVideoStream() = 0;
...
virtual vector<DemuxerStream*> GetAllStreams() = 0;
...
// anything else we need
};

My point is, not all clients need all streams, and it ends up a lot of duplicate
code looping all the streams trying to find something trivial.
 
> Re #2: My main question is still this: if we have multiple audio/video streams
> and the initially selected audio and video streams are not encrypted, but some
> other streams (that are initially disabled) are encrypted, then will we have
an
> opportunity to initialize CDM later, when one of the encrypted streams becomes
> enabled? Or does CDM need to be initialized before playback starts?
> At least in case of Chromecast we need to know upfront, before initializing
> media pipeline, whether we are gonna have clear or protected playback, since
> media pipeline is set up differently (e.g. protected playback uses secure
> memory), and switching between clear and encrypted playback will require
tearing
> down and reinitializing the whole media pipeline. I suspect that other
platforms
> might have similar limitations.
> If that's the case, then we will need to set up the media pipeline for
encrypted
> playback, even if only currently disabled streams are encrypted. And in that
> case we'll need to have some way to access/inspect disabled stream properties.

Good question. Please see this doc for the current plan:
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...

[xhwang, 2017-02-11 09:44:24]

On 2017/02/11 00:32:57, servolk wrote:
> On 2017/02/02 19:24:28, xhwang_slow wrote:
> > Thank you so much for splitting the CL!
> > 
> > Before we get into details again, I'd like to take a step back and discuss
> some
> > high level ideas.
> > 
> > dalecurtis / tguilbert: comments welcome!
> > 
> > 1. MediaResource API around GetStreams()
> > 
> > I found having all clients dealing with std::vector<DemuxerStream*> is a bit
> > annoying (check how may cases we use "for (auto& stream : streams)" in this
> CL).
> > Also, std::vector isn't very flexible to deal with. I wonder whether it
makes
> > sense to introduce an intermediate interface between MediaResource and
> > DemuxerStreams. For the sake of discussion, let's name it
> DemuxerStreamProvider
> > :) Sorry for bringing this name back, but it seems not a bad name for what
it
> > does.
> > 
> > Another benefit of an interface is that you can easily mock it for tests. I
am
> a
> > bit sad to see that we have to create empty vectors in multiple tests.
> > 
> > interface DemuxerStreamProvider {
> >   // Clients that don't support multiple streams don't have to go through
all
> > streams now.
> >   virtual DemuxerStream* GetFirstAudioStream() = 0;
> >   virtual DemuxerStream* GetFirstVideoStream() = 0;
> > 
> >   // Only returns enabled streams. In the implementation, you can store
> >   // both enabled and disabled streams internally.
> >   virtual vector<DemuxerStream*> GetAllStreams(DemuxerStream::Type type) =
0;
> >   // or
> >   virtual vector<DemuxerStream*> GetAllAudioStreams() = 0;
> >   virtual vector<DemuxerStream*> GetAllVideoStreams() = 0;
> >   virtual vector<DemuxerStream*> GetAllTextStreams() = 0;
> > 
> >   // Note this is not on MediaResource anymore so MediaUrlDemuxer won't
> >   // need to worry about it.
> >   virtual void SetStreamStatusChangeCB(const StreamStatusChangeCB& cb) = 0;
> > 
> >   // This could be limited to enabled streams, or for all streams.
> >   virtual bool HasEncryptedStreams() = 0;
> > }
> > 
> > interface MediaResource {
> >   virtual DemuxerStreamProvider* GetDemuxerStreamProvider() = 0;
> >   virtual MediaUrlParams GetMediaUrlParams() const;
> >   ...
> > }
> > 
> > 2. Encrypted playback
> > 
> > I am working on supporting switching between clear (unencrypted) and
encrypted
> > streams. There are definitely complicated but rare cases that we won't
support
> > (by choice). I am writing some docs to make this clear:
> > 
> >
>
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...
> > 
> > But overall, I don't think this should affect our decision on whether to
> expose
> > disabled streams.
> 
> Sorry for the delay on this, it was a branch cut week for Chromecast. I'm
> getting back to media track work now.
> 
> Re #1: The primary reason why I proposed refactoring the MediaResource
interface
> like this, is because having a single GetStreams method returning a collection
> of streams is IMHO the simplest API that covers the widest range of scenarios,
> including multiple media tracks. The new API is more generic and that makes it
> slightly less convenient to use at times. But don't overlook also the fact
that
> it brings some advantages too! Many places don't really need to care about the
> stream type and actually benefit from being able to process a collection of
> streams. The change in the mojo_renderer.cc in this CL is a good example -
> instead of explicitly dealing with separate audio and video streams, now we
can
> just use a for loop to avoid duplicating code. The code there is now simpler,
> smaller and more generic. We can now add even more demuxer stream types (for
> example text streams, or secondary video streams that are used for certain HDR
> formats) and this code will still work and won't need to be changed - I think
> it's actually a big advantage of this approach. So I'd say that having more
"for
> (auto& stream : streams)" in the code is not necessarily bad!
> As for extracting various GetStream* methods into a new separate interface -
TBH
> I'm not sure what problems it would solve, can you elaborate please?

I don't see anything you can do now but can't in the new interface.

In this case, you could just have:

class DemuxerStreamProvider {

virtual DemuxerStream* GetFirstAudioStream() = 0;
virtual DemuxerStream* GetFirstVideoStream() = 0;
...
virtual vector<DemuxerStream*> GetAllStreams() = 0;
...
// anything else we need
};

My point is, not all clients need all streams, and it ends up a lot of duplicate
code looping all the streams trying to find something trivial.
 
> Re #2: My main question is still this: if we have multiple audio/video streams
> and the initially selected audio and video streams are not encrypted, but some
> other streams (that are initially disabled) are encrypted, then will we have
an
> opportunity to initialize CDM later, when one of the encrypted streams becomes
> enabled? Or does CDM need to be initialized before playback starts?
> At least in case of Chromecast we need to know upfront, before initializing
> media pipeline, whether we are gonna have clear or protected playback, since
> media pipeline is set up differently (e.g. protected playback uses secure
> memory), and switching between clear and encrypted playback will require
tearing
> down and reinitializing the whole media pipeline. I suspect that other
platforms
> might have similar limitations.
> If that's the case, then we will need to set up the media pipeline for
encrypted
> playback, even if only currently disabled streams are encrypted. And in that
> case we'll need to have some way to access/inspect disabled stream properties.

Good question. Please see this doc for the current plan:
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...

[servolk, 2017-02-13 18:56:07]

On 2017/02/11 09:44:24, xhwang_slow wrote:
> On 2017/02/11 00:32:57, servolk wrote:
> > On 2017/02/02 19:24:28, xhwang_slow wrote:
> > > Thank you so much for splitting the CL!
> > > 
> > > Before we get into details again, I'd like to take a step back and discuss
> > some
> > > high level ideas.
> > >
> > Sorry for the delay on this, it was a branch cut week for Chromecast. I'm
> > getting back to media track work now.
> > 
> > Re #1: The primary reason why I proposed refactoring the MediaResource
> interface
> > like this, is because having a single GetStreams method returning a
collection
> > of streams is IMHO the simplest API that covers the widest range of
scenarios,
> > including multiple media tracks. The new API is more generic and that makes
it
> > slightly less convenient to use at times. But don't overlook also the fact
> that
> > it brings some advantages too! Many places don't really need to care about
the
> > stream type and actually benefit from being able to process a collection of
> > streams. The change in the mojo_renderer.cc in this CL is a good example -
> > instead of explicitly dealing with separate audio and video streams, now we
> can
> > just use a for loop to avoid duplicating code. The code there is now
simpler,
> > smaller and more generic. We can now add even more demuxer stream types (for
> > example text streams, or secondary video streams that are used for certain
HDR
> > formats) and this code will still work and won't need to be changed - I
think
> > it's actually a big advantage of this approach. So I'd say that having more
> "for
> > (auto& stream : streams)" in the code is not necessarily bad!
> > As for extracting various GetStream* methods into a new separate interface -
> TBH
> > I'm not sure what problems it would solve, can you elaborate please?
> 
> I don't see anything you can do now but can't in the new interface.
> 
> In this case, you could just have:
> 
> class DemuxerStreamProvider {
> 
> virtual DemuxerStream* GetFirstAudioStream() = 0;
> virtual DemuxerStream* GetFirstVideoStream() = 0;
> ...
> virtual vector<DemuxerStream*> GetAllStreams() = 0;
> ...
> // anything else we need
> };
> 
> My point is, not all clients need all streams, and it ends up a lot of
duplicate
> code looping all the streams trying to find something trivial.

Well, yes, once you have the GetAllStreams function you can do anything. I was
just trying to say that once we have the GetAllStreams we could drop the
GetFirstAudio/VideoStream, since those are trivially implemented on top of
GetAllStreams. I just wanted to avoid placing the burden on implementors of
MediaResource interface to provide several APIs for accessing streams where just
one would be sufficient. But I also understand your point about spreading
helpers over various clients of MediaResource. How about this - we can keep only
the GetAllStreams to be a pure virtual function in the MediaResource interface,
so that demuxers and other implementations need to override only one function.
But the we can also add GetFirstEnabledAudioStream and
GetFirstEnabledVideoStream as non-virtual function helpers directly into the
MediaResource class and implement them right there using the GetAllStreams
function. Then we'll be able to get rid of unnecessary duplication in
MediaResource clients, but will still have an interface generic enough to
support any future functionality, including multiple streams. WDYT?

  
> > Re #2: My main question is still this: if we have multiple audio/video
streams
> > and the initially selected audio and video streams are not encrypted, but
some
> > other streams (that are initially disabled) are encrypted, then will we have
> an
> > opportunity to initialize CDM later, when one of the encrypted streams
becomes
> > enabled? Or does CDM need to be initialized before playback starts?
> > At least in case of Chromecast we need to know upfront, before initializing
> > media pipeline, whether we are gonna have clear or protected playback, since
> > media pipeline is set up differently (e.g. protected playback uses secure
> > memory), and switching between clear and encrypted playback will require
> tearing
> > down and reinitializing the whole media pipeline. I suspect that other
> platforms
> > might have similar limitations.
> > If that's the case, then we will need to set up the media pipeline for
> encrypted
> > playback, even if only currently disabled streams are encrypted. And in that
> > case we'll need to have some way to access/inspect disabled stream
properties.
> 
> Good question. Please see this doc for the current plan:
>
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...

I don't have access to this doc, I've just asked for permission to access it.

[xhwang, 2017-02-13 21:08:21]

On 2017/02/13 18:56:07, servolk wrote:
> On 2017/02/11 09:44:24, xhwang_slow wrote:
> > On 2017/02/11 00:32:57, servolk wrote:
> > > On 2017/02/02 19:24:28, xhwang_slow wrote:
> > > > Thank you so much for splitting the CL!
> > > > 
> > > > Before we get into details again, I'd like to take a step back and
discuss
> > > some
> > > > high level ideas.
> > > >
> > > Sorry for the delay on this, it was a branch cut week for Chromecast. I'm
> > > getting back to media track work now.
> > > 
> > > Re #1: The primary reason why I proposed refactoring the MediaResource
> > interface
> > > like this, is because having a single GetStreams method returning a
> collection
> > > of streams is IMHO the simplest API that covers the widest range of
> scenarios,
> > > including multiple media tracks. The new API is more generic and that
makes
> it
> > > slightly less convenient to use at times. But don't overlook also the fact
> > that
> > > it brings some advantages too! Many places don't really need to care about
> the
> > > stream type and actually benefit from being able to process a collection
of
> > > streams. The change in the mojo_renderer.cc in this CL is a good example -
> > > instead of explicitly dealing with separate audio and video streams, now
we
> > can
> > > just use a for loop to avoid duplicating code. The code there is now
> simpler,
> > > smaller and more generic. We can now add even more demuxer stream types
(for
> > > example text streams, or secondary video streams that are used for certain
> HDR
> > > formats) and this code will still work and won't need to be changed - I
> think
> > > it's actually a big advantage of this approach. So I'd say that having
more
> > "for
> > > (auto& stream : streams)" in the code is not necessarily bad!
> > > As for extracting various GetStream* methods into a new separate interface
-
> > TBH
> > > I'm not sure what problems it would solve, can you elaborate please?
> > 
> > I don't see anything you can do now but can't in the new interface.
> > 
> > In this case, you could just have:
> > 
> > class DemuxerStreamProvider {
> > 
> > virtual DemuxerStream* GetFirstAudioStream() = 0;
> > virtual DemuxerStream* GetFirstVideoStream() = 0;
> > ...
> > virtual vector<DemuxerStream*> GetAllStreams() = 0;
> > ...
> > // anything else we need
> > };
> > 
> > My point is, not all clients need all streams, and it ends up a lot of
> duplicate
> > code looping all the streams trying to find something trivial.
> 
> Well, yes, once you have the GetAllStreams function you can do anything. I was
> just trying to say that once we have the GetAllStreams we could drop the
> GetFirstAudio/VideoStream, since those are trivially implemented on top of
> GetAllStreams. I just wanted to avoid placing the burden on implementors of
> MediaResource interface to provide several APIs for accessing streams where
just
> one would be sufficient. But I also understand your point about spreading
> helpers over various clients of MediaResource. How about this - we can keep
only
> the GetAllStreams to be a pure virtual function in the MediaResource
interface,
> so that demuxers and other implementations need to override only one function.
> But the we can also add GetFirstEnabledAudioStream and
> GetFirstEnabledVideoStream as non-virtual function helpers directly into the
> MediaResource class and implement them right there using the GetAllStreams
> function. Then we'll be able to get rid of unnecessary duplication in
> MediaResource clients, but will still have an interface generic enough to
> support any future functionality, including multiple streams. WDYT?

Thanks for thinking about this!

For your proposal, I guess we'll need a static cast based on the type of
MediaResource. We have code like that but it's generally not recommended by the
style guide. Also, it's not as flexible. I suspect if you want to have the
infomation of enabled/disabled streams somewhere, but doesn't want to expose it
to the Renderers, a intermediate class would be a good place to put it. Also, I
feel it's a good place to put HasEncryptedStream() or something similar.

> > > Re #2: My main question is still this: if we have multiple audio/video
> streams
> > > and the initially selected audio and video streams are not encrypted, but
> some
> > > other streams (that are initially disabled) are encrypted, then will we
have
> > an
> > > opportunity to initialize CDM later, when one of the encrypted streams
> becomes
> > > enabled? Or does CDM need to be initialized before playback starts?
> > > At least in case of Chromecast we need to know upfront, before
initializing
> > > media pipeline, whether we are gonna have clear or protected playback,
since
> > > media pipeline is set up differently (e.g. protected playback uses secure
> > > memory), and switching between clear and encrypted playback will require
> > tearing
> > > down and reinitializing the whole media pipeline. I suspect that other
> > platforms
> > > might have similar limitations.
> > > If that's the case, then we will need to set up the media pipeline for
> > encrypted
> > > playback, even if only currently disabled streams are encrypted. And in
that
> > > case we'll need to have some way to access/inspect disabled stream
> properties.
> > 
> > Good question. Please see this doc for the current plan:
> >
>
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...
> 
> I don't have access to this doc, I've just asked for permission to access it.

Shared, or use your chormium.org account.

In short, it's only required to support switching from encrypted to clear
streams when MediaKeys is set before load(). In that case, the cast pipeline
should already have CDM set, and can choose to setup the encrypted pipeline even
though the first decoder config is clear.

[servolk, 2017-02-14 03:25:08]

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

[commit-bot: I haz the power, 2017-02-14 03:25:43]

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

[commit-bot: I haz the power, 2017-02-14 05:08:55]

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

[commit-bot: I haz the power, 2017-02-14 05:08:57]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-02-14 17:51:15]

Description was changed from

==========
DemuxerStreamProvider refactoring to support multiple streams

This CL changes the old GetStream method returning a single stream into
a new GetStreams returning all available demuxer streams.

BUG=487288
==========

to

==========
MediaResource refactoring to support multiple streams

This CL changes the old GetStream method returning a single stream into
a new GetAllStreams returning all available demuxer streams.

BUG=487288
==========

[servolk, 2017-02-14 21:59:18]

On 2017/02/13 21:08:21, xhwang_slow wrote:
> On 2017/02/13 18:56:07, servolk wrote:
> > On 2017/02/11 09:44:24, xhwang_slow wrote:
> > > On 2017/02/11 00:32:57, servolk wrote:
> > > > On 2017/02/02 19:24:28, xhwang_slow wrote:
> > > > > Thank you so much for splitting the CL!
> > > > > 
> > > > > Before we get into details again, I'd like to take a step back and
> discuss
> > > > some
> > > > > high level ideas.
> > > > >
> > > > Sorry for the delay on this, it was a branch cut week for Chromecast.
I'm
> > > > getting back to media track work now.
> > > > 
> > > > Re #1: The primary reason why I proposed refactoring the MediaResource
> > > interface
> > > > like this, is because having a single GetStreams method returning a
> > collection
> > > > of streams is IMHO the simplest API that covers the widest range of
> > scenarios,
> > > > including multiple media tracks. The new API is more generic and that
> makes
> > it
> > > > slightly less convenient to use at times. But don't overlook also the
fact
> > > that
> > > > it brings some advantages too! Many places don't really need to care
about
> > the
> > > > stream type and actually benefit from being able to process a collection
> of
> > > > streams. The change in the mojo_renderer.cc in this CL is a good example
-
> > > > instead of explicitly dealing with separate audio and video streams, now
> we
> > > can
> > > > just use a for loop to avoid duplicating code. The code there is now
> > simpler,
> > > > smaller and more generic. We can now add even more demuxer stream types
> (for
> > > > example text streams, or secondary video streams that are used for
certain
> > HDR
> > > > formats) and this code will still work and won't need to be changed - I
> > think
> > > > it's actually a big advantage of this approach. So I'd say that having
> more
> > > "for
> > > > (auto& stream : streams)" in the code is not necessarily bad!
> > > > As for extracting various GetStream* methods into a new separate
interface
> -
> > > TBH
> > > > I'm not sure what problems it would solve, can you elaborate please?
> > > 
> > > I don't see anything you can do now but can't in the new interface.
> > > 
> > > In this case, you could just have:
> > > 
> > > class DemuxerStreamProvider {
> > > 
> > > virtual DemuxerStream* GetFirstAudioStream() = 0;
> > > virtual DemuxerStream* GetFirstVideoStream() = 0;
> > > ...
> > > virtual vector<DemuxerStream*> GetAllStreams() = 0;
> > > ...
> > > // anything else we need
> > > };
> > > 
> > > My point is, not all clients need all streams, and it ends up a lot of
> > duplicate
> > > code looping all the streams trying to find something trivial.
> > 
> > Well, yes, once you have the GetAllStreams function you can do anything. I
was
> > just trying to say that once we have the GetAllStreams we could drop the
> > GetFirstAudio/VideoStream, since those are trivially implemented on top of
> > GetAllStreams. I just wanted to avoid placing the burden on implementors of
> > MediaResource interface to provide several APIs for accessing streams where
> just
> > one would be sufficient. But I also understand your point about spreading
> > helpers over various clients of MediaResource. How about this - we can keep
> only
> > the GetAllStreams to be a pure virtual function in the MediaResource
> interface,
> > so that demuxers and other implementations need to override only one
function.
> > But the we can also add GetFirstEnabledAudioStream and
> > GetFirstEnabledVideoStream as non-virtual function helpers directly into the
> > MediaResource class and implement them right there using the GetAllStreams
> > function. Then we'll be able to get rid of unnecessary duplication in
> > MediaResource clients, but will still have an interface generic enough to
> > support any future functionality, including multiple streams. WDYT?
> 
> Thanks for thinking about this!
> 
> For your proposal, I guess we'll need a static cast based on the type of
> MediaResource. We have code like that but it's generally not recommended by
the
> style guide. Also, it's not as flexible. I suspect if you want to have the
> infomation of enabled/disabled streams somewhere, but doesn't want to expose
it
> to the Renderers, a intermediate class would be a good place to put it. Also,
I
> feel it's a good place to put HasEncryptedStream() or something similar.
> 
> > > > Re #2: My main question is still this: if we have multiple audio/video
> > streams
> > > > and the initially selected audio and video streams are not encrypted,
but
> > some
> > > > other streams (that are initially disabled) are encrypted, then will we
> have
> > > an
> > > > opportunity to initialize CDM later, when one of the encrypted streams
> > becomes
> > > > enabled? Or does CDM need to be initialized before playback starts?
> > > > At least in case of Chromecast we need to know upfront, before
> initializing
> > > > media pipeline, whether we are gonna have clear or protected playback,
> since
> > > > media pipeline is set up differently (e.g. protected playback uses
secure
> > > > memory), and switching between clear and encrypted playback will require
> > > tearing
> > > > down and reinitializing the whole media pipeline. I suspect that other
> > > platforms
> > > > might have similar limitations.
> > > > If that's the case, then we will need to set up the media pipeline for
> > > encrypted
> > > > playback, even if only currently disabled streams are encrypted. And in
> that
> > > > case we'll need to have some way to access/inspect disabled stream
> > properties.
> > > 
> > > Good question. Please see this doc for the current plan:
> > >
> >
>
https://docs.google.com/a/chromium.org/document/d/173ruVV4ofl0J4kNmz8E8d04gyI...
> > 
> > I don't have access to this doc, I've just asked for permission to access
it.
> 
> Shared, or use your http://chormium.org account.
> 
> In short, it's only required to support switching from encrypted to clear
> streams when MediaKeys is set before load(). In that case, the cast pipeline
> should already have CDM set, and can choose to setup the encrypted pipeline
even
> though the first decoder config is clear.

Ok, as we've discussed yesterday, I have added back the GetFirstStream method to
the MediaResource interface to allow easily getting the first enabled stream of
the requested type. I have also renamed GetStreams -> GetAllStreams and modified
it to return only enabled streams for now. PTAL.

[tguilbert, 2017-02-14 22:13:29]

Still LGTM % one question/comment

https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_im...
media/base/pipeline_impl.cc:863: if (demuxer_->GetAllStreams().empty()) {
In the future, could there ever be a situation where we only have a
DemuxerStream::Type::TEXT returned by GetAllStreams(), and run into problems
here?

[servolk, 2017-02-15 00:03:25]

https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2491043003/diff/560001/media/base/pipeline_im...
media/base/pipeline_impl.cc:863: if (demuxer_->GetAllStreams().empty()) {
On 2017/02/14 22:13:28, tguilbert wrote:
> In the future, could there ever be a situation where we only have a
> DemuxerStream::Type::TEXT returned by GetAllStreams(), and run into problems
> here?

TBH I don't know if we need to care about this very unlikely situation, but I
guess we could make this check more explicit, by removing non-AUDIO/VIDEO
streams from the collection returned by GetAllStreams(). Xiaohan, any thoughts
on this?

[xhwang, 2017-02-15 05:32:19]

LG, just have one question.

https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled,
base::TimeDelta timestamp) = 0;
Will we remove this method in later CLs? If so, can we have a TODO? Thanks!

[servolk, 2017-02-15 17:39:16]

https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled,
base::TimeDelta timestamp) = 0;
On 2017/02/15 05:32:19, xhwang_slow wrote:
> Will we remove this method in later CLs? If so, can we have a TODO? Thanks!

No, I wasn't planning to remove this. Why would we want to remove it? This
method is being used to actually disable and later re-enable tracks/streams. It
ends up being called when blink WebMediaPlayer asks us to enable or disable some
media track.

[xhwang, 2017-02-15 17:43:53]

https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled,
base::TimeDelta timestamp) = 0;
On 2017/02/15 17:39:16, servolk wrote:
> On 2017/02/15 05:32:19, xhwang_slow wrote:
> > Will we remove this method in later CLs? If so, can we have a TODO? Thanks!
> 
> No, I wasn't planning to remove this. Why would we want to remove it? This
> method is being used to actually disable and later re-enable tracks/streams.
It
> ends up being called when blink WebMediaPlayer asks us to enable or disable
some
> media track.

This should only be exposed to demuxers, not to the clients of the
DemuxerStream. What does it mean when a Renderer implementation calls
set_enabled() on one of the streams? Similar for enabled(), which seems
redundant now that we are only exposing enabled streams to clients.

[servolk, 2017-02-15 18:33:38]

On 2017/02/15 17:43:53, xhwang_slow wrote:
>
https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
> File media/base/demuxer_stream.h (right):
> 
>
https://codereview.chromium.org/2491043003/diff/560001/media/base/demuxer_str...
> media/base/demuxer_stream.h:105: virtual void set_enabled(bool enabled,
> base::TimeDelta timestamp) = 0;
> On 2017/02/15 17:39:16, servolk wrote:
> > On 2017/02/15 05:32:19, xhwang_slow wrote:
> > > Will we remove this method in later CLs? If so, can we have a TODO?
Thanks!
> > 
> > No, I wasn't planning to remove this. Why would we want to remove it? This
> > method is being used to actually disable and later re-enable tracks/streams.
> It
> > ends up being called when blink WebMediaPlayer asks us to enable or disable
> some
> > media track.
> 
> This should only be exposed to demuxers, not to the clients of the
> DemuxerStream. What does it mean when a Renderer implementation calls
> set_enabled() on one of the streams? Similar for enabled(), which seems
> redundant now that we are only exposing enabled streams to clients.

Ok, I see what you are saying. We might be able to remove enabled/set_enabled
methods from the base DemuxerStream interface and leave just the
enabled/set_enabled implementations in ChunkDemuxerStream/FFmpegDemuxerStream.
But I'd prefer to do that in a separate CL, since this CL is already pretty
large. I'll add a TODO in this CL for now and will upload a separate CL for
enabled/set_enabled shortly.

[servolk, 2017-02-15 18:36:46]

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

[commit-bot: I haz the power, 2017-02-15 18:38:16]

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

[xhwang, 2017-02-15 19:20:42]

Thank you for the patience! LGTM

[servolk, 2017-02-15 19:43:20]

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

[servolk, 2017-02-15 19:43:48]

On 2017/02/15 19:20:42, xhwang_slow wrote:
> Thank you for the patience! LGTM

Thanks for the review, Xiaohan!

+slan@ PTAL at chromecast/

[commit-bot: I haz the power, 2017-02-15 20:55:19]

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

[commit-bot: I haz the power, 2017-02-15 20:55:21]

Dry run: This issue passed the CQ dry run.

[slan, 2017-02-15 21:14:46]

On 2017/02/15 19:43:48, servolk wrote:
> On 2017/02/15 19:20:42, xhwang_slow wrote:
> > Thank you for the patience! LGTM
> 
> Thanks for the review, Xiaohan!
> 
> +slan@ PTAL at chromecast/
cast lgtm

[servolk, 2017-02-15 21:16:01]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-02-15 21:16:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2491043003/#ps580001
(title: "Added a TODO about DemuxerStream enabled/set_enabled methods")

[commit-bot: I haz the power, 2017-02-15 21:18:04]

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

[commit-bot: I haz the power, 2017-02-15 22:37:11]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1487193361956980,
"parent_rev": "8201abf336e367cdebf171d35332180ec59bcbac", "commit_rev":
"8acf5dcbc4e83fad7f23eb0380e0635058a636bc"}

[commit-bot: I haz the power, 2017-02-15 22:43:44]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1487193361956980,
"parent_rev": "8201abf336e367cdebf171d35332180ec59bcbac", "commit_rev":
"8acf5dcbc4e83fad7f23eb0380e0635058a636bc"}

[commit-bot: I haz the power, 2017-02-15 22:43:52]

Description was changed from

==========
MediaResource refactoring to support multiple streams

This CL changes the old GetStream method returning a single stream into
a new GetAllStreams returning all available demuxer streams.

BUG=487288
==========

to

==========
MediaResource refactoring to support multiple streams

This CL changes the old GetStream method returning a single stream into
a new GetAllStreams returning all available demuxer streams.

BUG=487288

Review-Url: https://codereview.chromium.org/2491043003
Cr-Commit-Position: refs/heads/master@{#450819}
Committed:
https://chromium.googlesource.com/chromium/src/+/8acf5dcbc4e83fad7f23eb0380e0...
==========

[commit-bot: I haz the power, 2017-02-15 22:43:54]

Committed patchset #30 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/8acf5dcbc4e83fad7f23eb0380e0...