Issue 1922333002: Implement mapping blink track id to demuxer streams

servolk

Description was changed from ========== Implement mapping blink track id to demuxer streams BUG=249427 ========== ...

4 years, 7 months ago (2016-05-12 22:00:14 UTC) #1

servolk

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

4 years, 7 months ago (2016-05-12 22:00:14 UTC) #2

chcunningham

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

4 years, 6 months ago (2016-06-02 21:24:44 UTC) #4

chcunningham

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_tracks.cc File media/base/media_tracks.cc (right): https://codereview.chromium.org/1922333002/diff/200001/media/base/media_tracks.cc#newcode19 media/base/media_tracks.cc:19: const std::string& id, What are the plans for this ...

4 years, 6 months ago (2016-06-02 21:24:46 UTC) #5

servolk

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_tracks.cc File media/base/media_tracks.cc (right): https://codereview.chromium.org/1922333002/diff/200001/media/base/media_tracks.cc#newcode19 media/base/media_tracks.cc:19: const std::string& id, On 2016/06/02 21:24:45, chcunningham wrote: > ...

4 years, 6 months ago (2016-06-02 23:53:12 UTC) #6

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_track...
File media/base/media_tracks.cc (right):

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_track...
media/base/media_tracks.cc:19: const std::string& id,
On 2016/06/02 21:24:45, chcunningham wrote:
> What are the plans for this "id"? Its a little confusing to call this ID while
a
> trackID also exists. IIUC, these strings are always things like "audio",
"text",
> "video"... but I'm guessing that may change if we later enable multi-track?
The
> core of my question is - can we name these something else, here and in the
> various places they're created/passed?

How about byteStreamTrackId?
Also, these are typically just numbers for now, so it's "1", "2", "3" instead of
"audio", "video". I've already prepared a separate CL to change the type from
string to StreamParser::TrackId (see
https://codereview.chromium.org/1861963008/). I'll do the renaming there as well
to keep this CL easier to review.

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_track...
media/base/media_tracks.cc:48: DCHECK(track_to_demux_stream_map_.find(track) ==
On 2016/06/02 21:24:45, chcunningham wrote:
> How are you deciding when to DCHECK vs CHECK? Both of the new methods here mix
> DCHECK and CHECK

In this case dcheck are for things that might indicate issues but might be
costly and unnecessary in release builds, whereas checks are used to protect us
from e.g. invalid memory accesses.

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

https://codereview.chromium.org/1922333002/diff/200001/media/blink/websourceb...
media/blink/websourcebuffer_impl.cc:196: for (const auto& id : blinkTrackIds)
On 2016/06/02 21:24:45, chcunningham wrote:
> Surprised there isn't a util function for this (or a method off WebVector to
> just get the underlying vector)? I can't find one though... 

Yep, I didn't find anything like that either.

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

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1050: const auto& new_track_id_map =
tracks.OnTrackIdsAssigned(track_ids);
On 2016/06/02 21:24:45, chcunningham wrote:
> This is a little strange to me.
> 
> For one, calling local.On* for a local you don't own is not typical. I think
> this is mostly just a case of name choice though.
MediaTracks.OnTrackIdsAssigned
> is a const method, purely for building this id->stream map. Could you instead
> call it something like MapTrackIdsToDemuxerStreams(...)?
> 
> I also think its weird that both here and in ffmpeg demuxer, the track and
> track_id args are passed in, only to have them be combined to produce the map
> and never used after that. I think we could instead have the callers do the
> combining and pass in the map.
> 
> I debated other designs where we let the map of DemuxerStreams to MediaTracks
> instead live here in ChunkDemuxer, or perhaps MediaSourceState... it seems
more
> complicated that way though and I like how these maps disappear along with
> MediaTracks once the final track-id -> DemuxerStream mapping is established. 

Yeah, in fact I tried maintaining both maps in demuxers, but that results in
more complicated code, so we came up with this after a few iterations of the
previous CL that did this (see https://codereview.chromium.org/1812543003). The
current approach here allows us to avoid duplicating the code for building the
maps between FFmpeg/ChunkDemuxer. And it gets rid of the media track -> demuxer
stream map as soon as it's no longer needed, which simplifies things a bit. Take
a look at the history of patchsets in the older CL
https://codereview.chromium.org/1812543003 and my discussion with Matt there.

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
File media/filters/chunk_demuxer.h (left):

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer.h:322: // Returns a new ChunkDemuxerStream instance
if a stream of this type
On 2016/06/02 21:24:45, chcunningham wrote:
> of this track's type

Done.

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (left):

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:84: 
On 2016/06/02 21:24:45, chcunningham wrote:
> Similar test for ffmpeg_demuxer would be good

FFmpeg demuxer currently doesn't support mid-stream config changes, so there's
only one set of tracks that is determined at the beginning of playback and never
changes after that.

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

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:97: // of the playback session, whereas
track numbers / bytestream track ids might
On 2016/06/02 21:24:45, chcunningham wrote:
> I was surprised to read this. Looking at the code here [0] it seems we expect
> bytestream track ids to map to blink track ids. And if we encounter a new
> bytstream track id, we seem to expect that it is already known (existing) in
> SourceBuffer if we've already processed the first init segment? What am I
> missing? 
> 
> [0]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That's right. If we encounter a new bytestream track id and there's only one
track of the given type, then we assume that new bytestream track corresponds to
the same track, even if the actual bytestream id is different. So the
external/blink track ids below map to the k*TrackNum in the first init segment
and kAlternate*TrackNum in the second init segment.

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:99: const int kVideoTrackId = 10;
On 2016/06/02 21:24:45, chcunningham wrote:
> Also, can you name these in a way that describes their different purpose? E.g.
> kExternalVideoTrackId vs kBystreamVideoTrackId

Done.

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:1416: 
On 2016/06/02 21:24:45, chcunningham wrote:
> This is busted for multi-track right? You might assert that you only see one
> track of a given type

Yes, but there are many other places in Chromium media pipeline where we assume
there's only one audio and one video track (e.g.
DemuxerStreamProvider::GetStream and media::Renderer implementations).

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

https://codereview.chromium.org/1922333002/diff/200001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1604: DCHECK(track_id_to_demux_stream_.empty());
On 2016/06/02 21:24:45, chcunningham wrote:
> Same question about DCHECK vs CHECK for these two methods

It's the same here. In release build we could bypass this dcheck and overwrite
the mapping and it would work fine (although it shouldn't happen, since
FFmpegDemuxer only reads track info once). But in the case below if |it|
indicates that the track with a given id wasn't found (unlikely, but might
happen), then we should not try to access it->second (since that could lead to
invalid memory access) and instead we better crash safely via CHECK.

xhwang

Could you please provide more information in the CL description on what this CL does ...

4 years, 6 months ago (2016-06-03 18:52:45 UTC) #7

servolk

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc#newcode884 media/blink/webmediaplayer_impl.cc:884: demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds); On 2016/06/03 18:52:45, xhwang wrote: > It ...

4 years, 6 months ago (2016-06-03 20:06:45 UTC) #8

xhwang

On 2016/06/03 20:06:45, servolk wrote: > https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc#newcode884 > ...

4 years, 6 months ago (2016-06-03 20:13:07 UTC) #9

On 2016/06/03 20:06:45, servolk wrote:
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:884:
> demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
> On 2016/06/03 18:52:45, xhwang wrote:
> > It looks really odd that we create the track in chromium, then call into
blink
> > to get the id, then call chromium again to assign that id...
> > 
> > Since the track ID is just a number, I guess as long as it's unique we are
> fine.
> > Can we actually just create that ID in chromium? Then we don't need
> > OnTrackIdsAssigned().
> > 
> > Also, having comments in a lot of places that the |tracks| and |track_ids|
> must
> > have the same size and must be in the same order sounds a bad pattern and is
> > error prone.
> 
> Thanks, good point. Blink has had separate bytestream track ids and
> WebMediaPlayer::TrackId for a long time (long before I touched any of that
> code), and I assumed that both were necessary for some reason. But now after
> these comments I went and looked closer and I've started wondering if we can
get
> rid of WebMediaPlayer::TrackId completely and live with identifying media
tracks
> by their bytestream ids. Sure, there will be some minor overhead for using
> strings instead of unsigned integers as ids. But I think it's reasonable to
> expect that the number of media tracks is typically very low (vast majority of
> cases is just 1 audio + 1 video track, in 99% of other cases I'm sure the
total
> number of track will be fewer than ~5-10), so the overhead shouldn't be
> significant.
> Matt, now that philipj@ is no longer active in this area, I guess it's your
> call. WDYT about getting rid of WebMediaPlayer::TrackId and using bytestream
> track ids everywhere instead?

I am not familiar with the byte stream ID and not sure how we can make sure
those IDs are unique (for strings). But my gut feeling is the less IDs we have
the better. The current heavy comments we have in this CL about the differences
between byte stream ID and track ID seems a pretty clear signal.

chcunningham

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_tracks.cc File media/base/media_tracks.cc (right): https://codereview.chromium.org/1922333002/diff/200001/media/base/media_tracks.cc#newcode19 media/base/media_tracks.cc:19: const std::string& id, On 2016/06/02 23:53:11, servolk wrote: > ...

4 years, 6 months ago (2016-06-03 20:42:49 UTC) #10

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_track...
File media/base/media_tracks.cc (right):

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_track...
media/base/media_tracks.cc:19: const std::string& id,
On 2016/06/02 23:53:11, servolk wrote:
> On 2016/06/02 21:24:45, chcunningham wrote:
> > What are the plans for this "id"? Its a little confusing to call this ID
while
> a
> > trackID also exists. IIUC, these strings are always things like "audio",
> "text",
> > "video"... but I'm guessing that may change if we later enable multi-track?
> The
> > core of my question is - can we name these something else, here and in the
> > various places they're created/passed?
> 
> How about byteStreamTrackId?
> Also, these are typically just numbers for now, so it's "1", "2", "3" instead
of
> "audio", "video". I've already prepared a separate CL to change the type from
> string to StreamParser::TrackId (see
> https://codereview.chromium.org/1861963008/). I'll do the renaming there as
well
> to keep this CL easier to review.

byteStreamTrackId sounds good

https://codereview.chromium.org/1922333002/diff/200001/media/base/media_track...
media/base/media_tracks.cc:48: DCHECK(track_to_demux_stream_map_.find(track) ==
On 2016/06/02 23:53:11, servolk wrote:
> On 2016/06/02 21:24:45, chcunningham wrote:
> > How are you deciding when to DCHECK vs CHECK? Both of the new methods here
mix
> > DCHECK and CHECK
> 
> In this case dcheck are for things that might indicate issues but might be
> costly and unnecessary in release builds, whereas checks are used to protect
us
> from e.g. invalid memory accesses.

Acknowledged.

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

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1050: const auto& new_track_id_map =
tracks.OnTrackIdsAssigned(track_ids);
On 2016/06/02 23:53:11, servolk wrote:
> On 2016/06/02 21:24:45, chcunningham wrote:
> > This is a little strange to me.
> > 
> > For one, calling local.On* for a local you don't own is not typical. I think
> > this is mostly just a case of name choice though.
> MediaTracks.OnTrackIdsAssigned
> > is a const method, purely for building this id->stream map. Could you
instead
> > call it something like MapTrackIdsToDemuxerStreams(...)?
> > 
> > I also think its weird that both here and in ffmpeg demuxer, the track and
> > track_id args are passed in, only to have them be combined to produce the
map
> > and never used after that. I think we could instead have the callers do the
> > combining and pass in the map.
> > 
> > I debated other designs where we let the map of DemuxerStreams to
MediaTracks
> > instead live here in ChunkDemuxer, or perhaps MediaSourceState... it seems
> more
> > complicated that way though and I like how these maps disappear along with
> > MediaTracks once the final track-id -> DemuxerStream mapping is established.

> 
> Yeah, in fact I tried maintaining both maps in demuxers, but that results in
> more complicated code, so we came up with this after a few iterations of the
> previous CL that did this (see https://codereview.chromium.org/1812543003).
The
> current approach here allows us to avoid duplicating the code for building the
> maps between FFmpeg/ChunkDemuxer. And it gets rid of the media track ->
demuxer
> stream map as soon as it's no longer needed, which simplifies things a bit.
Take
> a look at the history of patchsets in the older CL
> https://codereview.chromium.org/1812543003 and my discussion with Matt there.

I follow, but I think this only addresses my last paragraph. What about my first
two concerns?

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

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:97: // of the playback session, whereas
track numbers / bytestream track ids might
On 2016/06/02 23:53:11, servolk wrote:
> On 2016/06/02 21:24:45, chcunningham wrote:
> > I was surprised to read this. Looking at the code here [0] it seems we
expect
> > bytestream track ids to map to blink track ids. And if we encounter a new
> > bytstream track id, we seem to expect that it is already known (existing) in
> > SourceBuffer if we've already processed the first init segment? What am I
> > missing? 
> > 
> > [0]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> That's right. If we encounter a new bytestream track id and there's only one
> track of the given type, then we assume that new bytestream track corresponds
to
> the same track, even if the actual bytestream id is different. So the
> external/blink track ids below map to the k*TrackNum in the first init segment
> and kAlternate*TrackNum in the second init segment.

I see. Are we missing some code in SourceBuffer.cpp? IIUC we add tracks with the
original bystream track ids upon receiving the first init segment. But if a
later init segment changes the bytstream track id, we seem to expect to find a
track with that bytstream id already in the track list?

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

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:884:
demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
I'm also looking for away to let Chromium decide the track Ids, or at least a
way to get rid of the mapping being carried around in MediaTracks.

You mentioned in the chunk demuxer tests that separate bytestream tracks (from
separate init segments) may actually map to a single blink track - how would
this work if we just use bytestream tracks everywhere? 

If we can't use bytstream tracks everywhere, another proposal is to pass a
callback from blink into chromium to allow for reserving of a track id. Chromium
could reserve the id at the time its creating the demuxer stream and associate
them immediately. Then set that track id on the MediaTracks object when you
bubble things up to blink.

chcunningham

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_demuxer.cc#newcode1050 media/filters/chunk_demuxer.cc:1050: const auto& new_track_id_map = tracks.OnTrackIdsAssigned(track_ids); On 2016/06/03 20:42:49, chcunningham ...

4 years, 6 months ago (2016-06-03 20:44:25 UTC) #11

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

https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:1050: const auto& new_track_id_map =
tracks.OnTrackIdsAssigned(track_ids);
On 2016/06/03 20:42:49, chcunningham wrote:
> On 2016/06/02 23:53:11, servolk wrote:
> > On 2016/06/02 21:24:45, chcunningham wrote:
> > > This is a little strange to me.
> > > 
> > > For one, calling local.On* for a local you don't own is not typical. I
think
> > > this is mostly just a case of name choice though.
> > MediaTracks.OnTrackIdsAssigned
> > > is a const method, purely for building this id->stream map. Could you
> instead
> > > call it something like MapTrackIdsToDemuxerStreams(...)?
> > > 
> > > I also think its weird that both here and in ffmpeg demuxer, the track and
> > > track_id args are passed in, only to have them be combined to produce the
> map
> > > and never used after that. I think we could instead have the callers do
the
> > > combining and pass in the map.
> > > 
> > > I debated other designs where we let the map of DemuxerStreams to
> MediaTracks
> > > instead live here in ChunkDemuxer, or perhaps MediaSourceState... it seems
> > more
> > > complicated that way though and I like how these maps disappear along with
> > > MediaTracks once the final track-id -> DemuxerStream mapping is
established.
> 
> > 
> > Yeah, in fact I tried maintaining both maps in demuxers, but that results in
> > more complicated code, so we came up with this after a few iterations of the
> > previous CL that did this (see https://codereview.chromium.org/1812543003).
> The
> > current approach here allows us to avoid duplicating the code for building
the
> > maps between FFmpeg/ChunkDemuxer. And it gets rid of the media track ->
> demuxer
> > stream map as soon as it's no longer needed, which simplifies things a bit.
> Take
> > a look at the history of patchsets in the older CL
> > https://codereview.chromium.org/1812543003 and my discussion with Matt
there.
> 
> I follow, but I think this only addresses my last paragraph. What about my
first
> two concerns?

My comment here may take precedence over this
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...

servolk

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc#newcode884 media/blink/webmediaplayer_impl.cc:884: demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds); On 2016/06/03 20:42:49, chcunningham wrote: > I'm ...

4 years, 6 months ago (2016-06-03 20:57:10 UTC) #12

servolk

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc#newcode884 media/blink/webmediaplayer_impl.cc:884: demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds); On 2016/06/03 20:57:10, servolk wrote: > On ...

4 years, 6 months ago (2016-06-03 23:35:36 UTC) #13

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

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:884:
demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
On 2016/06/03 20:57:10, servolk wrote:
> On 2016/06/03 20:42:49, chcunningham wrote:
> > I'm also looking for away to let Chromium decide the track Ids, or at least
a
> > way to get rid of the mapping being carried around in MediaTracks.
> > 
> > You mentioned in the chunk demuxer tests that separate bytestream tracks
(from
> > separate init segments) may actually map to a single blink track - how would
> > this work if we just use bytestream tracks everywhere? 
> > 
> > If we can't use bytstream tracks everywhere, another proposal is to pass a
> > callback from blink into chromium to allow for reserving of a track id.
> Chromium
> > could reserve the id at the time its creating the demuxer stream and
associate
> > them immediately. Then set that track id on the MediaTracks object when you
> > bubble things up to blink. 
> 
> Yes, multiple bytestream track ids might map to the same blink track id, but
> only in cases when we have a single track of the given type (see the comment
at
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...).
> So if we have only a single audio and a single video track, we don't need to
pay
> attention to track ids at all, we'll know what track ids gets enabled/disabled
> simply because we know the track type. And for cases of multiple audio and/or
> video tracks bytestream ids will remain unchanged between different init
> segments.
> That's why I think we could get rid of WebMediaPlayer::TrackId. IIUC  track
type
> + bytestream track id should be sufficient to uniquely identify a media track.

Well, completely removing WebMediaPlayer::TrackId is difficult, since it's used
in many places. But I really wanted to give it a try, so I tried changing
WebMediaPlayer::TrackId type from unsigned to WebString and then removed blink
internal track ids completely and replaced them with byte stream track ids. Here
is what I tried:
https://codereview.chromium.org/2034273003/
And this seems to be working. Well, at least blink media and mediasource layout
tests pass. So I guess this is a viable idea.

servolk

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc#newcode884 media/blink/webmediaplayer_impl.cc:884: demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds); On 2016/06/03 23:35:36, servolk wrote: > On ...

4 years, 6 months ago (2016-06-06 17:42:56 UTC) #14

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

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:884:
demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
On 2016/06/03 23:35:36, servolk wrote:
> On 2016/06/03 20:57:10, servolk wrote:
> > On 2016/06/03 20:42:49, chcunningham wrote:
> > > I'm also looking for away to let Chromium decide the track Ids, or at
least
> a
> > > way to get rid of the mapping being carried around in MediaTracks.
> > > 
> > > You mentioned in the chunk demuxer tests that separate bytestream tracks
> (from
> > > separate init segments) may actually map to a single blink track - how
would
> > > this work if we just use bytestream tracks everywhere? 
> > > 
> > > If we can't use bytstream tracks everywhere, another proposal is to pass a
> > > callback from blink into chromium to allow for reserving of a track id.
> > Chromium
> > > could reserve the id at the time its creating the demuxer stream and
> associate
> > > them immediately. Then set that track id on the MediaTracks object when
you
> > > bubble things up to blink. 
> > 
> > Yes, multiple bytestream track ids might map to the same blink track id, but
> > only in cases when we have a single track of the given type (see the comment
> at
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...).
> > So if we have only a single audio and a single video track, we don't need to
> pay
> > attention to track ids at all, we'll know what track ids gets
enabled/disabled
> > simply because we know the track type. And for cases of multiple audio
and/or
> > video tracks bytestream ids will remain unchanged between different init
> > segments.
> > That's why I think we could get rid of WebMediaPlayer::TrackId. IIUC  track
> type
> > + bytestream track id should be sufficient to uniquely identify a media
track.
> 
> Well, completely removing WebMediaPlayer::TrackId is difficult, since it's
used
> in many places. But I really wanted to give it a try, so I tried changing
> WebMediaPlayer::TrackId type from unsigned to WebString and then removed blink
> internal track ids completely and replaced them with byte stream track ids.
Here
> is what I tried:
> https://codereview.chromium.org/2034273003/
> And this seems to be working. Well, at least blink media and mediasource
layout
> tests pass. So I guess this is a viable idea.

Update: I though about it some more over the weekend, and I realized that
switching to using bytestream track ids might be problematic in case of multiple
SourceBuffers. In that case bytestream track ids are guaranteed to be unique
with a bytestream (i.e. a single SourceBuffer), but are not guaranteed to be
unique across different SourceBuffers. So we'll still need to have the concept
of WebMediaPlayer::TrackId that will be guaranteed to be unique with the
WebMediaPlayer, even if it has multiple SourceBuffers. But we could still try to
move the WMP::TrackId generation from WMP into demuxers. That would simplify
mapping between TrackIds and demuxer streams. I'll give it a try today.

xhwang

On 2016/06/06 17:42:56, servolk wrote: > https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc#newcode884 > ...

4 years, 6 months ago (2016-06-06 17:48:04 UTC) #15

On 2016/06/06 17:42:56, servolk wrote:
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:884:
> demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
> On 2016/06/03 23:35:36, servolk wrote:
> > On 2016/06/03 20:57:10, servolk wrote:
> > > On 2016/06/03 20:42:49, chcunningham wrote:
> > > > I'm also looking for away to let Chromium decide the track Ids, or at
> least
> > a
> > > > way to get rid of the mapping being carried around in MediaTracks.
> > > > 
> > > > You mentioned in the chunk demuxer tests that separate bytestream tracks
> > (from
> > > > separate init segments) may actually map to a single blink track - how
> would
> > > > this work if we just use bytestream tracks everywhere? 
> > > > 
> > > > If we can't use bytstream tracks everywhere, another proposal is to pass
a
> > > > callback from blink into chromium to allow for reserving of a track id.
> > > Chromium
> > > > could reserve the id at the time its creating the demuxer stream and
> > associate
> > > > them immediately. Then set that track id on the MediaTracks object when
> you
> > > > bubble things up to blink. 
> > > 
> > > Yes, multiple bytestream track ids might map to the same blink track id,
but
> > > only in cases when we have a single track of the given type (see the
comment
> > at
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...).
> > > So if we have only a single audio and a single video track, we don't need
to
> > pay
> > > attention to track ids at all, we'll know what track ids gets
> enabled/disabled
> > > simply because we know the track type. And for cases of multiple audio
> and/or
> > > video tracks bytestream ids will remain unchanged between different init
> > > segments.
> > > That's why I think we could get rid of WebMediaPlayer::TrackId. IIUC 
track
> > type
> > > + bytestream track id should be sufficient to uniquely identify a media
> track.
> > 
> > Well, completely removing WebMediaPlayer::TrackId is difficult, since it's
> used
> > in many places. But I really wanted to give it a try, so I tried changing
> > WebMediaPlayer::TrackId type from unsigned to WebString and then removed
blink
> > internal track ids completely and replaced them with byte stream track ids.
> Here
> > is what I tried:
> > https://codereview.chromium.org/2034273003/
> > And this seems to be working. Well, at least blink media and mediasource
> layout
> > tests pass. So I guess this is a viable idea.
> 
> Update: I though about it some more over the weekend, and I realized that
> switching to using bytestream track ids might be problematic in case of
multiple
> SourceBuffers. In that case bytestream track ids are guaranteed to be unique
> with a bytestream (i.e. a single SourceBuffer), but are not guaranteed to be
> unique across different SourceBuffers. So we'll still need to have the concept
> of WebMediaPlayer::TrackId that will be guaranteed to be unique with the
> WebMediaPlayer, even if it has multiple SourceBuffers. But we could still try
to
> move the WMP::TrackId generation from WMP into demuxers. That would simplify
> mapping between TrackIds and demuxer streams. I'll give it a try today.

Thanks! I am not really familiar with how bytestream track ids are generated. If
anyone can point me to the correct doc/code I'd like to learn :) I wonder
whether we can make bytestream track ids unique in a process? If so then we
don't have this issue.

servolk

On 2016/06/06 17:48:04, xhwang wrote: > On 2016/06/06 17:42:56, servolk wrote: > > > https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediaplayer_impl.cc ...

4 years, 6 months ago (2016-06-06 18:09:49 UTC) #16

On 2016/06/06 17:48:04, xhwang wrote:
> On 2016/06/06 17:42:56, servolk wrote:
> >
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> > File media/blink/webmediaplayer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> > media/blink/webmediaplayer_impl.cc:884:
> > demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
> > On 2016/06/03 23:35:36, servolk wrote:
> > > On 2016/06/03 20:57:10, servolk wrote:
> > > > On 2016/06/03 20:42:49, chcunningham wrote:
> > > > > I'm also looking for away to let Chromium decide the track Ids, or at
> > least
> > > a
> > > > > way to get rid of the mapping being carried around in MediaTracks.
> > > > > 
> > > > > You mentioned in the chunk demuxer tests that separate bytestream
tracks
> > > (from
> > > > > separate init segments) may actually map to a single blink track - how
> > would
> > > > > this work if we just use bytestream tracks everywhere? 
> > > > > 
> > > > > If we can't use bytstream tracks everywhere, another proposal is to
pass
> a
> > > > > callback from blink into chromium to allow for reserving of a track
id.
> > > > Chromium
> > > > > could reserve the id at the time its creating the demuxer stream and
> > > associate
> > > > > them immediately. Then set that track id on the MediaTracks object
when
> > you
> > > > > bubble things up to blink. 
> > > > 
> > > > Yes, multiple bytestream track ids might map to the same blink track id,
> but
> > > > only in cases when we have a single track of the given type (see the
> comment
> > > at
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...).
> > > > So if we have only a single audio and a single video track, we don't
need
> to
> > > pay
> > > > attention to track ids at all, we'll know what track ids gets
> > enabled/disabled
> > > > simply because we know the track type. And for cases of multiple audio
> > and/or
> > > > video tracks bytestream ids will remain unchanged between different init
> > > > segments.
> > > > That's why I think we could get rid of WebMediaPlayer::TrackId. IIUC 
> track
> > > type
> > > > + bytestream track id should be sufficient to uniquely identify a media
> > track.
> > > 
> > > Well, completely removing WebMediaPlayer::TrackId is difficult, since it's
> > used
> > > in many places. But I really wanted to give it a try, so I tried changing
> > > WebMediaPlayer::TrackId type from unsigned to WebString and then removed
> blink
> > > internal track ids completely and replaced them with byte stream track
ids.
> > Here
> > > is what I tried:
> > > https://codereview.chromium.org/2034273003/
> > > And this seems to be working. Well, at least blink media and mediasource
> > layout
> > > tests pass. So I guess this is a viable idea.
> > 
> > Update: I though about it some more over the weekend, and I realized that
> > switching to using bytestream track ids might be problematic in case of
> multiple
> > SourceBuffers. In that case bytestream track ids are guaranteed to be unique
> > with a bytestream (i.e. a single SourceBuffer), but are not guaranteed to be
> > unique across different SourceBuffers. So we'll still need to have the
concept
> > of WebMediaPlayer::TrackId that will be guaranteed to be unique with the
> > WebMediaPlayer, even if it has multiple SourceBuffers. But we could still
try
> to
> > move the WMP::TrackId generation from WMP into demuxers. That would simplify
> > mapping between TrackIds and demuxer streams. I'll give it a try today.
> 
> Thanks! I am not really familiar with how bytestream track ids are generated.
If
> anyone can point me to the correct doc/code I'd like to learn :) I wonder
> whether we can make bytestream track ids unique in a process? If so then we
> don't have this issue.

Bytestream track ids are read from the bytestream by various stream parsers. The
spec for that is https://dev.w3.org/html5/html-sourcing-inband-tracks/. For
example for mp4 streams, bytestream track ids are read from track_id field of
the track header (TKHD) box
(https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?rcl...).
For webm streams bytestream track ids are decimal representations of the
TrackNumber field of the track in the Track section of the WebM file. etc.
Those bytestream track ids are used internally by stream parsers, for example to
determine which stream the demuxed/parsed data packet belongs to, so we can't
assign our own values to those.
And as I mentioned in my previous comment, in case of a single bytestream
(single SourceBuffer) the bytestream track ids are guaranteed to be unique. The
problem is that a MediaSource might have multiple SourceBuffers added to it,
which might make bytestream ids of different bytestreams clash. That's why we
need a different track id, that would be unique.

wolenetz

On 2016/06/06 18:09:49, servolk wrote: > On 2016/06/06 17:48:04, xhwang wrote: > > On 2016/06/06 ...

4 years, 6 months ago (2016-06-06 18:54:14 UTC) #17

On 2016/06/06 18:09:49, servolk wrote:
> On 2016/06/06 17:48:04, xhwang wrote:
> > On 2016/06/06 17:42:56, servolk wrote:
> > >
> >
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> > > File media/blink/webmediaplayer_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> > > media/blink/webmediaplayer_impl.cc:884:
> > > demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
> > > On 2016/06/03 23:35:36, servolk wrote:
> > > > On 2016/06/03 20:57:10, servolk wrote:
> > > > > On 2016/06/03 20:42:49, chcunningham wrote:
> > > > > > I'm also looking for away to let Chromium decide the track Ids, or
at
> > > least
> > > > a
> > > > > > way to get rid of the mapping being carried around in MediaTracks.
> > > > > > 
> > > > > > You mentioned in the chunk demuxer tests that separate bytestream
> tracks
> > > > (from
> > > > > > separate init segments) may actually map to a single blink track -
how
> > > would
> > > > > > this work if we just use bytestream tracks everywhere? 
> > > > > > 
> > > > > > If we can't use bytstream tracks everywhere, another proposal is to
> pass
> > a
> > > > > > callback from blink into chromium to allow for reserving of a track
> id.
> > > > > Chromium
> > > > > > could reserve the id at the time its creating the demuxer stream and
> > > > associate
> > > > > > them immediately. Then set that track id on the MediaTracks object
> when
> > > you
> > > > > > bubble things up to blink. 
> > > > > 
> > > > > Yes, multiple bytestream track ids might map to the same blink track
id,
> > but
> > > > > only in cases when we have a single track of the given type (see the
> > comment
> > > > at
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...).
> > > > > So if we have only a single audio and a single video track, we don't
> need
> > to
> > > > pay
> > > > > attention to track ids at all, we'll know what track ids gets
> > > enabled/disabled
> > > > > simply because we know the track type. And for cases of multiple audio
> > > and/or
> > > > > video tracks bytestream ids will remain unchanged between different
init
> > > > > segments.
> > > > > That's why I think we could get rid of WebMediaPlayer::TrackId. IIUC 
> > track
> > > > type
> > > > > + bytestream track id should be sufficient to uniquely identify a
media
> > > track.
> > > > 
> > > > Well, completely removing WebMediaPlayer::TrackId is difficult, since
it's
> > > used
> > > > in many places. But I really wanted to give it a try, so I tried
changing
> > > > WebMediaPlayer::TrackId type from unsigned to WebString and then removed
> > blink
> > > > internal track ids completely and replaced them with byte stream track
> ids.
> > > Here
> > > > is what I tried:
> > > > https://codereview.chromium.org/2034273003/
> > > > And this seems to be working. Well, at least blink media and mediasource
> > > layout
> > > > tests pass. So I guess this is a viable idea.
> > > 
> > > Update: I though about it some more over the weekend, and I realized that
> > > switching to using bytestream track ids might be problematic in case of
> > multiple
> > > SourceBuffers. In that case bytestream track ids are guaranteed to be
unique
> > > with a bytestream (i.e. a single SourceBuffer), but are not guaranteed to
be
> > > unique across different SourceBuffers. So we'll still need to have the
> concept
> > > of WebMediaPlayer::TrackId that will be guaranteed to be unique with the
> > > WebMediaPlayer, even if it has multiple SourceBuffers. But we could still
> try
> > to
> > > move the WMP::TrackId generation from WMP into demuxers. That would
simplify
> > > mapping between TrackIds and demuxer streams. I'll give it a try today.
> > 
> > Thanks! I am not really familiar with how bytestream track ids are
generated.
> If
> > anyone can point me to the correct doc/code I'd like to learn :) I wonder
> > whether we can make bytestream track ids unique in a process? If so then we
> > don't have this issue.
> 
> Bytestream track ids are read from the bytestream by various stream parsers.
The
> spec for that is https://dev.w3.org/html5/html-sourcing-inband-tracks/. For
> example for mp4 streams, bytestream track ids are read from track_id field of
> the track header (TKHD) box
>
(https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?rcl...).
> For webm streams bytestream track ids are decimal representations of the
> TrackNumber field of the track in the Track section of the WebM file. etc.
> Those bytestream track ids are used internally by stream parsers, for example
to
> determine which stream the demuxed/parsed data packet belongs to, so we can't
> assign our own values to those.
> And as I mentioned in my previous comment, in case of a single bytestream
> (single SourceBuffer) the bytestream track ids are guaranteed to be unique.
The
> problem is that a MediaSource might have multiple SourceBuffers added to it,
> which might make bytestream ids of different bytestreams clash. That's why we
> need a different track id, that would be unique.

I chatted a bit with servolk@ to help clarify MSE vs non-MSE
{audio,video,text}Track.id: in MSE, they need to be uniquely generated by the
UA. In src=, not the case. servolk@ can provide further detail from our chat.

servolk

On 2016/06/06 18:09:49, servolk wrote: > On 2016/06/06 17:48:04, xhwang wrote: > > On 2016/06/06 ...

4 years, 6 months ago (2016-06-06 18:55:17 UTC) #18

On 2016/06/06 18:09:49, servolk wrote:
> On 2016/06/06 17:48:04, xhwang wrote:
> > On 2016/06/06 17:42:56, servolk wrote:
> > >
> >
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> > > File media/blink/webmediaplayer_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...
> > > media/blink/webmediaplayer_impl.cc:884:
> > > demuxer_->OnTrackIdsAssigned(*tracks.get(), blinkTrackIds);
> > > On 2016/06/03 23:35:36, servolk wrote:
> > > > On 2016/06/03 20:57:10, servolk wrote:
> > > > > On 2016/06/03 20:42:49, chcunningham wrote:
> > > > > > I'm also looking for away to let Chromium decide the track Ids, or
at
> > > least
> > > > a
> > > > > > way to get rid of the mapping being carried around in MediaTracks.
> > > > > > 
> > > > > > You mentioned in the chunk demuxer tests that separate bytestream
> tracks
> > > > (from
> > > > > > separate init segments) may actually map to a single blink track -
how
> > > would
> > > > > > this work if we just use bytestream tracks everywhere? 
> > > > > > 
> > > > > > If we can't use bytstream tracks everywhere, another proposal is to
> pass
> > a
> > > > > > callback from blink into chromium to allow for reserving of a track
> id.
> > > > > Chromium
> > > > > > could reserve the id at the time its creating the demuxer stream and
> > > > associate
> > > > > > them immediately. Then set that track id on the MediaTracks object
> when
> > > you
> > > > > > bubble things up to blink. 
> > > > > 
> > > > > Yes, multiple bytestream track ids might map to the same blink track
id,
> > but
> > > > > only in cases when we have a single track of the given type (see the
> > comment
> > > > at
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...).
> > > > > So if we have only a single audio and a single video track, we don't
> need
> > to
> > > > pay
> > > > > attention to track ids at all, we'll know what track ids gets
> > > enabled/disabled
> > > > > simply because we know the track type. And for cases of multiple audio
> > > and/or
> > > > > video tracks bytestream ids will remain unchanged between different
init
> > > > > segments.
> > > > > That's why I think we could get rid of WebMediaPlayer::TrackId. IIUC 
> > track
> > > > type
> > > > > + bytestream track id should be sufficient to uniquely identify a
media
> > > track.
> > > > 
> > > > Well, completely removing WebMediaPlayer::TrackId is difficult, since
it's
> > > used
> > > > in many places. But I really wanted to give it a try, so I tried
changing
> > > > WebMediaPlayer::TrackId type from unsigned to WebString and then removed
> > blink
> > > > internal track ids completely and replaced them with byte stream track
> ids.
> > > Here
> > > > is what I tried:
> > > > https://codereview.chromium.org/2034273003/
> > > > And this seems to be working. Well, at least blink media and mediasource
> > > layout
> > > > tests pass. So I guess this is a viable idea.
> > > 
> > > Update: I though about it some more over the weekend, and I realized that
> > > switching to using bytestream track ids might be problematic in case of
> > multiple
> > > SourceBuffers. In that case bytestream track ids are guaranteed to be
unique
> > > with a bytestream (i.e. a single SourceBuffer), but are not guaranteed to
be
> > > unique across different SourceBuffers. So we'll still need to have the
> concept
> > > of WebMediaPlayer::TrackId that will be guaranteed to be unique with the
> > > WebMediaPlayer, even if it has multiple SourceBuffers. But we could still
> try
> > to
> > > move the WMP::TrackId generation from WMP into demuxers. That would
simplify
> > > mapping between TrackIds and demuxer streams. I'll give it a try today.
> > 
> > Thanks! I am not really familiar with how bytestream track ids are
generated.
> If
> > anyone can point me to the correct doc/code I'd like to learn :) I wonder
> > whether we can make bytestream track ids unique in a process? If so then we
> > don't have this issue.
> 
> Bytestream track ids are read from the bytestream by various stream parsers.
The
> spec for that is https://dev.w3.org/html5/html-sourcing-inband-tracks/. For
> example for mp4 streams, bytestream track ids are read from track_id field of
> the track header (TKHD) box
>
(https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?rcl...).
> For webm streams bytestream track ids are decimal representations of the
> TrackNumber field of the track in the Track section of the WebM file. etc.
> Those bytestream track ids are used internally by stream parsers, for example
to
> determine which stream the demuxed/parsed data packet belongs to, so we can't
> assign our own values to those.
> And as I mentioned in my previous comment, in case of a single bytestream
> (single SourceBuffer) the bytestream track ids are guaranteed to be unique.
The
> problem is that a MediaSource might have multiple SourceBuffers added to it,
> which might make bytestream ids of different bytestreams clash. That's why we
> need a different track id, that would be unique.

FYI: I just had some further discussion with Matt about this, to confirm if my
understanding is correct:

Sergey Volk: hi Matt, could you take a look at the discussion going on at
https://codereview.chromium.org/1922333002/#msg16 ?
I've looked at the specs and I'm starting to get doubts
the specs explicitly say that bytestream track ids must be unique within each
init segment
but they don't seem to address whether we are allowed to have multiple
SourceBuffers with clashing bytestream ids
if we know that bytestream ids won't clash even in case of multiple
SourceBuffers that would make our code much simpler

Matt Wolenetz:
Hmm. I suppose a compliant UA with the additional support for multiple
audio/video tracks could allow:
two sourcebuffers
each sourcebuffer gets appended the exact same stream as the other
the way the spec reads (i think) is that mediaElement.{audio,video}Tracks should
still have distinct entries for each.
note that a track could have multiple kinds (this is the first time i noticed
this piece in the mse spec), and for *each* kinds entry, there must be a unique
ID generated for the 'id' property on new audio track.
that unique id is something that must truly be unique across the sourcebuffers
does this help?

Sergey Volk: somewhat

Matt Wolenetz: IOW, bytestream IDs could clash, but track.id must not.
(the latter is generated by the UA)

Sergey Volk:
I'm trying to understand how that MSE spec requirement works in the light of w3c
spec for track id:
The AudioTrack.id and VideTrack.id attributes must return the identifier of the
track, if it has one, or the empty string otherwise. If the media resource is in
a format that supports the Media Fragments URI fragment identifier syntax, the
identifier returned for a particular track must be the same identifier that
would enable the track if used as the name of a track in the track dimension of
such a fragment identifier. [MEDIAFRAG]
the w3c spec seems to expect track ids to be bytestream ids (in order for that
to work with Media Fragments URI)

Matt Wolenetz:
yes, mse allows further indirection, and requires unique .id generation
mse doesn't use mediafrag, thankfully

Sergey Volk:
ok, so for FFmpeg playback case track ids will be bytestream ids, and for MSE
they must be generated by ChunkDemuxer or SourceBuffer, right?

Matt Wolenetz:
that's indeed how I understand it. also, note that just since we don't allow
multi-track audio (or video) yet, we're still not guaranteed of uniqueness of
bytestream track id across video vs audio in the bytestream (and they could
change across init segments, too). I think this is further reason why there is
indirection: UA generates a stable id for each MSE track kind.
If you want, copy this into the CL thread - I've no problem with that ☺

Sergey Volk:
ok, will do

servolk

> > > > > > Thanks! I am not really familiar with how bytestream ...

4 years, 6 months ago (2016-06-07 02:08:02 UTC) #19

> > > 
> > > Thanks! I am not really familiar with how bytestream track ids are
> generated.
> > If
> > > anyone can point me to the correct doc/code I'd like to learn :) I wonder
> > > whether we can make bytestream track ids unique in a process? If so then
we
> > > don't have this issue.
> > 
> > Bytestream track ids are read from the bytestream by various stream parsers.
> The
> > spec for that is https://dev.w3.org/html5/html-sourcing-inband-tracks/. For
> > example for mp4 streams, bytestream track ids are read from track_id field
of
> > the track header (TKHD) box
> >
>
(https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?rcl...).
> > For webm streams bytestream track ids are decimal representations of the
> > TrackNumber field of the track in the Track section of the WebM file. etc.
> > Those bytestream track ids are used internally by stream parsers, for
example
> to
> > determine which stream the demuxed/parsed data packet belongs to, so we
can't
> > assign our own values to those.
> > And as I mentioned in my previous comment, in case of a single bytestream
> > (single SourceBuffer) the bytestream track ids are guaranteed to be unique.
> The
> > problem is that a MediaSource might have multiple SourceBuffers added to it,
> > which might make bytestream ids of different bytestreams clash. That's why
we
> > need a different track id, that would be unique.
> 
> FYI: I just had some further discussion with Matt about this, to confirm if my
> understanding is correct:
> 
> Sergey Volk: hi Matt, could you take a look at the discussion going on at
> https://codereview.chromium.org/1922333002/#msg16 ?
> I've looked at the specs and I'm starting to get doubts
> the specs explicitly say that bytestream track ids must be unique within each
> init segment
> but they don't seem to address whether we are allowed to have multiple
> SourceBuffers with clashing bytestream ids
> if we know that bytestream ids won't clash even in case of multiple
> SourceBuffers that would make our code much simpler
> 
> Matt Wolenetz:
> Hmm. I suppose a compliant UA with the additional support for multiple
> audio/video tracks could allow:
> two sourcebuffers
> each sourcebuffer gets appended the exact same stream as the other
> the way the spec reads (i think) is that mediaElement.{audio,video}Tracks
should
> still have distinct entries for each.
> note that a track could have multiple kinds (this is the first time i noticed
> this piece in the mse spec), and for *each* kinds entry, there must be a
unique
> ID generated for the 'id' property on new audio track.
> that unique id is something that must truly be unique across the sourcebuffers
> does this help?
> 
> Sergey Volk: somewhat
> 
> Matt Wolenetz: IOW, bytestream IDs could clash, but track.id must not.
> (the latter is generated by the UA)
> 
> Sergey Volk:
> I'm trying to understand how that MSE spec requirement works in the light of
w3c
> spec for track id:
> The AudioTrack.id and VideTrack.id attributes must return the identifier of
the
> track, if it has one, or the empty string otherwise. If the media resource is
in
> a format that supports the Media Fragments URI fragment identifier syntax, the
> identifier returned for a particular track must be the same identifier that
> would enable the track if used as the name of a track in the track dimension
of
> such a fragment identifier. [MEDIAFRAG]
> the w3c spec seems to expect track ids to be bytestream ids (in order for that
> to work with Media Fragments URI)
> 
> Matt Wolenetz:
> yes, mse allows further indirection, and requires unique .id generation
> mse doesn't use mediafrag, thankfully
> 
> Sergey Volk:
> ok, so for FFmpeg playback case track ids will be bytestream ids, and for MSE
> they must be generated by ChunkDemuxer or SourceBuffer, right?
> 
> Matt Wolenetz:
> that's indeed how I understand it. also, note that just since we don't allow
> multi-track audio (or video) yet, we're still not guaranteed of uniqueness of
> bytestream track id across video vs audio in the bytestream (and they could
> change across init segments, too). I think this is further reason why there is
> indirection: UA generates a stable id for each MSE track kind.
> If you want, copy this into the CL thread - I've no problem with that ☺
> 
> Sergey Volk:
> ok, will do

Another update after a bit more research:
First of all, MSE spec seems to imply that there can be multiple kinds for each
media track (see steps 5.2.8 and 5.3.8 of the 'init segment processing'
algorithm at
https://w3c.github.io/media-source/#sourcebuffer-init-segment-received), but
it's not clear to me how that could ever happen, given that
https://dev.w3.org/html5/html-sourcing-inband-tracks/ specifies only a single
track kind for every possible container type.
Then, the MSE spec doesn't actually specify how to generate unique track ids,
all it says is this:
5.2.8.3/5.3.8.3 Generate a unique ID and assign it to the id property on new
video track.
And finally, the MSE spec doesn't seem to explicitly say that it's a valid use
case to add two SourceBuffers with identical bitstreams in them, and I find it
hard to imagine why it would be useful.

Having said that, I wonder if we could do this:
1. Make the case of two SourceBuffers with identical bitstreams in a single
MediaSource a quality-of-implementation issue in the MSE spec and not implement
it in Chrome (since it's useless IMO).
2. Clarify in the MSE spec how the media track kind information should be
sourced.

Depending on the outcome of #2 above: if we decide that there is only a single
kind for each media track, then bytestream track ids become unique and we can
use them directly in blink and chromium media pipeline. If we decide that we
want to support having multiple media track kinds for each media track, then
generate unique track ids described in the MSE spec such that they allow easy
mapping between the unique track id and the underlying bytestream track id. E.g.
if we have a track with a bytestream track id '1' and that track is assigned
multiple kinds 'kind1' and 'kind2', then we could generate unique track ids that
would be '1-kind1' and '1-kind2'. If we generated track ids in that manner we
wouldn't need to maintain explicit mappings between external/blink track ids and
internal/bytestream track ids and thus most of this CL would be no longer
necessary.

Matt, what do you think about this?

chcunningham

I agree that two SourceBuffers with identical bytestreams is silly, but IIUC this is just ...

4 years, 6 months ago (2016-06-07 16:35:21 UTC) #20

servolk

On 2016/06/07 16:35:21, chcunningham wrote: > I agree that two SourceBuffers with identical bytestreams is ...

4 years, 6 months ago (2016-06-07 17:53:02 UTC) #21

wolenetz

Please see inline: On 2016/06/07 02:08:02, servolk wrote: > > > > > > > ...

4 years, 6 months ago (2016-06-07 17:58:31 UTC) #22

Please see inline:

On 2016/06/07 02:08:02, servolk wrote:
> > > > 
> > > > Thanks! I am not really familiar with how bytestream track ids are
> > generated.
> > > If
> > > > anyone can point me to the correct doc/code I'd like to learn :) I
wonder
> > > > whether we can make bytestream track ids unique in a process? If so then
> we
> > > > don't have this issue.
> > > 
> > > Bytestream track ids are read from the bytestream by various stream
parsers.
> > The
> > > spec for that is https://dev.w3.org/html5/html-sourcing-inband-tracks/.
For
> > > example for mp4 streams, bytestream track ids are read from track_id field
> of
> > > the track header (TKHD) box
> > >
> >
>
(https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?rcl...).
> > > For webm streams bytestream track ids are decimal representations of the
> > > TrackNumber field of the track in the Track section of the WebM file. etc.
> > > Those bytestream track ids are used internally by stream parsers, for
> example
> > to
> > > determine which stream the demuxed/parsed data packet belongs to, so we
> can't
> > > assign our own values to those.
> > > And as I mentioned in my previous comment, in case of a single bytestream
> > > (single SourceBuffer) the bytestream track ids are guaranteed to be
unique.
> > The
> > > problem is that a MediaSource might have multiple SourceBuffers added to
it,
> > > which might make bytestream ids of different bytestreams clash. That's why
> we
> > > need a different track id, that would be unique.
> > 
> > FYI: I just had some further discussion with Matt about this, to confirm if
my
> > understanding is correct:
> > 
> > Sergey Volk: hi Matt, could you take a look at the discussion going on at
> > https://codereview.chromium.org/1922333002/#msg16 ?
> > I've looked at the specs and I'm starting to get doubts
> > the specs explicitly say that bytestream track ids must be unique within
each
> > init segment
> > but they don't seem to address whether we are allowed to have multiple
> > SourceBuffers with clashing bytestream ids
> > if we know that bytestream ids won't clash even in case of multiple
> > SourceBuffers that would make our code much simpler
> > 
> > Matt Wolenetz:
> > Hmm. I suppose a compliant UA with the additional support for multiple
> > audio/video tracks could allow:
> > two sourcebuffers
> > each sourcebuffer gets appended the exact same stream as the other
> > the way the spec reads (i think) is that mediaElement.{audio,video}Tracks
> should
> > still have distinct entries for each.
> > note that a track could have multiple kinds (this is the first time i
noticed
> > this piece in the mse spec), and for *each* kinds entry, there must be a
> unique
> > ID generated for the 'id' property on new audio track.
> > that unique id is something that must truly be unique across the
sourcebuffers
> > does this help?
> > 
> > Sergey Volk: somewhat
> > 
> > Matt Wolenetz: IOW, bytestream IDs could clash, but track.id must not.
> > (the latter is generated by the UA)
> > 
> > Sergey Volk:
> > I'm trying to understand how that MSE spec requirement works in the light of
> w3c
> > spec for track id:
> > The AudioTrack.id and VideTrack.id attributes must return the identifier of
> the
> > track, if it has one, or the empty string otherwise. If the media resource
is
> in
> > a format that supports the Media Fragments URI fragment identifier syntax,
the
> > identifier returned for a particular track must be the same identifier that
> > would enable the track if used as the name of a track in the track dimension
> of
> > such a fragment identifier. [MEDIAFRAG]
> > the w3c spec seems to expect track ids to be bytestream ids (in order for
that
> > to work with Media Fragments URI)
> > 
> > Matt Wolenetz:
> > yes, mse allows further indirection, and requires unique .id generation
> > mse doesn't use mediafrag, thankfully
> > 
> > Sergey Volk:
> > ok, so for FFmpeg playback case track ids will be bytestream ids, and for
MSE
> > they must be generated by ChunkDemuxer or SourceBuffer, right?
> > 
> > Matt Wolenetz:
> > that's indeed how I understand it. also, note that just since we don't allow
> > multi-track audio (or video) yet, we're still not guaranteed of uniqueness
of
> > bytestream track id across video vs audio in the bytestream (and they could
> > change across init segments, too). I think this is further reason why there
is
> > indirection: UA generates a stable id for each MSE track kind.
> > If you want, copy this into the CL thread - I've no problem with that ☺
> > 
> > Sergey Volk:
> > ok, will do
> 
> Another update after a bit more research:
> First of all, MSE spec seems to imply that there can be multiple kinds for
each
> media track (see steps 5.2.8 and 5.3.8 of the 'init segment processing'
> algorithm at
> https://w3c.github.io/media-source/#sourcebuffer-init-segment-received), but
> it's not clear to me how that could ever happen, given that
> https://dev.w3.org/html5/html-sourcing-inband-tracks/ specifies only a single
> track kind for every possible container type.

The MSE "defaultTrackKinds algorithm" allows the app to setup a situation where
a matching track that has no in-band-parsed-from-bytestream 'kinds' uses instead
an app-provided 'kinds' which could be more than one. See
https://w3c.github.io/media-source/#sourcebuffer-default-track-kinds

> Then, the MSE spec doesn't actually specify how to generate unique track ids,
> all it says is this:
> 5.2.8.3/5.3.8.3 Generate a unique ID and assign it to the id property on new
> video track.

Is this an issue? It needs to be unique and a DOMString.
{audio/video/text}TrackList.getTrackById(such a string) must work even for MSE.
But MSE spec leaves the format of the string to the user agent implementation.

> And finally, the MSE spec doesn't seem to explicitly say that it's a valid use
> case to add two SourceBuffers with identical bitstreams in them, and I find it
> hard to imagine why it would be useful.

As chcunningham@ points out, my scenario was silly (over-simplified). He's
correct that multiple distinct bytestreams could have inband bytestream track
IDs that collide.

> Having said that, I wonder if we could do this:
> 1. Make the case of two SourceBuffers with identical bitstreams in a single
> MediaSource a quality-of-implementation issue in the MSE spec and not
implement
> it in Chrome (since it's useless IMO).
> 2. Clarify in the MSE spec how the media track kind information should be
> sourced.
> 
> Depending on the outcome of #2 above: if we decide that there is only a single
> kind for each media track, then bytestream track ids become unique and we can
> use them directly in blink and chromium media pipeline. If we decide that we
> want to support having multiple media track kinds for each media track, then
> generate unique track ids described in the MSE spec such that they allow easy
> mapping between the unique track id and the underlying bytestream track id.
E.g.
> if we have a track with a bytestream track id '1' and that track is assigned
> multiple kinds 'kind1' and 'kind2', then we could generate unique track ids
that
> would be '1-kind1' and '1-kind2'. If we generated track ids in that manner we
> wouldn't need to maintain explicit mappings between external/blink track ids
and
> internal/bytestream track ids and thus most of this CL would be no longer
> necessary.
> 
> Matt, what do you think about this?

See inline, above.

wolenetz

On 2016/06/07 17:53:02, servolk wrote: > On 2016/06/07 16:35:21, chcunningham wrote: > > I agree ...

4 years, 6 months ago (2016-06-07 18:04:01 UTC) #23

servolk

On 2016/06/07 18:04:01, wolenetz_slow_reviews wrote: > On 2016/06/07 17:53:02, servolk wrote: > > On 2016/06/07 ...

4 years, 6 months ago (2016-06-09 00:30:46 UTC) #24

wolenetz

On 2016/06/09 00:30:46, servolk wrote: > On 2016/06/07 18:04:01, wolenetz_slow_reviews wrote: > > On 2016/06/07 ...

4 years, 6 months ago (2016-06-16 19:54:54 UTC) #25

Message was sent while issue was closed.

On 2016/06/09 00:30:46, servolk wrote:
> On 2016/06/07 18:04:01, wolenetz_slow_reviews wrote:
> > On 2016/06/07 17:53:02, servolk wrote:
> > > On 2016/06/07 16:35:21, chcunningham wrote:
> > > > I agree that two SourceBuffers with identical bytestreams is silly, but
> IIUC
> > > > this is just a toy example to make obvious that two source buffers would
> > have
> > > > bytestream track ids. Even if we dont allow identical bytestreams in
> Chrome,
> > > is
> > > > it not also possible that you could have two SourceBuffers with
> non-matching
> > > > bytestreams that still have the same bytestream track ids?
> > > 
> > > Yes, I see your point. I guess it's possible for two non-identical
> bitstreams
> > to
> > > have matching bytestream track ids. So, unfortunately, it looks like we'll
> > have
> > > to generate a more unique track ids and perform the translation from those
> > > unique track ids into the low-level bytestream ids somewhere. I'll see if
we
> > can
> > > at least hide that from the higher-level code in the FFmpeg/ChunkDemuxer.
> > 
> > SGTM. Note that the eventual initialization segment received algorithm will
> need
> > to handle multiple 'kind' in kinds from defaultKindsAlgorithm result (or
leave
> > that case explicitly called out as an implementation bug to fix later).
> 
> Ok, so generating media track ids in the demuxers rather than in blink is
quite
> different from this CL, so I've uploaded it as a separate CL for now, PTAL at
> https://codereview.chromium.org/2050043002/. But if prefer I can also upload
it
> into this CL as a new patchset (although the diff between the patchsets is
going
> to be fairly large). That new CL achieves the same goal of providing a
> Demuxer::GetDemuxerStreamByTrackId function that is going to allow us to
> translate media track status changes coming from blink in the form of sets of
> track ids into demuxer streams. So that new CL could also serve as a
foundation
> for the CL that implements enabling/disabling media tracks and streams with
only
> minor changes.

Now that https://codereview.chromium.org/2050043002/ has landed, is this CL
obsolete?

servolk

4 years, 6 months ago (2016-06-16 20:47:26 UTC) #26

Message was sent while issue was closed.

On 2016/06/16 19:54:54, wolenetz wrote:
> On 2016/06/09 00:30:46, servolk wrote:
> > On 2016/06/07 18:04:01, wolenetz_slow_reviews wrote:
> > > On 2016/06/07 17:53:02, servolk wrote:
> > > > On 2016/06/07 16:35:21, chcunningham wrote:
> > > > > I agree that two SourceBuffers with identical bytestreams is silly,
but
> > IIUC
> > > > > this is just a toy example to make obvious that two source buffers
would
> > > have
> > > > > bytestream track ids. Even if we dont allow identical bytestreams in
> > Chrome,
> > > > is
> > > > > it not also possible that you could have two SourceBuffers with
> > non-matching
> > > > > bytestreams that still have the same bytestream track ids?
> > > > 
> > > > Yes, I see your point. I guess it's possible for two non-identical
> > bitstreams
> > > to
> > > > have matching bytestream track ids. So, unfortunately, it looks like
we'll
> > > have
> > > > to generate a more unique track ids and perform the translation from
those
> > > > unique track ids into the low-level bytestream ids somewhere. I'll see
if
> we
> > > can
> > > > at least hide that from the higher-level code in the
FFmpeg/ChunkDemuxer.
> > > 
> > > SGTM. Note that the eventual initialization segment received algorithm
will
> > need
> > > to handle multiple 'kind' in kinds from defaultKindsAlgorithm result (or
> leave
> > > that case explicitly called out as an implementation bug to fix later).
> > 
> > Ok, so generating media track ids in the demuxers rather than in blink is
> quite
> > different from this CL, so I've uploaded it as a separate CL for now, PTAL
at
> > https://codereview.chromium.org/2050043002/. But if prefer I can also upload
> it
> > into this CL as a new patchset (although the diff between the patchsets is
> going
> > to be fairly large). That new CL achieves the same goal of providing a
> > Demuxer::GetDemuxerStreamByTrackId function that is going to allow us to
> > translate media track status changes coming from blink in the form of sets
of
> > track ids into demuxer streams. So that new CL could also serve as a
> foundation
> > for the CL that implements enabling/disabling media tracks and streams with
> only
> > minor changes.
> 
> Now that https://codereview.chromium.org/2050043002/ has landed, is this CL
> obsolete?

Correct, this one is obsolete, so I've closed it.

Issue 1922333002: Implement mapping blink track id to demuxer streams (Closed)

Description

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : rebase #

Patch Set 4 : build fix for 64-bit platforms #

Patch Set 5 : Handle track status changes in WMPI #

Patch Set 6 : rebase #

Patch Set 7 : Fix tests #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : nits #

Patch Set 11 : rebase #

Patch Set 12 : CR feedback #

Messages