Basic media tracks implementation for media stream parsers

Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in MSE stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427, 249428
Committed: https://crrev.com/95dc4ad373d2aef14c9b439397f1a4599b64bd6a
Cr-Commit-Position: refs/heads/master@{#378583}

[servolk, 2016-02-18 23:29:45]

Description was changed from

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427
==========

to

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427
==========

[servolk, 2016-02-18 23:29:45]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, ddorwin@chromium.org, wolenetz@chromium.org

[servolk, 2016-02-18 23:47:26]

Chromecast team is interested in supporting media tracks and media track
controls through standard JS APIs, so I've put together a few CLs to help out
with kickstarting the media track work laid out by Matt in
go/mse_initsegmentreceived
This first CL adds a basic media track implementation, subsequent CLs will build
on top of it to pass media track info to the blink level
(https://codereview.chromium.org/1659653002/) and eventually implement the MSE
init segment handling on blink level
(https://codereview.chromium.org/1678523003/)
This is an early prototype, any comments/suggestions on improving the code and
design are welcome.

[ddorwin, 2016-02-19 00:06:17]

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

[DaleCurtis, 2016-02-22 18:46:42]

dalecurtis@chromium.org changed reviewers:
+ chcunningham@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2016-02-22 18:46:43]

=> chcunningham

[servolk, 2016-02-24 00:27:38]

On 2016/02/22 18:46:43, DaleCurtis wrote:
> => chcunningham

ping

[servolk, 2016-02-24 05:07:25]

On 2016/02/24 00:27:38, servolk wrote:
> On 2016/02/22 18:46:43, DaleCurtis wrote:
> > => chcunningham
> 
> ping

I guess one additional change that we could explore after this is unifying media
track reporting on a demuxer level, so that media tracks would work (mostly) the
same in both FFmpegDemuxer and ChunkDemuxer/MSE cases, something like this:
https://codereview.chromium.org/1727243002

[servolk, 2016-02-25 02:16:36]

On 2016/02/24 05:07:25, servolk wrote:
> On 2016/02/24 00:27:38, servolk wrote:
> > On 2016/02/22 18:46:43, DaleCurtis wrote:
> > > => chcunningham
> > 
> > ping
> 
> I guess one additional change that we could explore after this is unifying
media
> track reporting on a demuxer level, so that media tracks would work (mostly)
the
> same in both FFmpegDemuxer and ChunkDemuxer/MSE cases, something like this:
> https://codereview.chromium.org/1727243002

FYI: In a blink CL https://codereview.chromium.org/1659653002/ Philip was asking
if it would be possible to unify media track handling between MSE and non-MSE
use cases. I've investigated this a bit more today, and while I haven't yet
achieved a complete unification, I've found a solution that will allow us to
unify somewhat and unblock further development of media tracks functionality for
both non-MSE (FFmpegDemuxer) and MSE (ChunkDemuxer).
I think it might be easier to review all those changes as separate independent
CLs: this one + https://codereview.chromium.org/1727243002/ +
https://codereview.chromium.org/1735763002/. But if you prefer, I've also
combined these three CLs into a single large CL to show the total scope of
changes:
https://codereview.chromium.org/1735803002/

I believe my other CLs that take care of handling media tracks for
MSE/ChunkDemuxer case should be trivial to cherry-pick on top of this work.

[wolenetz, 2016-02-26 00:04:07]

On 2016/02/25 02:16:36, servolk wrote:
> ...
> I believe my other CLs that take care of handling media tracks for
> MSE/ChunkDemuxer case should be trivial to cherry-pick on top of this work.

Did you mean ffmpeg/demuxer level unification instead of MSE/ChunkDemuxer in
that last line?

Thanks for putting this together! It's great to see progress towards at least
MSE initialization segment received compliance!
I'll have some comments for this small CL shortly. As I mentioned on email
thread, thank you for providing both a sequence of smaller CLs (with concerns
separated for easier review, iteration, and landing in sequence), as well as a
combined CL demonstrating how they work together (and allowing overall strategy
review/comment).

[wolenetz, 2016-02-26 00:06:22]

Looking pretty good.
As I understand it, this will allow the initialization segment received
algorithm to be more directly done in Blink, and affords more easily future
expansion to multi-track A/V as well as sourcing inband track attributes in the
parsers.
Mostly minor comments:

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track.h
File media/base/media_track.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track....
media/base/media_track.h:22: virtual ~MediaTrack();
Is this class meant to be subtyped? Do we need virtual destructor? Is this a
partial interface?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.cc
File media/base/media_tracks.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.cc:9: #include "media/base/media_track.h"
nit: this is already included by this class' header.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.cc:26: tracks_.push_back(std::move(track));
What if there is already an audio track with same |id| ?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.cc:38: tracks_.push_back(std::move(track));
Ditto (what if there is already a video track with same |id|?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.cc:63: return getAudioConfig(track->id());
for example, this would return probably an unexpected config if more than one
audio track had same id in |tracks_|.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.cc:73: return getVideoConfig(track->id());
ditto example

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:12: #include "base/callback.h"
nit: is this include necessary in this file?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:26: virtual ~MediaTracks();
ditto: is virtual needed here?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:28: void AddAudioTrack(const AudioDecoderConfig&
config,
What is the behavior if any of these params are invalid (ditto for
AddVideoTrack())?
Similarly, where do we centralize the logic for "max 1 audio" "max 1 video"
track in Chromium as it is limited to right now? Should an attempt to add a
second video / audio track here be disallowed here and/or some other place too?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:39: typedef std::vector<scoped_ptr<MediaTrack>>
MediaTracksCollection;
nit: typedef at top of class (per
https://google.github.io/styleguide/cppguide.html#Declaration_Order)

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:42: const AudioDecoderConfig& getAudioConfig(const
std::string& id) const;
What is the behavior if there is no audio/video config for the supplied id?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:45: // TODO(servolk): These are temporary helpers
useful until all code paths are
nit: what about text tracks?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:47: const AudioDecoderConfig& getFirstAudioConfig()
const;
What is the behavior if there is no first audio/video config?

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:51: DISALLOW_COPY_AND_ASSIGN(MediaTracks);
nit: should be last thing in the class
(https://google.github.io/styleguide/cppguide.html#Declaration_Order)

https://codereview.chromium.org/1716503002/diff/60001/media/blink/websourcebu...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/blink/websourcebu...
media/blink/websourcebuffer_impl.cc:164:
client_->initializationSegmentReceived();
nit: if landing separately from the rest (as currently constituted), perhaps a
TODO + crbug reference here to pass |tracks| to |client_| for processing there.

https://codereview.chromium.org/1716503002/diff/60001/media/filters/media_sou...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/filters/media_sou...
media/filters/media_source_state.cc:475: DCHECK(tracks);
nit: though scoped_ptr allows this IIUC, I prefer DCHECK(tracks.get()) for
readability

https://codereview.chromium.org/1716503002/diff/60001/media/formats/common/st...
File media/formats/common/stream_parser_test_base.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/common/st...
media/formats/common/stream_parser_test_base.cc:92:
EXPECT_FALSE(tracks->getFirstVideoConfig().IsValidConfig());
nit: also expect number of tracks in |tracks| to be exactly 1 until / unless
test behavior and product support changes to allow multitrack audio/video?

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser.cc:529:
media_tracks->AddAudioTrack(audio_config, "audio", "", "", "");
Please add TODO and crbug for sourceing mp2ts inband track information correctly
here. See https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg2ts

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser.cc:532:
media_tracks->AddVideoTrack(video_config, "video", "", "", "");
ditto (same TODO/crbug should cover this portion of this method..

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser.cc:631: const AudioDecoderConfig&
audio_config = queue_with_config.audio_config;
nit: lots of new code duplication: please add a helper that does all this.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:121: const VideoDecoderConfig&
vc = tracks->getFirstVideoConfig();
nit: let's also preserve the previous DVLOG(1) which would provide additional
info about config validity for each of A/V.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:357: scoped_ptr<MediaTracks>
media_tracks(new MediaTracks());
ditto: TODO+crbug for sourcing inband track metadata, this time for mp4-isobmff
(https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4)

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:106: video_decoder_config_ =
tracks->getFirstVideoConfig();
ditto: let's preserve also the previous DVLOG(1) functionality (though
s/ac/audio_decoder_config_/ and s/vc/.../ in it, of course

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:225: if
(config_.IsValidConfig()) {
Hmm. Do we instead want to preserve the potential for invalid codec config to be
in |tracks| for use in determining in InitSegmentReceivedCB a particular error
path or error message?

https://codereview.chromium.org/1716503002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_stream_parser.cc:227: scoped_ptr<MediaTracks>
media_tracks(new MediaTracks());
ditto: TODO+crbug for sourcing inband track attributes; this time for webm:
https://dev.w3.org/html5/html-sourcing-inband-tracks/#webm

[wolenetz, 2016-02-26 00:21:18]

Description was changed from

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427
==========

to

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in MSE stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427, 249428
==========

[wolenetz, 2016-02-26 00:21:51]

On 2016/02/26 00:06:22, wolenetz wrote:
> Looking pretty good.
> As I understand it, this will allow the initialization segment received
> algorithm to be more directly done in Blink, and affords more easily future
> expansion to multi-track A/V as well as sourcing inband track attributes in
the
> parsers.
> Mostly minor comments:
> 
> https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track.h
> File media/base/media_track.h (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track....
> media/base/media_track.h:22: virtual ~MediaTrack();
> Is this class meant to be subtyped? Do we need virtual destructor? Is this a
> partial interface?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.cc
> File media/base/media_tracks.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.cc:9: #include "media/base/media_track.h"
> nit: this is already included by this class' header.
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.cc:26: tracks_.push_back(std::move(track));
> What if there is already an audio track with same |id| ?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.cc:38: tracks_.push_back(std::move(track));
> Ditto (what if there is already a video track with same |id|?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.cc:63: return getAudioConfig(track->id());
> for example, this would return probably an unexpected config if more than one
> audio track had same id in |tracks_|.
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.cc:73: return getVideoConfig(track->id());
> ditto example
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.h
> File media/base/media_tracks.h (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:12: #include "base/callback.h"
> nit: is this include necessary in this file?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:26: virtual ~MediaTracks();
> ditto: is virtual needed here?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:28: void AddAudioTrack(const AudioDecoderConfig&
> config,
> What is the behavior if any of these params are invalid (ditto for
> AddVideoTrack())?
> Similarly, where do we centralize the logic for "max 1 audio" "max 1 video"
> track in Chromium as it is limited to right now? Should an attempt to add a
> second video / audio track here be disallowed here and/or some other place
too?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:39: typedef std::vector<scoped_ptr<MediaTrack>>
> MediaTracksCollection;
> nit: typedef at top of class (per
> https://google.github.io/styleguide/cppguide.html#Declaration_Order)
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:42: const AudioDecoderConfig& getAudioConfig(const
> std::string& id) const;
> What is the behavior if there is no audio/video config for the supplied id?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:45: // TODO(servolk): These are temporary helpers
> useful until all code paths are
> nit: what about text tracks?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:47: const AudioDecoderConfig& getFirstAudioConfig()
> const;
> What is the behavior if there is no first audio/video config?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
> media/base/media_tracks.h:51: DISALLOW_COPY_AND_ASSIGN(MediaTracks);
> nit: should be last thing in the class
> (https://google.github.io/styleguide/cppguide.html#Declaration_Order)
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/blink/websourcebu...
> File media/blink/websourcebuffer_impl.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/blink/websourcebu...
> media/blink/websourcebuffer_impl.cc:164:
> client_->initializationSegmentReceived();
> nit: if landing separately from the rest (as currently constituted), perhaps a
> TODO + crbug reference here to pass |tracks| to |client_| for processing
there.
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/filters/media_sou...
> File media/filters/media_source_state.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/filters/media_sou...
> media/filters/media_source_state.cc:475: DCHECK(tracks);
> nit: though scoped_ptr allows this IIUC, I prefer DCHECK(tracks.get()) for
> readability
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/common/st...
> File media/formats/common/stream_parser_test_base.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/common/st...
> media/formats/common/stream_parser_test_base.cc:92:
> EXPECT_FALSE(tracks->getFirstVideoConfig().IsValidConfig());
> nit: also expect number of tracks in |tracks| to be exactly 1 until / unless
> test behavior and product support changes to allow multitrack audio/video?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
> File media/formats/mp2t/mp2t_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
> media/formats/mp2t/mp2t_stream_parser.cc:529:
> media_tracks->AddAudioTrack(audio_config, "audio", "", "", "");
> Please add TODO and crbug for sourceing mp2ts inband track information
correctly
> here. See https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg2ts
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
> media/formats/mp2t/mp2t_stream_parser.cc:532:
> media_tracks->AddVideoTrack(video_config, "video", "", "", "");
> ditto (same TODO/crbug should cover this portion of this method..
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
> media/formats/mp2t/mp2t_stream_parser.cc:631: const AudioDecoderConfig&
> audio_config = queue_with_config.audio_config;
> nit: lots of new code duplication: please add a helper that does all this.
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
> File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
> media/formats/mp2t/mp2t_stream_parser_unittest.cc:121: const
VideoDecoderConfig&
> vc = tracks->getFirstVideoConfig();
> nit: let's also preserve the previous DVLOG(1) which would provide additional
> info about config validity for each of A/V.
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
> File media/formats/mp4/mp4_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
> media/formats/mp4/mp4_stream_parser.cc:357: scoped_ptr<MediaTracks>
> media_tracks(new MediaTracks());
> ditto: TODO+crbug for sourcing inband track metadata, this time for
mp4-isobmff
> (https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4)
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
> File media/formats/mp4/mp4_stream_parser_unittest.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
> media/formats/mp4/mp4_stream_parser_unittest.cc:106: video_decoder_config_ =
> tracks->getFirstVideoConfig();
> ditto: let's preserve also the previous DVLOG(1) functionality (though
> s/ac/audio_decoder_config_/ and s/vc/.../ in it, of course
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
> File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
> media/formats/mpeg/mpeg_audio_stream_parser_base.cc:225: if
> (config_.IsValidConfig()) {
> Hmm. Do we instead want to preserve the potential for invalid codec config to
be
> in |tracks| for use in determining in InitSegmentReceivedCB a particular error
> path or error message?
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/webm/webm...
> File media/formats/webm/webm_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/1716503002/diff/60001/media/formats/webm/webm...
> media/formats/webm/webm_stream_parser.cc:227: scoped_ptr<MediaTracks>
> media_tracks(new MediaTracks());
> ditto: TODO+crbug for sourcing inband track attributes; this time for webm:
> https://dev.w3.org/html5/html-sourcing-inband-tracks/#webm

I also added 249428 to the CL description, and the word "MSE" to it, too.

[wolenetz, 2016-02-26 00:59:41]

one more nit :)

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track.h
File media/base/media_track.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track....
media/base/media_track.h:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
nit: here and elsewhere in the new files, no (c) these days...
(https://www.chromium.org/developers/coding-style)

[servolk, 2016-02-26 01:56:33]

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track.h
File media/base/media_track.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track....
media/base/media_track.h:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
On 2016/02/26 00:59:41, wolenetz wrote:
> nit: here and elsewhere in the new files, no (c) these days...
> (https://www.chromium.org/developers/coding-style)

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track....
media/base/media_track.h:22: virtual ~MediaTrack();
On 2016/02/26 00:06:20, wolenetz wrote:
> Is this class meant to be subtyped? Do we need virtual destructor? Is this a
> partial interface?

This class is essentially a Chromium-level counterpart for blink::TrackBase, it
contains the same set of properties. On blink level they derive
AudioTrack/VideoTrack from TrackBase (to add AudioTrack.enabled and
VideoTrack.selected properties). So I think that yes, later on this will be a
base class for actual a/v/t tracks, that's why I added the virtual destructor.
But right now it's not needed, so removing the 'virtual' for now.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.cc
File media/base/media_tracks.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.cc:9: #include "media/base/media_track.h"
On 2016/02/26 00:06:20, wolenetz wrote:
> nit: this is already included by this class' header.

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:12: #include "base/callback.h"
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: is this include necessary in this file?

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:26: virtual ~MediaTracks();
On 2016/02/26 00:06:21, wolenetz wrote:
> ditto: is virtual needed here?

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:28: void AddAudioTrack(const AudioDecoderConfig&
config,
On 2016/02/26 00:06:21, wolenetz wrote:
> What is the behavior if any of these params are invalid (ditto for
> AddVideoTrack())?
> Similarly, where do we centralize the logic for "max 1 audio" "max 1 video"
> track in Chromium as it is limited to right now? Should an attempt to add a
> second video / audio track here be disallowed here and/or some other place
too?

Well this CL was meant to be a quick prototype to see if it's even feasible to
achieve my goals this way, so I did a fairly minimal implementation and didn't
bother with proper error handling and all the corner cases. We can harden it
later.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:39: typedef std::vector<scoped_ptr<MediaTrack>>
MediaTracksCollection;
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: typedef at top of class (per
> https://google.github.io/styleguide/cppguide.html#Declaration_Order)

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:42: const AudioDecoderConfig& getAudioConfig(const
std::string& id) const;
On 2016/02/26 00:06:21, wolenetz wrote:
> What is the behavior if there is no audio/video config for the supplied id?

it'll return an invalid a/v config.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:45: // TODO(servolk): These are temporary helpers
useful until all code paths are
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: what about text tracks?

Currently the text tracks seem to be already supported to some extent, although
in a very different way (at least from stream parser level perspective), I
wanted to avoid breaking them while prototyping this, so I didn't touch text
track code for now. Once we have a/v tracks working I can look into unifying
handling of a/v and t tracks.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:47: const AudioDecoderConfig& getFirstAudioConfig()
const;
On 2016/02/26 00:06:21, wolenetz wrote:
> What is the behavior if there is no first audio/video config?

Current behavior is to return invalid A/V configs. These methods will be removed
soon, there's no major obstacles, I added them just to simplify converting old
code that typically used a pair of A/V configs directly, so that I could unblock
myself on making further progress on actual task at hand (media tracks).

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:51: DISALLOW_COPY_AND_ASSIGN(MediaTracks);
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: should be last thing in the class
> (https://google.github.io/styleguide/cppguide.html#Declaration_Order)

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/blink/websourcebu...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/blink/websourcebu...
media/blink/websourcebuffer_impl.cc:164:
client_->initializationSegmentReceived();
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: if landing separately from the rest (as currently constituted), perhaps a
> TODO + crbug reference here to pass |tracks| to |client_| for processing
there.

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/filters/media_sou...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/filters/media_sou...
media/filters/media_source_state.cc:475: DCHECK(tracks);
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: though scoped_ptr allows this IIUC, I prefer DCHECK(tracks.get()) for
> readability

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/common/st...
File media/formats/common/stream_parser_test_base.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/common/st...
media/formats/common/stream_parser_test_base.cc:92:
EXPECT_FALSE(tracks->getFirstVideoConfig().IsValidConfig());
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: also expect number of tracks in |tracks| to be exactly 1 until / unless
> test behavior and product support changes to allow multitrack audio/video?

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser.cc:529:
media_tracks->AddAudioTrack(audio_config, "audio", "", "", "");
On 2016/02/26 00:06:21, wolenetz wrote:
> Please add TODO and crbug for sourceing mp2ts inband track information
correctly
> here. See https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg2ts

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser.cc:532:
media_tracks->AddVideoTrack(video_config, "video", "", "", "");
On 2016/02/26 00:06:21, wolenetz wrote:
> ditto (same TODO/crbug should cover this portion of this method..

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser.cc:631: const AudioDecoderConfig&
audio_config = queue_with_config.audio_config;
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: lots of new code duplication: please add a helper that does all this.

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:121: const VideoDecoderConfig&
vc = tracks->getFirstVideoConfig();
On 2016/02/26 00:06:21, wolenetz wrote:
> nit: let's also preserve the previous DVLOG(1) which would provide additional
> info about config validity for each of A/V.

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:357: scoped_ptr<MediaTracks>
media_tracks(new MediaTracks());
On 2016/02/26 00:06:21, wolenetz wrote:
> ditto: TODO+crbug for sourcing inband track metadata, this time for
mp4-isobmff
> (https://dev.w3.org/html5/html-sourcing-inband-tracks/#mpeg4)

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:106: video_decoder_config_ =
tracks->getFirstVideoConfig();
On 2016/02/26 00:06:21, wolenetz wrote:
> ditto: let's preserve also the previous DVLOG(1) functionality (though
> s/ac/audio_decoder_config_/ and s/vc/.../ in it, of course

Done.

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:225: if
(config_.IsValidConfig()) {
On 2016/02/26 00:06:21, wolenetz wrote:
> Hmm. Do we instead want to preserve the potential for invalid codec config to
be
> in |tracks| for use in determining in InitSegmentReceivedCB a particular error
> path or error message?

I think it's better to just pass an empty MediaTracks collection to higher
levels, the MSE spec explicitly prescribes how to handle that (see step #2 in
https://w3c.github.io/media-source/#sourcebuffer-init-segment-received).

https://codereview.chromium.org/1716503002/diff/60001/media/formats/webm/webm...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/webm/webm...
media/formats/webm/webm_stream_parser.cc:227: scoped_ptr<MediaTracks>
media_tracks(new MediaTracks());
On 2016/02/26 00:06:21, wolenetz wrote:
> ditto: TODO+crbug for sourcing inband track attributes; this time for webm:
> https://dev.w3.org/html5/html-sourcing-inband-tracks/#webm

Done.

[wolenetz, 2016-02-26 19:29:28]

lgtm % 2 code comment nits:

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track.h
File media/base/media_track.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_track....
media/base/media_track.h:22: virtual ~MediaTrack();
On 2016/02/26 01:56:31, servolk wrote:
> On 2016/02/26 00:06:20, wolenetz wrote:
> > Is this class meant to be subtyped? Do we need virtual destructor? Is this a
> > partial interface?
> 
> This class is essentially a Chromium-level counterpart for blink::TrackBase,
it
> contains the same set of properties. On blink level they derive
> AudioTrack/VideoTrack from TrackBase (to add AudioTrack.enabled and
> VideoTrack.selected properties). So I think that yes, later on this will be a
> base class for actual a/v/t tracks, that's why I added the virtual destructor.
> But right now it's not needed, so removing the 'virtual' for now.

Acknowledged.

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:28: void AddAudioTrack(const AudioDecoderConfig&
config,
On 2016/02/26 01:56:32, servolk wrote:
> On 2016/02/26 00:06:21, wolenetz wrote:
> > What is the behavior if any of these params are invalid (ditto for
> > AddVideoTrack())?
> > Similarly, where do we centralize the logic for "max 1 audio" "max 1 video"
> > track in Chromium as it is limited to right now? Should an attempt to add a
> > second video / audio track here be disallowed here and/or some other place
> too?
> 
> Well this CL was meant to be a quick prototype to see if it's even feasible to
> achieve my goals this way, so I did a fairly minimal implementation and didn't
> bother with proper error handling and all the corner cases. We can harden it
> later.

Please add a comment that |id| collision within each track type (audio/video)
must be protected against by caller. Otherwise, we risk misunderstanding this
API later and assuming too much of it :)

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/1716503002/diff/60001/media/formats/mpeg/mpeg...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:225: if
(config_.IsValidConfig()) {
On 2016/02/26 01:56:33, servolk wrote:
> On 2016/02/26 00:06:21, wolenetz wrote:
> > Hmm. Do we instead want to preserve the potential for invalid codec config
to
> be
> > in |tracks| for use in determining in InitSegmentReceivedCB a particular
error
> > path or error message?
> 
> I think it's better to just pass an empty MediaTracks collection to higher
> levels, the MSE spec explicitly prescribes how to handle that (see step #2 in
> https://w3c.github.io/media-source/#sourcebuffer-init-segment-received).

Ack. If we need more info on why a particular expected-to-be-valid codec config
wasn't present in a MediaTracks, we'd probably need to log this reason somewhere
in the parser, not at that higher level, so sgtm.

https://codereview.chromium.org/1716503002/diff/80001/media/blink/websourcebu...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1716503002/diff/80001/media/blink/websourcebu...
media/blink/websourcebuffer_impl.cc:164: // TODO(servolk): Implement passing
MediaTrack info to blink level.
really minor nit: TODO needs crbug reference. I suggest https://crbug.com/249428
for this one.

https://codereview.chromium.org/1716503002/diff/80001/media/formats/mpeg/mpeg...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/1716503002/diff/80001/media/formats/mpeg/mpeg...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:226:
media_tracks->AddAudioTrack(config_, "audio", "", "", "");
aside: the corresponding MSE bytestream spec allows for implementations to
choose if and how to expose IDv1/3/icecast headers (and suggests as text
tracks). Hence, I don't think we need to prioritize sourcing inband audio track
attributes yet in this particular stream parser at the moment.

[chcunningham, 2016-02-26 20:59:25]

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

[servolk, 2016-02-26 22:02:41]

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks.h
File media/base/media_tracks.h (right):

https://codereview.chromium.org/1716503002/diff/60001/media/base/media_tracks...
media/base/media_tracks.h:28: void AddAudioTrack(const AudioDecoderConfig&
config,
On 2016/02/26 19:29:27, wolenetz wrote:
> On 2016/02/26 01:56:32, servolk wrote:
> > On 2016/02/26 00:06:21, wolenetz wrote:
> > > What is the behavior if any of these params are invalid (ditto for
> > > AddVideoTrack())?
> > > Similarly, where do we centralize the logic for "max 1 audio" "max 1
video"
> > > track in Chromium as it is limited to right now? Should an attempt to add
a
> > > second video / audio track here be disallowed here and/or some other place
> > too?
> > 
> > Well this CL was meant to be a quick prototype to see if it's even feasible
to
> > achieve my goals this way, so I did a fairly minimal implementation and
didn't
> > bother with proper error handling and all the corner cases. We can harden it
> > later.
> 
> Please add a comment that |id| collision within each track type (audio/video)
> must be protected against by caller. Otherwise, we risk misunderstanding this
> API later and assuming too much of it :)

Done + actually added CHECKs in the method impl to ensure we don't get
duplicated ids from callers.

https://codereview.chromium.org/1716503002/diff/80001/media/blink/websourcebu...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1716503002/diff/80001/media/blink/websourcebu...
media/blink/websourcebuffer_impl.cc:164: // TODO(servolk): Implement passing
MediaTrack info to blink level.
On 2016/02/26 19:29:28, wolenetz wrote:
> really minor nit: TODO needs crbug reference. I suggest
https://crbug.com/249428
> for this one.

Done.

https://codereview.chromium.org/1716503002/diff/80001/media/formats/mpeg/mpeg...
File media/formats/mpeg/mpeg_audio_stream_parser_base.cc (right):

https://codereview.chromium.org/1716503002/diff/80001/media/formats/mpeg/mpeg...
media/formats/mpeg/mpeg_audio_stream_parser_base.cc:226:
media_tracks->AddAudioTrack(config_, "audio", "", "", "");
On 2016/02/26 19:29:28, wolenetz wrote:
> aside: the corresponding MSE bytestream spec allows for implementations to
> choose if and how to expose IDv1/3/icecast headers (and suggests as text
> tracks). Hence, I don't think we need to prioritize sourcing inband audio
track
> attributes yet in this particular stream parser at the moment.

Acknowledged.

[wolenetz, 2016-02-26 22:06:19]

lgtm

[servolk, 2016-03-01 17:13:34]

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

[commit-bot: I haz the power, 2016-03-01 17:13:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1716503002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1716503002/100001

[servolk, 2016-03-01 18:24:33]

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

[commit-bot: I haz the power, 2016-03-01 18:25:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1716503002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1716503002/120001

[servolk, 2016-03-01 20:46:18]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2016-03-01 20:46:22]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-03-01 20:46:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1716503002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-01 20:47:29]

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

[commit-bot: I haz the power, 2016-03-01 21:30:51]

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

[commit-bot: I haz the power, 2016-03-01 21:30:52]

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

[servolk, 2016-03-01 21:31:25]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-03-01 21:32:34]

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

[commit-bot: I haz the power, 2016-03-01 22:44:47]

Description was changed from

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in MSE stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427, 249428
==========

to

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in MSE stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427, 249428
==========

[commit-bot: I haz the power, 2016-03-01 22:44:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-01 22:45:59]

Description was changed from

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in MSE stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427, 249428
==========

to

==========
Basic media tracks implementation for media stream parsers

This CL adds basic media::MediaTrack implementation, media::MediaTracks
collection that associates media tracks with audio/video configs, and
makes necessary changes in MSE stream parsers to pass around the
MediaTracks collection instead of individual audio/video configs.

BUG=249427, 249428

Committed: https://crrev.com/95dc4ad373d2aef14c9b439397f1a4599b64bd6a
Cr-Commit-Position: refs/heads/master@{#378583}
==========

[commit-bot: I haz the power, 2016-03-01 22:46:01]

Patchset 7 (id:??) landed as
https://crrev.com/95dc4ad373d2aef14c9b439397f1a4599b64bd6a
Cr-Commit-Position: refs/heads/master@{#378583}