Index: media/filters/source_buffer_stream.cc |
diff --git a/media/filters/source_buffer_stream.cc b/media/filters/source_buffer_stream.cc |
index 619ff47ab7d4c54a1dcc070384c3ab0bfcfe5c70..103aaf6e1409aa4b42b5d385787e059ec37c7f11 100644 |
--- a/media/filters/source_buffer_stream.cc |
+++ b/media/filters/source_buffer_stream.cc |
@@ -37,6 +37,15 @@ const int kMaxTrackBufferGapWarningLogs = 20; |
// Limit the number of MEDIA_LOG() logs for MSE GC algorithm warnings. |
const int kMaxGarbageCollectAlgorithmWarningLogs = 20; |
+// Limit the number of MEDIA_LOG() logs for same DTS for non-keyframe followed |
+// by keyframe. Prior to relaxing the "media segments must begin with a |
+// keyframe" requirement, we issued decode error for this situation. That was |
+// likely too strict, and now that the keyframe requirement is relaxed, we have |
+// no knowledge of media segment boundaries here. Now, we log but don't trigger |
+// decode error, since we allow these sequences which may cause extra decoder |
+// work or other side-effects. |
+const int kMaxStrangeSameTimestampsLogs = 20; |
+ |
// Helper method that returns true if |ranges| is sorted in increasing order, |
// false otherwise. |
bool IsRangeListSorted(const std::list<media::SourceBufferRange*>& ranges) { |
@@ -137,7 +146,7 @@ SourceBufferStream::SourceBufferStream(const AudioDecoderConfig& audio_config, |
bool splice_frames_enabled) |
: media_log_(media_log), |
seek_buffer_timestamp_(kNoTimestamp()), |
- media_segment_start_time_(kNoDecodeTimestamp()), |
+ coded_frame_group_start_time_(kNoDecodeTimestamp()), |
range_for_next_append_(ranges_.end()), |
last_output_buffer_timestamp_(kNoDecodeTimestamp()), |
max_interbuffer_distance_(kNoTimestamp()), |
@@ -152,7 +161,7 @@ SourceBufferStream::SourceBufferStream(const VideoDecoderConfig& video_config, |
bool splice_frames_enabled) |
: media_log_(media_log), |
seek_buffer_timestamp_(kNoTimestamp()), |
- media_segment_start_time_(kNoDecodeTimestamp()), |
+ coded_frame_group_start_time_(kNoDecodeTimestamp()), |
range_for_next_append_(ranges_.end()), |
last_output_buffer_timestamp_(kNoDecodeTimestamp()), |
max_interbuffer_distance_(kNoTimestamp()), |
@@ -168,7 +177,7 @@ SourceBufferStream::SourceBufferStream(const TextTrackConfig& text_config, |
: media_log_(media_log), |
text_track_config_(text_config), |
seek_buffer_timestamp_(kNoTimestamp()), |
- media_segment_start_time_(kNoDecodeTimestamp()), |
+ coded_frame_group_start_time_(kNoDecodeTimestamp()), |
range_for_next_append_(ranges_.end()), |
last_output_buffer_timestamp_(kNoDecodeTimestamp()), |
max_interbuffer_distance_(kNoTimestamp()), |
@@ -182,22 +191,22 @@ SourceBufferStream::~SourceBufferStream() { |
} |
} |
-void SourceBufferStream::OnNewMediaSegment( |
- DecodeTimestamp media_segment_start_time) { |
- DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() |
- << " (" << media_segment_start_time.InSecondsF() << ")"; |
+void SourceBufferStream::OnStartOfCodedFrameGroup( |
+ DecodeTimestamp coded_frame_group_start_time) { |
+ DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() << " (" |
+ << coded_frame_group_start_time.InSecondsF() << ")"; |
DCHECK(!end_of_stream_); |
- media_segment_start_time_ = media_segment_start_time; |
- new_media_segment_ = true; |
+ coded_frame_group_start_time_ = coded_frame_group_start_time; |
+ new_coded_frame_group_ = true; |
RangeList::iterator last_range = range_for_next_append_; |
- range_for_next_append_ = FindExistingRangeFor(media_segment_start_time); |
+ range_for_next_append_ = FindExistingRangeFor(coded_frame_group_start_time); |
- // Only reset |last_appended_buffer_timestamp_| if this new media segment is |
- // not adjacent to the previous media segment appended to the stream. |
+ // Only reset |last_appended_buffer_timestamp_| if this new coded frame group |
+ // is not adjacent to the previous coded frame group appended to the stream. |
if (range_for_next_append_ == ranges_.end() || |
!AreAdjacentInSequence(last_appended_buffer_timestamp_, |
- media_segment_start_time)) { |
+ coded_frame_group_start_time)) { |
last_appended_buffer_timestamp_ = kNoDecodeTimestamp(); |
last_appended_buffer_duration_ = kNoTimestamp(); |
last_appended_buffer_is_keyframe_ = false; |
@@ -215,41 +224,29 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) { |
"buffers to append", buffers.size()); |
DCHECK(!buffers.empty()); |
- DCHECK(media_segment_start_time_ != kNoDecodeTimestamp()); |
- DCHECK(media_segment_start_time_ <= buffers.front()->GetDecodeTimestamp()); |
+ DCHECK(coded_frame_group_start_time_ != kNoDecodeTimestamp()); |
+ DCHECK(coded_frame_group_start_time_ <= |
+ buffers.front()->GetDecodeTimestamp()); |
DCHECK(!end_of_stream_); |
DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() |
<< ": buffers " << BufferQueueToLogString(buffers); |
- // New media segments must begin with a keyframe. |
- // TODO(wolenetz): Relax this requirement. See http://crbug.com/229412. |
- if (new_media_segment_ && !buffers.front()->is_key_frame()) { |
- MEDIA_LOG(ERROR, media_log_) << "Media segment did not begin with key " |
- "frame. Support for such segments will be " |
- "available in a future version. Please see " |
- "https://crbug.com/229412."; |
- return false; |
- } |
- |
- // Buffers within a media segment should be monotonically increasing. |
- if (!IsMonotonicallyIncreasing(buffers)) |
- return false; |
+ // New coded frame groups emitted by the coded frame processor must begin with |
+ // a keyframe. TODO(wolenetz): Change this to [DCHECK + MEDIA_LOG(ERROR...) + |
+ // return false] once the CHECK has baked in a stable release. See |
+ // https://crbug.com/580621. |
+ CHECK(!new_coded_frame_group_ || buffers.front()->is_key_frame()); |
- if (media_segment_start_time_ < DecodeTimestamp() || |
- buffers.front()->GetDecodeTimestamp() < DecodeTimestamp()) { |
- MEDIA_LOG(ERROR, media_log_) |
- << "Cannot append a media segment with negative timestamps."; |
+ // Buffers within a coded frame group should be monotonically increasing. |
wolenetz
2016/02/05 23:27:24
Note that IsMonotonicallyIncreasing() does the sam
chcunningham
2016/02/09 00:21:08
Acknowledged.
|
+ if (!IsMonotonicallyIncreasing(buffers)) { |
return false; |
} |
- if (!IsNextTimestampValid(buffers.front()->GetDecodeTimestamp(), |
- buffers.front()->is_key_frame())) { |
- const DecodeTimestamp& dts = buffers.front()->GetDecodeTimestamp(); |
+ if (coded_frame_group_start_time_ < DecodeTimestamp() || |
+ buffers.front()->GetDecodeTimestamp() < DecodeTimestamp()) { |
MEDIA_LOG(ERROR, media_log_) |
- << "Invalid same timestamp construct detected at" |
- << " time " << dts.InSecondsF(); |
- |
+ << "Cannot append a coded frame group with negative timestamps."; |
return false; |
} |
@@ -271,15 +268,14 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) { |
last_appended_buffer_is_keyframe_ = buffers.back()->is_key_frame(); |
} else { |
DecodeTimestamp new_range_start_time = std::min( |
- media_segment_start_time_, buffers.front()->GetDecodeTimestamp()); |
+ coded_frame_group_start_time_, buffers.front()->GetDecodeTimestamp()); |
const BufferQueue* buffers_for_new_range = &buffers; |
BufferQueue trimmed_buffers; |
- // If the new range is not being created because of a new media |
- // segment, then we must make sure that we start with a key frame. |
- // This can happen if the GOP in the previous append gets destroyed |
- // by a Remove() call. |
- if (!new_media_segment_) { |
+ // If the new range is not being created because of a new coded frame group, |
+ // then we must make sure that we start with a key frame. This can happen |
+ // if the GOP in the previous append gets destroyed by a Remove() call. |
+ if (!new_coded_frame_group_) { |
BufferQueue::const_iterator itr = buffers.begin(); |
// Scan past all the non-key-frames. |
@@ -294,7 +290,7 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) { |
last_appended_buffer_duration_ = buffers.back()->duration(); |
last_appended_buffer_is_keyframe_ = buffers.back()->is_key_frame(); |
DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() |
- << ": new buffers in the middle of media segment depend on" |
+ << ": new buffers in the middle of coded frame group depend on" |
"keyframe that has been removed, and contain no keyframes." |
"Skipping further processing."; |
DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName() |
@@ -324,7 +320,7 @@ bool SourceBufferStream::Append(const BufferQueue& buffers) { |
buffers_for_new_range->back()->is_key_frame(); |
} |
- new_media_segment_ = false; |
+ new_coded_frame_group_ = false; |
MergeWithAdjacentRangeIfNecessary(range_for_next_append_); |
@@ -440,6 +436,19 @@ void SourceBufferStream::RemoveInternal(DecodeTimestamp start, |
SourceBufferRange* new_range = range->SplitRange(end); |
if (new_range) { |
itr = ranges_.insert(++itr, new_range); |
+ |
+ // Update |range_for_next_append_| if it was previously |range| and should |
wolenetz
2016/02/05 23:27:24
Note: this is the core of the fix for crbug 581458
chcunningham
2016/02/09 00:21:08
Acknowledged.
|
+ // be |new_range| now. |
+ if (range_for_next_append_ != ranges_.end() && |
+ *range_for_next_append_ == range && |
+ last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) { |
chcunningham
2016/02/09 00:21:08
I'm wondering about the case where we've just gott
wolenetz
2016/02/09 00:47:19
I'll need to grok this a little more. I'll get an
wolenetz
2016/02/09 01:36:41
Great comment, Chris!
We've never tested any case
wolenetz
2016/02/12 01:23:47
Done. (as of patch set #5)
|
+ DecodeTimestamp potential_next_append_timestamp = |
+ last_appended_buffer_timestamp_ + |
+ base::TimeDelta::FromInternalValue(1); |
+ if (new_range->BelongsToRange(potential_next_append_timestamp)) |
+ range_for_next_append_ = itr; |
+ } |
+ |
--itr; |
// Update the selected range if the next buffer position was transferred |
@@ -522,8 +531,7 @@ bool SourceBufferStream::ShouldSeekToStartOfBuffered( |
beginning_of_buffered < kSeekToStartFudgeRoom()); |
} |
-bool SourceBufferStream::IsMonotonicallyIncreasing( |
- const BufferQueue& buffers) const { |
+bool SourceBufferStream::IsMonotonicallyIncreasing(const BufferQueue& buffers) { |
DCHECK(!buffers.empty()); |
DecodeTimestamp prev_timestamp = last_appended_buffer_timestamp_; |
bool prev_is_keyframe = last_appended_buffer_is_keyframe_; |
@@ -546,12 +554,13 @@ bool SourceBufferStream::IsMonotonicallyIncreasing( |
} |
if (current_timestamp == prev_timestamp && |
- !SourceBufferRange::AllowSameTimestamp(prev_is_keyframe, |
- current_is_keyframe)) { |
- MEDIA_LOG(ERROR, media_log_) << "Unexpected combination of buffers with" |
- << " the same timestamp detected at " |
- << current_timestamp.InSecondsF(); |
- return false; |
+ SourceBufferRange::IsUncommonSameTimestampSequence( |
wolenetz
2016/02/05 23:27:24
Note, this is the core of the fix for crbug 581125
chcunningham
2016/02/09 00:21:08
Acknowledged.
|
+ prev_is_keyframe, current_is_keyframe)) { |
+ LIMITED_MEDIA_LOG(DEBUG, media_log_, num_strange_same_timestamps_logs_, |
+ kMaxStrangeSameTimestampsLogs) |
+ << "Detected an append sequence with keyframe following a " |
+ "non-keyframe, both with the same decode timestamp of " |
+ << current_timestamp.InSecondsF(); |
} |
} |
@@ -561,15 +570,6 @@ bool SourceBufferStream::IsMonotonicallyIncreasing( |
return true; |
} |
-bool SourceBufferStream::IsNextTimestampValid( |
- DecodeTimestamp next_timestamp, bool next_is_keyframe) const { |
- return (last_appended_buffer_timestamp_ != next_timestamp) || |
- new_media_segment_ || |
- SourceBufferRange::AllowSameTimestamp(last_appended_buffer_is_keyframe_, |
- next_is_keyframe); |
-} |
- |
- |
bool SourceBufferStream::OnlySelectedRangeIsSeeked() const { |
for (RangeList::const_iterator itr = ranges_.begin(); |
itr != ranges_.end(); ++itr) { |
@@ -927,9 +927,7 @@ void SourceBufferStream::PrepareRangesForNextAppend( |
GenerateSpliceFrame(new_buffers); |
DecodeTimestamp prev_timestamp = last_appended_buffer_timestamp_; |
- bool prev_is_keyframe = last_appended_buffer_is_keyframe_; |
DecodeTimestamp next_timestamp = new_buffers.front()->GetDecodeTimestamp(); |
- bool next_is_keyframe = new_buffers.front()->is_key_frame(); |
if (prev_timestamp != kNoDecodeTimestamp() && |
prev_timestamp != next_timestamp) { |
@@ -938,19 +936,14 @@ void SourceBufferStream::PrepareRangesForNextAppend( |
RemoveInternal(prev_timestamp, next_timestamp, true, deleted_buffers); |
} |
- // Make the delete range exclusive if we are dealing with an allowed same |
- // timestamp situation. This prevents the first buffer in the current append |
- // from deleting the last buffer in the previous append if both buffers |
- // have the same timestamp. |
- // |
- // The delete range should never be exclusive if a splice frame was generated |
- // because we don't generate splice frames for same timestamp situations. |
+ // Always make the start of the delete range exclusive for same timestamp |
+ // across the last buffer in the previous append and the first buffer in the |
+ // current append. Never be exclusive if a splice frame was generated because |
chcunningham
2016/02/09 00:21:08
Does the splice buffers check really make sense he
wolenetz
2016/02/09 00:47:19
Hmm. PrepareRangesForNextAppend() actually calls G
wolenetz
2016/02/12 01:23:47
From chat, this is fodder for a low pri later CL.
|
+ // we don't generate splice frames for same timestamp situations. |
DCHECK(new_buffers.front()->splice_timestamp() != |
new_buffers.front()->timestamp()); |
- const bool exclude_start = |
- new_buffers.front()->splice_buffers().empty() && |
- prev_timestamp == next_timestamp && |
- SourceBufferRange::AllowSameTimestamp(prev_is_keyframe, next_is_keyframe); |
+ const bool exclude_start = new_buffers.front()->splice_buffers().empty() && |
+ prev_timestamp == next_timestamp; |
// Delete the buffers that |new_buffers| overlaps. |
DecodeTimestamp start = new_buffers.front()->GetDecodeTimestamp(); |