Unify media track info reporting on a demuxer level

Unify media track info reporting on a demuxer level

This will work for both FFmpegDemuxer and MSE/ChunkDemuxer

Committed: https://crrev.com/81e01e07b928180ce12b0229dbb7ca0e157984d2
Cr-Commit-Position: refs/heads/master@{#379449}

[servolk, 2016-03-01 19:53:28]

Description was changed from

==========
Attempt to unify media track updates reporting on a demuxer level

This should work for both FFmpegDemuxer and MSE/ChunkDemuxer
==========

to

==========
Unify media track info reporting on a demuxer level

This will work for both FFmpegDemuxer and MSE/ChunkDemuxer
==========

[servolk, 2016-03-01 19:53:28]

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

[servolk, 2016-03-02 18:37:53]

On 2016/03/01 19:53:36, servolk wrote:

Ping.
Matt, please review this one next, as all subsequent media tracks work depends
on this one.

[servolk, 2016-03-04 01:46:05]

On 2016/03/02 18:37:53, servolk wrote:
> On 2016/03/01 19:53:36, servolk wrote:
> 
> Ping.
> Matt, please review this one next, as all subsequent media tracks work depends
> on this one.

ping

[servolk, 2016-03-04 21:40:05]

On 2016/03/04 01:46:05, servolk wrote:
> On 2016/03/02 18:37:53, servolk wrote:
> > On 2016/03/01 19:53:36, servolk wrote:
> > 
> > Ping.
> > Matt, please review this one next, as all subsequent media tracks work
depends
> > on this one.
> 
> ping

ping

[wolenetz, 2016-03-05 01:47:43]

This is a tough one and gets to the core of why my previous quick attempt to do
similar stalled: Can there be a unification of the reporting to Blink of ffmpeg
(initial tracks) and MSE (initial tracks and subsequent tracks)? At a minimum,
elevating the demuxer "new tracks callback" to be common in the demuxer.h
interface seems a good idea. I'm open to suggestions to unify the routing; we
can even consider hooking up initsegmentreceived for MSE in a different path
than via WSBI if that's better for unification. wdyt?

https://codereview.chromium.org/1727243002/diff/340001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1727243002/diff/340001/media/base/demuxer.h#n...
media/base/demuxer.h:66: // (e.g. the inital stream metadata has been parsed
successfully, or a new
nit:initial

https://codereview.chromium.org/1727243002/diff/340001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1727243002/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:268: void
OnFFmpegMediaTracksUpdated(scoped_ptr<MediaTracks> tracks);
Hmm. Having distinct routes for reporting this info into blink src= vs MSE
smells funny on first look, but I'm not sure immediately of a better way.

https://codereview.chromium.org/1727243002/diff/340001/media/blink/websourceb...
File media/blink/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/1727243002/diff/340001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:49: demuxer_->SetTracksWatcher(
This seems like a method that could be elevated to demuxer.h and therefore be in
common across ffmpegdemuxer and chunkdemuxer.
wdyt?

https://codereview.chromium.org/1727243002/diff/340001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/1727243002/diff/340001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer_unittest.cc:211: void
OnMediaTracksUpdated(scoped_ptr<MediaTracks> tracks) {}
Now that we have this callback being exercised, seems a good time to also add
verification that expected track info (of at least some basic form) occurs.
wdyt?

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
media/filters/media_source_state.cc:488: CHECK(append_in_progress_);
ok. I see why you have this flag now. I'm fine with keeping it.
And I see the old DCHECK is moved to where the cb is used - that's fine.

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
File media/filters/media_source_state.h (right):

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
media/filters/media_source_state.h:116: void SetTracksWatcher(const
Demuxer::MediaTracksUpdatedCB& tracks_updated_cb);
nit: document what this means; when it should be called, and when the callback
will be called.

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
media/filters/media_source_state.h:202: bool append_in_progress_ = false;
this flag and associated comment and usage seem unnecessary. the previous
comment was there simply because the value of the member callback variable could
only be trusted during an append. so long as we only call it during
append-related processing, we need no such flag.

[servolk, 2016-03-05 01:56:19]

https://codereview.chromium.org/1727243002/diff/340001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1727243002/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:268: void
OnFFmpegMediaTracksUpdated(scoped_ptr<MediaTracks> tracks);
On 2016/03/05 01:47:43, wolenetz wrote:
> Hmm. Having distinct routes for reporting this info into blink src= vs MSE
> smells funny on first look, but I'm not sure immediately of a better way.

I've spend quite a bit of time trying different approaches, and at this point
I'm pretty sure that this is the right way to do this. See further CLs to see
how it all ties together. I've already been able to verify this approach with
basic layout tests on blink level for both FFmpeg and MSE!

https://codereview.chromium.org/1727243002/diff/340001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/1727243002/diff/340001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer_unittest.cc:211: void
OnMediaTracksUpdated(scoped_ptr<MediaTracks> tracks) {}
On 2016/03/05 01:47:43, wolenetz wrote:
> Now that we have this callback being exercised, seems a good time to also add
> verification that expected track info (of at least some basic form) occurs.
> wdyt?

Well, this CL doesn't yet include the changes to actually populate MediaTracks
object with any useful data. Check out the new unit tests in
https://codereview.chromium.org/1735003004/. I think the unit tests in that CL
should be enough to provide basic coverage of this functionality.

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
File media/filters/media_source_state.cc (right):

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
media/filters/media_source_state.cc:488: CHECK(append_in_progress_);
On 2016/03/05 01:47:43, wolenetz wrote:
> ok. I see why you have this flag now. I'm fine with keeping it.
> And I see the old DCHECK is moved to where the cb is used - that's fine.

Acknowledged.

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
File media/filters/media_source_state.h (right):

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
media/filters/media_source_state.h:116: void SetTracksWatcher(const
Demuxer::MediaTracksUpdatedCB& tracks_updated_cb);
On 2016/03/05 01:47:43, wolenetz wrote:
> nit: document what this means; when it should be called, and when the callback
> will be called.

Will do, but at this point there are three other CLs that are based on this one.
So to avoid too much rebasing, I'll add a comment in separate CL.

https://codereview.chromium.org/1727243002/diff/340001/media/filters/media_so...
media/filters/media_source_state.h:202: bool append_in_progress_ = false;
On 2016/03/05 01:47:43, wolenetz wrote:
> this flag and associated comment and usage seem unnecessary. the previous
> comment was there simply because the value of the member callback variable
could
> only be trusted during an append. so long as we only call it during
> append-related processing, we need no such flag.

Done.

[wolenetz, 2016-03-05 02:17:04]

lgtm
I think the chromecast/ needs an OWNER l*tm too

[wolenetz, 2016-03-05 02:18:06]

Regarding the comment on the routing approaches for signalling tracks in src= vs
mse, this CL lgtm (I was just hoping there was a simpler, unifying way of doing
this). Some such improvement can come later, of course.

[servolk, 2016-03-05 02:18:27]

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

[servolk, 2016-03-05 02:19:05]

On 2016/03/05 02:17:04, wolenetz wrote:
> lgtm
> I think the chromecast/ needs an OWNER l*tm too

Thanks!
+slan@ for quick chromecast/ approval.

[slan, 2016-03-05 02:21:34]

chromecast lgtm

[servolk, 2016-03-05 02:22:08]

The CQ bit was checked by servolk@chromium.org

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

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

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

Description was changed from

==========
Unify media track info reporting on a demuxer level

This will work for both FFmpegDemuxer and MSE/ChunkDemuxer
==========

to

==========
Unify media track info reporting on a demuxer level

This will work for both FFmpegDemuxer and MSE/ChunkDemuxer
==========

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

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-03-05 03:30:53]

Description was changed from

==========
Unify media track info reporting on a demuxer level

This will work for both FFmpegDemuxer and MSE/ChunkDemuxer
==========

to

==========
Unify media track info reporting on a demuxer level

This will work for both FFmpegDemuxer and MSE/ChunkDemuxer

Committed: https://crrev.com/81e01e07b928180ce12b0229dbb7ca0e157984d2
Cr-Commit-Position: refs/heads/master@{#379449}
==========

[commit-bot: I haz the power, 2016-03-05 03:30:54]

Patchset 18 (id:??) landed as
https://crrev.com/81e01e07b928180ce12b0229dbb7ca0e157984d2
Cr-Commit-Position: refs/heads/master@{#379449}