Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226443002:
Support multiple media tracks in MSE / ChunkDemuxer (Closed)
|
Created:
4 years, 4 months ago by servolk Modified:
4 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport multiple media tracks in MSE / ChunkDemuxer
This will also allow multiple audio/video SourceBuffers for a single
MediaSource. Note that for now we'll still only select/play the first
audio and video track by default. Switching between tracks will require
further work in media renderers. A few notes:
Most of the changes are straightforward, except media_source_state.*
In order to allow multiple tracks in a single SourceBuffer, we'll now
pass the codecs parameter from WSBI further down to ChunkDemuxer and
MediaSourceState, replacing the old has_audio/has_video flags.
The new MSS::OnNewConfigs structure is much closer to the MSE init
segment handling algorithm, with essentially the same logic for
handling new/existing track ids and such.
BUG=487288
Committed: https://crrev.com/d3acf22f7d91ad262a07075848fad13b94d15226
Cr-Commit-Position: refs/heads/master@{#420113}
Patch Set 1 #Patch Set 2 : wip #Patch Set 3 : wip #Patch Set 4 : wip #Patch Set 5 : wip #Patch Set 6 : rebase #Patch Set 7 : wip #Patch Set 8 : wip #Patch Set 9 : Fixed MSE/EME tests #Patch Set 10 : rebase #Patch Set 11 : Improved tests #Patch Set 12 : rebase #Patch Set 13 : Refactored OnNewConfigs #Patch Set 14 : Fixed addSourceBuffer test #Patch Set 15 : A few clean ups #
Total comments: 24
Patch Set 16 : Replace pending_source_init_done_count_ with pending_source_init_ids_ set #
Total comments: 5
Patch Set 17 : Remove demuxer streams created for the removed MediaSourceState/SourceBuffer #Patch Set 18 : Keep alive demuxer streams for removed ChunkDemuxer ids #Patch Set 19 : mp4 format is not supported on some trybots, so use webm #
Total comments: 69
Patch Set 20 : Fixed EvictCodecFrame + other CR feedback #Patch Set 21 : nit #
Total comments: 9
Patch Set 22 : CR feedback #
Total comments: 8
Patch Set 23 : CR feedback #
Total comments: 15
Patch Set 24 : CR feedback #Patch Set 25 : nit #Patch Set 26 : Fixed integer overflow #Dependent Patchsets: Messages
Total messages: 149 (116 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== wip: Allow multiple tracks/stream in MSE ========== to ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=249427, 249428 ==========
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org, wolenetz@chromium.org
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Description was changed from ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=249427, 249428 ========== to ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=487288 ==========
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/v2/patch-status/codereview.chromium.or...
dalecurtis@chromium.org changed reviewers: + chcunningham@chromium.org - dalecurtis@chromium.org
This one's on the MSE experts.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I didn't review beyond CD.cc on this first pass, though I found some things that look like they're broken or at least very confusing to me (see comments, below). One spec-allowed behavior which might simplify some of the broken behavior might be to disallow MediaSource.addSourceBuffer(...) once we've received init segments for all source buffers added (and not yet removed) so far. WDYT? https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:9: #include <set> nit: why did this change? https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:608: pending_source_init_done_count_++; Hmm. I think we may have some exacerbation of previous bad behavior which can occur if: addId(x); removeId(x); addId(y); appendInitSegment(y);-->OnSourceInitDone(). At this point, we have just one SourceBuffer, and should be expecting only 1 init done, but a OnSourceInitDone() for (x) will never arrive and playback won't reach HAVE_METADATA. Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think OnSourceInitDone() won't like this either. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:610: std::string expected_mss_codecs = codecs; The parsing and expectation matching is confusing now that it's split even further. Can this be more centralized? https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1079: ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN); This patchset allows (see above) AddId(z) + appendData(z) -> OnSourceInitDone(z) to occur *after* demuxer has been successfully opened. This error seems wrong, then, though some error should occur if timeline_offset_ differs in this scenario. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1086: if (params.liveness != DemuxerStream::LIVENESS_UNKNOWN) { Hmm. We appear to be exacerbating previously confusing behavior here, where depending on the order in which the OnSourceInitDone()'s occur, the liveness may change both before *and* after reaching HAVE_METADATA. This may have unintended consequences. Can this be cleaned up? https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1098: pending_source_init_done_count_--; See comment above. Once we've reached 0 here, the count may increase again and a later OnSourceInitDone() may cause it to reach 0 again, triggering bad behavior: over-logging of track counts, rappor videocodec, seeking everything back to 0, and attempting to rerun a now-null init_cb. Another bad behavior (never reaching 0 here) may occur if a RemoveId() occurs after AddId and before the associated OnSourceInitDone() has occurred. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1155: } break; inconsistent {...}break; vs {... break;} https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1164: } break; ditto
https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:9: #include <set> On 2016/09/06 21:20:50, wolenetz wrote: > nit: why did this change? Because I don't see std::list being used explicitly in this file anywhere and I'm now using std::set in OnEnabledAudioTracksChanged. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:608: pending_source_init_done_count_++; On 2016/09/06 21:20:50, wolenetz wrote: > Hmm. I think we may have some exacerbation of previous bad behavior which can > occur if: > addId(x); > removeId(x); > addId(y); > appendInitSegment(y);-->OnSourceInitDone(). > At this point, we have just one SourceBuffer, and should be expecting only 1 > init done, but a OnSourceInitDone() for (x) will never arrive and playback won't > reach HAVE_METADATA. > > Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think > OnSourceInitDone() won't like this either. Well, it's not exacerbation, it's actually exactly the same behavior that we have now, only without the restriction of having only 1 audio and 1 video :) I agree that it's a problem, but I believe it should be easy to fix - we can remove the pending_source_init_done_count_ counter and instead just maintain a map<id, bool> indicating whether a given id has been initialized or not. We'll then check that map in the OnSourceInitDone and will proceed with the transition into the initialized state only if all the ids have been initialized. Re your second point, tbh I don't understand why you say '... now we allow ...', could you explain? AFAICT I've preserved the existing logic: PipelineImpl reaches the HAVE_METADATA state only after ChunkDemuxer invokes init_cb_, which happens from ChunkDemuxer::OnSourceInitDone when all SourceBuffers/ids have received the init segment. But that also means that ChunkDemuxer has transitioned from the INITIALIZING to INITIALIZED state, and so we want allow any more SourceBuffers added after that due to this check: https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:610: std::string expected_mss_codecs = codecs; On 2016/09/06 21:20:49, wolenetz wrote: > The parsing and expectation matching is confusing now that it's split even > further. Can this be more centralized? Well, we can only do the matching in the OnNewConfigs where we get the information about the actual codecs present in the bytestream. So I'm not sure where else we can centralize it - any suggestions? Also the fact that video and audio codecs use separate incompatible enums (AudioCodec and VideoCodec enums respectively) makes it somewhat cumbersome to extract the codec matching logic that is common between video and audio. I guess we could use a C++ template function which would take enum type as a parameter, WDYT? https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1079: ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN); On 2016/09/06 21:20:50, wolenetz wrote: > This patchset allows (see above) AddId(z) + appendData(z) -> OnSourceInitDone(z) > to occur *after* demuxer has been successfully opened. This error seems wrong, > then, though some error should occur if timeline_offset_ differs in this > scenario. Nope, see my explanation in the other comment. AddId will fail if the demuxer has been opened (i.e. fired the init_cb_ and transitioned to INITIALIZED state) already due to a check at https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... Btw, I have actually replaced the old layout test that was making an assumption about more than ~20 SourceBuffers being supported with a test that tries to add a SourceBuffer after initializing existing SourceBuffers - that also results in the QuotaExceeded exception being thrown (see the changes in mediasource-addsourcebuffer.html in this CL) https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1086: if (params.liveness != DemuxerStream::LIVENESS_UNKNOWN) { On 2016/09/06 21:20:50, wolenetz wrote: > Hmm. We appear to be exacerbating previously confusing behavior here, where > depending on the order in which the OnSourceInitDone()'s occur, the liveness may > change both before *and* after reaching HAVE_METADATA. This may have unintended > consequences. Can this be cleaned up? I believe this shouldn't be a problem, as HAVE_METADATA should only be reached after OnSourceInitDone. See my comments above. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1098: pending_source_init_done_count_--; On 2016/09/06 21:20:50, wolenetz wrote: > See comment above. Once we've reached 0 here, the count may increase again and a > later OnSourceInitDone() may cause it to reach 0 again, triggering bad behavior: > over-logging of track counts, rappor videocodec, seeking everything back to 0, > and attempting to rerun a now-null init_cb. > > Another bad behavior (never reaching 0 here) may occur if a RemoveId() occurs > after AddId and before the associated OnSourceInitDone() has occurred. Yep, as I've explained above I believe we can solve this but getting rid of the counter and keeping track of which ids have been initialized via a map<id, bool is_initialized>. But note that this is not a regression from the current behavior! https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1155: } break; On 2016/09/06 21:20:49, wolenetz wrote: > inconsistent {...}break; vs {... break;} Actually I just noticed that the break is redundant here, since there is a return above it. Removed. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1164: } break; On 2016/09/06 21:20:50, wolenetz wrote: > ditto Done.
On 2016/09/07 01:35:10, servolk wrote: > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > File media/filters/chunk_demuxer.cc (right): > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:9: #include <set> > On 2016/09/06 21:20:50, wolenetz wrote: > > nit: why did this change? > > Because I don't see std::list being used explicitly in this file anywhere and > I'm now using std::set in OnEnabledAudioTracksChanged. > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:608: pending_source_init_done_count_++; > On 2016/09/06 21:20:50, wolenetz wrote: > > Hmm. I think we may have some exacerbation of previous bad behavior which can > > occur if: > > addId(x); > > removeId(x); > > addId(y); > > appendInitSegment(y);-->OnSourceInitDone(). > > At this point, we have just one SourceBuffer, and should be expecting only 1 > > init done, but a OnSourceInitDone() for (x) will never arrive and playback > won't > > reach HAVE_METADATA. > > > > Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think > > OnSourceInitDone() won't like this either. > > Well, it's not exacerbation, it's actually exactly the same behavior that we > have now, only without the restriction of having only 1 audio and 1 video :) > I agree that it's a problem, but I believe it should be easy to fix - we can > remove the pending_source_init_done_count_ counter and instead just maintain a > map<id, bool> indicating whether a given id has been initialized or not. We'll > then check that map in the OnSourceInitDone and will proceed with the transition > into the initialized state only if all the ids have been initialized. > > Re your second point, tbh I don't understand why you say '... now we allow ...', > could you explain? > AFAICT I've preserved the existing logic: PipelineImpl reaches the HAVE_METADATA > state only after ChunkDemuxer invokes init_cb_, which happens from > ChunkDemuxer::OnSourceInitDone when all SourceBuffers/ids have received the init > segment. But that also means that ChunkDemuxer has transitioned from the > INITIALIZING to INITIALIZED state, and so we want allow any more SourceBuffers > added after that due to this check: > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:610: std::string expected_mss_codecs = codecs; > On 2016/09/06 21:20:49, wolenetz wrote: > > The parsing and expectation matching is confusing now that it's split even > > further. Can this be more centralized? > > Well, we can only do the matching in the OnNewConfigs where we get the > information about the actual codecs present in the bytestream. So I'm not sure > where else we can centralize it - any suggestions? Also the fact that video and > audio codecs use separate incompatible enums (AudioCodec and VideoCodec enums > respectively) makes it somewhat cumbersome to extract the codec matching logic > that is common between video and audio. I guess we could use a C++ template > function which would take enum type as a parameter, WDYT? > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:1079: > ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN); > On 2016/09/06 21:20:50, wolenetz wrote: > > This patchset allows (see above) AddId(z) + appendData(z) -> > OnSourceInitDone(z) > > to occur *after* demuxer has been successfully opened. This error seems wrong, > > then, though some error should occur if timeline_offset_ differs in this > > scenario. > > Nope, see my explanation in the other comment. AddId will fail if the demuxer > has been opened (i.e. fired the init_cb_ and transitioned to INITIALIZED state) > already due to a check at > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > Btw, I have actually replaced the old layout test that was making an assumption > about more than ~20 SourceBuffers being supported with a test that tries to add > a SourceBuffer after initializing existing SourceBuffers - that also results in > the QuotaExceeded exception being thrown (see the changes in > mediasource-addsourcebuffer.html in this CL) > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:1086: if (params.liveness != > DemuxerStream::LIVENESS_UNKNOWN) { > On 2016/09/06 21:20:50, wolenetz wrote: > > Hmm. We appear to be exacerbating previously confusing behavior here, where > > depending on the order in which the OnSourceInitDone()'s occur, the liveness > may > > change both before *and* after reaching HAVE_METADATA. This may have > unintended > > consequences. Can this be cleaned up? > > I believe this shouldn't be a problem, as HAVE_METADATA should only be reached > after OnSourceInitDone. See my comments above. > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:1098: pending_source_init_done_count_--; > On 2016/09/06 21:20:50, wolenetz wrote: > > See comment above. Once we've reached 0 here, the count may increase again and > a > > later OnSourceInitDone() may cause it to reach 0 again, triggering bad > behavior: > > over-logging of track counts, rappor videocodec, seeking everything back to 0, > > and attempting to rerun a now-null init_cb. > > > > Another bad behavior (never reaching 0 here) may occur if a RemoveId() occurs > > after AddId and before the associated OnSourceInitDone() has occurred. > > Yep, as I've explained above I believe we can solve this but getting rid of the > counter and keeping track of which ids have been initialized via a map<id, bool > is_initialized>. But note that this is not a regression from the current > behavior! > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:1155: } break; > On 2016/09/06 21:20:49, wolenetz wrote: > > inconsistent {...}break; vs {... break;} > > Actually I just noticed that the break is redundant here, since there is a > return above it. Removed. > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > media/filters/chunk_demuxer.cc:1164: } break; > On 2016/09/06 21:20:50, wolenetz wrote: > > ditto > > Done. Ok, I have actually fixed the logic for tracking the init state of various ids in the latest patchset. Instead of using the counter, we could use a set of the ids that are still waiting for init segment. I've also added a new unit test that verifies that if we AddId(id1); AddId(id2); RemoveId(id2); and then Append init segment for id1 that this triggers the InitDoneCB, as it should. Also note that there is already an existing test that verifies that calling AddId fails after the ChunkDemuxer initialization has been completed (see the AddIdFailures test https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...).
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
On 2016/09/07 02:23:26, servolk wrote: > On 2016/09/07 01:35:10, servolk wrote: > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > File media/filters/chunk_demuxer.cc (right): > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:9: #include <set> > > On 2016/09/06 21:20:50, wolenetz wrote: > > > nit: why did this change? > > > > Because I don't see std::list being used explicitly in this file anywhere and > > I'm now using std::set in OnEnabledAudioTracksChanged. > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:608: pending_source_init_done_count_++; > > On 2016/09/06 21:20:50, wolenetz wrote: > > > Hmm. I think we may have some exacerbation of previous bad behavior which > can > > > occur if: > > > addId(x); > > > removeId(x); > > > addId(y); > > > appendInitSegment(y);-->OnSourceInitDone(). > > > At this point, we have just one SourceBuffer, and should be expecting only 1 > > > init done, but a OnSourceInitDone() for (x) will never arrive and playback > > won't > > > reach HAVE_METADATA. > > > > > > Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think > > > OnSourceInitDone() won't like this either. > > > > Well, it's not exacerbation, it's actually exactly the same behavior that we > > have now, only without the restriction of having only 1 audio and 1 video :) > > I agree that it's a problem, but I believe it should be easy to fix - we can > > remove the pending_source_init_done_count_ counter and instead just maintain a > > map<id, bool> indicating whether a given id has been initialized or not. We'll > > then check that map in the OnSourceInitDone and will proceed with the > transition > > into the initialized state only if all the ids have been initialized. > > > > Re your second point, tbh I don't understand why you say '... now we allow > ...', > > could you explain? > > AFAICT I've preserved the existing logic: PipelineImpl reaches the > HAVE_METADATA > > state only after ChunkDemuxer invokes init_cb_, which happens from > > ChunkDemuxer::OnSourceInitDone when all SourceBuffers/ids have received the > init > > segment. But that also means that ChunkDemuxer has transitioned from the > > INITIALIZING to INITIALIZED state, and so we want allow any more SourceBuffers > > added after that due to this check: > > > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:610: std::string expected_mss_codecs = codecs; > > On 2016/09/06 21:20:49, wolenetz wrote: > > > The parsing and expectation matching is confusing now that it's split even > > > further. Can this be more centralized? > > > > Well, we can only do the matching in the OnNewConfigs where we get the > > information about the actual codecs present in the bytestream. So I'm not sure > > where else we can centralize it - any suggestions? Also the fact that video > and > > audio codecs use separate incompatible enums (AudioCodec and VideoCodec enums > > respectively) makes it somewhat cumbersome to extract the codec matching logic > > that is common between video and audio. I guess we could use a C++ template > > function which would take enum type as a parameter, WDYT? > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:1079: > > ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN); > > On 2016/09/06 21:20:50, wolenetz wrote: > > > This patchset allows (see above) AddId(z) + appendData(z) -> > > OnSourceInitDone(z) > > > to occur *after* demuxer has been successfully opened. This error seems > wrong, > > > then, though some error should occur if timeline_offset_ differs in this > > > scenario. > > > > Nope, see my explanation in the other comment. AddId will fail if the demuxer > > has been opened (i.e. fired the init_cb_ and transitioned to INITIALIZED > state) > > already due to a check at > > > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > > Btw, I have actually replaced the old layout test that was making an > assumption > > about more than ~20 SourceBuffers being supported with a test that tries to > add > > a SourceBuffer after initializing existing SourceBuffers - that also results > in > > the QuotaExceeded exception being thrown (see the changes in > > mediasource-addsourcebuffer.html in this CL) > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:1086: if (params.liveness != > > DemuxerStream::LIVENESS_UNKNOWN) { > > On 2016/09/06 21:20:50, wolenetz wrote: > > > Hmm. We appear to be exacerbating previously confusing behavior here, where > > > depending on the order in which the OnSourceInitDone()'s occur, the liveness > > may > > > change both before *and* after reaching HAVE_METADATA. This may have > > unintended > > > consequences. Can this be cleaned up? > > > > I believe this shouldn't be a problem, as HAVE_METADATA should only be reached > > after OnSourceInitDone. See my comments above. > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:1098: pending_source_init_done_count_--; > > On 2016/09/06 21:20:50, wolenetz wrote: > > > See comment above. Once we've reached 0 here, the count may increase again > and > > a > > > later OnSourceInitDone() may cause it to reach 0 again, triggering bad > > behavior: > > > over-logging of track counts, rappor videocodec, seeking everything back to > 0, > > > and attempting to rerun a now-null init_cb. > > > > > > Another bad behavior (never reaching 0 here) may occur if a RemoveId() > occurs > > > after AddId and before the associated OnSourceInitDone() has occurred. > > > > Yep, as I've explained above I believe we can solve this but getting rid of > the > > counter and keeping track of which ids have been initialized via a map<id, > bool > > is_initialized>. But note that this is not a regression from the current > > behavior! > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:1155: } break; > > On 2016/09/06 21:20:49, wolenetz wrote: > > > inconsistent {...}break; vs {... break;} > > > > Actually I just noticed that the break is redundant here, since there is a > > return above it. Removed. > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > media/filters/chunk_demuxer.cc:1164: } break; > > On 2016/09/06 21:20:50, wolenetz wrote: > > > ditto > > > > Done. > > Ok, I have actually fixed the logic for tracking the init state of various ids > in the latest patchset. Instead of using the counter, we could use a set of the > ids that are still waiting for init segment. I've also added a new unit test > that verifies that if we AddId(id1); AddId(id2); RemoveId(id2); and then Append > init segment for id1 that this triggers the InitDoneCB, as it should. Also note > that there is already an existing test that verifies that calling AddId fails > after the ChunkDemuxer initialization has been completed (see the AddIdFailures > test > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...). Oops, sorry, bad link for the AddIdFailures test, should be: https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?...
On 2016/09/07 17:26:59, servolk wrote: > On 2016/09/07 02:23:26, servolk wrote: > > On 2016/09/07 01:35:10, servolk wrote: > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > File media/filters/chunk_demuxer.cc (right): > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:9: #include <set> > > > On 2016/09/06 21:20:50, wolenetz wrote: > > > > nit: why did this change? > > > > > > Because I don't see std::list being used explicitly in this file anywhere > and > > > I'm now using std::set in OnEnabledAudioTracksChanged. > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:608: pending_source_init_done_count_++; > > > On 2016/09/06 21:20:50, wolenetz wrote: > > > > Hmm. I think we may have some exacerbation of previous bad behavior which > > can > > > > occur if: > > > > addId(x); > > > > removeId(x); > > > > addId(y); > > > > appendInitSegment(y);-->OnSourceInitDone(). > > > > At this point, we have just one SourceBuffer, and should be expecting only > 1 > > > > init done, but a OnSourceInitDone() for (x) will never arrive and playback > > > won't > > > > reach HAVE_METADATA. > > > > > > > > Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think > > > > OnSourceInitDone() won't like this either. > > > > > > Well, it's not exacerbation, it's actually exactly the same behavior that we > > > have now, only without the restriction of having only 1 audio and 1 video :) > > > I agree that it's a problem, but I believe it should be easy to fix - we can > > > remove the pending_source_init_done_count_ counter and instead just maintain > a > > > map<id, bool> indicating whether a given id has been initialized or not. > We'll > > > then check that map in the OnSourceInitDone and will proceed with the > > transition > > > into the initialized state only if all the ids have been initialized. > > > > > > Re your second point, tbh I don't understand why you say '... now we allow > > ...', > > > could you explain? > > > AFAICT I've preserved the existing logic: PipelineImpl reaches the > > HAVE_METADATA > > > state only after ChunkDemuxer invokes init_cb_, which happens from > > > ChunkDemuxer::OnSourceInitDone when all SourceBuffers/ids have received the > > init > > > segment. But that also means that ChunkDemuxer has transitioned from the > > > INITIALIZING to INITIALIZED state, and so we want allow any more > SourceBuffers > > > added after that due to this check: > > > > > > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:610: std::string expected_mss_codecs = > codecs; > > > On 2016/09/06 21:20:49, wolenetz wrote: > > > > The parsing and expectation matching is confusing now that it's split even > > > > further. Can this be more centralized? > > > > > > Well, we can only do the matching in the OnNewConfigs where we get the > > > information about the actual codecs present in the bytestream. So I'm not > sure > > > where else we can centralize it - any suggestions? Also the fact that video > > and > > > audio codecs use separate incompatible enums (AudioCodec and VideoCodec > enums > > > respectively) makes it somewhat cumbersome to extract the codec matching > logic > > > that is common between video and audio. I guess we could use a C++ template > > > function which would take enum type as a parameter, WDYT? > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:1079: > > > ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN); > > > On 2016/09/06 21:20:50, wolenetz wrote: > > > > This patchset allows (see above) AddId(z) + appendData(z) -> > > > OnSourceInitDone(z) > > > > to occur *after* demuxer has been successfully opened. This error seems > > wrong, > > > > then, though some error should occur if timeline_offset_ differs in this > > > > scenario. > > > > > > Nope, see my explanation in the other comment. AddId will fail if the > demuxer > > > has been opened (i.e. fired the init_cb_ and transitioned to INITIALIZED > > state) > > > already due to a check at > > > > > > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > > > Btw, I have actually replaced the old layout test that was making an > > assumption > > > about more than ~20 SourceBuffers being supported with a test that tries to > > add > > > a SourceBuffer after initializing existing SourceBuffers - that also results > > in > > > the QuotaExceeded exception being thrown (see the changes in > > > mediasource-addsourcebuffer.html in this CL) > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:1086: if (params.liveness != > > > DemuxerStream::LIVENESS_UNKNOWN) { > > > On 2016/09/06 21:20:50, wolenetz wrote: > > > > Hmm. We appear to be exacerbating previously confusing behavior here, > where > > > > depending on the order in which the OnSourceInitDone()'s occur, the > liveness > > > may > > > > change both before *and* after reaching HAVE_METADATA. This may have > > > unintended > > > > consequences. Can this be cleaned up? > > > > > > I believe this shouldn't be a problem, as HAVE_METADATA should only be > reached > > > after OnSourceInitDone. See my comments above. > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:1098: pending_source_init_done_count_--; > > > On 2016/09/06 21:20:50, wolenetz wrote: > > > > See comment above. Once we've reached 0 here, the count may increase again > > and > > > a > > > > later OnSourceInitDone() may cause it to reach 0 again, triggering bad > > > behavior: > > > > over-logging of track counts, rappor videocodec, seeking everything back > to > > 0, > > > > and attempting to rerun a now-null init_cb. > > > > > > > > Another bad behavior (never reaching 0 here) may occur if a RemoveId() > > occurs > > > > after AddId and before the associated OnSourceInitDone() has occurred. > > > > > > Yep, as I've explained above I believe we can solve this but getting rid of > > the > > > counter and keeping track of which ids have been initialized via a map<id, > > bool > > > is_initialized>. But note that this is not a regression from the current > > > behavior! > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:1155: } break; > > > On 2016/09/06 21:20:49, wolenetz wrote: > > > > inconsistent {...}break; vs {... break;} > > > > > > Actually I just noticed that the break is redundant here, since there is a > > > return above it. Removed. > > > > > > > > > https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... > > > media/filters/chunk_demuxer.cc:1164: } break; > > > On 2016/09/06 21:20:50, wolenetz wrote: > > > > ditto > > > > > > Done. > > > > Ok, I have actually fixed the logic for tracking the init state of various ids > > in the latest patchset. Instead of using the counter, we could use a set of > the > > ids that are still waiting for init segment. I've also added a new unit test > > that verifies that if we AddId(id1); AddId(id2); RemoveId(id2); and then > Append > > init segment for id1 that this triggers the InitDoneCB, as it should. Also > note > > that there is already an existing test that verifies that calling AddId fails > > after the ChunkDemuxer initialization has been completed (see the > AddIdFailures > > test > > > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...). > > Oops, sorry, bad link for the AddIdFailures test, should be: > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?... If I don't get to this today, I will Monday. Thanks for patience.
Looking better - I found another pre-existing case which we should look into fixing with this CL: https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:9: #include <set> On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:50, wolenetz wrote: > > nit: why did this change? > > Because I don't see std::list being used explicitly in this file anywhere and > I'm now using std::set in OnEnabledAudioTracksChanged. Acknowledged. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:608: pending_source_init_done_count_++; On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:50, wolenetz wrote: > > Hmm. I think we may have some exacerbation of previous bad behavior which can > > occur if: > > addId(x); > > removeId(x); > > addId(y); > > appendInitSegment(y);-->OnSourceInitDone(). > > At this point, we have just one SourceBuffer, and should be expecting only 1 > > init done, but a OnSourceInitDone() for (x) will never arrive and playback > won't > > reach HAVE_METADATA. > > > > Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think > > OnSourceInitDone() won't like this either. > > Well, it's not exacerbation, it's actually exactly the same behavior that we > have now, only without the restriction of having only 1 audio and 1 video :) > I agree that it's a problem, but I believe it should be easy to fix - we can > remove the pending_source_init_done_count_ counter and instead just maintain a > map<id, bool> indicating whether a given id has been initialized or not. We'll > then check that map in the OnSourceInitDone and will proceed with the transition > into the initialized state only if all the ids have been initialized. > > Re your second point, tbh I don't understand why you say '... now we allow ...', > could you explain? > AFAICT I've preserved the existing logic: PipelineImpl reaches the HAVE_METADATA > state only after ChunkDemuxer invokes init_cb_, which happens from > ChunkDemuxer::OnSourceInitDone when all SourceBuffers/ids have received the init > segment. But that also means that ChunkDemuxer has transitioned from the > INITIALIZING to INITIALIZED state, and so we want allow any more SourceBuffers > added after that due to this check: > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... First point and map: SGTM. I'll review that. Second point (was invalid point): Ack. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:610: std::string expected_mss_codecs = codecs; On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:49, wolenetz wrote: > > The parsing and expectation matching is confusing now that it's split even > > further. Can this be more centralized? > > Well, we can only do the matching in the OnNewConfigs where we get the > information about the actual codecs present in the bytestream. So I'm not sure > where else we can centralize it - any suggestions? Also the fact that video and > audio codecs use separate incompatible enums (AudioCodec and VideoCodec enums > respectively) makes it somewhat cumbersome to extract the codec matching logic > that is common between video and audio. I guess we could use a C++ template > function which would take enum type as a parameter, WDYT? Would using the previous logic and something like (expect_audio) and (expect_video) to enforce that the OnNewConfigs() contains tracks with at least the expected audio and video tracks work? Or are you proposing deeper codec parameter (profile, etc for avc, for example) be done in OnNewConfigs? I suppose the route you're taking here might help if we want to differentiate, for instance, h264 vs vp9 in ISO-BMFF at the OnNewConfigs() level. Or is there some other reason I've missed why this logic is decentralized a bit further in this CL? https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1079: ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN); On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:50, wolenetz wrote: > > This patchset allows (see above) AddId(z) + appendData(z) -> > OnSourceInitDone(z) > > to occur *after* demuxer has been successfully opened. This error seems wrong, > > then, though some error should occur if timeline_offset_ differs in this > > scenario. > > Nope, see my explanation in the other comment. AddId will fail if the demuxer > has been opened (i.e. fired the init_cb_ and transitioned to INITIALIZED state) > already due to a check at > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731... > Btw, I have actually replaced the old layout test that was making an assumption > about more than ~20 SourceBuffers being supported with a test that tries to add > a SourceBuffer after initializing existing SourceBuffers - that also results in > the QuotaExceeded exception being thrown (see the changes in > mediasource-addsourcebuffer.html in this CL) Acknowledged. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1086: if (params.liveness != DemuxerStream::LIVENESS_UNKNOWN) { On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:50, wolenetz wrote: > > Hmm. We appear to be exacerbating previously confusing behavior here, where > > depending on the order in which the OnSourceInitDone()'s occur, the liveness > may > > change both before *and* after reaching HAVE_METADATA. This may have > unintended > > consequences. Can this be cleaned up? > > I believe this shouldn't be a problem, as HAVE_METADATA should only be reached > after OnSourceInitDone. See my comments above. Ack, though IIUC, this code now allows a video track from one SourceBuffer to be low_latency, and another video track from a distinct SourceBuffer to not be low_latency. Does our track switching logic in the renderer allow for switching among these? If so, not a problem here. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1098: pending_source_init_done_count_--; On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:50, wolenetz wrote: > > See comment above. Once we've reached 0 here, the count may increase again and > a > > later OnSourceInitDone() may cause it to reach 0 again, triggering bad > behavior: > > over-logging of track counts, rappor videocodec, seeking everything back to 0, > > and attempting to rerun a now-null init_cb. > > > > Another bad behavior (never reaching 0 here) may occur if a RemoveId() occurs > > after AddId and before the associated OnSourceInitDone() has occurred. > > Yep, as I've explained above I believe we can solve this but getting rid of the > counter and keeping track of which ids have been initialized via a map<id, bool > is_initialized>. But note that this is not a regression from the current > behavior! Ack. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1155: } break; On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:49, wolenetz wrote: > > inconsistent {...}break; vs {... break;} > > Actually I just noticed that the break is redundant here, since there is a > return above it. Removed. Acknowledged. https://codereview.chromium.org/2226443002/diff/280001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1164: } break; On 2016/09/07 01:35:10, servolk wrote: > On 2016/09/06 21:20:50, wolenetz wrote: > > ditto > > Done. Acknowledged. https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:630: void ChunkDemuxer::RemoveId(const std::string& id) { Another (possibly pre-existing) issue : addId(x with expected streams a and b); addId(y with expected streams c and d); appendBuffer(x, init segment with streams a and b) ->OnSourceInitDone(not yet done, need y still) removeId(x); appendBuffer(y, init segment with streams c and d) --> OnSourceInitDone(all done, but we still retain a refcount on streams a and b --> I suspect this causes problems or at least fragile code for anything that traverses audio_streams_ or video_streams_ in this class at least). https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1066: DCHECK_EQ(state_, INITIALIZING); nit: also DCHECK that source_id is in pending_source_init_ids_.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:630: void ChunkDemuxer::RemoveId(const std::string& id) { On 2016/09/12 21:41:06, wolenetz wrote: > Another (possibly pre-existing) issue : > addId(x with expected streams a and b); > addId(y with expected streams c and d); > appendBuffer(x, init segment with streams a and b) > ->OnSourceInitDone(not yet done, need y still) > removeId(x); > appendBuffer(y, init segment with streams c and d) > --> OnSourceInitDone(all done, but we still retain a refcount on streams a and b > --> I suspect this causes problems or at least fragile code for anything that > traverses audio_streams_ or video_streams_ in this class at least). Indeed, this is another pre-existing issue. We could fix it by removing / releasing the demuxer streams created for the removed MediaSourceState. But interestingly when I tried, that caused failure of the RemoveId unit test (see https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?...), which apparently expects to be able to continue reading from the removed stream. I think we can probably simply remove the attempt to read from removed stream (and I'm planning to do just that in the latest patchset). But if never invalidating a demuxer stream or being able to read from the stream corresponding to a removed MediaSourceState is important, please let me know, we can try to find another way to deal with it. https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:1066: DCHECK_EQ(state_, INITIALIZING); On 2016/09/12 21:41:06, wolenetz wrote: > nit: also DCHECK that source_id is in pending_source_init_ids_. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/300001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:630: void ChunkDemuxer::RemoveId(const std::string& id) { On 2016/09/13 02:09:30, servolk wrote: > On 2016/09/12 21:41:06, wolenetz wrote: > > Another (possibly pre-existing) issue : > > addId(x with expected streams a and b); > > addId(y with expected streams c and d); > > appendBuffer(x, init segment with streams a and b) > > ->OnSourceInitDone(not yet done, need y still) > > removeId(x); > > appendBuffer(y, init segment with streams c and d) > > --> OnSourceInitDone(all done, but we still retain a refcount on streams a and > b > > --> I suspect this causes problems or at least fragile code for anything that > > traverses audio_streams_ or video_streams_ in this class at least). > > Indeed, this is another pre-existing issue. > We could fix it by removing / releasing the demuxer streams created for the > removed MediaSourceState. But interestingly when I tried, that caused failure of > the RemoveId unit test (see > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?...), > which apparently expects to be able to continue reading from the removed stream. > I think we can probably simply remove the attempt to read from removed stream > (and I'm planning to do just that in the latest patchset). But if never > invalidating a demuxer stream or being able to read from the stream > corresponding to a removed MediaSourceState is important, please let me know, we > can try to find another way to deal with it. Well, this has caused a few more test failures in the CQ, so we do need to find another way to deal with this. It turns out that demuxer clients might save raw pointers returned by ChunkDemuxer::GetStream and might try to use them later, after id has been removed (one particular place that does that is DecryptingDemuxerStream). So in the latest patchset I've tried a slightly different approach: when some MediaSourceState is removed, we'll move the demuxer streams created for the MediaSourceState into the removed_streams_ collection, which will keep those stream object alive. MediaSourceState puts its streams into shutdown state when it's destroyed, so attempting to read from those streams will return EOS buffers, just like before this change.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
Looking quite good. Almost there. Full review pass this time, one further case noted which needs fixing and testing: multiple tracks of same codec type in same SourceBuffer should be allowed. But fail if not all the codecs in the codecs string were in the init segment (or if any codecs not in the codecs string were in the init segment, except for text tracks which we currently don't verify codecs). Some other comments and nits. Again, looking quite good. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:496: if (s->enabled()) Add TODO and crbug (if missing) w.r.t. mixing multiple enabled audio tracks. Maybe "GetFirstEnabledStreamOfType()" vs "GetAllStreamsOfType()" (one for current usage by renderer {with TODO+crbug to support audio mixing of multiple enabled audio tracks}, and the latter for usage in UMA reporting). WDYT? https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:641: for (size_t i = 0; i < audio_streams_.size(); ++i) { nit: CHECK that we actually found and moved all the streams for id into removed_streams_. Not DCHECK, since a raw pointer in demuxer client might still be used to deref something that's been deleted if not correctly moved into removed_streams_. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4457: // Audio stream had 8 buffers, video stream had 15. We told EvictCodedFrames Why is this portion of this test dropped? https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:401: memcpy(buf, audio_track_entry->data(), audio_track_entry->data_size()); aside: Why reorder this block here vs before "if (has_video)" previously? https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:2538: ASSERT_EQ(AddId(video_id, HAS_VIDEO), ChunkDemuxer::kReachedIdLimit); aside: good that we had this already :) https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3467: EXPECT_EQ(ChunkDemuxer::kOk, AddId(kSourceId, kMp2tMimeType, kMp2tCodecs)); aside: nice elimination of that helper, above. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4736: EXPECT_MEDIA_LOG(SegmentMissingFrames("1")).Times(AnyNumber()); Is this because the test media indeed has muxed segments that sometimes have missing track media frames? I just want to make sure that's understood. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4741: CheckExpectedRanges(kId2, "{ [0,10007) }"); nit: might be a slightly better test if the two sourcebuffers had differing buffered range(s). https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4750: // Add two ids, then remove one of the ids and verify that adding data only nit: s/data/init segment/ https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4759: EXPECT_MEDIA_LOG(WebMSimpleBlockDurationEstimated(30)); nit ditto of below. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4764: AppendSingleStreamCluster(kId1, kVideoTrackNum, "0K 30 60 90"); nit: this line should not be necessary to trigger the initdone. https://codereview.chromium.org/2226443002/diff/360001/media/filters/frame_pr... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/frame_pr... media/filters/frame_processor.cc:568: StreamParser::TrackId track_id = frame->track_id(); aside: WOOT! :) https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:36: const char* ToStr(MediaTrack::Type type) { nit: useful elsewhere? If so, move to some method in media_track.cc please. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:254: estimatedAudioSize = newDataSize / 16; nit: Assumption is overall, across all A+V tracks in this SB, 1/16 of the size of new data is audio (across all audio tracks). If one video track + 10 audio tracks, though, this breaks down rapidly. Can the assumption be more like: each audio track gets factor 1, each video segment gets factor 15. Sum the factors for denominator and numerator is per-stream factor to get per-stream multiplier of newDataSize for estimate. s/15/7/ or any other reasonable 1 track video : 1 track audio proportion. Also, this change now ignores any already-buffered proportion of audio/video, though. Probably fine. Hence just nit. Regardless, would be good to document the reasons for particular estimation choices here. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:287: Ranges<TimeDelta> MediaSourceState::GetBufferedRanges(TimeDelta duration, hmm. I think that TODO was wrong. SourceBuffer.buffered spec: For each track buffer managed by this SourceBuffer, run the following steps:... Note that it isn't scoped to just the currently enabled/selected. Note that the extended-for-MSE HTMLME.buffered *is* scoped to just the SB in activeSourceBuffers. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:466: void MediaSourceState::SetMemoryLimits(DemuxerStream::Type type, nit:ForTest here, in ChunkDemuxer, and in Pipeline? Or are there uses of this method outside of tests still (even with the cmdline flags now available)? https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:513: bool CheckBytestreamTrackIds( move to anon namespace at top of file https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:546: << "Error: duplicate bytestream track ids detected"; nit: s/Error: duplicate/Duplicate/ (ERROR already populates as error, and if it doesn't that's a distinct bug not to fix by putting it in this message here.) https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:549: MEDIA_LOG(ERROR, media_log_) << ToStr(track->type()) << " track " nit:s/ERROR/DEBUG/ so we can expose the last cached ERROR message eventually to devtools with at least some meaning still. Keep the ERROR type of log on l.545. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:562: ParseCodecString(expected_codecs, &expected_codecs_parsed, false); nit: Every init segment, we parse these? Can we instead parse once (including splitting out the codecs expectations by type, and validating none are kUnknown{Audio,Video}Codec when creating the StreamParser and curry the expectation there? Perhaps the cost difference is negligible, since we'd need to deepcopy the expectations (or maintain a map of "codecs found in this init segment") below, so doing the parse here is potentially more maintainable and not really perf-regressing. WDYT? https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:597: expected_acodecs.erase(it); This is a problem for the following test I suggest adding: addId(x, "audio/webm" "vorbis") appendInitSegment(x, two vorbis tracks) That's a valid init segment, but the second audio track would cause parse/decode error in l.591-596, above, with this current patch set. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:615: // If there is only one video track then bytestream id might change in nit:s/video/audio/ https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:651: expected_vcodecs.erase(it); ditto missing test and problem for multiple video tracks of same codec in same SourceBuffer. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:815: MEDIA_LOG(INFO, media_log_) << "Custom audio SourceBuffer size limit=" nit: audio *per-track* SourceBuffer size limit=.... https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:827: MEDIA_LOG(INFO, media_log_) << "Custom video SourceBuffer size limit=" nit ditto https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:148: }, "Test addSourceBuffer() throws QuotaExceededError after data has been appended."); nit: change this to be similar to the unit test: only fail after the last byte of the first init segment (for all SourceBuffers currently in mediaSource.sourceBuffers) is appended. Or add secondary tests here that do deeper coverage of this Chrome-specific behavior.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:496: if (s->enabled()) On 2016/09/13 21:03:13, wolenetz wrote: > Add TODO and crbug (if missing) w.r.t. mixing multiple enabled audio tracks. > Maybe "GetFirstEnabledStreamOfType()" vs "GetAllStreamsOfType()" (one for > current usage by renderer {with TODO+crbug to support audio mixing of multiple > enabled audio tracks}, and the latter for usage in UMA reporting). WDYT? We should probably keep the GetStream name for now, since that's part of the DemuxerStreamProvider interface and is overridden in 11 places and called in ~22 places according to code search. I agree that going forward we'll probably need to do some overhaul of the DemuxerStreamProvider interface so that it allows handling multiple demuxer streams of each type. I have some ideas around that, but it'll need to be a separate CL since that's going to be a pretty big change. Filed a bug and added a TODO to explain this. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:641: for (size_t i = 0; i < audio_streams_.size(); ++i) { On 2016/09/13 21:03:13, wolenetz wrote: > nit: CHECK that we actually found and moved all the streams for id into > removed_streams_. Not DCHECK, since a raw pointer in demuxer client might still > be used to deref something that's been deleted if not correctly moved into > removed_streams_. Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4457: // Audio stream had 8 buffers, video stream had 15. We told EvictCodedFrames On 2016/09/13 21:03:13, wolenetz wrote: > Why is this portion of this test dropped? This logic got broken when I temporarily switched to hard-coded estimates of the eviction sizes for each stream. But now I've restored the logic in EvictCodedFrames to use current stream buffered size to estimate the amount of data that belongs to the current stream in the muxed data chunk, and now this logic is valid again, so I've restored the test in the latest patchset. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:401: memcpy(buf, audio_track_entry->data(), audio_track_entry->data_size()); On 2016/09/13 21:03:13, wolenetz wrote: > aside: Why reorder this block here vs before "if (has_video)" previously? Some tests in this file verify that media log events come in the expected order by using testing::InSequence. Previously we would always report audio before video (since in the old OnNewConfigs we had the code for handling audio above the code for handling video). But with the new OnNewConfigs implementation the order of a/v reporting will match the actual stream order in the init segment/bitstream. And it just so happens that most of our test files have video stream #0 and audio stream #1. So I've changed the order here as well to match that. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:2538: ASSERT_EQ(AddId(video_id, HAS_VIDEO), ChunkDemuxer::kReachedIdLimit); On 2016/09/13 21:03:13, wolenetz wrote: > aside: good that we had this already :) Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3467: EXPECT_EQ(ChunkDemuxer::kOk, AddId(kSourceId, kMp2tMimeType, kMp2tCodecs)); On 2016/09/13 21:03:13, wolenetz wrote: > aside: nice elimination of that helper, above. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4736: EXPECT_MEDIA_LOG(SegmentMissingFrames("1")).Times(AnyNumber()); On 2016/09/13 21:03:13, wolenetz wrote: > Is this because the test media indeed has muxed segments that sometimes have > missing track media frames? I just want to make sure that's understood. Yes, if I remove this then the test fails due to unmatched media log message. I'm not sure why some media segments are missing in the files generated by ffmpeg. Perhaps we should use different set of codecs for this test? https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4741: CheckExpectedRanges(kId2, "{ [0,10007) }"); On 2016/09/13 21:03:13, wolenetz wrote: > nit: might be a slightly better test if the two sourcebuffers had differing > buffered range(s). Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4750: // Add two ids, then remove one of the ids and verify that adding data only On 2016/09/13 21:03:13, wolenetz wrote: > nit: s/data/init segment/ Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4759: EXPECT_MEDIA_LOG(WebMSimpleBlockDurationEstimated(30)); On 2016/09/13 21:03:13, wolenetz wrote: > nit ditto of below. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4764: AppendSingleStreamCluster(kId1, kVideoTrackNum, "0K 30 60 90"); On 2016/09/13 21:03:13, wolenetz wrote: > nit: this line should not be necessary to trigger the initdone. I agree this is not strictly necessary, so I can remove it, if you insist. But I think it's useful to get some extra test coverage by appending some data as well, since this will ensure that we exercise also the paths that handle data stream parsing after RemoveId. For example, this will ensure that there is still a track buffer for the remaining video id/stream, and that data parsing succeeds (or at least doesn't cause any crashes due to missing some internal structures that might have been accidentally removed by RemoveId). https://codereview.chromium.org/2226443002/diff/360001/media/filters/frame_pr... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/frame_pr... media/filters/frame_processor.cc:568: StreamParser::TrackId track_id = frame->track_id(); On 2016/09/13 21:03:13, wolenetz wrote: > aside: WOOT! :) Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:36: const char* ToStr(MediaTrack::Type type) { On 2016/09/13 21:03:14, wolenetz wrote: > nit: useful elsewhere? If so, move to some method in media_track.cc please. Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:254: estimatedAudioSize = newDataSize / 16; On 2016/09/13 21:03:14, wolenetz wrote: > nit: Assumption is overall, across all A+V tracks in this SB, 1/16 of the size > of new data is audio (across all audio tracks). If one video track + 10 audio > tracks, though, this breaks down rapidly. Can the assumption be more like: each > audio track gets factor 1, each video segment gets factor 15. Sum the factors > for denominator and numerator is per-stream factor to get per-stream multiplier > of newDataSize for estimate. s/15/7/ or any other reasonable 1 track video : 1 > track audio proportion. > Also, this change now ignores any already-buffered proportion of audio/video, > though. Probably fine. Hence just nit. > Regardless, would be good to document the reasons for particular estimation > choices here. Yeah, after pondering this a bit more, I think we can simplify this a bit. I have just realized that we can always use the proportion of the stream->GetBufferedSize to the current total buffered size for estimating how much data actually belongs to a given stream in the new data chunk. We don't need to handle cases when total buffered size is 0 or the current stream buffered size is 0, since in those cases there's no point in even trying to evict codec frames - there's nothing to evict. With this change I've also restored the previously removed part of the EvictCodedFrames test. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:287: Ranges<TimeDelta> MediaSourceState::GetBufferedRanges(TimeDelta duration, On 2016/09/13 21:03:14, wolenetz wrote: > hmm. I think that TODO was wrong. SourceBuffer.buffered spec: > For each track buffer managed by this SourceBuffer, run the following steps:... > > Note that it isn't scoped to just the currently enabled/selected. > > Note that the extended-for-MSE HTMLME.buffered *is* scoped to just the SB in > activeSourceBuffers. Ok, in that case we can just drop the check for enabled() and remove the TODO. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:466: void MediaSourceState::SetMemoryLimits(DemuxerStream::Type type, On 2016/09/13 21:03:14, wolenetz wrote: > nit:ForTest here, in ChunkDemuxer, and in Pipeline? Or are there uses of this > method outside of tests still (even with the cmdline flags now available)? Well, because of the cmdline flags I think we shouldn't call these ForTest. Since flags can be added anywhere, not necessarily in tests. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:513: bool CheckBytestreamTrackIds( On 2016/09/13 21:03:14, wolenetz wrote: > move to anon namespace at top of file Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:546: << "Error: duplicate bytestream track ids detected"; On 2016/09/13 21:03:13, wolenetz wrote: > nit: s/Error: duplicate/Duplicate/ (ERROR already populates as error, and if it > doesn't that's a distinct bug not to fix by putting it in this message here.) Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:549: MEDIA_LOG(ERROR, media_log_) << ToStr(track->type()) << " track " On 2016/09/13 21:03:13, wolenetz wrote: > nit:s/ERROR/DEBUG/ so we can expose the last cached ERROR message eventually to > devtools with at least some meaning still. Keep the ERROR type of log on l.545. Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:562: ParseCodecString(expected_codecs, &expected_codecs_parsed, false); On 2016/09/13 21:03:14, wolenetz wrote: > nit: Every init segment, we parse these? Can we instead parse once (including > splitting out the codecs expectations by type, and validating none are > kUnknown{Audio,Video}Codec when creating the StreamParser and curry the > expectation there? Perhaps the cost difference is negligible, since we'd need to > deepcopy the expectations (or maintain a map of "codecs found in this init > segment") below, so doing the parse here is potentially more maintainable and > not really perf-regressing. WDYT? Yeah, I don't think it's going to make a big difference, but it's easy enough to do, so why not? https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:597: expected_acodecs.erase(it); On 2016/09/13 21:03:13, wolenetz wrote: > This is a problem for the following test I suggest adding: > > addId(x, "audio/webm" "vorbis") > appendInitSegment(x, two vorbis tracks) > > That's a valid init segment, but the second audio track would cause parse/decode > error in l.591-596, above, with this current patch set. Wait, shouldn't the mime type be 'audio/webm; codecs=vorbis,vorbis' if there are actually two vorbis tracks? I tried creating DASH manifests from multi-track mp4 files using GPAC MP4Box and I got manifests that specified the same codec multiple times i.e. it looked like codecs="avc1.4D401F,avc1.4D401F". So I assumed the codecs parameter must have one value for each stream/track, and in fact I was wondering if we need to enforce also the order of those tracks/streams to match the codecs parameter (since that could simplify the logic here). I don't see this covered explicitly in the MSE spec. It simply says that the parameter of the addSourceBuffer is a mime type string, and does't go into details about how the codecs parameter should be interpreted. Can you please clarify this in the MSE spec? Also, unfortunately we can't create a test like you described yet, because our WebM parser currently doesn't support multiple audio/video tracks, it still has that 1 a + 1 v track limitation. And MP4 parser, which supports multiple tracks, cannot be used in tests due to MP4 using proprietary codecs. I'll leave a TODO to add such a test in the future, when WebM parser support multiple tracks. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:615: // If there is only one video track then bytestream id might change in On 2016/09/13 21:03:14, wolenetz wrote: > nit:s/video/audio/ Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:651: expected_vcodecs.erase(it); On 2016/09/13 21:03:13, wolenetz wrote: > ditto missing test and problem for multiple video tracks of same codec in same > SourceBuffer. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:815: MEDIA_LOG(INFO, media_log_) << "Custom audio SourceBuffer size limit=" On 2016/09/13 21:03:14, wolenetz wrote: > nit: audio *per-track* SourceBuffer size limit=.... Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:827: MEDIA_LOG(INFO, media_log_) << "Custom video SourceBuffer size limit=" On 2016/09/13 21:03:14, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:148: }, "Test addSourceBuffer() throws QuotaExceededError after data has been appended."); On 2016/09/13 21:03:14, wolenetz wrote: > nit: change this to be similar to the unit test: only fail after the last byte > of the first init segment (for all SourceBuffers currently in > mediaSource.sourceBuffers) is appended. Or add secondary tests here that do > deeper coverage of this Chrome-specific behavior. Wait. First, there's only one sourceBuffer here, the second one is never added due to data already being appended to the first one and thus ChunkDemuxer being in the INITIALIZED state. Second, I'm not sure how we can detect the "last byte of the init segment" here. Do we need to parse mediaData somehow?
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:496: if (s->enabled()) On 2016/09/14 18:15:24, servolk wrote: > On 2016/09/13 21:03:13, wolenetz wrote: > > Add TODO and crbug (if missing) w.r.t. mixing multiple enabled audio tracks. > > Maybe "GetFirstEnabledStreamOfType()" vs "GetAllStreamsOfType()" (one for > > current usage by renderer {with TODO+crbug to support audio mixing of multiple > > enabled audio tracks}, and the latter for usage in UMA reporting). WDYT? > > We should probably keep the GetStream name for now, since that's part of the > DemuxerStreamProvider interface and is overridden in 11 places and called in ~22 > places according to code search. I agree that going forward we'll probably need > to do some overhaul of the DemuxerStreamProvider interface so that it allows > handling multiple demuxer streams of each type. I have some ideas around that, > but it'll need to be a separate CL since that's going to be a pretty big change. > Filed a bug and added a TODO to explain this. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:401: memcpy(buf, audio_track_entry->data(), audio_track_entry->data_size()); On 2016/09/14 18:15:24, servolk wrote: > On 2016/09/13 21:03:13, wolenetz wrote: > > aside: Why reorder this block here vs before "if (has_video)" previously? > > Some tests in this file verify that media log events come in the expected order > by using testing::InSequence. Previously we would always report audio before > video (since in the old OnNewConfigs we had the code for handling audio above > the code for handling video). But with the new OnNewConfigs implementation the > order of a/v reporting will match the actual stream order in the init > segment/bitstream. And it just so happens that most of our test files have video > stream #0 and audio stream #1. So I've changed the order here as well to match > that. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4736: EXPECT_MEDIA_LOG(SegmentMissingFrames("1")).Times(AnyNumber()); On 2016/09/14 18:15:25, servolk wrote: > On 2016/09/13 21:03:13, wolenetz wrote: > > Is this because the test media indeed has muxed segments that sometimes have > > missing track media frames? I just want to make sure that's understood. > > Yes, if I remove this then the test fails due to unmatched media log message. > I'm not sure why some media segments are missing in the files generated by > ffmpeg. Perhaps we should use different set of codecs for this test? The binary files used by this test don't apply when I try to apply this patch locally, so I can't tell directly if the webm files are muxed with only audio or only video in some segments. Can you share these files with me directly please? https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4764: AppendSingleStreamCluster(kId1, kVideoTrackNum, "0K 30 60 90"); On 2016/09/14 18:15:25, servolk wrote: > On 2016/09/13 21:03:13, wolenetz wrote: > > nit: this line should not be necessary to trigger the initdone. > > I agree this is not strictly necessary, so I can remove it, if you insist. But I > think it's useful to get some extra test coverage by appending some data as > well, since this will ensure that we exercise also the paths that handle data > stream parsing after RemoveId. For example, this will ensure that there is still > a track buffer for the remaining video id/stream, and that data parsing succeeds > (or at least doesn't cause any crashes due to missing some internal structures > that might have been accidentally removed by RemoveId). I'm fine with keeping it. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:254: estimatedAudioSize = newDataSize / 16; On 2016/09/14 18:15:26, servolk wrote: > On 2016/09/13 21:03:14, wolenetz wrote: > > nit: Assumption is overall, across all A+V tracks in this SB, 1/16 of the size > > of new data is audio (across all audio tracks). If one video track + 10 audio > > tracks, though, this breaks down rapidly. Can the assumption be more like: > each > > audio track gets factor 1, each video segment gets factor 15. Sum the factors > > for denominator and numerator is per-stream factor to get per-stream > multiplier > > of newDataSize for estimate. s/15/7/ or any other reasonable 1 track video : 1 > > track audio proportion. > > Also, this change now ignores any already-buffered proportion of audio/video, > > though. Probably fine. Hence just nit. > > Regardless, would be good to document the reasons for particular estimation > > choices here. > > Yeah, after pondering this a bit more, I think we can simplify this a bit. I > have just realized that we can always use the proportion of the > stream->GetBufferedSize to the current total buffered size for estimating how > much data actually belongs to a given stream in the new data chunk. We don't > need to handle cases when total buffered size is 0 or the current stream > buffered size is 0, since in those cases there's no point in even trying to > evict codec frames - there's nothing to evict. With this change I've also > restored the previously removed part of the EvictCodedFrames test. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:466: void MediaSourceState::SetMemoryLimits(DemuxerStream::Type type, On 2016/09/14 18:15:26, servolk wrote: > On 2016/09/13 21:03:14, wolenetz wrote: > > nit:ForTest here, in ChunkDemuxer, and in Pipeline? Or are there uses of this > > method outside of tests still (even with the cmdline flags now available)? > > Well, because of the cmdline flags I think we shouldn't call these ForTest. > Since flags can be added anywhere, not necessarily in tests. Unless I'm missing something new in this CL, or cs.chromium.org isn't showing all call hierarchies, the latter shows only unit tests exercise this method. Is that different perhaps only in downstream repos? https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:562: ParseCodecString(expected_codecs, &expected_codecs_parsed, false); On 2016/09/14 18:15:25, servolk wrote: > On 2016/09/13 21:03:14, wolenetz wrote: > > nit: Every init segment, we parse these? Can we instead parse once (including > > splitting out the codecs expectations by type, and validating none are > > kUnknown{Audio,Video}Codec when creating the StreamParser and curry the > > expectation there? Perhaps the cost difference is negligible, since we'd need > to > > deepcopy the expectations (or maintain a map of "codecs found in this init > > segment") below, so doing the parse here is potentially more maintainable and > > not really perf-regressing. WDYT? > > Yeah, I don't think it's going to make a big difference, but it's easy enough to > do, so why not? Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:597: expected_acodecs.erase(it); On 2016/09/14 18:15:26, servolk wrote: > On 2016/09/13 21:03:13, wolenetz wrote: > > This is a problem for the following test I suggest adding: > > > > addId(x, "audio/webm" "vorbis") > > appendInitSegment(x, two vorbis tracks) > > > > That's a valid init segment, but the second audio track would cause > parse/decode > > error in l.591-596, above, with this current patch set. > > Wait, shouldn't the mime type be 'audio/webm; codecs=vorbis,vorbis' if there are > actually two vorbis tracks? I tried creating DASH manifests from multi-track mp4 > files using GPAC MP4Box and I got manifests that specified the same codec > multiple times i.e. it looked like codecs="avc1.4D401F,avc1.4D401F". So I > assumed the codecs parameter must have one value for each stream/track, and in > fact I was wondering if we need to enforce also the order of those > tracks/streams to match the codecs parameter (since that could simplify the > logic here). > I don't see this covered explicitly in the MSE spec. It simply says that the > parameter of the addSourceBuffer is a mime type string, and does't go into > details about how the codecs parameter should be interpreted. Can you please > clarify this in the MSE spec? > > Also, unfortunately we can't create a test like you described yet, because our > WebM parser currently doesn't support multiple audio/video tracks, it still has > that 1 a + 1 v track limitation. And MP4 parser, which supports multiple tracks, > cannot be used in tests due to MP4 using proprietary codecs. I'll leave a TODO > to add such a test in the future, when WebM parser support multiple tracks. I've filed spec bug (MSE vNext) https://github.com/w3c/media-source/issues/161 to get clarity. Unfortunately other UAs don't always require codec strings and may instead sniff them out during initialization segment received processing; isTypeSupported('video/mp4') on Edge, for instance, is true and addSourceBUffer() of same might work (I'm unsure there). Following EME's route of strict matching, Chrome MSE is more strict, and MSE vNext bug (https://github.com/w3c/media-source/issues/137) tracks making the spec more strict too. However, such strictness (#137) does not necessarily imply that multiple tracks of same codec type each have distinct entries in the codec string. I propose we start strict here (like you have currently), and only relax as necessary if web authors discover issue here only in Chrome while #161 remains open in the spec. Please add a "TODO(wolenetz): Update codec string strictness, if necessary, once spec issue https://github.com/w3c/media-source/issues/161 is resolved." https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:148: }, "Test addSourceBuffer() throws QuotaExceededError after data has been appended."); On 2016/09/14 18:15:27, servolk wrote: > On 2016/09/13 21:03:14, wolenetz wrote: > > nit: change this to be similar to the unit test: only fail after the last byte > > of the first init segment (for all SourceBuffers currently in > > mediaSource.sourceBuffers) is appended. Or add secondary tests here that do > > deeper coverage of this Chrome-specific behavior. > > Wait. First, there's only one sourceBuffer here, the second one is never added > due to data already being appended to the first one and thus ChunkDemuxer being > in the INITIALIZED state. > Second, I'm not sure how we can detect the "last byte of the init segment" here. > Do we need to parse mediaData somehow? I'm being a little pedantic here, but tests kind of are :) segmentInfo describes mediaData, so something like the following can extract just the init segment: // var initSegment = mediaData.subarray(segmentInfo.init.offset, segmentInfo.init.offset + segmentInfo.init.size); Then, append all but the last N bytes of initSegment to the only sourceBuffer currently added. At this point: a) adding a second sourcebuffer should succeed b) then removing that second sourcebuffer should succeed. c) then append the last N bytes of the init segment to the first sourcebuffer d) then adding another sourcebuffer should fail. Given this sequence of operations is a bit complex versus this patch set's test, I suggest adding a secondary test that drills down into a->d. Note that ideally N == 1, but there could be mistakes in the segmentInfo or the muxed media might contain optional pieces within that init segment range. I'd try N==1 for both webm and mp4 first, and if that works, just keep to 1. https://codereview.chromium.org/2226443002/diff/400001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2226443002/diff/400001/media/base/media_track... media/base/media_track.h:64: const char* TrackTypeToStr(MediaTrack::Type type); nit: MEDIA_EXPORT for usage in component build + potentially tests? https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:655: for (size_t i = 0; i < video_streams_.size(); ++i) { s/i </!stream_found && i </ https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4808: // supports multiple tracks. crbug.com/646900 aside: include multi-track-of-same-type coded frame eviction in such new test(s) -- I've added such a note in that crbug, so no further code comment here is necessary. https://codereview.chromium.org/2226443002/diff/400001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/400001/media/filters/media_so... media/filters/media_source_state.cc:785: void MediaSourceState::SetStreamMemoryLimits() { Note: This method is *not* used solely by tests (vs SetMemoryLimits(), above).
One further note: https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:148: }, "Test addSourceBuffer() throws QuotaExceededError after data has been appended."); On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:27, servolk wrote: > > On 2016/09/13 21:03:14, wolenetz wrote: > > > nit: change this to be similar to the unit test: only fail after the last > byte > > > of the first init segment (for all SourceBuffers currently in > > > mediaSource.sourceBuffers) is appended. Or add secondary tests here that do > > > deeper coverage of this Chrome-specific behavior. > > > > Wait. First, there's only one sourceBuffer here, the second one is never added > > due to data already being appended to the first one and thus ChunkDemuxer > being > > in the INITIALIZED state. > > Second, I'm not sure how we can detect the "last byte of the init segment" > here. > > Do we need to parse mediaData somehow? > > I'm being a little pedantic here, but tests kind of are :) > > segmentInfo describes mediaData, so something like the following can extract > just the init segment: > // var initSegment = mediaData.subarray(segmentInfo.init.offset, > segmentInfo.init.offset + segmentInfo.init.size); > > Then, append all but the last N bytes of initSegment to the only sourceBuffer > currently added. At this point: > a) adding a second sourcebuffer should succeed > b) then removing that second sourcebuffer should succeed. > c) then append the last N bytes of the init segment to the first sourcebuffer > d) then adding another sourcebuffer should fail. > > Given this sequence of operations is a bit complex versus this patch set's test, > I suggest adding a secondary test that drills down into a->d. > > Note that ideally N == 1, but there could be mistakes in the segmentInfo or the > muxed media might contain optional pieces within that init segment range. I'd > try N==1 for both webm and mp4 first, and if that works, just keep to 1. mediasource-util.js includes a helper for this. Use it like so: var initSegment = extractSegmentData(mediaData, segmentInfo.init);
One further note: https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:148: }, "Test addSourceBuffer() throws QuotaExceededError after data has been appended."); On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:27, servolk wrote: > > On 2016/09/13 21:03:14, wolenetz wrote: > > > nit: change this to be similar to the unit test: only fail after the last > byte > > > of the first init segment (for all SourceBuffers currently in > > > mediaSource.sourceBuffers) is appended. Or add secondary tests here that do > > > deeper coverage of this Chrome-specific behavior. > > > > Wait. First, there's only one sourceBuffer here, the second one is never added > > due to data already being appended to the first one and thus ChunkDemuxer > being > > in the INITIALIZED state. > > Second, I'm not sure how we can detect the "last byte of the init segment" > here. > > Do we need to parse mediaData somehow? > > I'm being a little pedantic here, but tests kind of are :) > > segmentInfo describes mediaData, so something like the following can extract > just the init segment: > // var initSegment = mediaData.subarray(segmentInfo.init.offset, > segmentInfo.init.offset + segmentInfo.init.size); > > Then, append all but the last N bytes of initSegment to the only sourceBuffer > currently added. At this point: > a) adding a second sourcebuffer should succeed > b) then removing that second sourcebuffer should succeed. > c) then append the last N bytes of the init segment to the first sourcebuffer > d) then adding another sourcebuffer should fail. > > Given this sequence of operations is a bit complex versus this patch set's test, > I suggest adding a secondary test that drills down into a->d. > > Note that ideally N == 1, but there could be mistakes in the segmentInfo or the > muxed media might contain optional pieces within that init segment range. I'd > try N==1 for both webm and mp4 first, and if that works, just keep to 1. mediasource-util.js includes a helper for this. Use it like so: var initSegment = extractSegmentData(mediaData, segmentInfo.init);
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4736: EXPECT_MEDIA_LOG(SegmentMissingFrames("1")).Times(AnyNumber()); On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:25, servolk wrote: > > On 2016/09/13 21:03:13, wolenetz wrote: > > > Is this because the test media indeed has muxed segments that sometimes have > > > missing track media frames? I just want to make sure that's understood. > > > > Yes, if I remove this then the test fails due to unmatched media log message. > > I'm not sure why some media segments are missing in the files generated by > > ffmpeg. Perhaps we should use different set of codecs for this test? > > The binary files used by this test don't apply when I try to apply this patch > locally, so I can't tell directly if the webm files are muxed with only audio or > only video in some segments. Can you share these files with me directly please? Done. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4764: AppendSingleStreamCluster(kId1, kVideoTrackNum, "0K 30 60 90"); On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:25, servolk wrote: > > On 2016/09/13 21:03:13, wolenetz wrote: > > > nit: this line should not be necessary to trigger the initdone. > > > > I agree this is not strictly necessary, so I can remove it, if you insist. But > I > > think it's useful to get some extra test coverage by appending some data as > > well, since this will ensure that we exercise also the paths that handle data > > stream parsing after RemoveId. For example, this will ensure that there is > still > > a track buffer for the remaining video id/stream, and that data parsing > succeeds > > (or at least doesn't cause any crashes due to missing some internal structures > > that might have been accidentally removed by RemoveId). > > I'm fine with keeping it. Acknowledged. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:466: void MediaSourceState::SetMemoryLimits(DemuxerStream::Type type, On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:26, servolk wrote: > > On 2016/09/13 21:03:14, wolenetz wrote: > > > nit:ForTest here, in ChunkDemuxer, and in Pipeline? Or are there uses of > this > > > method outside of tests still (even with the cmdline flags now available)? > > > > Well, because of the cmdline flags I think we shouldn't call these ForTest. > > Since flags can be added anywhere, not necessarily in tests. > > Unless I'm missing something new in this CL, or http://cs.chromium.org isn't showing > all call hierarchies, the latter shows only unit tests exercise this method. Is > that different perhaps only in downstream repos? Ah, ok, you are right, I've got confused, thought we were talking about MSS::SetLimits. Renamed. https://codereview.chromium.org/2226443002/diff/360001/media/filters/media_so... media/filters/media_source_state.cc:597: expected_acodecs.erase(it); On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:26, servolk wrote: > > On 2016/09/13 21:03:13, wolenetz wrote: > > > This is a problem for the following test I suggest adding: > > > > > > addId(x, "audio/webm" "vorbis") > > > appendInitSegment(x, two vorbis tracks) > > > > > > That's a valid init segment, but the second audio track would cause > > parse/decode > > > error in l.591-596, above, with this current patch set. > > > > Wait, shouldn't the mime type be 'audio/webm; codecs=vorbis,vorbis' if there > are > > actually two vorbis tracks? I tried creating DASH manifests from multi-track > mp4 > > files using GPAC MP4Box and I got manifests that specified the same codec > > multiple times i.e. it looked like codecs="avc1.4D401F,avc1.4D401F". So I > > assumed the codecs parameter must have one value for each stream/track, and in > > fact I was wondering if we need to enforce also the order of those > > tracks/streams to match the codecs parameter (since that could simplify the > > logic here). > > I don't see this covered explicitly in the MSE spec. It simply says that the > > parameter of the addSourceBuffer is a mime type string, and does't go into > > details about how the codecs parameter should be interpreted. Can you please > > clarify this in the MSE spec? > > > > Also, unfortunately we can't create a test like you described yet, because our > > WebM parser currently doesn't support multiple audio/video tracks, it still > has > > that 1 a + 1 v track limitation. And MP4 parser, which supports multiple > tracks, > > cannot be used in tests due to MP4 using proprietary codecs. I'll leave a TODO > > to add such a test in the future, when WebM parser support multiple tracks. > > I've filed spec bug (MSE vNext) https://github.com/w3c/media-source/issues/161 > to get clarity. Unfortunately other UAs don't always require codec strings and > may instead sniff them out during initialization segment received processing; > isTypeSupported('video/mp4') on Edge, for instance, is true and > addSourceBUffer() of same might work (I'm unsure there). Following EME's route > of strict matching, Chrome MSE is more strict, and MSE vNext bug > (https://github.com/w3c/media-source/issues/137) tracks making the spec more > strict too. However, such strictness (#137) does not necessarily imply that > multiple tracks of same codec type each have distinct entries in the codec > string. I propose we start strict here (like you have currently), and only relax > as necessary if web authors discover issue here only in Chrome while #161 > remains open in the spec. > > Please add a "TODO(wolenetz): Update codec string strictness, if necessary, once > spec issue https://github.com/w3c/media-source/issues/161 is resolved." Done. https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:148: }, "Test addSourceBuffer() throws QuotaExceededError after data has been appended."); On 2016/09/14 23:31:21, wolenetz wrote: > On 2016/09/14 18:15:27, servolk wrote: > > On 2016/09/13 21:03:14, wolenetz wrote: > > > nit: change this to be similar to the unit test: only fail after the last > byte > > > of the first init segment (for all SourceBuffers currently in > > > mediaSource.sourceBuffers) is appended. Or add secondary tests here that do > > > deeper coverage of this Chrome-specific behavior. > > > > Wait. First, there's only one sourceBuffer here, the second one is never added > > due to data already being appended to the first one and thus ChunkDemuxer > being > > in the INITIALIZED state. > > Second, I'm not sure how we can detect the "last byte of the init segment" > here. > > Do we need to parse mediaData somehow? > > I'm being a little pedantic here, but tests kind of are :) > > segmentInfo describes mediaData, so something like the following can extract > just the init segment: > // var initSegment = mediaData.subarray(segmentInfo.init.offset, > segmentInfo.init.offset + segmentInfo.init.size); > > Then, append all but the last N bytes of initSegment to the only sourceBuffer > currently added. At this point: > a) adding a second sourcebuffer should succeed > b) then removing that second sourcebuffer should succeed. > c) then append the last N bytes of the init segment to the first sourcebuffer > d) then adding another sourcebuffer should fail. > > Given this sequence of operations is a bit complex versus this patch set's test, > I suggest adding a secondary test that drills down into a->d. > > Note that ideally N == 1, but there could be mistakes in the segmentInfo or the > muxed media might contain optional pieces within that init segment range. I'd > try N==1 for both webm and mp4 first, and if that works, just keep to 1. Ah, ok, I see what you mean now. Done. I guess we could append just half of the init segment if we wanted to avoid any potential issues with init segment not being defined precisely in the segmentInfo, but at least for now even N-1 seems to be working, so I kept it. https://codereview.chromium.org/2226443002/diff/400001/media/base/media_track.h File media/base/media_track.h (right): https://codereview.chromium.org/2226443002/diff/400001/media/base/media_track... media/base/media_track.h:64: const char* TrackTypeToStr(MediaTrack::Type type); On 2016/09/14 23:31:21, wolenetz wrote: > nit: MEDIA_EXPORT for usage in component build + potentially tests? Done. https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:655: for (size_t i = 0; i < video_streams_.size(); ++i) { On 2016/09/14 23:31:21, wolenetz wrote: > s/i </!stream_found && i </ Done (only slightly differently to preserve the canonical for loop form for better readability). https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4808: // supports multiple tracks. crbug.com/646900 On 2016/09/14 23:31:21, wolenetz wrote: > aside: include multi-track-of-same-type coded frame eviction in such new test(s) > -- I've added such a note in that crbug, so no further code comment here is > necessary. Acknowledged. https://codereview.chromium.org/2226443002/diff/400001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/400001/media/filters/media_so... media/filters/media_source_state.cc:785: void MediaSourceState::SetStreamMemoryLimits() { On 2016/09/14 23:31:21, wolenetz wrote: > Note: This method is *not* used solely by tests (vs SetMemoryLimits(), above). Acknowledged.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Aside from nits, almost l*tm except there are some problems in last few patch
sets on the *_chromium_rel bots for the append{buffer,stream} quota exceeded
tessts (and I suspect same will occur on current patch set once those bots
complete CQ dry run).
https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de...
File media/filters/chunk_demuxer_unittest.cc (right):
https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de...
media/filters/chunk_demuxer_unittest.cc:4736:
EXPECT_MEDIA_LOG(SegmentMissingFrames("1")).Times(AnyNumber());
On 2016/09/15 00:18:32, servolk wrote:
> On 2016/09/14 23:31:21, wolenetz wrote:
> > On 2016/09/14 18:15:25, servolk wrote:
> > > On 2016/09/13 21:03:13, wolenetz wrote:
> > > > Is this because the test media indeed has muxed segments that sometimes
> have
> > > > missing track media frames? I just want to make sure that's understood.
> > >
> > > Yes, if I remove this then the test fails due to unmatched media log
> message.
> > > I'm not sure why some media segments are missing in the files generated by
> > > ffmpeg. Perhaps we should use different set of codecs for this test?
> >
> > The binary files used by this test don't apply when I try to apply this
patch
> > locally, so I can't tell directly if the webm files are muxed with only
audio
> or
> > only video in some segments. Can you share these files with me directly
> please?
>
> Done.
Thank you. The files look ok. Specifically, "red-a500hz.webm" has a trailing
audio-only media segment that contains audio coded frames included in the last
video frame's presentation interval in the previous cluster. I don't know why
ffmpeg chose to partition the clusters this way, but that media log is
warranted. green-a300hz.webm has no such trailing single-track cluster, nor any
media log warning.
red-a500hz.webm shows the following in chrome://media-internals:
Media segment did not contain any video coded frames, mismatching initialization
segment. Therefore, MSE coded frame processing may not interoperably detect
discontinuities in appended media.
So, s/AnyNumber()/1/ and we can resolve this comment :)
https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):
https://codereview.chromium.org/2226443002/diff/400001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:655: for (size_t i = 0; i <
video_streams_.size(); ++i) {
On 2016/09/15 00:18:32, servolk wrote:
> On 2016/09/14 23:31:21, wolenetz wrote:
> > s/i </!stream_found && i </
>
> Done (only slightly differently to preserve the canonical for loop form for
> better readability).
Acknowledged.
https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html
(right):
https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:145:
"addSourceBuffer must throw an exception if the MediaSource has already got some
data");
nit: s/has already.../has already received init segments for all sourcebuffers
added at the time
--or instead--
s/.../if the media element has already reached HAVE_METADATA/ (we test this
better in the case below, so I kind of prefer the first alternative in this
comment for this test, and the second alternative for the test, below)
Perhaps similarly adjust the test case descriptions here and below.
I don't have any terribly strong feeling about this nit :)
https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:167:
test.waitForExpectedEvents(function()
nit: This wait doesn't need to nest inside the previous wait. The utils take
care of unwinding this correctly.
https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:169:
// MediaSource is fully initialized now, so adding new SourceBuffers is no
longer possible.
nit: assert_equals(mediaElement.readyState, HTMLMediaElement.HAVE_METADATA);
https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:172:
"addSourceBuffer must throw an exception if the MediaSource has already got some
data");
nit ditto of previous test.
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/v2/patch-status/codereview.chromium.or...
All done and I believe the test failures in patchset #21 were random flakes, since patchset #22 didn't have them, and earlier patchsets also didn't indicate any problems there. We'll see what happens with the latest patchset. https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2226443002/diff/360001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4736: EXPECT_MEDIA_LOG(SegmentMissingFrames("1")).Times(AnyNumber()); On 2016/09/15 01:00:08, wolenetz wrote: > On 2016/09/15 00:18:32, servolk wrote: > > On 2016/09/14 23:31:21, wolenetz wrote: > > > On 2016/09/14 18:15:25, servolk wrote: > > > > On 2016/09/13 21:03:13, wolenetz wrote: > > > > > Is this because the test media indeed has muxed segments that sometimes > > have > > > > > missing track media frames? I just want to make sure that's understood. > > > > > > > > Yes, if I remove this then the test fails due to unmatched media log > > message. > > > > I'm not sure why some media segments are missing in the files generated by > > > > ffmpeg. Perhaps we should use different set of codecs for this test? > > > > > > The binary files used by this test don't apply when I try to apply this > patch > > > locally, so I can't tell directly if the webm files are muxed with only > audio > > or > > > only video in some segments. Can you share these files with me directly > > please? > > > > Done. > > Thank you. The files look ok. Specifically, "red-a500hz.webm" has a trailing > audio-only media segment that contains audio coded frames included in the last > video frame's presentation interval in the previous cluster. I don't know why > ffmpeg chose to partition the clusters this way, but that media log is > warranted. green-a300hz.webm has no such trailing single-track cluster, nor any > media log warning. > > red-a500hz.webm shows the following in chrome://media-internals: > > Media segment did not contain any video coded frames, mismatching initialization > segment. Therefore, MSE coded frame processing may not interoperably detect > discontinuities in appended media. > > So, s/AnyNumber()/1/ and we can resolve this comment :) Done. https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:145: "addSourceBuffer must throw an exception if the MediaSource has already got some data"); On 2016/09/15 01:00:08, wolenetz wrote: > nit: s/has already.../has already received init segments for all sourcebuffers > added at the time > --or instead-- > s/.../if the media element has already reached HAVE_METADATA/ (we test this > better in the case below, so I kind of prefer the first alternative in this > comment for this test, and the second alternative for the test, below) > > Perhaps similarly adjust the test case descriptions here and below. > > I don't have any terribly strong feeling about this nit :) Done. https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:167: test.waitForExpectedEvents(function() On 2016/09/15 01:00:08, wolenetz wrote: > nit: This wait doesn't need to nest inside the previous wait. The utils take > care of unwinding this correctly. Done. https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:169: // MediaSource is fully initialized now, so adding new SourceBuffers is no longer possible. On 2016/09/15 01:00:08, wolenetz wrote: > nit: assert_equals(mediaElement.readyState, HTMLMediaElement.HAVE_METADATA); Done. https://codereview.chromium.org/2226443002/diff/420001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:172: "addSourceBuffer must throw an exception if the MediaSource has already got some data"); On 2016/09/15 01:00:08, wolenetz wrote: > nit ditto of previous test. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
LGTM % patch set 22 is having trouble with the *quota-exceeded-* tests again (pass before patchset, fail after, on at least win_chromium_rel_ng CQ dry run try bot. This amount of likely regression (or heavy flakiness) in those tests, specifically for tests related to coded frame eviction related changes in this CL, needs to be more stable before this lands. Please investigate.
On 2016/09/15 18:04:50, wolenetz wrote: > LGTM % patch set 22 is having trouble with the *quota-exceeded-* tests again > (pass before patchset, fail after, on at least win_chromium_rel_ng CQ dry run > try bot. > > This amount of likely regression (or heavy flakiness) in those tests, > specifically for tests related to coded frame eviction related changes in this > CL, needs to be more stable before this lands. Please investigate. s/22/23/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Please also await chcunningham@'s review of this.
Nice change. Kudos to Matt for doing the heavy lifting in this CR. https://codereview.chromium.org/2226443002/diff/440001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:725: selected_stream = track_id_to_demux_stream_map_[track_ids[0]]; DCHECK(track_ids.size() == 1)? Also, I forget, why is this a vector and not just a single track id? https://codereview.chromium.org/2226443002/diff/440001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:733: if (stream->type() == DemuxerStream::VIDEO && Do you need to check the type is video? Could maybe be a DCHECK? You might also add a guard to only do this for already enabled streams. That way your DVLOG is more useful https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:534: DVLOG(1) << __func__ << " expected_codecs=" << expected_codecs Do you need to pass in expected codecs here? You've already parsed them in init... not seeing any usage below. https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:592: if (audio_streams_.size() > 1) { I think this core part of your change (adding multiple tracks) should have some test coverage. This might be a good time to start the media_source_state_unittest file. Definitely not asking you to cover all of the existing behavior, but would love some tests to exercise the various paths in this method. https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:834: << it.first << ", mismatching initialization segment. Therefore, MSE" I don't follow the "mismatching initialization segment" parts of these warnings. I get that in general we want each track to get some data for each segment... https://codereview.chromium.org/2226443002/diff/440001/media/filters/stream_p... File media/filters/stream_parser_factory.h (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/stream_p... media/filters/stream_parser_factory.h:31: // Returns NULL otherwise. The values of |has_audio| and |has_video| are Nit: remove mentions of has_audio/video here
https://codereview.chromium.org/2226443002/diff/440001/media/filters/chunk_de... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:725: selected_stream = track_id_to_demux_stream_map_[track_ids[0]]; On 2016/09/16 00:05:06, chcunningham wrote: > DCHECK(track_ids.size() == 1)? > > Also, I forget, why is this a vector and not just a single track id? This is a vector because the track id might be absent (which is represented by empty vector). I guess we could replace this with base::Optional, but I'd prefer to do that in a separate change. https://codereview.chromium.org/2226443002/diff/440001/media/filters/chunk_de... media/filters/chunk_demuxer.cc:733: if (stream->type() == DemuxerStream::VIDEO && On 2016/09/16 00:05:05, chcunningham wrote: > Do you need to check the type is video? Could maybe be a DCHECK? > > You might also add a guard to only do this for already enabled streams. That way > your DVLOG is more useful In earlier patchsets I was using a common demuxer_streams_ collection that contained both audio and video stream, which made this necessary, but later I've split that into separate audio_/video_streams_ collection. So we can replace this with a DCHECK now. https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:534: DVLOG(1) << __func__ << " expected_codecs=" << expected_codecs On 2016/09/16 00:05:06, chcunningham wrote: > Do you need to pass in expected codecs here? You've already parsed them in > init... not seeing any usage below. Yes, expected codecs is no longer used here, except for the log. But I think it's worth it, since it makes the log easier to read - in case of failure you'll be able to see both the expected set of codecs and the actual codecs that we've got in the bitstream. This is especially important for Chromecast, where log space is limited, and so if we logged the expected codecs only at the beginning of playback and later got this failure, the expected codecs might be pushed out of the log by then. https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:592: if (audio_streams_.size() > 1) { On 2016/09/16 00:05:06, chcunningham wrote: > I think this core part of your change (adding multiple tracks) should have some > test coverage. > > This might be a good time to start the media_source_state_unittest file. > Definitely not asking you to cover all of the existing behavior, but would love > some tests to exercise the various paths in this method. > I've added some new tests (including ones where we have multiple tracks) in the chunk_demuxer_unittest.cc. As I've also explained there one issue that is holding back adding more tests is the fact that for now WebM parser still supports only 1 track of each type (see earlier discussion and TODO in chunk_demuxer_unittest.cc). Oh, and btw, the existing tests already provide some decent coverage for this. E.g. I have added a ChunkDemuxer::AudioVideoTrackIdsChange test in some previous CL that verifies that we properly handle the cases when bytestream track ids change in subsequent segments (covering lines 597-604 below), see https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?... https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:834: << it.first << ", mismatching initialization segment. Therefore, MSE" On 2016/09/16 00:05:06, chcunningham wrote: > I don't follow the "mismatching initialization segment" parts of these warnings. > I get that in general we want each track to get some data for each segment... This was added by Matt. IIUC the logic here is that if init segment has specified N tracks, then we expect each appended media segment to have at least one frame of data for each of those N tracks. https://codereview.chromium.org/2226443002/diff/440001/media/filters/stream_p... File media/filters/stream_parser_factory.h (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/stream_p... media/filters/stream_parser_factory.h:31: // Returns NULL otherwise. The values of |has_audio| and |has_video| are On 2016/09/16 00:05:06, chcunningham wrote: > Nit: remove mentions of has_audio/video 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/v2/patch-status/codereview.chromium.or...
On 2016/09/15 18:04:50, wolenetz wrote: > LGTM % patch set 22 is having trouble with the *quota-exceeded-* tests again > (pass before patchset, fail after, on at least win_chromium_rel_ng CQ dry run > try bot. > > This amount of likely regression (or heavy flakiness) in those tests, > specifically for tests related to coded frame eviction related changes in this > CL, needs to be more stable before this lands. Please investigate. Indeed something is wrong here, after seeing the results of patchset #23 I agree that it's more than just flakiness. But these tests always pass when ran locally on my workstation and I find it curious that failures were only see on Windows platform. Don't have a good explanation for this yet. I'll look into it further.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
chcunningham@google.com changed reviewers: + chcunningham@google.com
https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:592: if (audio_streams_.size() > 1) { Right, I see your todo. I think you can side step the webm issue by writing tests for media source state. As the logic in this class grows I don't think forever testing it via chunk demuxer is the best solution. https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:834: << it.first << ", mismatching initialization segment. Therefore, MSE" On 2016/09/16 00:34:31, servolk wrote: > On 2016/09/16 00:05:06, chcunningham wrote: > > I don't follow the "mismatching initialization segment" parts of these > warnings. > > I get that in general we want each track to get some data for each segment... > > This was added by Matt. IIUC the logic here is that if init segment has > specified N tracks, then we expect each appended media segment to have at least > one frame of data for each of those N tracks. Acknowledged.
https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2226443002/diff/440001/media/filters/media_so... media/filters/media_source_state.cc:592: if (audio_streams_.size() > 1) { On 2016/09/16 16:49:19, chcunningham1 wrote: > Right, I see your todo. I think you can side step the webm issue by writing > tests for media source state. As the logic in this class grows I don't think > forever testing it via chunk demuxer is the best solution. I think testing via chunk demuxer _is_ the best solution (at least for now). For example, there is no explicit API in the MediaSourceState to add tracks. Tracks are created according to what's read from the bitstream. So I don't see how having a separate MediaSourceState would help with testing this particular scenario. Or perhaps I misunderstand your proposal? How can we create a test for MSS that would create multiple tracks, other than generating some bitstream with multiple tracks?
My first thought is to use a mock stream parser. Then the parser can claim as many tracks and init segments as you like. To me its a bit of a mess that chunk_demuxer tests don't do this already (building real webm clusters and such).
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/v2/patch-status/codereview.chromium.or...
On 2016/09/16 18:47:55, chcunningham1 wrote: > My first thought is to use a mock stream parser. Then the parser can claim as > many tracks and init segments as you like. > > To me its a bit of a mess that chunk_demuxer tests don't do this already > (building real webm clusters and such). Ok, I've added a MockStreamParser and added several new MediaSourceState unit tests to cover main code flows in the MediaSourceState::OnNewConfigs method changed in this CL. I've added those new tests in a separate CL https://codereview.chromium.org/2352253002/, since I needed to move some test helpers around and do some test code refactoring and I'd prefer to keep this CL focused on the media track support. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM!
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2226443002/#ps500001 (title: "Fixed integer overflow")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=487288 ========== to ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=487288 ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=487288 ========== to ========== Support multiple media tracks in MSE / ChunkDemuxer This will also allow multiple audio/video SourceBuffers for a single MediaSource. Note that for now we'll still only select/play the first audio and video track by default. Switching between tracks will require further work in media renderers. A few notes: Most of the changes are straightforward, except media_source_state.* In order to allow multiple tracks in a single SourceBuffer, we'll now pass the codecs parameter from WSBI further down to ChunkDemuxer and MediaSourceState, replacing the old has_audio/has_video flags. The new MSS::OnNewConfigs structure is much closer to the MSE init segment handling algorithm, with essentially the same logic for handling new/existing track ids and such. BUG=487288 Committed: https://crrev.com/d3acf22f7d91ad262a07075848fad13b94d15226 Cr-Commit-Position: refs/heads/master@{#420113} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/d3acf22f7d91ad262a07075848fad13b94d15226 Cr-Commit-Position: refs/heads/master@{#420113} |
