Pass ffmpeg media track info to HTMLMediaElement

Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.

BUG=249427
Committed: https://crrev.com/ef1e5efdbb7458908a9018e92ab59ba24b43f8ca
Cr-Commit-Position: refs/heads/master@{#383250}

[servolk, 2016-03-01 19:39:12]

Description was changed from

==========
WIP: Read ffmpeg media track info and pass it to blink
==========

to

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.
==========

[servolk, 2016-03-05 02:09:04]

servolk@chromium.org changed reviewers:
+ philipj@opera.com, wolenetz@chromium.org

[philipj_slow, 2016-03-11 13:10:54]

Ping wolenetz@, not sure how to proceed :)

[wolenetz, 2016-03-11 23:19:54]

tl;dr: I was going through these CLs trying to find a good way of making the
signals common to src= and MSE and would like to chat with you at your earliest
convenience.

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:857: DCHECK(client_);
nit: are you aware of any path, in any WMPI method, where client_ might be null?
I think the basic assumption of all of WMPI is a non-null |client_|. Do we need
this line? If we do, maybe put it in ctor instead of here?

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:859: // Remove the old tracks first, if any
were present
I'm not aware of any code path that allows multiple calls to
OnFFmpegMediaTracksUpdated per WMPI instance. OnFindStreamInfoDone is done only
as part of FFmpegDemuxer::Initialize() processing, which is once per player
IIUC.

Are midstream track changes expected in any FFmpegDemuxer-based playback? I
don't think so now, and if this ever changes, we'll need to also adjust usage of
the media_tracks_updated_ callback. All this makes me think that the callback's
name and parameter needs some clarification, at least like "called exactly once
as part of initialization" (documented in code comments of FFmpegDemuxer::ctor
and FFmpegDemuxer::Initialize()).

I'm also growing concerned that, due to route this set of CLs is taking (this
one, https://codereview.chromium.org/1659653002/, and
https://codereview.chromium.org/1658033002/), where *initial* knowledge of the
various tracks is known at different times on the chromium side of the media
engine for MSE playbacks, that there could be some problems. For instance, as
part of compliant initialization segment received processing, if there are new
tracks, at what point will WMPI become aware of those blink track ids? We'll
need to have some hopefully common-to-at-least-WMPI across src= and MSE handling
of Blink signals like "enable audio track X" and "select video track Y". If X
and Y are known only to ChunkDemuxer, WebSourceBufferImpl, and Blink
SourceBuffer/HTMLMediaElement/etc, but not to WMPI, how will WMPI be able to
operate on the Blink enable/select signals? Maybe we should chat about this to
make sure we're on the same page. I'd be happy to hangout today, if you have
time.

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

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:463:
std::vector<blink::WebMediaPlayer::TrackId> blink_audio_tracks_;
nit: do we really need the "blink_" prefix on these members? If that prefix were
removed, would there be some confusion or name conflict introduced? I suggest
s/blink_audio_tracks_/audio_track_ids/ (and similar for video).

[servolk, 2016-03-18 18:36:43]

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:857: DCHECK(client_);
On 2016/03/11 23:19:54, wolenetz wrote:
> nit: are you aware of any path, in any WMPI method, where client_ might be
null?
> I think the basic assumption of all of WMPI is a non-null |client_|. Do we
need
> this line? If we do, maybe put it in ctor instead of here?

Good point, indeed the client_ is set only once in ctor, so I've moved the
DCHECK there.

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:859: // Remove the old tracks first, if any
were present
On 2016/03/11 23:19:54, wolenetz wrote:
> I'm not aware of any code path that allows multiple calls to
> OnFFmpegMediaTracksUpdated per WMPI instance. OnFindStreamInfoDone is done
only
> as part of FFmpegDemuxer::Initialize() processing, which is once per player
> IIUC.
> 
> Are midstream track changes expected in any FFmpegDemuxer-based playback? I
> don't think so now, and if this ever changes, we'll need to also adjust usage
of
> the media_tracks_updated_ callback. All this makes me think that the
callback's
> name and parameter needs some clarification, at least like "called exactly
once
> as part of initialization" (documented in code comments of FFmpegDemuxer::ctor
> and FFmpegDemuxer::Initialize()).
> 
> I'm also growing concerned that, due to route this set of CLs is taking (this
> one, https://codereview.chromium.org/1659653002/, and
> https://codereview.chromium.org/1658033002/), where *initial* knowledge of the
> various tracks is known at different times on the chromium side of the media
> engine for MSE playbacks, that there could be some problems. For instance, as
> part of compliant initialization segment received processing, if there are new
> tracks, at what point will WMPI become aware of those blink track ids? We'll
> need to have some hopefully common-to-at-least-WMPI across src= and MSE
handling
> of Blink signals like "enable audio track X" and "select video track Y". If X
> and Y are known only to ChunkDemuxer, WebSourceBufferImpl, and Blink
> SourceBuffer/HTMLMediaElement/etc, but not to WMPI, how will WMPI be able to
> operate on the Blink enable/select signals? Maybe we should chat about this to
> make sure we're on the same page. I'd be happy to hangout today, if you have
> time.

Yes, currently FFmpeg tracks are only going to be reported once, I was just
curious to try and make this code a bit more future-proof, and as you can see
it's not that difficult to be future-proof here and allow the media track info
to be reported multiple times per WMPI lifetime (which would allow mid-stream
track configuration changes). But I agree for now this is not strictly necessary
and we can simplify things a bit by removing this, so I'll remove it for now.

Re the second part, I've put together another CL to demonstrate how we can
achieve handling of blink enable/disable/select track signals. All we need for
that is to be able to translate blink track ids into DemuxerStreams on demuxer
level. Then we can propagate those signals through the rest of media stack as a
set of DemuxStream* objects and that works just fine, see
https://codereview.chromium.org/1812543003/.

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

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:463:
std::vector<blink::WebMediaPlayer::TrackId> blink_audio_tracks_;
On 2016/03/11 23:19:54, wolenetz wrote:
> nit: do we really need the "blink_" prefix on these members? If that prefix
were
> removed, would there be some confusion or name conflict introduced? I suggest
> s/blink_audio_tracks_/audio_track_ids/ (and similar for video).

The main reason I wanted to include 'blink_' in here is to avoid confusion about
whether these are bytestream track ids or blink track ids (which are
different!). I guess it's already clear here, since we have 'blink::' in the
type definition, but I wanted this to be also clear in the code where these
collections are used (e.g. in WMPI::OnFFmpegMediaTracksUpdated).
But now that we are going to assume FFmpeg tracks are only going to be reported
once (see the other comment), we can just remove these collections altogether.

[wolenetz, 2016-03-23 22:53:04]

lgtm
[I found that the new test.webm is the same as the chromium
//src/media/test/data/bear.webm]

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

https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:463:
std::vector<blink::WebMediaPlayer::TrackId> blink_audio_tracks_;
On 2016/03/18 18:36:42, servolk wrote:
> On 2016/03/11 23:19:54, wolenetz wrote:
> > nit: do we really need the "blink_" prefix on these members? If that prefix
> were
> > removed, would there be some confusion or name conflict introduced? I
suggest
> > s/blink_audio_tracks_/audio_track_ids/ (and similar for video).
> 
> The main reason I wanted to include 'blink_' in here is to avoid confusion
about
> whether these are bytestream track ids or blink track ids (which are
> different!). I guess it's already clear here, since we have 'blink::' in the
> type definition, but I wanted this to be also clear in the code where these
> collections are used (e.g. in WMPI::OnFFmpegMediaTracksUpdated).
> But now that we are going to assume FFmpeg tracks are only going to be
reported
> once (see the other comment), we can just remove these collections altogether.

Acknowledged.

https://codereview.chromium.org/1735763002/diff/380001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1735763002/diff/380001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1237:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnFFmpegMediaTracksUpdated);
Good catch that this needs to be handled on the renderer main thread! Otherwise
it would run on the pipeline/media thread!
Further, BIND_TO_RENDER_LOOP takes care of the ugly edge case where WMPI might
be gone by the time ffmpeg figures out the various tracks in the stream and
executes the callback.

[servolk, 2016-03-24 01:37:39]

On 2016/03/23 22:53:04, wolenetz wrote:
> lgtm
> [I found that the new test.webm is the same as the chromium
> //src/media/test/data/bear.webm]
> 
>
https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.h (right):
> 
>
https://codereview.chromium.org/1735763002/diff/240001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.h:463:
> std::vector<blink::WebMediaPlayer::TrackId> blink_audio_tracks_;
> On 2016/03/18 18:36:42, servolk wrote:
> > On 2016/03/11 23:19:54, wolenetz wrote:
> > > nit: do we really need the "blink_" prefix on these members? If that
prefix
> > were
> > > removed, would there be some confusion or name conflict introduced? I
> suggest
> > > s/blink_audio_tracks_/audio_track_ids/ (and similar for video).
> > 
> > The main reason I wanted to include 'blink_' in here is to avoid confusion
> about
> > whether these are bytestream track ids or blink track ids (which are
> > different!). I guess it's already clear here, since we have 'blink::' in the
> > type definition, but I wanted this to be also clear in the code where these
> > collections are used (e.g. in WMPI::OnFFmpegMediaTracksUpdated).
> > But now that we are going to assume FFmpeg tracks are only going to be
> reported
> > once (see the other comment), we can just remove these collections
altogether.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1735763002/diff/380001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/1735763002/diff/380001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:1237:
> BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnFFmpegMediaTracksUpdated);
> Good catch that this needs to be handled on the renderer main thread!
Otherwise
> it would run on the pipeline/media thread!
> Further, BIND_TO_RENDER_LOOP takes care of the ugly edge case where WMPI might
> be gone by the time ffmpeg figures out the various tracks in the stream and
> executes the callback.

Yes, test.webm is a copy of bear.webm. I needed a .webm file for new layout
tests, since the proprietary test.mp4 is not available on some trybots (e.g. see
linux_blink_oilpan_rel trybot failure in patchset #19), so I've made a copy of
bear.webm for now, but I'm planning to replace it with a better webm file (that
specifies track languages and titles) later.

[wolenetz, 2016-03-24 01:49:00]

On 2016/03/24 01:37:39, servolk wrote:
> On 2016/03/23 22:53:04, wolenetz wrote:
...
> > [I found that the new test.webm is the same as the chromium
> > //src/media/test/data/bear.webm]
...
> Yes, test.webm is a copy of bear.webm. I needed a .webm file for new layout
> tests, since the proprietary test.mp4 is not available on some trybots (e.g.
see
> linux_blink_oilpan_rel trybot failure in patchset #19), so I've made a copy of
> bear.webm for now, but I'm planning to replace it with a better webm file
(that
> specifies track languages and titles) later.

Acknowledged

[servolk, 2016-03-25 02:11:08]

Description was changed from

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.
==========

to

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.

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

[servolk, 2016-03-25 02:11:25]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-03-25 02:11:25]

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/1735763002/#ps400001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-25 02:11:40]

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

[commit-bot: I haz the power, 2016-03-25 04:55:34]

Description was changed from

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.

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

to

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.

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

[commit-bot: I haz the power, 2016-03-25 04:55:35]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-03-25 04:57:32]

Description was changed from

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.

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

to

==========
Pass ffmpeg media track info to HTMLMediaElement

This CL implements passing FFmpeg track info generated by FFmpegDemuxer
to blink HTMLMediaElement. Also adds a few new blink layout tests for
that.

BUG=249427

Committed: https://crrev.com/ef1e5efdbb7458908a9018e92ab59ba24b43f8ca
Cr-Commit-Position: refs/heads/master@{#383250}
==========

[commit-bot: I haz the power, 2016-03-25 04:57:33]

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