|
|
DescriptionImplement mapping blink track id to demuxer streams
BUG=249427
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 #
Total comments: 25
Patch Set 12 : CR feedback #
Total comments: 10
Messages
Total messages: 26 (3 generated)
Description was changed from ========== Implement mapping blink track id to demuxer streams BUG=249427 ========== to ========== Implement mapping blink track id to demuxer streams BUG=249427 ==========
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org, wolenetz@chromium.org, xhwang@chromium.org
chcunningham@chromium.org changed reviewers: + chcunningham@chromium.org
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, 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? 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) == How are you deciding when to DCHECK vs CHECK? Both of the new methods here mix DCHECK and CHECK 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) 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... 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); 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. 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 of this track's type 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: Similar test for ffmpeg_demuxer would be good 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 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... https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:99: const int kVideoTrackId = 10; Also, can you name these in a way that describes their different purpose? E.g. kExternalVideoTrackId vs kBystreamVideoTrackId https://codereview.chromium.org/1922333002/diff/200001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:1416: This is busted for multi-track right? You might assert that you only see one track of a given type 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()); Same question about DCHECK vs CHECK for these two methods
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.
Could you please provide more information in the CL description on what this CL does exactly? I didn't review all the details. I have a general question about whether we can remove OnTrackIdsAssigned() and make the code simpler. https://codereview.chromium.org/1922333002/diff/220001/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/1922333002/diff/220001/media/base/demuxer.h#n... media/base/demuxer.h:138: // Notifies the demuxer that blink track ids have been assigned to |tracks|. nit: "media" doesn't know about blink. So the |track_id| should be a generic concept in "media" as well. https://codereview.chromium.org/1922333002/diff/220001/media/base/demuxer.h#n... media/base/demuxer.h:143: const std::vector<unsigned>& track_ids) = 0; We don't like to use "unsigned" or "unsigned int" in chromium. I see that this is coming from blink, but typically we'll just use uint32_t in chromium: https://google.github.io/styleguide/cppguide.html#Integer_Types Also feel free to typedef a media::TrackId if you like. https://codereview.chromium.org/1922333002/diff/220001/media/base/media_tracks.h File media/base/media_tracks.h (right): https://codereview.chromium.org/1922333002/diff/220001/media/base/media_track... media/base/media_tracks.h:26: using TrackIdToDemuxStreamMap = std::map<unsigned, const DemuxerStream*>; nit: s/DemuxStream/DemuxerStream/ https://codereview.chromium.org/1922333002/diff/220001/media/base/media_track... media/base/media_tracks.h:31: // Callers need to ensure that track id is unique. nit: Is it "unique" within this object, or in the process? What is the return value? Can the return value be null? 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); 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.
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?
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.
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.
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...
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: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.
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.
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.
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.
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.
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.
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
> > > > > > 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?
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?
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.
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.
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).
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.
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?
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. |