Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220103002:
Fix unit test failures with estimated durations and splice frames. (Closed)
|
Created:
6 years, 8 months ago by DaleCurtis Modified:
6 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix unit test failures with estimated durations and splice frames.
When an append results in a splice frame where the overlapping buffer is
the start of the media segment, the new range start time should be the
start of the first pre splice buffer.
WebM duration estimation should always use the minimum duration, not
the maximum to ensure frames are not overestimated -- otherwise large
amounts of incorrect splice frames are generated.
Enables splice frame generation for all test code.
BUG=356805
TEST=media_unittests is fixed. New unittest added for segment start.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264708
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix all the things. #
Total comments: 19
Patch Set 3 : Comments. #
Total comments: 11
Patch Set 4 : Rebase. Comments. #
Total comments: 4
Patch Set 5 : Comments. #Patch Set 6 : Fix DCHECK. #
Total comments: 2
Patch Set 7 : Fix removal of old buffers. #
Messages
Total messages: 44 (0 generated)
Not sure if this is correct, but it resolves the test issues.
Please also add tests to SourceBufferStream's unit test that specifically isolates the specific issue we are running into. https://codereview.chromium.org/220103002/diff/1/media/filters/source_buffer_... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/1/media/filters/source_buffer_... media/filters/source_buffer_stream.cc:1652: media_segment_start_time_ = pre_splice_buffers.front()->timestamp(); I believe this makes the crash go away, but it isn't really addressing the underlying problem. media_segment_start_time_ is intended to represent the start of the media segment being appended. You should not be overriding it here since doing so erases that information and replaces it with something else completely unrelated to that. I believe the fix should be in Append() around where media_segment_start_time_ is assigned to new_range_start_time. What you really want is the min of the first buffer or the media_segment_start_time_. I think the actual problem is that there is a mismatch between the range start time and the buffers you are passing in. SourceBufferRange should probably have the following DCHECK to make sure it isn't passed bad data. DCHECK(media_segment_start_time == kNoTimestamp() || media_segment_start_time <= new_buffers.front()->GetDecodeTimestamp()); Append() should probably also have a DCHECK at the top that makes sure that the buffers coming in have a timestamp that is >= media_segment_start_time_.
https://codereview.chromium.org/220103002/diff/1/media/filters/source_buffer_... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/1/media/filters/source_buffer_... media/filters/source_buffer_stream.cc:1652: media_segment_start_time_ = pre_splice_buffers.front()->timestamp(); On 2014/04/01 00:39:11, acolwell wrote: > I believe this makes the crash go away, but it isn't really addressing the > underlying problem. media_segment_start_time_ is intended to represent the start > of the media segment being appended. You should not be overriding it here since > doing so erases that information and replaces it with something else completely > unrelated to that. I believe the fix should be in Append() around where > media_segment_start_time_ is assigned to new_range_start_time. What you really > want is the min of the first buffer or the media_segment_start_time_. > > I think the actual problem is that there is a mismatch between the range start > time and the buffers you are passing in. SourceBufferRange should probably have > the following DCHECK to make sure it isn't passed bad data. > > DCHECK(media_segment_start_time == kNoTimestamp() || > media_segment_start_time <= new_buffers.front()->GetDecodeTimestamp()); > > Append() should probably also have a DCHECK at the top that makes sure that the > buffers coming in have a timestamp that is >= media_segment_start_time_. Your suggestion also fixes the issue. However adding the recommended DCHECK() to SourceBufferStream::Append() causes many tests to fail. Here are two: [ RUN ] ChunkDemuxerTest.AppendWindow_Text [24692:24692:0401/143118:329900817718:FATAL:source_buffer_stream.cc(450)] Check failed: media_segment_start_time_ <= buffers.front()->timestamp(). 120000 vs. 100000 [ RUN ] SourceBufferStreamTest.Append_DoesNotBeginWithKeyframe [24760:24760:0401/143201:329943721497:FATAL:source_buffer_stream.cc(450)] Check failed: media_segment_start_time_ <= buffers.front()->timestamp(). 99999 vs. 66666 Bad behavior?
New patch set fixes duration issues (and subsequently unittest issues) while enabling splice frames in tests.
Updated bear files are available at: http://xorax.sea/bear-320x240.webm http://xorax.sea/bear-640x480.webm Aside from breaking all the tests, these don't see any more correct, but I may be missing something.
https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2288: // Video: [0,66) [200,366) s/366/334 https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2296: // Video: [0,66) [300,366) s/366/334 https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2304: // Video: [0,66) [200,266) [300,366) nit: this comment is now incorrect, since 1ms was the minimum video frame duration since last webm segment. s/266/234/ and s/366/334/ https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2312: // from [200,246) to [200,366) which is why the intersection results in the s/366/334, but then the rest of the comment is incorrect. Maybe drop the 299 video frame from ever being appended, above, to not have 1ms be minimum video frame duration estimation? https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2572: const AudioDecoderConfig& audio_config_1_too = audio->audio_decoder_config(); I'm not convinced that we need to emit new, but unchanged, decoder configs. If the config has not changed since the last read, why not suppress such redundant notifications? https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2574: EXPECT_EQ(audio_config_1_too.samples_per_second(), 44100); nit: directly compare configs using ::Matches(), here and below? https://codereview.chromium.org/220103002/diff/40001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/source_buf... media/filters/source_buffer_stream.cc:493: base::TimeDelta new_range_start_time = If we allow range starts to be < media_segment_start_time_, does that mean that we could be mishandling some edge cases instead of properly signaling OnNewMediaSegment with the earlier start time?
https://codereview.chromium.org/220103002/diff/40001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/source_buf... media/filters/source_buffer_stream.cc:493: base::TimeDelta new_range_start_time = On 2014/04/02 00:47:07, wolenetz wrote: > If we allow range starts to be < media_segment_start_time_, does that mean that > we could be mishandling some edge cases instead of properly signaling > OnNewMediaSegment with the earlier start time? I believe adding a DCHECK(media_segment_start_time_ <= buffers.front()->timestamp()) at the top of this function would mitigate this concern. If this fires it means that the caller is doing something wrong.
https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2288: // Video: [0,66) [200,366) On 2014/04/02 00:47:07, wolenetz wrote: > s/366/334 Done. https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2296: // Video: [0,66) [300,366) On 2014/04/02 00:47:07, wolenetz wrote: > s/366/334 Done. https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2304: // Video: [0,66) [200,266) [300,366) On 2014/04/02 00:47:07, wolenetz wrote: > nit: this comment is now incorrect, since 1ms was the minimum video frame > duration since last webm segment. s/266/234/ and s/366/334/ Done. https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2312: // from [200,246) to [200,366) which is why the intersection results in the On 2014/04/02 00:47:07, wolenetz wrote: > s/366/334, but then the rest of the comment is incorrect. Maybe drop the 299 > video frame from ever being appended, above, to not have 1ms be minimum video > frame duration estimation? Comment removed. Nah, that avoids the multiple generated ranges, which I assume was intentional. https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2572: const AudioDecoderConfig& audio_config_1_too = audio->audio_decoder_config(); On 2014/04/02 00:47:07, wolenetz wrote: > I'm not convinced that we need to emit new, but unchanged, decoder configs. If > the config has not changed since the last read, why not suppress such redundant > notifications? Switched to matches. https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2574: EXPECT_EQ(audio_config_1_too.samples_per_second(), 44100); On 2014/04/02 00:47:07, wolenetz wrote: > nit: directly compare configs using ::Matches(), here and below? Done. https://codereview.chromium.org/220103002/diff/40001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/source_buf... media/filters/source_buffer_stream.cc:493: base::TimeDelta new_range_start_time = On 2014/04/02 17:19:33, acolwell wrote: > On 2014/04/02 00:47:07, wolenetz wrote: > > If we allow range starts to be < media_segment_start_time_, does that mean > that > > we could be mishandling some edge cases instead of properly signaling > > OnNewMediaSegment with the earlier start time? > > I believe adding a DCHECK(media_segment_start_time_ <= > buffers.front()->timestamp()) at the top of this function would mitigate this > concern. If this fires it means that the caller is doing something wrong. DCHECK() discussion moved to: https://codereview.chromium.org/222783007/
https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2318: // middle range getting larger AND the new range appearing. Please keep this comment and just update the info. I think that this comment helps people understand why the expectations are what they are. https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) [200,234) [300,334) }"); Why doesn't the middle range extend to 246 now? Eventhough the video buffered range is now the smaller of the two, I'd expect the range to expand to the larger of the 2 ranges. https://codereview.chromium.org/220103002/diff/60001/media/filters/pipeline_i... File media/filters/pipeline_integration_test.cc (left): https://codereview.chromium.org/220103002/diff/60001/media/filters/pipeline_i... media/filters/pipeline_integration_test.cc:669: EXPECT_EQ(kAppendTimeMs + k640WebMFileDurationMs + 1, These go away because the max value was pushing us just past the end? https://codereview.chromium.org/220103002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:494: std::min(media_segment_start_time_, buffers.front()->timestamp()); I believe this should actually be GetDecodeTimestamp() instead. media_segment_start_time_ is derived from decode timestamps in LegacyFrameProcessor and not presentation timestamps.
https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2304: // Video: [0,66) [200,266) [300,366) On 2014/04/02 00:47:07, wolenetz wrote: > nit: this comment is now incorrect, since 1ms was the minimum video frame > duration since last webm segment. s/266/234/ and s/366/334/ (For posterity/clarification, I meant "since last webm init segment". This doesn't change my meaning nor impact your "done" comment.) https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2312: // from [200,246) to [200,366) which is why the intersection results in the On 2014/04/02 21:53:43, DaleCurtis wrote: > On 2014/04/02 00:47:07, wolenetz wrote: > > s/366/334, but then the rest of the comment is incorrect. Maybe drop the 299 > > video frame from ever being appended, above, to not have 1ms be minimum video > > frame duration estimation? > > Comment removed. Nah, that avoids the multiple generated ranges, which I assume > was intentional. That's fine, though removing the 299 append would still result in multiple generated video ranges due to the subsequent demuxer_->Remove(200-300). Leaving 299 in (and 1ms minimum video frame dur) is more interesting - see acolwell's comment https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2572: const AudioDecoderConfig& audio_config_1_too = audio->audio_decoder_config(); On 2014/04/02 21:53:43, DaleCurtis wrote: > On 2014/04/02 00:47:07, wolenetz wrote: > > I'm not convinced that we need to emit new, but unchanged, decoder configs. If > > the config has not changed since the last read, why not suppress such > redundant > > notifications? > > Switched to matches. I believe my original question is not yet answered. https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) [200,234) [300,334) }"); On 2014/04/03 01:09:34, acolwell wrote: > Why doesn't the middle range extend to 246 now? Eventhough the video buffered > range is now the smaller of the two, I'd expect the range to expand to the > larger of the 2 ranges. Interesting. Is the expectation that playback from time 200 to time 246 after the MarkEndOfStream, above, should not stall at time 234? Is this clarified in the spec somewhere?
https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) [200,234) [300,334) }"); On 2014/04/03 20:01:40, wolenetz wrote: > On 2014/04/03 01:09:34, acolwell wrote: > > Why doesn't the middle range extend to 246 now? Eventhough the video buffered > > range is now the smaller of the two, I'd expect the range to expand to the > > larger of the 2 ranges. > > Interesting. Is the expectation that playback from time 200 to time 246 after > the MarkEndOfStream, above, should not stall at time 234? Is this clarified in > the spec somewhere? This is just an extreme case of the the streams being different lengths. In this case there just happens to be a hole in the longer stream. Playback should stall when it reaches a region not covered by the buffered ranges. This should be covered by the logic the "SourceBuffer Monitoring" algorithm and trigger a transition to HAVE_CURRENT_DATA once it reaches the end of the buffered range.
https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) [200,234) [300,334) }"); On 2014/04/03 20:16:23, acolwell wrote: > On 2014/04/03 20:01:40, wolenetz wrote: > > On 2014/04/03 01:09:34, acolwell wrote: > > > Why doesn't the middle range extend to 246 now? Eventhough the video > buffered > > > range is now the smaller of the two, I'd expect the range to expand to the > > > larger of the 2 ranges. > > > > Interesting. Is the expectation that playback from time 200 to time 246 after > > the MarkEndOfStream, above, should not stall at time 234? Is this clarified in > > the spec somewhere? > > This is just an extreme case of the the streams being different lengths. In this > case there just happens to be a hole in the longer stream. Playback should stall > when it reaches a region not covered by the buffered ranges. This should be > covered by the logic the "SourceBuffer Monitoring" algorithm and trigger a > transition to HAVE_CURRENT_DATA once it reaches the end of the buffered range. I think I understand the monitoring/stalling piece. I'm more curious if there is specification for "expanding the range to the larger of the 2 ranges", which directs where the hole begins.
On 2014/04/03 20:31:02, wolenetz wrote: > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) > [200,234) [300,334) }"); > On 2014/04/03 20:16:23, acolwell wrote: > > On 2014/04/03 20:01:40, wolenetz wrote: > > > On 2014/04/03 01:09:34, acolwell wrote: > > > > Why doesn't the middle range extend to 246 now? Eventhough the video > > buffered > > > > range is now the smaller of the two, I'd expect the range to expand to the > > > > larger of the 2 ranges. > > > > > > Interesting. Is the expectation that playback from time 200 to time 246 > after > > > the MarkEndOfStream, above, should not stall at time 234? Is this clarified > in > > > the spec somewhere? > > > > This is just an extreme case of the the streams being different lengths. In > this > > case there just happens to be a hole in the longer stream. Playback should > stall > > when it reaches a region not covered by the buffered ranges. This should be > > covered by the logic the "SourceBuffer Monitoring" algorithm and trigger a > > transition to HAVE_CURRENT_DATA once it reaches the end of the buffered range. > > I think I understand the monitoring/stalling piece. I'm more curious if there is > specification for "expanding the range to the larger of the 2 ranges", which > directs where the hole begins. Not explicitly right now. The SourceBuffer.buffered section should probably have text to clarify that it should behave like HTMLMediaElement.buffered(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#dom-htmlmediaelement.buffered) for muxed streams. Basically replace instances of SourceBuffer with "track buffer" in that algorithm.
On 2014/04/03 20:36:31, acolwell wrote: > On 2014/04/03 20:31:02, wolenetz wrote: > > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > > File media/filters/chunk_demuxer_unittest.cc (right): > > > > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > > media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) > > [200,234) [300,334) }"); > > On 2014/04/03 20:16:23, acolwell wrote: > > > On 2014/04/03 20:01:40, wolenetz wrote: > > > > On 2014/04/03 01:09:34, acolwell wrote: > > > > > Why doesn't the middle range extend to 246 now? Eventhough the video > > > buffered > > > > > range is now the smaller of the two, I'd expect the range to expand to > the > > > > > larger of the 2 ranges. > > > > > > > > Interesting. Is the expectation that playback from time 200 to time 246 > > after > > > > the MarkEndOfStream, above, should not stall at time 234? Is this > clarified > > in > > > > the spec somewhere? > > > > > > This is just an extreme case of the the streams being different lengths. In > > this > > > case there just happens to be a hole in the longer stream. Playback should > > stall > > > when it reaches a region not covered by the buffered ranges. This should be > > > covered by the logic the "SourceBuffer Monitoring" algorithm and trigger a > > > transition to HAVE_CURRENT_DATA once it reaches the end of the buffered > range. > > > > I think I understand the monitoring/stalling piece. I'm more curious if there > is > > specification for "expanding the range to the larger of the 2 ranges", which > > directs where the hole begins. > > Not explicitly right now. The SourceBuffer.buffered section should probably have > text to clarify that it should behave like > HTMLMediaElement.buffered(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#dom-htmlmediaelement.buffered) > for muxed streams. Basically replace instances of SourceBuffer with "track > buffer" in that algorithm. I'm still confused. IIUC: Before endOfStream: Audio: [0,46) [200,246) Video: [0,66) [200,234) [300,334) endOfStream algorithm causing readyState to be ended should result in the following intermediate track buffer range calculations in the 'buffered' call on the SourceBuffer: Audio: [0,46) [200,334) // Increased to highest range end time across all track buffers Video: [0,66) [200,234) [300,334) // No change And buffered would then return the intersection, without the middle range extended since the intermediate Video middle range was not expanded: [0,46) [200,234) [300,334) Do I misunderstand?
On 2014/04/03 21:35:43, wolenetz wrote: > On 2014/04/03 20:36:31, acolwell wrote: > > On 2014/04/03 20:31:02, wolenetz wrote: > > > > > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > > > File media/filters/chunk_demuxer_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > > > media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) > > > [200,234) [300,334) }"); > > > On 2014/04/03 20:16:23, acolwell wrote: > > > > On 2014/04/03 20:01:40, wolenetz wrote: > > > > > On 2014/04/03 01:09:34, acolwell wrote: > > > > > > Why doesn't the middle range extend to 246 now? Eventhough the video > > > > buffered > > > > > > range is now the smaller of the two, I'd expect the range to expand to > > the > > > > > > larger of the 2 ranges. > > > > > > > > > > Interesting. Is the expectation that playback from time 200 to time 246 > > > after > > > > > the MarkEndOfStream, above, should not stall at time 234? Is this > > clarified > > > in > > > > > the spec somewhere? > > > > > > > > This is just an extreme case of the the streams being different lengths. > In > > > this > > > > case there just happens to be a hole in the longer stream. Playback should > > > stall > > > > when it reaches a region not covered by the buffered ranges. This should > be > > > > covered by the logic the "SourceBuffer Monitoring" algorithm and trigger a > > > > transition to HAVE_CURRENT_DATA once it reaches the end of the buffered > > range. > > > > > > I think I understand the monitoring/stalling piece. I'm more curious if > there > > is > > > specification for "expanding the range to the larger of the 2 ranges", which > > > directs where the hole begins. > > > > Not explicitly right now. The SourceBuffer.buffered section should probably > have > > text to clarify that it should behave like > > > HTMLMediaElement.buffered(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#dom-htmlmediaelement.buffered) > > for muxed streams. Basically replace instances of SourceBuffer with "track > > buffer" in that algorithm. > > I'm still confused. IIUC: > Before endOfStream: > Audio: [0,46) [200,246) > Video: [0,66) [200,234) [300,334) > > endOfStream algorithm causing readyState to be ended should result in the > following intermediate track buffer range calculations in the 'buffered' call on > the SourceBuffer: > Audio: [0,46) [200,334) // Increased to highest range end time across all track > buffers > Video: [0,66) [200,234) [300,334) // No change > > And buffered would then return the intersection, without the middle range > extended since the intermediate Video middle range was not expanded: > [0,46) [200,234) [300,334) > > Do I misunderstand? Oh. You're right. Nevermind. I was thinking about it getting extended while at the same time expecting the end value to be taken into account. Duh. I do think we should rework the test case so that the original extension of the middle range is still exposed. I think it is useful to keep the coverage of that subtlety.
On 2014/04/03 22:15:38, acolwell wrote: > On 2014/04/03 21:35:43, wolenetz wrote: > > On 2014/04/03 20:36:31, acolwell wrote: > > > On 2014/04/03 20:31:02, wolenetz wrote: > > > > > > > > > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > > > > File media/filters/chunk_demuxer_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... > > > > media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ > [0,46) > > > > [200,234) [300,334) }"); > > > > On 2014/04/03 20:16:23, acolwell wrote: > > > > > On 2014/04/03 20:01:40, wolenetz wrote: > > > > > > On 2014/04/03 01:09:34, acolwell wrote: > > > > > > > Why doesn't the middle range extend to 246 now? Eventhough the video > > > > > buffered > > > > > > > range is now the smaller of the two, I'd expect the range to expand > to > > > the > > > > > > > larger of the 2 ranges. > > > > > > > > > > > > Interesting. Is the expectation that playback from time 200 to time > 246 > > > > after > > > > > > the MarkEndOfStream, above, should not stall at time 234? Is this > > > clarified > > > > in > > > > > > the spec somewhere? > > > > > > > > > > This is just an extreme case of the the streams being different lengths. > > In > > > > this > > > > > case there just happens to be a hole in the longer stream. Playback > should > > > > stall > > > > > when it reaches a region not covered by the buffered ranges. This should > > be > > > > > covered by the logic the "SourceBuffer Monitoring" algorithm and trigger > a > > > > > transition to HAVE_CURRENT_DATA once it reaches the end of the buffered > > > range. > > > > > > > > I think I understand the monitoring/stalling piece. I'm more curious if > > there > > > is > > > > specification for "expanding the range to the larger of the 2 ranges", > which > > > > directs where the hole begins. > > > > > > Not explicitly right now. The SourceBuffer.buffered section should probably > > have > > > text to clarify that it should behave like > > > > > > HTMLMediaElement.buffered(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#dom-htmlmediaelement.buffered) > > > for muxed streams. Basically replace instances of SourceBuffer with "track > > > buffer" in that algorithm. > > > > I'm still confused. IIUC: > > Before endOfStream: > > Audio: [0,46) [200,246) > > Video: [0,66) [200,234) [300,334) > > > > endOfStream algorithm causing readyState to be ended should result in the > > following intermediate track buffer range calculations in the 'buffered' call > on > > the SourceBuffer: > > Audio: [0,46) [200,334) // Increased to highest range end time across all > track > > buffers > > Video: [0,66) [200,234) [300,334) // No change > > > > And buffered would then return the intersection, without the middle range > > extended since the intermediate Video middle range was not expanded: > > [0,46) [200,234) [300,334) > > > > Do I misunderstand? > > Oh. You're right. Nevermind. I was thinking about it getting extended while at > the same time expecting the end value to be taken into account. Duh. > I do think we should rework the test case so that the original extension of the > middle range is still exposed. I think it is useful to keep the coverage of that > subtlety. Whew :) Thank you for ridding me of my confusion. I agree the case should be reworked as you describe.
All comments addressed. PTAL. https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2572: const AudioDecoderConfig& audio_config_1_too = audio->audio_decoder_config(); On 2014/04/03 20:01:40, wolenetz wrote: > On 2014/04/02 21:53:43, DaleCurtis wrote: > > On 2014/04/02 00:47:07, wolenetz wrote: > > > I'm not convinced that we need to emit new, but unchanged, decoder configs. > If > > > the config has not changed since the last read, why not suppress such > > redundant > > > notifications? > > > > Switched to matches. > > I believe my original question is not yet answered. Every time a config change occurs it must be completed with a call to retrieve the decoder config. https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2318: // middle range getting larger AND the new range appearing. On 2014/04/03 01:09:34, acolwell_OOO_4-6_to_4-10 wrote: > Please keep this comment and just update the info. I think that this comment > helps people understand why the expectations are what they are. Done. https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2311: CheckExpectedRanges("{ [0,46) [200,234) [300,334) }"); On 2014/04/03 20:31:03, wolenetz wrote: > On 2014/04/03 20:16:23, acolwell wrote: > > On 2014/04/03 20:01:40, wolenetz wrote: > > > On 2014/04/03 01:09:34, acolwell wrote: > > > > Why doesn't the middle range extend to 246 now? Eventhough the video > > buffered > > > > range is now the smaller of the two, I'd expect the range to expand to the > > > > larger of the 2 ranges. > > > > > > Interesting. Is the expectation that playback from time 200 to time 246 > after > > > the MarkEndOfStream, above, should not stall at time 234? Is this clarified > in > > > the spec somewhere? > > > > This is just an extreme case of the the streams being different lengths. In > this > > case there just happens to be a hole in the longer stream. Playback should > stall > > when it reaches a region not covered by the buffered ranges. This should be > > covered by the logic the "SourceBuffer Monitoring" algorithm and trigger a > > transition to HAVE_CURRENT_DATA once it reaches the end of the buffered range. > > I think I understand the monitoring/stalling piece. I'm more curious if there is > specification for "expanding the range to the larger of the 2 ranges", which > directs where the hole begins. I've reworked the test so the original expectations are still tested. https://codereview.chromium.org/220103002/diff/60001/media/filters/pipeline_i... File media/filters/pipeline_integration_test.cc (left): https://codereview.chromium.org/220103002/diff/60001/media/filters/pipeline_i... media/filters/pipeline_integration_test.cc:669: EXPECT_EQ(kAppendTimeMs + k640WebMFileDurationMs + 1, On 2014/04/03 01:09:34, acolwell_OOO_4-6_to_4-10 wrote: > These go away because the max value was pushing us just past the end? Correct. https://codereview.chromium.org/220103002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/220103002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:494: std::min(media_segment_start_time_, buffers.front()->timestamp()); On 2014/04/03 01:09:34, acolwell_OOO_4-6_to_4-10 wrote: > I believe this should actually be GetDecodeTimestamp() instead. > media_segment_start_time_ is derived from decode timestamps in > LegacyFrameProcessor and not presentation timestamps. Done.
lgtm % nits https://codereview.chromium.org/220103002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2315: // Video: [0,66) [200,234) [332,398) nit: s/234/266/? Otherwise I'd expect the range in the check below to be [200,234) since that would be the result of an intersection between these 2 ranges. https://codereview.chromium.org/220103002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2323: // from [200,246) to [200,266) which is why the intersection results in the nit: s/266/398/
wolenetz: Any more comments? https://codereview.chromium.org/220103002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/220103002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2315: // Video: [0,66) [200,234) [332,398) On 2014/04/11 18:16:05, acolwell wrote: > nit: s/234/266/? Otherwise I'd expect the range in the check below to be > [200,234) since that would be the result of an intersection between these 2 > ranges. Done. https://codereview.chromium.org/220103002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:2323: // from [200,246) to [200,266) which is why the intersection results in the On 2014/04/11 18:16:05, acolwell wrote: > nit: s/266/398/ Done.
lgtm. Note that this CL changes from "max" to "min" duration so far behavior, and https://codereview.chromium.org/238273002/ (patch set 2) currently depends on the previous "max" behavior. So you'll probably want to land this CL first and then I would change that other one :) Thanks!
Thanks, I'll CQ this now.
The CQ bit was checked by dalecurtis@chromium.org
On 2014/04/15 00:34:54, wolenetz wrote: > lgtm. Note that this CL changes from "max" to "min" duration so far behavior, > and https://codereview.chromium.org/238273002/ (patch set 2) currently depends > on the previous "max" behavior. So you'll probably want to land this CL first > and then I would change that other one :) > > Thanks! Note, this change could very well cause layout test failures until those tests are fixed (both WebM and MP4 media duration calculations are likely changed by the "max" to "min-so-far" duration estimation change. I suggest you try to find which layout tests need temporary failure expectations added first, so this can cleanly land.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/220103002/100001
https://codereview.chromium.org/238473002/ to disable the layout tests for now.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/220103002/100001
On 2014/04/15 01:52:37, DaleCurtis wrote: > The CQ bit was checked by mailto:dalecurtis@chromium.org Dale, I'm hitting crash in: LegacyFrameProcessor/PipelineIntegrationTest.MediaSource_ConfigChange_WebM/0 FATAL:audio_splicer.cc(47)] Check failed: buffer->timestamp() == timestamp_helper.GetTimestamp(). (I've rebased a local dependent change on this CL's PS5 and then hit this error.. It looks like the bots are producing similar failure.)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
Hmm, what's happening is there are multiple instances where a timestamp is off by a microsecond or so due to multiple config changes. With a splice this happens to results in the pre splice buffer having a timestamp that's a few microseconds off what we expect. Normally this is ignored under the kMinGapSize, so I've updated the DCHECK() to enforce this instead of an exact timestamp match. However, we should probably consider if over the lifetime of a stream with many config changes, how much drift we want to allow... Is it necessary to resync every now and then? Should we be rewriting durations that are off by microseconds instead? Does it even matter? acolwell: PTAL.
Hmm, this CL is also failing for MP3 timestamp offset tests because of "allowed same timestamp" situations: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... I can fix this by ensuring is_exclusive is false for splices, but I wonder if that violates the intent of allowed same timestamp situations. acolwell, wolenetz: Can you elaborate on what same timestamp situations are? Is the right fix just ignoring it for splice frames?
On 2014/04/17 01:11:53, DaleCurtis wrote: > Hmm, this CL is also failing for MP3 timestamp offset tests because of "allowed > same timestamp" situations: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... > > I can fix this by ensuring is_exclusive is false for splices, but I wonder if > that violates the intent of allowed same timestamp situations. > > acolwell, wolenetz: Can you elaborate on what same timestamp situations are? Is > the right fix just ignoring it for splice frames? If you are running into same timestamp situations it makes me think there might be a bug in the splice frame code somewhere. What is triggering to buffers to be created w/ the same decode timestamp? This situation implies that we have two buffers that are occupying the same space in the timeline which doesn't sound right to me. My understanding of the splice code is that anything that is included as part of the splice is in the "splice buffers". I would have expected that a splice starting at the start timestamp of an existing buffer would move that original buffer into "splice buffers". Is that not the case?
As discussed offline, old buffers aren't removed until the RemoveInternal() calls in PrepareRangesForNextAppend(). I looked into modifying the AreAdjacentInSequence() checks, but that's too perilous since it's web facing and we don't always have accurate durations. Instead, since we never generate splice frames for same timestamp situations I believe the fix is simply to check whether a splice frame was generated when determining if a same timestamp situation is allowed. If a splice was generated it couldn't have been a same timestamp situation. I'm also confident of the DCHECK() change in the AudioSplicer. Since timestamps are not rewritten and we allow gaps of up to 2 frames, a DCHECK() for matching timestamps may always be slightly off. PTAL.
lgtm
lgtm % nits: nit: It looks like PS6 currently assumes https://codereview.chromium.org/240123004/ is in baseline (but it isn't, yet). So long as the rest of this patch has no dependency on https://codereview.chromium.org/240123004/, and so long as the commit doesn't include content from https://codereview.chromium.org/240123004/, then ignore this particular nit. Otherwise, please clarify. https://codereview.chromium.org/220103002/diff/140001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/220103002/diff/140001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:298: ASSERT_TRUE(active_splice_timestamp == kNoTimestamp() || nit: If buffer->splice_timestamp() incorrectly reverts to kNoTimestamp() without buffer->timestamp() == active_splice_timestamp, this method doesn't catch. Please add this check.
On 2014/04/17 21:25:40, wolenetz wrote: > lgtm % nits: > > nit: It looks like PS6 currently assumes > https://codereview.chromium.org/240123004/ is in baseline (but it isn't, yet). > So long as the rest of this patch has no dependency on > https://codereview.chromium.org/240123004/, and so long as the commit doesn't > include content from https://codereview.chromium.org/240123004/, then ignore > this particular nit. Otherwise, please clarify. > > https://codereview.chromium.org/220103002/diff/140001/media/filters/source_bu... > File media/filters/source_buffer_stream_unittest.cc (right): > > https://codereview.chromium.org/220103002/diff/140001/media/filters/source_bu... > media/filters/source_buffer_stream_unittest.cc:298: > ASSERT_TRUE(active_splice_timestamp == kNoTimestamp() || > nit: If buffer->splice_timestamp() incorrectly reverts to kNoTimestamp() without > buffer->timestamp() == active_splice_timestamp, this method doesn't catch. > Please add this check. s/ps6/ps7..
https://codereview.chromium.org/220103002/diff/140001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/220103002/diff/140001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:298: ASSERT_TRUE(active_splice_timestamp == kNoTimestamp() || On 2014/04/17 21:25:41, wolenetz wrote: > nit: If buffer->splice_timestamp() incorrectly reverts to kNoTimestamp() without > buffer->timestamp() == active_splice_timestamp, this method doesn't catch. > Please add this check. Never mind. It does check this already. See similar comment on previous CL @ https://codereview.chromium.org/228663006/diff/50001/media/filters/source_buf...
Thanks for review!
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/220103002/180001
Message was sent while issue was closed.
Change committed as 264708 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
