Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361303003:
Add AppendMuxedCluster() method to make test intentions clearer. (Closed)
|
Created:
6 years, 5 months ago by acolwell GONE FROM CHROMIUM Modified:
6 years, 5 months ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd AppendMuxedCluster() method to make test intentions clearer.
Several tests were intended to verify muxed content behavior, but
are not actually using muxed clusters. This is a precursor change
that makes the use of muxed clusters more explicit. There are
only slight changes to some of the tests to make them properly reflect
the original intent. A followup change will actually create proper
muxed clusters and make any necessary updates to the test expectations.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281693
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address CR comments." #Patch Set 3 : Add MuxedStreamInfo constructors #
Total comments: 5
Patch Set 4 : Address CR comments #Messages
Total messages: 9 (0 generated)
Looking pretty good. Lack of testing muxed content processing by our demuxer has been bugging me - thanks for sending this CL. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:441: const char* description; nit: s/description/cluster_description/. Also maybe relocate or duplicate |cluster_description| parameter comment from AppendSingleStreamCluster()? https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:464: AppendSingleStreamCluster(kSourceId, nit: Include a TODO for the follow-up CL to actually append just 1 muxed cluster here? At your discretion based on your expectation of sending follow-up CL. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2423: { kTextTrackNum, "100K 200K"}); nit: s/"}/" }/ https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2428: // Remove the buffered ranges. nit: Fix related to my comment below, since the text track may still contain non-empty track buffer ranges. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2431: CheckExpectedRanges("{ }"); nit (maybe best left to a separate CL): If MarkEndOfStream(PIPELINE_OK) were done after this line, would a subsequent CheckExpectedRanges() need to expect just "{ }", even though there is still a not-removed text track buffer [200, 300) and (possibly if supported) a partially remaining text track buffer [146, 200)? (SourceBuffer.buffered() step 4.2 cannot change end time on last range in a completely empty track's track ranges, so resulting intersection ranges even in "ended" readyState for this scenario should be empty, right? EndOfStreamRangeChanges and GetBufferedRanges_EndOfStream don't test this specific scenario where only text track buffer(s) remain after remove() + endOfStream(). Further, re-appending some audio+video like just 0K for each + endOfStream() should then expose the still-not-removed text buffer(s) in CheckExpectedRanges(). ISTM like a good scenario to test. Spec reference: https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.... https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2442: { kVideoTrackNum, "0K 33"}); nit: s/"}/" }/ https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:3492: CheckExpectedBuffers(video_stream, "180 210"); Hmm. Perhaps I misunderstand, but now that we have appended a new keyframe at time 120 that frame at time 180 depends on, should we really expect 180 to be next at this point? If so, which one is it (the first one appended, or the second?) To me, this looks wrong or at least confusing and in need of a clarifying comment.
On 2014/07/07 21:31:45, wolenetz wrote: > Looking pretty good. Lack of testing muxed content processing by our demuxer has > been bugging me - thanks for sending this CL. > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:441: const char* description; > nit: s/description/cluster_description/. Also maybe relocate or duplicate > |cluster_description| parameter comment from AppendSingleStreamCluster()? > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:464: > AppendSingleStreamCluster(kSourceId, > nit: Include a TODO for the follow-up CL to actually append just 1 muxed cluster > here? At your discretion based on your expectation of sending follow-up CL. > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:2423: { kTextTrackNum, "100K 200K"}); > nit: s/"}/" }/ > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:2428: // Remove the buffered ranges. > nit: Fix related to my comment below, since the text track may still contain > non-empty track buffer ranges. > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:2431: CheckExpectedRanges("{ }"); > nit (maybe best left to a separate CL): If MarkEndOfStream(PIPELINE_OK) were > done after this line, would a subsequent CheckExpectedRanges() need to expect > just "{ }", even though there is still a not-removed text track buffer [200, > 300) and (possibly if supported) a partially remaining text track buffer [146, > 200)? (SourceBuffer.buffered() step 4.2 cannot change end time on last range in > a completely empty track's track ranges, so resulting intersection ranges even > in "ended" readyState for this scenario should be empty, right? > EndOfStreamRangeChanges and GetBufferedRanges_EndOfStream don't test this > specific scenario where only text track buffer(s) remain after remove() + > endOfStream(). Further, re-appending some audio+video like just 0K for each + > endOfStream() should then expose the still-not-removed text buffer(s) in > CheckExpectedRanges(). ISTM like a good scenario to test. Spec reference: > https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.... > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:2442: { kVideoTrackNum, "0K 33"}); > nit: s/"}/" }/ > > https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... > media/filters/chunk_demuxer_unittest.cc:3492: CheckExpectedBuffers(video_stream, > "180 210"); > Hmm. Perhaps I misunderstand, but now that we have appended a new keyframe at > time 120 that frame at time 180 depends on, should we really expect 180 to be > next at this point? If so, which one is it (the first one appended, or the > second?) To me, this looks wrong or at least confusing and in need of a > clarifying comment. The linux*rel bot compilers don't like extended initializer lists. eg http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...
https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:441: const char* description; On 2014/07/07 21:31:45, wolenetz wrote: > nit: s/description/cluster_description/. Done. > Also maybe relocate or duplicate |cluster_description| parameter comment from AppendSingleStreamCluster()? For now I just added a comment that references AppendSingleStreamCluster(). The follow-up CL factors out the parsing logic so I'll update the to point to the parsing function in the followup. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:464: AppendSingleStreamCluster(kSourceId, On 2014/07/07 21:31:44, wolenetz wrote: > nit: Include a TODO for the follow-up CL to actually append just 1 muxed cluster > here? At your discretion based on your expectation of sending follow-up CL. I have the follow-up CL written already. I was just waiting to send it once this lands. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2423: { kTextTrackNum, "100K 200K"}); On 2014/07/07 21:31:44, wolenetz wrote: > nit: s/"}/" }/ Done. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2428: // Remove the buffered ranges. On 2014/07/07 21:31:44, wolenetz wrote: > nit: Fix related to my comment below, since the text track may still contain > non-empty track buffer ranges. I changed the remove range so that everything gets removed. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2431: CheckExpectedRanges("{ }"); On 2014/07/07 21:31:44, wolenetz wrote: > nit (maybe best left to a separate CL): If MarkEndOfStream(PIPELINE_OK) were > done after this line, would a subsequent CheckExpectedRanges() need to expect > just "{ }", even though there is still a not-removed text track buffer [200, > 300) and (possibly if supported) a partially remaining text track buffer [146, > 200)? (SourceBuffer.buffered() step 4.2 cannot change end time on last range in > a completely empty track's track ranges, so resulting intersection ranges even > in "ended" readyState for this scenario should be empty, right? > EndOfStreamRangeChanges and GetBufferedRanges_EndOfStream don't test this > specific scenario where only text track buffer(s) remain after remove() + > endOfStream(). Further, re-appending some audio+video like just 0K for each + > endOfStream() should then expose the still-not-removed text buffer(s) in > CheckExpectedRanges(). ISTM like a good scenario to test. Spec reference: > https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.... This seems like a completely separate issue that should be addressed in a different CL. Please file a bug to track the scenario you are worried about. Since text tracks aren't on by default this should probably be a P3. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2442: { kVideoTrackNum, "0K 33"}); On 2014/07/07 21:31:44, wolenetz wrote: > nit: s/"}/" }/ Done. https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:3492: CheckExpectedBuffers(video_stream, "180 210"); On 2014/07/07 21:31:44, wolenetz wrote: > Hmm. Perhaps I misunderstand, but now that we have appended a new keyframe at > time 120 that frame at time 180 depends on, should we really expect 180 to be > next at this point? If so, which one is it (the first one appended, or the > second?) To me, this looks wrong or at least confusing and in need of a > clarifying comment. I changed the code around so that there isn't an overlap. 180 was likely getting returned because the overlap caused the buffers in the current GOP to get moved into the track buffer.
lgtm % two nits: https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:464: AppendSingleStreamCluster(kSourceId, On 2014/07/07 23:56:36, acolwell wrote: > On 2014/07/07 21:31:44, wolenetz wrote: > > nit: Include a TODO for the follow-up CL to actually append just 1 muxed > cluster > > here? At your discretion based on your expectation of sending follow-up CL. > > I have the follow-up CL written already. I was just waiting to send it once this > lands. > sgtm https://codereview.chromium.org/361303003/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2431: CheckExpectedRanges("{ }"); On 2014/07/07 23:56:36, acolwell wrote: > On 2014/07/07 21:31:44, wolenetz wrote: > > nit (maybe best left to a separate CL): If MarkEndOfStream(PIPELINE_OK) were > > done after this line, would a subsequent CheckExpectedRanges() need to expect > > just "{ }", even though there is still a not-removed text track buffer [200, > > 300) and (possibly if supported) a partially remaining text track buffer [146, > > 200)? (SourceBuffer.buffered() step 4.2 cannot change end time on last range > in > > a completely empty track's track ranges, so resulting intersection ranges even > > in "ended" readyState for this scenario should be empty, right? > > EndOfStreamRangeChanges and GetBufferedRanges_EndOfStream don't test this > > specific scenario where only text track buffer(s) remain after remove() + > > endOfStream(). Further, re-appending some audio+video like just 0K for each + > > endOfStream() should then expose the still-not-removed text buffer(s) in > > CheckExpectedRanges(). ISTM like a good scenario to test. Spec reference: > > > https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.... > > This seems like a completely separate issue that should be addressed in a > different CL. Please file a bug to track the scenario you are worried about. > Since text tracks aren't on by default this should probably be a P3. sgtm. I've filed https://crbug.com/392006 https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:451: // The cluster description paseed to AppendSingleStreamCluster(). nit: s/paseed/passed/ https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1245: { kAudioTrackNum, "0K 23K" }, nit: linux*rel bots are still complaining about extended initializer lists. Perhaps explicit construction like AppendMuxedCluster(MuxedStreamInfo(sometracknum, someclusterdescription), ...) could work. See also http://stackoverflow.com/questions/7930773/why-is-this-considered-an-extended... https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3498: // NOTE: we start at 175 here because the buffer at 125 was returned nit: s/175/275/ and s/125/225/
https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:451: // The cluster description paseed to AppendSingleStreamCluster(). On 2014/07/08 00:47:33, wolenetz wrote: > nit: s/paseed/passed/ Done. https://codereview.chromium.org/361303003/diff/20002/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1245: { kAudioTrackNum, "0K 23K" }, On 2014/07/08 00:47:33, wolenetz wrote: > nit: linux*rel bots are still complaining about extended initializer lists. > Perhaps explicit construction like > AppendMuxedCluster(MuxedStreamInfo(sometracknum, someclusterdescription), ...) > could work. See also > http://stackoverflow.com/questions/7930773/why-is-this-considered-an-extended... Done.
The CQ bit was checked by acolwell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/361303003/40001
Message was sent while issue was closed.
Change committed as 281693 |
