Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050043002:
Generate and assign media track ids in demuxers. (Closed)
|
Created:
4 years, 6 months ago by servolk Modified:
4 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nessy, posciak+watch_chromium.org, gasubic, blink-reviews-html_chromium.org, jam, fs, eric.carlson_apple.com, blink-reviews-api_chromium.org, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, vcarbune.chromium, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@use-streamparser-trackid Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGenerate and assign media track ids in demuxers.
Currently media track ids are generated on blink level, but this
complicates media track management. In particular when media tracks are
created on demuxer level we don't know their unique ids yet, so we
can't easily set up track id -> demuxer stream mappings. Assigning
track ids at the demuxer level makes that much easier.
BUG=249427, 341581
Committed: https://crrev.com/fa5c37c65c5f038882af6ca8222566d0928930a0
Cr-Commit-Position: refs/heads/master@{#400198}
Patch Set 1 #
Total comments: 34
Patch Set 2 : cr feedback #Patch Set 3 : Remove Demuxer::GetDemuxerStreamByTrackId #
Total comments: 16
Patch Set 4 : Rebase to fix build errors #Patch Set 5 : Rebase #
Total comments: 16
Patch Set 6 : CR feedback #Patch Set 7 : rebase #
Total comments: 6
Patch Set 8 : More CR feedback (codec validity checks in mp4 parser) #Patch Set 9 : rebase #Patch Set 10 : rebase to ToT #Dependent Patchsets: Messages
Total messages: 62 (17 generated)
Description was changed from ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. ========== to ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. ==========
servolk@chromium.org changed reviewers: + chcunningham@chromium.org, wolenetz@chromium.org, xhwang@chromium.org
On 2016/06/09 00:31:43, servolk wrote: See the discussion in https://codereview.chromium.org/1922333002/ for more context and explanations why we might prefer to generate track ids in demuxers, rather than on blink level.
I like this better. Here are my comments. https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/demuxer.h... media/base/demuxer.h:138: virtual const DemuxerStream* GetDemuxerStreamByTrackId( This is actually not used in this CL. Also, in your other CL, it's still TBD whether we want/need to do this. Shall we drop this change from this CL and then iterate on this later? https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/demuxer.h... media/base/demuxer.h:139: std::string track_id) const = 0; const ref https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... File media/base/media_track.cc (right): https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.cc:19: id_("") {} nit: not necessary https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.cc:29: DCHECK(id != ""); nit: use empty()? https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.cc:31: } nit: These two functions can be inlined in the header https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... File media/base/media_track.h (right): https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:14: class DemuxerStream; Why is this needed? https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:19: using TrackId = std::string; Will it work if s/Trackid/Id? MediaTrack::TrackId seems redundant. Also Id is consistent with id() and setId(). https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:35: void setId(TrackId id); set_id() https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:39: StreamParser::TrackId byteStreamTrackId_; nit: use lower_cast instead of CamelCase https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:42: std::string language_; Can you add comments about what these fields are? What are the expected values? Why these have to be string instead of enums? https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:43: TrackId id_; Can you comment about the difference b/w byteStreamTrackId_ and id_? https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_track.h:43: TrackId id_; What's the requirement on the uniqueness of the track id? Is it sufficient to have unique track IDs in one media element? https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... File media/base/media_tracks.cc (right): https://chromiumcodereview.appspot.com/2050043002/diff/1/media/base/media_tra... media/base/media_tracks.cc:43: return tracks_.back().get(); nit: You can store a raw pointer of track.get() above, and return it here. https://chromiumcodereview.appspot.com/2050043002/diff/1/third_party/WebKit/S... File third_party/WebKit/Source/core/html/track/TrackBase.h (right): https://chromiumcodereview.appspot.com/2050043002/diff/1/third_party/WebKit/S... third_party/WebKit/Source/core/html/track/TrackBase.h:65: String m_id; Shall this be WebMediaPlayer::TrackId m_trackId?
https://codereview.chromium.org/2050043002/diff/1/media/blink/websourcebuffer... File media/blink/websourcebuffer_impl.cc (right): https://codereview.chromium.org/2050043002/diff/1/media/blink/websourcebuffer... media/blink/websourcebuffer_impl.cc:186: trackInfo.byteStreamTrackId = blink::WebString::fromUTF8(track->id()); I think you this is not the bytestreamTrackId - you probably want to rename the member in trackInfo. https://codereview.chromium.org/2050043002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2050043002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1237: media_track->setId(base::UintToString(track_id)); So for src=, we re-use the bytstream track Id to set media track id. Should be fine since the audio and video tracks should all have unique bytstream ids. I'd add a CHECK (to catch bugs in ffmpeg or bugs in our assumptions) that you don't already have the track id in the map before you add the new streams below.
https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... media/base/demuxer.h:138: virtual const DemuxerStream* GetDemuxerStreamByTrackId( On 2016/06/09 06:06:02, xhwang wrote: > This is actually not used in this CL. Also, in your other CL, it's still TBD > whether we want/need to do this. Shall we drop this change from this CL and then > iterate on this later? At the moment I'm inclined to think it's better to keep this API, since it's more generic and useful. I'm not sure if I fully understand your suggestions in https://codereview.chromium.org/1935873002/#msg19. I think this is a useful generic API that essentially allows translating media track ids (which is what blink operates with) into demuxer streams, and the functionality might be useful beyond just enabling/disabling tracks. Let's have some further discussion in https://codereview.chromium.org/1935873002 before removing this. I'll add my thoughts in there shortly. https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... media/base/demuxer.h:139: std::string track_id) const = 0; On 2016/06/09 06:06:02, xhwang wrote: > const ref Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.cc File media/base/media_track.cc (right): https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.cc#n... media/base/media_track.cc:19: id_("") {} On 2016/06/09 06:06:02, xhwang wrote: > nit: not necessary Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.cc#n... media/base/media_track.cc:29: DCHECK(id != ""); On 2016/06/09 06:06:02, xhwang wrote: > nit: use empty()? Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.cc#n... media/base/media_track.cc:31: } On 2016/06/09 06:06:02, xhwang wrote: > nit: These two functions can be inlined in the header Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:14: class DemuxerStream; On 2016/06/09 06:06:02, xhwang wrote: > Why is this needed? Not needed, leftover from earlier prototyping. Removed. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:19: using TrackId = std::string; On 2016/06/09 06:06:02, xhwang wrote: > Will it work if s/Trackid/Id? MediaTrack::TrackId seems redundant. Also Id is > consistent with id() and setId(). Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:35: void setId(TrackId id); On 2016/06/09 06:06:02, xhwang wrote: > set_id() Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:39: StreamParser::TrackId byteStreamTrackId_; On 2016/06/09 06:06:02, xhwang wrote: > nit: use lower_cast instead of CamelCase Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:42: std::string language_; On 2016/06/09 06:06:02, xhwang wrote: > Can you add comments about what these fields are? What are the expected values? > Why these have to be string instead of enums? Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:43: TrackId id_; On 2016/06/09 06:06:02, xhwang wrote: > What's the requirement on the uniqueness of the track id? Is it sufficient to > have unique track IDs in one media element? IIUC these need to be unique in the scope of a single media element. But for some reason these were made unique in the scope of a single process, since blink media track id generation used a global (static) counter (see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/trac...). I'll make the track id generation to be the same in ChunkDemuxer for now, to avoid any potential issues. But we can reconsider this later. https://codereview.chromium.org/2050043002/diff/1/media/base/media_track.h#ne... media/base/media_track.h:43: TrackId id_; On 2016/06/09 06:06:02, xhwang wrote: > Can you comment about the difference b/w byteStreamTrackId_ and id_? Done. https://codereview.chromium.org/2050043002/diff/1/media/base/media_tracks.cc File media/base/media_tracks.cc (right): https://codereview.chromium.org/2050043002/diff/1/media/base/media_tracks.cc#... media/base/media_tracks.cc:43: return tracks_.back().get(); On 2016/06/09 06:06:02, xhwang wrote: > nit: You can store a raw pointer of track.get() above, and return it here. Done. https://codereview.chromium.org/2050043002/diff/1/media/blink/websourcebuffer... File media/blink/websourcebuffer_impl.cc (right): https://codereview.chromium.org/2050043002/diff/1/media/blink/websourcebuffer... media/blink/websourcebuffer_impl.cc:186: trackInfo.byteStreamTrackId = blink::WebString::fromUTF8(track->id()); On 2016/06/09 17:57:19, chcunningham wrote: > I think you this is not the bytestreamTrackId - you probably want to rename the > member in trackInfo. Done. https://codereview.chromium.org/2050043002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2050043002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1237: media_track->setId(base::UintToString(track_id)); On 2016/06/09 17:57:19, chcunningham wrote: > So for src=, we re-use the bytstream track Id to set media track id. Should be > fine since the audio and video tracks should all have unique bytstream ids. I'd > add a CHECK (to catch bugs in ffmpeg or bugs in our assumptions) that you don't > already have the track id in the map before you add the new streams below. Done. Only I've added a DCHECK, don't think it deserves a crash. We might be able to continue normally by just overwriting the old id.
Note that MSE V1 spec is (today) about to republish entry into CR and the expectation is that the timeline disallows substantive changes. My comment, below, is related also to the only currently known substantive V1 change that spec editors are working on today to close down (https://github.com/w3c/media-source/issues/91). https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:588: // https://w3c.github.io/media-source/#sourcebuffer-init-segment-received There can be an N:1 {HTMLMediaElement,SourceBuffer}.{audio,video,text}Track entry:bytstream_parsed_track in MSE case because the "default track kinds algorithm" can allow an app to supply multiple kinds for a matching bytestream track. The MSE spec requires multiple HTMLME/SourceBuffer tracklist entries (1 per kind), though only 1 MSE track buffer. Consider track buffer the demuxerstream, and the tracklist entry the one that needs a unique app-visible |id| property. The latter can alias N:1 the former. How does this CL envision supporting this scenario? I think we'd previously mis-assumed a 1:1 BlinkTrack:BytestreamTrack for MSE.
On 2016/06/09 19:20:58, wolenetz_slow_reviews wrote: > Note that MSE V1 spec is (today) about to republish entry into CR and the > expectation is that the timeline disallows substantive changes. My comment, > below, is related also to the only currently known substantive V1 change that > spec editors are working on today to close down > (https://github.com/w3c/media-source/issues/91). > > https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:588: // > https://w3c.github.io/media-source/#sourcebuffer-init-segment-received > There can be an N:1 {HTMLMediaElement,SourceBuffer}.{audio,video,text}Track > entry:bytstream_parsed_track in MSE case because the "default track kinds > algorithm" can allow an app to supply multiple kinds for a matching bytestream > track. The MSE spec requires multiple HTMLME/SourceBuffer tracklist entries (1 > per kind), though only 1 MSE track buffer. Consider track buffer the > demuxerstream, and the tracklist entry the one that needs a unique app-visible > |id| property. The latter can alias N:1 the former. > > How does this CL envision supporting this scenario? I think we'd previously > mis-assumed a 1:1 BlinkTrack:BytestreamTrack for MSE. To help us proceed, I'm asking on the spec issue if all that TrackDefault stuff can be marked at-risk. As such, it may not make it into MSE V1. So far, we haven't decided yet whether or not to mark TrackDefault stuff at-risk.
On 2016/06/09 19:30:02, wolenetz_slow_reviews wrote: > On 2016/06/09 19:20:58, wolenetz_slow_reviews wrote: > > Note that MSE V1 spec is (today) about to republish entry into CR and the > > expectation is that the timeline disallows substantive changes. My comment, > > below, is related also to the only currently known substantive V1 change that > > spec editors are working on today to close down > > (https://github.com/w3c/media-source/issues/91). > > > > > https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > > > > https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:588: // > > https://w3c.github.io/media-source/#sourcebuffer-init-segment-received > > There can be an N:1 {HTMLMediaElement,SourceBuffer}.{audio,video,text}Track > > entry:bytstream_parsed_track in MSE case because the "default track kinds > > algorithm" can allow an app to supply multiple kinds for a matching bytestream > > track. The MSE spec requires multiple HTMLME/SourceBuffer tracklist entries (1 > > per kind), though only 1 MSE track buffer. Consider track buffer the > > demuxerstream, and the tracklist entry the one that needs a unique app-visible > > |id| property. The latter can alias N:1 the former. > > > > How does this CL envision supporting this scenario? I think we'd previously > > mis-assumed a 1:1 BlinkTrack:BytestreamTrack for MSE. > > To help us proceed, I'm asking on the spec issue if all that TrackDefault stuff > can be marked at-risk. As such, it may not make it into MSE V1. So far, we > haven't decided yet whether or not to mark TrackDefault stuff at-risk. Prognosis is probably that TrackDefaults will *not* be marked at-risk, so this N:1 stuff will be needed in Chrome (canary at least) within 6 weeks.
https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2050043002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:588: // https://w3c.github.io/media-source/#sourcebuffer-init-segment-received On 2016/06/09 19:20:58, wolenetz_slow_reviews wrote: > There can be an N:1 {HTMLMediaElement,SourceBuffer}.{audio,video,text}Track > entry:bytstream_parsed_track in MSE case because the "default track kinds > algorithm" can allow an app to supply multiple kinds for a matching bytestream > track. The MSE spec requires multiple HTMLME/SourceBuffer tracklist entries (1 > per kind), though only 1 MSE track buffer. Consider track buffer the > demuxerstream, and the tracklist entry the one that needs a unique app-visible > |id| property. The latter can alias N:1 the former. > > How does this CL envision supporting this scenario? I think we'd previously > mis-assumed a 1:1 BlinkTrack:BytestreamTrack for MSE. Yes, I remember that there might be multiple kinds for a single media track. I tried to explain how we could deal with that in https://codereview.chromium.org/1922333002#msg19. To repeat/clarify here: I think that when we do have multiple kinds, then we could generate more unique track ids in blink by appending the '-<kind>' string to the track id generated by the demuxer. For example, if we have a single audio track and the demuxer assigned it a unique id that is '1', then we have 'main' and 'alternative' kinds for that track in the TrackDefaults, then here in the init segment handling algorithm we would add two audio tracks, one with id '1-main' and another one with id '1-alternative'. Although to be honest I don't fully understand how it would be useful. Why would it be useful to have multiple tracks of different kinds if they are backed by a single bytestream on the demuxer level? How would we handle the enabling/disabling in those cases? E.g. in the scenario I gave above was would be the expected effect of disabling the '1-alternative' audio track? The '1-main' is still enabled, so I guess the audio should continue playing until all tracks corresponding to that bytestream/demuxer track id are disabled.
https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... media/base/demuxer.h:138: virtual const DemuxerStream* GetDemuxerStreamByTrackId( On 2016/06/09 19:14:46, servolk wrote: > On 2016/06/09 06:06:02, xhwang wrote: > > This is actually not used in this CL. Also, in your other CL, it's still TBD > > whether we want/need to do this. Shall we drop this change from this CL and > then > > iterate on this later? > > At the moment I'm inclined to think it's better to keep this API, since it's > more generic and useful. I'm not sure if I fully understand your suggestions in > https://codereview.chromium.org/1935873002/#msg19. I think this is a useful > generic API that essentially allows translating media track ids (which is what > blink operates with) into demuxer streams, and the functionality might be useful > beyond just enabling/disabling tracks. Let's have some further discussion in > https://codereview.chromium.org/1935873002 before removing this. I'll add my > thoughts in there shortly. In general we don't add an API because it's generic and could be useful. We add an API because we use it. I think it'd be great if we can hide the track ID as an implementation detail of the demuxer stack and the rest of the media pipeline don't need to know about it. This API definitely opens a door for that.
On 2016/06/09 21:04:16, xhwang wrote: > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h > File media/base/demuxer.h (right): > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... > media/base/demuxer.h:138: virtual const DemuxerStream* > GetDemuxerStreamByTrackId( > On 2016/06/09 19:14:46, servolk wrote: > > On 2016/06/09 06:06:02, xhwang wrote: > > > This is actually not used in this CL. Also, in your other CL, it's still TBD > > > whether we want/need to do this. Shall we drop this change from this CL and > > then > > > iterate on this later? > > > > At the moment I'm inclined to think it's better to keep this API, since it's > > more generic and useful. I'm not sure if I fully understand your suggestions > in > > https://codereview.chromium.org/1935873002/#msg19. I think this is a useful > > generic API that essentially allows translating media track ids (which is what > > blink operates with) into demuxer streams, and the functionality might be > useful > > beyond just enabling/disabling tracks. Let's have some further discussion in > > https://codereview.chromium.org/1935873002 before removing this. I'll add my > > thoughts in there shortly. > > In general we don't add an API because it's generic and could be useful. We add > an API because we use it. > > I think it'd be great if we can hide the track ID as an implementation detail of > the demuxer stack and the rest of the media pipeline don't need to know about > it. This API definitely opens a door for that. It would be impossible to hide track ids, because track ids are more than just an implementation detail. Track ids are already part of the public APIs all the way from Javascript/blink level, so what's the point of trying to hide them in demuxers, if users already see them in blink::WebMediaPlayer and WebMediaPlayerClient interfaces? Hiding track ids in the demuxer interface then would just make it hard to translate track ids into demuxer streams, but it would not completely hide track ids from users. Also, to give you one more specific case where I think this API would be very useful: unit tests. For example we have a ChunkDemuxer unit test that checks that when underlying bytestream track ids change in subsequent init segments the unique track ids assigned by the blink/demuxer stay the same and new data gets appended to the correct track buffers (see AudioVideoTrackIdsChange unit test at https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...). I was going to add a few additional checks to that unit test similar to what I've done in my previous CL (see https://codereview.chromium.org/1922333002/patch/220001/230010), to verify that changing bytestream ids still produce correct media track -> demuxer stream mappings. Do you find that case valuable enough to justify that function? I can remove it, if you insist, but I think there's more value in it that might be obvious from the first glance.
On 2016/06/09 22:16:57, servolk wrote: > On 2016/06/09 21:04:16, xhwang wrote: > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h > > File media/base/demuxer.h (right): > > > > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... > > media/base/demuxer.h:138: virtual const DemuxerStream* > > GetDemuxerStreamByTrackId( > > On 2016/06/09 19:14:46, servolk wrote: > > > On 2016/06/09 06:06:02, xhwang wrote: > > > > This is actually not used in this CL. Also, in your other CL, it's still > TBD > > > > whether we want/need to do this. Shall we drop this change from this CL > and > > > then > > > > iterate on this later? > > > > > > At the moment I'm inclined to think it's better to keep this API, since it's > > > more generic and useful. I'm not sure if I fully understand your suggestions > > in > > > https://codereview.chromium.org/1935873002/#msg19. I think this is a useful > > > generic API that essentially allows translating media track ids (which is > what > > > blink operates with) into demuxer streams, and the functionality might be > > useful > > > beyond just enabling/disabling tracks. Let's have some further discussion in > > > https://codereview.chromium.org/1935873002 before removing this. I'll add my > > > thoughts in there shortly. > > > > In general we don't add an API because it's generic and could be useful. We > add > > an API because we use it. > > > > I think it'd be great if we can hide the track ID as an implementation detail > of > > the demuxer stack and the rest of the media pipeline don't need to know about > > it. This API definitely opens a door for that. > > It would be impossible to hide track ids, because track ids are more than just > an implementation detail. Track ids are already part of the public APIs all the > way from Javascript/blink level, so what's the point of trying to hide them in > demuxers, if users already see them in blink::WebMediaPlayer and > WebMediaPlayerClient interfaces? Hiding track ids in the demuxer interface then > would just make it hard to translate track ids into demuxer streams, but it > would not completely hide track ids from users. > Also, to give you one more specific case where I think this API would be very > useful: unit tests. For example we have a ChunkDemuxer unit test that checks > that when underlying bytestream track ids change in subsequent init segments the > unique track ids assigned by the blink/demuxer stay the same and new data gets > appended to the correct track buffers (see AudioVideoTrackIdsChange unit test at > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...). > I was going to add a few additional checks to that unit test similar to what > I've done in my previous CL (see > https://codereview.chromium.org/1922333002/patch/220001/230010), to verify that > changing bytestream ids still produce correct media track -> demuxer stream > mappings. Do you find that case valuable enough to justify that function? I can > remove it, if you insist, but I think there's more value in it that might be > obvious from the first glance. I am not trying to hide this from Blink, or the user. I am trying to reduce the surface that Demuxer exposes to the "rest of the media pipeline", e.g. Renderers. To be honest, I am open to any solution. But since we are still discussing this in another CL, I wonder whether we can drop the API from this CL, land this and move on. Then we can focus on the design in the next CL. BTW, if it's only used for unittests, we should have something like *ForTesting(). For testing APIs we are much more permissive :)
On 2016/06/09 22:55:57, xhwang wrote: > On 2016/06/09 22:16:57, servolk wrote: > > On 2016/06/09 21:04:16, xhwang wrote: > > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h > > > File media/base/demuxer.h (right): > > > > > > > > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... > > > media/base/demuxer.h:138: virtual const DemuxerStream* > > > GetDemuxerStreamByTrackId( > > > On 2016/06/09 19:14:46, servolk wrote: > > > > On 2016/06/09 06:06:02, xhwang wrote: > > > > > This is actually not used in this CL. Also, in your other CL, it's still > > TBD > > > > > whether we want/need to do this. Shall we drop this change from this CL > > and > > > > then > > > > > iterate on this later? > > > > > > > > At the moment I'm inclined to think it's better to keep this API, since > it's > > > > more generic and useful. I'm not sure if I fully understand your > suggestions > > > in > > > > https://codereview.chromium.org/1935873002/#msg19. I think this is a > useful > > > > generic API that essentially allows translating media track ids (which is > > what > > > > blink operates with) into demuxer streams, and the functionality might be > > > useful > > > > beyond just enabling/disabling tracks. Let's have some further discussion > in > > > > https://codereview.chromium.org/1935873002 before removing this. I'll add > my > > > > thoughts in there shortly. > > > > > > In general we don't add an API because it's generic and could be useful. We > > add > > > an API because we use it. > > > > > > I think it'd be great if we can hide the track ID as an implementation > detail > > of > > > the demuxer stack and the rest of the media pipeline don't need to know > about > > > it. This API definitely opens a door for that. > > > > It would be impossible to hide track ids, because track ids are more than just > > an implementation detail. Track ids are already part of the public APIs all > the > > way from Javascript/blink level, so what's the point of trying to hide them in > > demuxers, if users already see them in blink::WebMediaPlayer and > > WebMediaPlayerClient interfaces? Hiding track ids in the demuxer interface > then > > would just make it hard to translate track ids into demuxer streams, but it > > would not completely hide track ids from users. > > Also, to give you one more specific case where I think this API would be very > > useful: unit tests. For example we have a ChunkDemuxer unit test that checks > > that when underlying bytestream track ids change in subsequent init segments > the > > unique track ids assigned by the blink/demuxer stay the same and new data gets > > appended to the correct track buffers (see AudioVideoTrackIdsChange unit test > at > > > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...). > > I was going to add a few additional checks to that unit test similar to what > > I've done in my previous CL (see > > https://codereview.chromium.org/1922333002/patch/220001/230010), to verify > that > > changing bytestream ids still produce correct media track -> demuxer stream > > mappings. Do you find that case valuable enough to justify that function? I > can > > remove it, if you insist, but I think there's more value in it that might be > > obvious from the first glance. > > I am not trying to hide this from Blink, or the user. I am trying to reduce the > surface that Demuxer exposes to the "rest of the media pipeline", e.g. > Renderers. To be honest, I am open to any solution. But since we are still > discussing this in another CL, I wonder whether we can drop the API from this > CL, land this and move on. Then we can focus on the design in the next CL. > > BTW, if it's only used for unittests, we should have something like > *ForTesting(). For testing APIs we are much more permissive :) Ok, yes, I think we can do this. I'll try to extract the subset of changes in the CL that added the GetDemuxerStreamByTrackId and will move that to the CL that enables/disables tracks, let's discuss further there.
On 2016/06/09 23:59:45, servolk wrote: > On 2016/06/09 22:55:57, xhwang wrote: > > On 2016/06/09 22:16:57, servolk wrote: > > > On 2016/06/09 21:04:16, xhwang wrote: > > > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h > > > > File media/base/demuxer.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2050043002/diff/1/media/base/demuxer.h#newcod... > > > > media/base/demuxer.h:138: virtual const DemuxerStream* > > > > GetDemuxerStreamByTrackId( > > > > On 2016/06/09 19:14:46, servolk wrote: > > > > > On 2016/06/09 06:06:02, xhwang wrote: > > > > > > This is actually not used in this CL. Also, in your other CL, it's > still > > > TBD > > > > > > whether we want/need to do this. Shall we drop this change from this > CL > > > and > > > > > then > > > > > > iterate on this later? > > > > > > > > > > At the moment I'm inclined to think it's better to keep this API, since > > it's > > > > > more generic and useful. I'm not sure if I fully understand your > > suggestions > > > > in > > > > > https://codereview.chromium.org/1935873002/#msg19. I think this is a > > useful > > > > > generic API that essentially allows translating media track ids (which > is > > > what > > > > > blink operates with) into demuxer streams, and the functionality might > be > > > > useful > > > > > beyond just enabling/disabling tracks. Let's have some further > discussion > > in > > > > > https://codereview.chromium.org/1935873002 before removing this. I'll > add > > my > > > > > thoughts in there shortly. > > > > > > > > In general we don't add an API because it's generic and could be useful. > We > > > add > > > > an API because we use it. > > > > > > > > I think it'd be great if we can hide the track ID as an implementation > > detail > > > of > > > > the demuxer stack and the rest of the media pipeline don't need to know > > about > > > > it. This API definitely opens a door for that. > > > > > > It would be impossible to hide track ids, because track ids are more than > just > > > an implementation detail. Track ids are already part of the public APIs all > > the > > > way from Javascript/blink level, so what's the point of trying to hide them > in > > > demuxers, if users already see them in blink::WebMediaPlayer and > > > WebMediaPlayerClient interfaces? Hiding track ids in the demuxer interface > > then > > > would just make it hard to translate track ids into demuxer streams, but it > > > would not completely hide track ids from users. > > > Also, to give you one more specific case where I think this API would be > very > > > useful: unit tests. For example we have a ChunkDemuxer unit test that checks > > > that when underlying bytestream track ids change in subsequent init segments > > the > > > unique track ids assigned by the blink/demuxer stay the same and new data > gets > > > appended to the correct track buffers (see AudioVideoTrackIdsChange unit > test > > at > > > > > > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...). > > > I was going to add a few additional checks to that unit test similar to what > > > I've done in my previous CL (see > > > https://codereview.chromium.org/1922333002/patch/220001/230010), to verify > > that > > > changing bytestream ids still produce correct media track -> demuxer stream > > > mappings. Do you find that case valuable enough to justify that function? I > > can > > > remove it, if you insist, but I think there's more value in it that might be > > > obvious from the first glance. > > > > I am not trying to hide this from Blink, or the user. I am trying to reduce > the > > surface that Demuxer exposes to the "rest of the media pipeline", e.g. > > Renderers. To be honest, I am open to any solution. But since we are still > > discussing this in another CL, I wonder whether we can drop the API from this > > CL, land this and move on. Then we can focus on the design in the next CL. > > > > BTW, if it's only used for unittests, we should have something like > > *ForTesting(). For testing APIs we are much more permissive :) > > Ok, yes, I think we can do this. I'll try to extract the subset of changes in > the CL that added the GetDemuxerStreamByTrackId and will move that to the CL > that enables/disables tracks, let's discuss further there. Ok, this is done. Removed GetDemuxerStreamByTrackId here and added in the patchset #25 in https://codereview.chromium.org/1935873002/.
Looking great. I just have some more ideas for further simplification. Please take a look and let me know what you think. https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.... media/base/media_track.h:40: } I wonder whether we can generate this ID when we create MediaTrack, instead of getting it somewhere else (e.g. from ChunkDemuxer) and set it later. Does it matter where and how we generate this |id|. Is uniqueness (within one MediaSource or media element) the only requirement? If so, can we have a static global incrementing counter here so that every MediaTrack in this process will get a unique ID? For example, you can move GenerateMediaTrackId() to this file. But that means for FFmpegDemuxer, the ID will be different from the bytestreadm_track_id. If we still want to maintain this, we can add a |id| parameter in the constructor, so that FFmpegDemuxer can just use bytestreadm_track_id, and ChunkDemuxer parsers can use a global counter defined somewhere. Actually do we care whether bytestreadm_track_id is the same as |id| for FFmpegDemuxer? won't it be nicer that the TrackId is in a consistent format, e.g. starting from 1, no matter what demuxer we are using? https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.... media/base/media_track.h:51: // So we generate truly unique media track |id_| on the Demuxer level. I <3 this comment. Thanks! https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.... media/base/media_track.h:52: StreamParser::TrackId bytestream_track_id_; nit: I wonder whether StreamParser::TrackId should be StreamParser::BytestreamTrackId, or just StreamParser::BytestreamId, since it's not used for Blink TrackId directly. But I might be missing the big picture. https://codereview.chromium.org/2050043002/diff/40001/media/base/media_tracks.h File media/base/media_tracks.h (right): https://codereview.chromium.org/2050043002/diff/40001/media/base/media_tracks... media/base/media_tracks.h:53: const AudioDecoderConfig& getFirstAudioConfig() const; nit: s/get/Get https://codereview.chromium.org/2050043002/diff/40001/media/base/media_tracks... media/base/media_tracks.h:54: const VideoDecoderConfig& getFirstVideoConfig() const; Actually do we still need these two functions? Seems like they are not used now. https://codereview.chromium.org/2050043002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2050043002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:1011: static unsigned g_trackCount = 0; g_track_count https://codereview.chromium.org/2050043002/diff/40001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:476: bool MediaSourceState::OnNewConfigs( nit: This function alone is >200 lines :) It'd be nice to split this into smaller functions. https://codereview.chromium.org/2050043002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:488: for (const auto& track : tracks->tracks()) { So if we have multiple audio (or video) tracks, we'll only use the first audio (or video) one. Maybe add a comment about this behavior. https://codereview.chromium.org/2050043002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:489: const auto& track_id = track->bytestream_track_id(); nit: It's really hard to follow that whether a |track_id| is a bytestream_track_id, or a MediaTrack:Id... See my comment above about the possibility to improve naming.
Description was changed from ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. ========== to ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. BUG=249427,341581 ==========
https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.... media/base/media_track.h:40: } On 2016/06/10 17:06:53, xhwang wrote: > I wonder whether we can generate this ID when we create MediaTrack, instead of > getting it somewhere else (e.g. from ChunkDemuxer) and set it later. > > Does it matter where and how we generate this |id|. Is uniqueness (within one > MediaSource or media element) the only requirement? If so, can we have a static > global incrementing counter here so that every MediaTrack in this process will > get a unique ID? For example, you can move GenerateMediaTrackId() to this file. > > But that means for FFmpegDemuxer, the ID will be different from the > bytestreadm_track_id. If we still want to maintain this, we can add a |id| > parameter in the constructor, so that FFmpegDemuxer can just use > bytestreadm_track_id, and ChunkDemuxer parsers can use a global counter defined > somewhere. > > Actually do we care whether bytestreadm_track_id is the same as |id| for > FFmpegDemuxer? won't it be nicer that the TrackId is in a consistent format, > e.g. starting from 1, no matter what demuxer we are using? I don't think we can simplify this much further. In MSE case media track are created in stream parsers. But stream parsers don't keep track of whether a demuxer stream already exists for a given media track or if it's going to be created later. So media track ids must be generated (or reused) where we establish the associations between media tracks and demuxer streams, i.e. in MediaSourceState::OnNewConfigs. That's the only place where we make a decision if we need to create a new demuxer stream or reuse an existing one. I have chosen to use bytestream track ids as unique track ids in the FFmpegDemuxer case since it was just the easiest thing to do (in FFmpeg we know that bytestream track ids are guaranteed to be unique, so we don't need to introduce some other track id generation logic). I guess we could use the same logic for generating track ids in FFmpeg as we do in ChunkDemuxer, but I'm not sure if that's the right thing to do. W3C spec says this about HTMLMediaElement track ids: > 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] According to Matt MSE doesn't support MEDIAFRAG uri by design, but I wasn't sure about FFmpegDemuxer and so I've decided to leave things unchanged by using bytestream track ids for FFmpeg case to avoid accidentally breaking something that I don't fully understand :) https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.... media/base/media_track.h:51: // So we generate truly unique media track |id_| on the Demuxer level. On 2016/06/10 17:06:54, xhwang wrote: > I <3 this comment. Thanks! Acknowledged. https://codereview.chromium.org/2050043002/diff/40001/media/base/media_track.... media/base/media_track.h:52: StreamParser::TrackId bytestream_track_id_; On 2016/06/10 17:06:53, xhwang wrote: > nit: I wonder whether StreamParser::TrackId should be > StreamParser::BytestreamTrackId, or just StreamParser::BytestreamId, since it's > not used for Blink TrackId directly. But I might be missing the big picture. Yes, I think we could rename StreamParser::TrackId into StreamParser::BytestreamTrackId or something like this. Matt/Chris, WDYT? If everyone agrees then I'll do that in a separate follow up CL. https://codereview.chromium.org/2050043002/diff/40001/media/base/media_tracks.h File media/base/media_tracks.h (right): https://codereview.chromium.org/2050043002/diff/40001/media/base/media_tracks... media/base/media_tracks.h:53: const AudioDecoderConfig& getFirstAudioConfig() const; On 2016/06/10 17:06:54, xhwang wrote: > nit: s/get/Get Acknowledged. https://codereview.chromium.org/2050043002/diff/40001/media/base/media_tracks... media/base/media_tracks.h:54: const VideoDecoderConfig& getFirstVideoConfig() const; On 2016/06/10 17:06:54, xhwang wrote: > Actually do we still need these two functions? Seems like they are not used now. These are still used in a couple of unit tests. I have a separate CL to remove these: https://codereview.chromium.org/1833053005/ https://codereview.chromium.org/2050043002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2050043002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:1011: static unsigned g_trackCount = 0; On 2016/06/10 17:06:54, xhwang wrote: > g_track_count Done. https://codereview.chromium.org/2050043002/diff/40001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:476: bool MediaSourceState::OnNewConfigs( On 2016/06/10 17:06:54, xhwang wrote: > nit: This function alone is >200 lines :) It'd be nice to split this into > smaller functions. Agree, it looks like it should be relatively trivial to move at least |text_configs| processing into a separate function (~73 lines). But I'd prefer to do that cleanup in a separate follow up CL, to keep this CL focused on track ids.
I chat with servolk offline. The MediaTrack should be fixed for a DemuxerStream after the first init segment. So setting the ID in DemxuerStream level (e.g. MediaSourceState) looks right. One problem is that we store the MediaTrack in blink and do verification there, but traditionally we verify things in chromium (e.g. check whether codec is the same as the codec in the first init segment). So every time when we have a new init segment, we'll have to pop up a new MediaTrack to be verified in Blink. And to make it consistent, we have to set the ID in MediaSourceState every time. If we store the MediaTrack in ChunkDemuxerStream and do verification there, we won't have this problem. But as servolk mentioned in the discussion, the MSE implementation is already split between blink and chromium, and the plan is to move more impl into blink. With that, this CL lgtm. I'll defer the final review to MSE owners :)
On 2016/06/10 23:37:30, xhwang wrote: > I chat with servolk offline. The MediaTrack should be fixed for a DemuxerStream > after the first init segment. So setting the ID in DemxuerStream level (e.g. > MediaSourceState) looks right. > > One problem is that we store the MediaTrack in blink and do verification there, > but traditionally we verify things in chromium (e.g. check whether codec is the > same as the codec in the first init segment). So every time when we have a new > init segment, we'll have to pop up a new MediaTrack to be verified in Blink. And > to make it consistent, we have to set the ID in MediaSourceState every time. > > If we store the MediaTrack in ChunkDemuxerStream and do verification there, we > won't have this problem. But as servolk mentioned in the discussion, the MSE > implementation is already split between blink and chromium, and the plan is to > move more impl into blink. > > With that, this CL lgtm. I'll defer the final review to MSE owners :) Thanks for the review, Xiaohan. Matt/Chris, do you have any further comments? Matt, are you ok with the approach I described in https://codereview.chromium.org/2050043002/#msg11 for handling multiple track kinds?
On 2016/06/10 23:37:30, xhwang wrote: > I chat with servolk offline. The MediaTrack should be fixed for a DemuxerStream > after the first init segment. So setting the ID in DemxuerStream level (e.g. > MediaSourceState) looks right. > > One problem is that we store the MediaTrack in blink and do verification there, > but traditionally we verify things in chromium (e.g. check whether codec is the > same as the codec in the first init segment). So every time when we have a new > init segment, we'll have to pop up a new MediaTrack to be verified in Blink. And > to make it consistent, we have to set the ID in MediaSourceState every time. > > If we store the MediaTrack in ChunkDemuxerStream and do verification there, we > won't have this problem. But as servolk mentioned in the discussion, the MSE > implementation is already split between blink and chromium, and the plan is to > move more impl into blink. > > With that, this CL lgtm. I'll defer the final review to MSE owners :) I see your concern, xhwang@ - since chromium now owns ID generation, it could potentially own more of the initsegmentreceived algorithm. I had suggested moving the initsegmentreceived algo into Blink originally because: 1) Centralized bulk of the spec compliance logic (for initsegmentreceived) into one place would help maintainability, 2) Blink manipulation of HTMLME and SourceBuffer tracks lists and associated events are part of the initsegmentreceived algo. If there is indeed serious redundancy that's hurting maintainability and that we can fix, let's fix now. If more not serious, we could still consider cleanup later. Given your l*tm, I suspect this case is the latter. Note that, for text tracks, we already have a larger set of classes, including chromium-side text_track_impl, which helps maintain consistency between chromium and blink, when both a web-app can "add" text tracks, as well as the media could find them in-band. We might need to consider doing something similar for audio/video in cleanup.
On 2016/06/13 19:00:37, wolenetz_slow_reviews wrote: > On 2016/06/10 23:37:30, xhwang wrote: > > I chat with servolk offline. The MediaTrack should be fixed for a > DemuxerStream > > after the first init segment. So setting the ID in DemxuerStream level (e.g. > > MediaSourceState) looks right. > > > > One problem is that we store the MediaTrack in blink and do verification > there, > > but traditionally we verify things in chromium (e.g. check whether codec is > the > > same as the codec in the first init segment). So every time when we have a new > > init segment, we'll have to pop up a new MediaTrack to be verified in Blink. > And > > to make it consistent, we have to set the ID in MediaSourceState every time. > > > > If we store the MediaTrack in ChunkDemuxerStream and do verification there, we > > won't have this problem. But as servolk mentioned in the discussion, the MSE > > implementation is already split between blink and chromium, and the plan is to > > move more impl into blink. > > > > With that, this CL lgtm. I'll defer the final review to MSE owners :) > > I see your concern, xhwang@ - since chromium now owns ID generation, it could > potentially own more of the initsegmentreceived algorithm. I had suggested > moving the initsegmentreceived algo into Blink originally because: > 1) Centralized bulk of the spec compliance logic (for initsegmentreceived) into > one place would help maintainability, > 2) Blink manipulation of HTMLME and SourceBuffer tracks lists and associated > events are part of the initsegmentreceived algo. > > If there is indeed serious redundancy that's hurting maintainability and that we > can fix, let's fix now. If more not serious, we could still consider cleanup > later. Given your l*tm, I suspect this case is the latter. > > Note that, for text tracks, we already have a larger set of classes, including > chromium-side text_track_impl, which helps maintain consistency between chromium > and blink, when both a web-app can "add" text tracks, as well as the media could > find them in-band. We might need to consider doing something similar for > audio/video in cleanup. Later clean-up is totally fine. Thanks for the info!
On 2016/06/13 18:50:48, servolk wrote: > Matt, are you ok with the approach I described in > https://codereview.chromium.org/2050043002/#msg11 for handling multiple track > kinds? That approach sgtm, though I just took a look at the spec and it doesn't disallow TrackDefault's "kinds" constructor argument from having duplicates ! Given that, when generating a track from each default "kind", maybe indicate the "index" or "positiion" in the TrackDefault getKind()'s sequence: Say: 1-default1-main 1-default2-main FYI - TrackDefault-feature is now officially requested to become 'at-risk' - there's a call-for-consensus that will make this feature at-risk if there's no objection by approx. June 16th. Edge couldn't commit to getting it done in the required MSE v1 time-frame, though they agree it's a good-to-have feature IIUC. Chrome TrackDefault "kinds" implementation will also need to adjust the IDL around []->sequence webIDL type change and the former "kinds" attribute becoming a "getKinds()" accessor (I've filed bug 619665 to track getting that done.)
On 2016/06/09 20:42:04, servolk wrote: > ... Although to be honest I don't > fully understand how it would be useful. Why would it be useful to have multiple > tracks of different kinds if they are backed by a single bytestream on the > demuxer level? How would we handle the enabling/disabling in those cases? E.g. > in the scenario I gave above was would be the expected effect of disabling the > '1-alternative' audio track? The '1-main' is still enabled, so I guess the audio > should continue playing until all tracks corresponding to that > bytestream/demuxer track id are disabled. This allows an app to alias multiple "kinds" to the same stream. Since it's at-risk, and I haven't heard much, I don't think it's a high-pri scenario, but we'll possibly need it for things like accessibility. It seems we have a fairly simple way to support it as you've described here.
On 2016/06/13 19:19:00, wolenetz_slow_reviews wrote: > On 2016/06/09 20:42:04, servolk wrote: > > > ... Although to be honest I don't > > fully understand how it would be useful. Why would it be useful to have > multiple > > tracks of different kinds if they are backed by a single bytestream on the > > demuxer level? How would we handle the enabling/disabling in those cases? E.g. > > in the scenario I gave above was would be the expected effect of disabling the > > '1-alternative' audio track? The '1-main' is still enabled, so I guess the > audio > > should continue playing until all tracks corresponding to that > > bytestream/demuxer track id are disabled. > > This allows an app to alias multiple "kinds" to the same stream. Since it's > at-risk, and I haven't heard much, I don't think it's a high-pri scenario, but > we'll possibly need it for things like accessibility. It seems we have a fairly > simple way to support it as you've described here. Oh, and regarding "The '1-main' is still enabled, so I guess the audio should continue playing until all tracks corresponding to that bytestream/demuxer track id are disabled." : yes :)
lgtm % nits https://codereview.chromium.org/2050043002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/2050043002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer.h:336: void OnInitSegmentReported(const MediaTracksUpdatedCB& tracks_updated_cb, Is this being used somewhere? I think you mean to remove it https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/TrackBase.h (right): https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/TrackBase.h:43: WebMediaPlayer::TrackId trackId() const { return m_id; } Why do we have both this and id() methods? Can you delete this one and make all callers use id() https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebSourceBufferClient.h (right): https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebSourceBufferClient.h:21: WebString id; Why do you typedef WebString for webmediaplayer, but not here?
https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2396: DVLOG(MEDIA_LOG_LEVEL) << "audioTrackChanged(" << (void*)this << ") trackId= " << (String)trackId << " enabled=" << boolString(enabled); Track Id is a string now right? is this needed? If so, static_cast?
Only one non-nit: (why OnNewConfigs now allows mix of invalid and valid configs to not always emit decode error) https://codereview.chromium.org/2050043002/diff/80001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/80001/media/base/media_track.... media/base/media_track.h:46: // unique only within the scope of single bytestream. But we might have nit: s//single bytestream/single bytestream's initialization segment/ Note the following scenario is allowed by MSE: Single SourceBuffer, containing muxed single audio + single video track. Init Segment #1: Audio bs_id="1", Video bs_id="2" Init Segment #2: Audio bs_id="2", Video bs_id="1" Note that the bytestream ID's were not mapped to the same track. https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... media/filters/media_source_state.cc:488: for (const auto& track : tracks->tracks()) { This CL drops all usage of MediaTracks::getFirst{Audio,Video}Config(). Please remove those methods, too in this CL. https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... media/filters/media_source_state.cc:491: tracks->getAudioConfig(track_id).IsValidConfig()) { I'm not sure we should now allow multiple invalid configs to pass here without emitting decode error. Why are we filtering on IsValidConfig() here? (and similarly, for video case in else clause)?
servolk@chromium.org changed reviewers: + foolip@chromium.org
https://codereview.chromium.org/2050043002/diff/80001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/80001/media/base/media_track.... media/base/media_track.h:46: // unique only within the scope of single bytestream. But we might have On 2016/06/13 19:51:37, wolenetz_slow_reviews wrote: > nit: s//single bytestream/single bytestream's initialization segment/ > > Note the following scenario is allowed by MSE: > Single SourceBuffer, containing muxed single audio + single video track. > Init Segment #1: Audio bs_id="1", Video bs_id="2" > Init Segment #2: Audio bs_id="2", Video bs_id="1" > > Note that the bytestream ID's were not mapped to the same track. Yep, good catch, thanks, I've updated the comment to mention this. https://codereview.chromium.org/2050043002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/2050043002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer.h:336: void OnInitSegmentReported(const MediaTracksUpdatedCB& tracks_updated_cb, On 2016/06/13 19:30:43, chcunningham wrote: > Is this being used somewhere? I think you mean to remove it Done. https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... media/filters/media_source_state.cc:488: for (const auto& track : tracks->tracks()) { On 2016/06/13 19:51:37, wolenetz_slow_reviews wrote: > This CL drops all usage of MediaTracks::getFirst{Audio,Video}Config(). Please > remove those methods, too in this CL. No, there's still a couple of places where we use getFirstA/VConfig in unit tests. I'll update and land this removal of getFirstA/VConfig as a separate CL after this one lands: https://codereview.chromium.org/1833053005/ https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... media/filters/media_source_state.cc:491: tracks->getAudioConfig(track_id).IsValidConfig()) { On 2016/06/13 19:51:37, wolenetz_slow_reviews wrote: > I'm not sure we should now allow multiple invalid configs to pass here without > emitting decode error. Why are we filtering on IsValidConfig() here? (and > similarly, for video case in else clause)? Yeah, good point, I've updated this to reject multiple a/v tracks with logging into MEDIA_LOG. Also, now that media track code is getting stable I believe each track should have a valid a/v config associated with it, so converted the IsValidConfig to DCHECKs. https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2396: DVLOG(MEDIA_LOG_LEVEL) << "audioTrackChanged(" << (void*)this << ") trackId= " << (String)trackId << " enabled=" << boolString(enabled); On 2016/06/13 19:32:12, chcunningham wrote: > Track Id is a string now right? is this needed? If so, static_cast? Track id is a blink::WebString, and surprisingly blink::WebString doesn't support operator<< and there's no implicit conversion from blink::WebString into WTF::String. But blink::WebString has a conversion operator that converts into WTF::String, so we need to be more explicit here. To make it look better I guess we could construct a String out of track id here. https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/TrackBase.h (right): https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/TrackBase.h:43: WebMediaPlayer::TrackId trackId() const { return m_id; } On 2016/06/13 19:30:43, chcunningham wrote: > Why do we have both this and id() methods? Can you delete this one and make all > callers use id() Done. https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebSourceBufferClient.h (right): https://codereview.chromium.org/2050043002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebSourceBufferClient.h:21: WebString id; On 2016/06/13 19:30:44, chcunningham wrote: > Why do you typedef WebString for webmediaplayer, but not here? Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050043002/100001
2 Comment nits, plus a request for inspection with IsValidConfig() and conditional parse failure in mp4_stream_parser before adding tracks to MediaTracks that is passed to OnNewConfigs. Otherwise looking good. https://codereview.chromium.org/2050043002/diff/80001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/80001/media/base/media_track.... media/base/media_track.h:46: // unique only within the scope of single bytestream. But we might have On 2016/06/13 22:00:15, servolk wrote: > On 2016/06/13 19:51:37, wolenetz_slow_reviews wrote: > > nit: s//single bytestream/single bytestream's initialization segment/ > > > > Note the following scenario is allowed by MSE: > > Single SourceBuffer, containing muxed single audio + single video track. > > Init Segment #1: Audio bs_id="1", Video bs_id="2" > > Init Segment #2: Audio bs_id="2", Video bs_id="1" > > > > Note that the bytestream ID's were not mapped to the same track. > > Yep, good catch, thanks, I've updated the comment to mention this. Acknowledged. https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/80001/media/filters/media_sou... media/filters/media_source_state.cc:488: for (const auto& track : tracks->tracks()) { On 2016/06/13 22:00:16, servolk wrote: > On 2016/06/13 19:51:37, wolenetz_slow_reviews wrote: > > This CL drops all usage of MediaTracks::getFirst{Audio,Video}Config(). Please > > remove those methods, too in this CL. > > No, there's still a couple of places where we use getFirstA/VConfig in unit > tests. I'll update and land this removal of getFirstA/VConfig as a separate CL > after this one lands: > https://codereview.chromium.org/1833053005/ Acknowledged. My apologies :) https://codereview.chromium.org/2050043002/diff/120001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/120001/media/base/media_track... media/base/media_track.h:49: // multiple/ MediaSourceStates and multiple bytestreams) or subsequent init nit: s/multiple\//multiple/ https://codereview.chromium.org/2050043002/diff/120001/media/base/media_track... media/base/media_track.h:50: // segments bytestream ids might be redefined in subsequent init segments. nit: s/{whole line}/ // segments' may redefine the bytestream ids./ https://codereview.chromium.org/2050043002/diff/120001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/120001/media/filters/media_so... media/filters/media_source_state.cc:499: DCHECK(audio_config.IsValidConfig()); Mp4StreamParser can create audio or video MediaTracks entry with invalid config (which could be possible at least if the IsValidConfig() check changes versus the parameters passed to config::Initialize(...)). (Though it skips subsequent tracks if it has a valid one of same type, it doesn't check validity via IsValidConfig prior to adding the initialized config to MediaTracks.) ISTM a check and MEDIA_LOG for validity should be in mp4_stream_parser between config intialization and addition of the track to the MediaTracks it's building for passing to this method. tl;dr: I don't object to this DCHECK here, but if adding one here, let's try to add protection at the callers. See webm_tracks_parser's calls to AddAudioTrack and AddVideoTrack to see how it first conditions on ability to create a valid config (or otherwise gives parse error), for example. In looking around our parser, mp4_stream_parser.cc is the only one giving me concern that it might hit this DCHECK.
https://codereview.chromium.org/2050043002/diff/120001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2050043002/diff/120001/media/base/media_track... media/base/media_track.h:49: // multiple/ MediaSourceStates and multiple bytestreams) or subsequent init On 2016/06/13 23:23:58, wolenetz_slow_reviews wrote: > nit: s/multiple\//multiple/ Done. https://codereview.chromium.org/2050043002/diff/120001/media/base/media_track... media/base/media_track.h:50: // segments bytestream ids might be redefined in subsequent init segments. On 2016/06/13 23:23:57, wolenetz_slow_reviews wrote: > nit: s/{whole line}/ // segments' may redefine the bytestream ids./ Done. https://codereview.chromium.org/2050043002/diff/120001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2050043002/diff/120001/media/filters/media_so... media/filters/media_source_state.cc:499: DCHECK(audio_config.IsValidConfig()); On 2016/06/13 23:23:58, wolenetz_slow_reviews wrote: > Mp4StreamParser can create audio or video MediaTracks entry with invalid config > (which could be possible at least if the IsValidConfig() check changes versus > the parameters passed to config::Initialize(...)). (Though it skips subsequent > tracks if it has a valid one of same type, it doesn't check validity via > IsValidConfig prior to adding the initialized config to MediaTracks.) ISTM a > check and MEDIA_LOG for validity should be in mp4_stream_parser between config > intialization and addition of the track to the MediaTracks it's building for > passing to this method. > tl;dr: I don't object to this DCHECK here, but if adding one here, let's try to > add protection at the callers. See webm_tracks_parser's calls to AddAudioTrack > and AddVideoTrack to see how it first conditions on ability to create a valid > config (or otherwise gives parse error), for example. In looking around our > parser, mp4_stream_parser.cc is the only one giving me concern that it might hit > this DCHECK. Done.
lgtm
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2050043002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050043002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
owner LGTM for WebKit/.
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050043002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
servolk@chromium.org changed reviewers: + rbyers@chromium.org
On 2016/06/15 00:03:08, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) +rbyers@ for third_party/WebKit/public/ approval
On 2016/06/15 00:22:04, servolk wrote: > On 2016/06/15 00:03:08, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > +rbyers@ for third_party/WebKit/public/ approval We have lots of CLs depending on this landing first. foolip@, can you take a look for third_party/WebKit/public OWNERS in case rbyers@ review is delayed?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050043002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
public LGTM
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, wolenetz@chromium.org, haraken@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2050043002/#ps180001 (title: "rebase to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050043002/180001
Message was sent while issue was closed.
Description was changed from ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. BUG=249427,341581 ========== to ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. BUG=249427,341581 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. BUG=249427,341581 ========== to ========== Generate and assign media track ids in demuxers. Currently media track ids are generated on blink level, but this complicates media track management. In particular when media tracks are created on demuxer level we don't know their unique ids yet, so we can't easily set up track id -> demuxer stream mappings. Assigning track ids at the demuxer level makes that much easier. BUG=249427,341581 Committed: https://crrev.com/fa5c37c65c5f038882af6ca8222566d0928930a0 Cr-Commit-Position: refs/heads/master@{#400198} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/fa5c37c65c5f038882af6ca8222566d0928930a0 Cr-Commit-Position: refs/heads/master@{#400198} |
