Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
378863002:
Update ChunkDemuxer unit tests to use real muxed clusters. (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. |
DescriptionUpdate ChunkDemuxer unit tests to use real muxed clusters.
This change fixed the AppendMuxedCluster() so that it actually creates
proper multiplexed clusters instead of appending a sequence of
unmultiplexed clusters. Minor changes to a few test expectations were
needed to properly reflect what should happen when the blocks are
actually multiplexed properly.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281954
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address CR comments #
Total comments: 6
Patch Set 3 : Address CR comments #
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:2471: EXPECT_CALL(host_, SetDuration(base::TimeDelta::FromMilliseconds(246))); This extra call was being triggered because there were 2 appends. Now there is only one append so only 1 duration change occurs. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:1352: MuxedStreamInfo(kTextTrackNum, "25K 40K")); This was moved to avoid the text keyframe from causing the range to start at 0 which could mask what is going on in the other streams. The original code had a text keyframe even though the string doesn't show it. I added an assert in ParseBlockDescription() to make sure that all text blocks are actually marked as keyframes in the string. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:3412: CheckExpectedRanges(kSourceId, "{ [100,270) }"); The text buffer is the first frame within the append window so it becomes the media segment start time. https://codereview.chromium.org/378863002/diff/1/media/filters/frame_processo... File media/filters/frame_processor.cc (left): https://codereview.chromium.org/378863002/diff/1/media/filters/frame_processo... media/filters/frame_processor.cc:557: if (!sequence_mode_) { This was causing broken behavior in ChunkDemuxerTest.AppendWindow_Text. The need for a RAP on one track buffer should not trigger new media segment behavior on all other track buffers.
looking pretty good. mostly just nits + a few questions: https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:407: // We want pop() to return blocks in timestamp increasing order nit: This could become confusing later if BlockInfo used for other purposes. Could we just use < and populate the blocks vector in AppendMuxedCluster() in reverse order as we top/pop from the pri queue? https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:418: // |blocks| with 3 BlockInfo objects: a keyframe with timestamp 0 |and 2 nit: s/|and/and https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:444: ASSERT_EQ(block_info.flags, kWebMFlagKeyframe) nit: ASSERT_EQ(expected, actual) per https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... Though this is not really adhered to much in this file. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:455: ClusterBuilder cb; nit: Add ASSERT_LT(0, blocks.size()); since we won't have a cluster timecode if blocks is empty; caller could instead explicitly use GenerateEmptyCluster()? https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:500: const char* block_description; nit: s/block_description/block_descriptions or blocks_description or block_description. Name similarly the ParseBlockDescription parameter to whichever you choose here. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:777: scoped_ptr<Cluster> GenerateCluster(int first_audio_timecode, We also have this version of GenerateCluster that creates a muxed cluster, optionally with an 'unknown size'. Wrap the unknown size option into the new GenerateCluster from blocks_descriptions, above, and rework the tests further to use just one muxed cluster generator? I'm beginning to think these Generate*Cluster methods belong within a helper class like ClusterBuilder. Perhaps for a separate CL? https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:1353: CheckExpectedRanges(kSourceId, "{ [23,46) }"); IIUC, this change from 30 to 23 is unrelated to the text buffers, right? (Rather, it's related to previous code appended multiple clusters, each triggering a new media segment notification, and this CL appends one cluster with minimum surviving timestamp of 23.) https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:3557: // NOTE: we start at 175 here because the buffer at 125 was returned nit: Precursor CL introduced change to 275. s/175/275 and s/125/225 to fix the comment. https://codereview.chromium.org/378863002/diff/1/media/filters/frame_processo... File media/filters/frame_processor.cc (left): https://codereview.chromium.org/378863002/diff/1/media/filters/frame_processo... media/filters/frame_processor.cc:557: if (!sequence_mode_) { On 2014/07/08 16:32:00, acolwell wrote: > This was causing broken behavior in ChunkDemuxerTest.AppendWindow_Text. The need > for a RAP on one track buffer should not trigger new media segment behavior on > all other track buffers. Ok. If enough frames are dropped such that a discontinuity is introduced, we'll still do the new media segment triggering during steps 7.1-7.6, above.
https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:407: // We want pop() to return blocks in timestamp increasing order On 2014/07/08 19:33:53, wolenetz wrote: > nit: This could become confusing later if BlockInfo used for other purposes. > Could we just use < and populate the blocks vector in AppendMuxedCluster() in > reverse order as we top/pop from the pri queue? Done. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:418: // |blocks| with 3 BlockInfo objects: a keyframe with timestamp 0 |and 2 On 2014/07/08 19:33:53, wolenetz wrote: > nit: s/|and/and Done. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:444: ASSERT_EQ(block_info.flags, kWebMFlagKeyframe) On 2014/07/08 19:33:53, wolenetz wrote: > nit: ASSERT_EQ(expected, actual) per > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... > Though this is not really adhered to much in this file. Done. /me shakes fist at junit. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:455: ClusterBuilder cb; On 2014/07/08 19:33:53, wolenetz wrote: > nit: Add ASSERT_LT(0, blocks.size()); since we won't have a cluster timecode if > blocks is empty; caller could instead explicitly use GenerateEmptyCluster()? Done. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:500: const char* block_description; On 2014/07/08 19:33:53, wolenetz wrote: > nit: s/block_description/block_descriptions or blocks_description or > block_description. Name similarly the ParseBlockDescription parameter to > whichever you choose here. Done. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:777: scoped_ptr<Cluster> GenerateCluster(int first_audio_timecode, On 2014/07/08 19:33:53, wolenetz wrote: > We also have this version of GenerateCluster that creates a muxed cluster, > optionally with an 'unknown size'. Wrap the unknown size option into the new > GenerateCluster from blocks_descriptions, above, and rework the tests further to > use just one muxed cluster generator? I'm beginning to think these > Generate*Cluster methods belong within a helper class like ClusterBuilder. > Perhaps for a separate CL? Done. Converted this method to use BlockInfo to generate the cluster. I think moving code into the ClusterBuilder should be in a different CL. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:1353: CheckExpectedRanges(kSourceId, "{ [23,46) }"); On 2014/07/08 19:33:53, wolenetz wrote: > IIUC, this change from 30 to 23 is unrelated to the text buffers, right? > (Rather, it's related to previous code appended multiple clusters, each > triggering a new media segment notification, and this CL appends one cluster > with minimum surviving timestamp of 23.) That is correct. https://codereview.chromium.org/378863002/diff/1/media/filters/chunk_demuxer_... media/filters/chunk_demuxer_unittest.cc:3557: // NOTE: we start at 175 here because the buffer at 125 was returned On 2014/07/08 19:33:53, wolenetz wrote: > nit: Precursor CL introduced change to 275. s/175/275 and s/125/225 to fix the > comment. Done. https://codereview.chromium.org/378863002/diff/1/media/filters/frame_processo... File media/filters/frame_processor.cc (left): https://codereview.chromium.org/378863002/diff/1/media/filters/frame_processo... media/filters/frame_processor.cc:557: if (!sequence_mode_) { On 2014/07/08 19:33:53, wolenetz wrote: > On 2014/07/08 16:32:00, acolwell wrote: > > This was causing broken behavior in ChunkDemuxerTest.AppendWindow_Text. The > need > > for a RAP on one track buffer should not trigger new media segment behavior on > > all other track buffers. > > Ok. If enough frames are dropped such that a discontinuity is introduced, we'll > still do the new media segment triggering during steps 7.1-7.6, above. Yes. I believe that is the proper place for it. I think this path was just to make the AppendWindow_Text test happy and it appeared to be the only one that cared that this code was removed.
lgtm % nits. Thank you! https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:426: // |blocks_description| - A space delimited string of block info that nit: s/blocks_description/block_descriptions/ here and in method. https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:432: void ParseBlockDescription(int track_number, nit: s/tion/tions/ https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:808: int size = 10; nit: remove unused size and data locals.
thanks for the review. https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:426: // |blocks_description| - A space delimited string of block info that On 2014/07/08 22:45:34, wolenetz wrote: > nit: s/blocks_description/block_descriptions/ here and in method. Done. https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:432: void ParseBlockDescription(int track_number, On 2014/07/08 22:45:34, wolenetz wrote: > nit: s/tion/tions/ Done. https://codereview.chromium.org/378863002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:808: int size = 10; On 2014/07/08 22:45:34, wolenetz wrote: > nit: remove unused size and data locals. 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/378863002/40001
Message was sent while issue was closed.
Change committed as 281954 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
