Return EOS when selected_range_ is NULL in SourceBufferStream

media/filters/source_buffer_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/source_buffer_stream.h"
 
 #include <algorithm>
 #include <map>
 #include <sstream>
 
@@ skipping 910 matching lines @@
   }
 
   if (itr == ranges_.end())
     return;
 
   SeekAndSetSelectedRange(*itr, seek_dts);
   seek_pending_ = false;
 }
 
 bool SourceBufferStream::IsSeekPending() const {
-  return !(end_of_stream_ && IsEndSelected()) && seek_pending_;
+  return !IsEndOfStreamReached() && seek_pending_;

[wolenetz, 2015/05/12 20:36:24]

nit: please swap the order of the evaluation here to enable rapid short-circuit
evaluation in the common case of seek_pending_ == false.

[landell, 2015/05/13 09:55:56]

Done.

 }
 
 void SourceBufferStream::OnSetDuration(base::TimeDelta duration) {
   DecodeTimestamp duration_dts =
       DecodeTimestamp::FromPresentationTime(duration);
   DVLOG(1) << __FUNCTION__ << " " << GetStreamTypeName()
            << " (" << duration.InSecondsF() << ")";
 
   RangeList::iterator itr = ranges_.end();
   for (itr = ranges_.begin(); itr != ranges_.end(); ++itr) {
@@ skipping 148 matching lines @@
     track_buffer_.pop_front();
     last_output_buffer_timestamp_ = (*out_buffer)->GetDecodeTimestamp();
 
     // If the track buffer becomes empty, then try to set the selected range
     // based on the timestamp of this buffer being returned.
     if (track_buffer_.empty())
       SetSelectedRangeIfNeeded(last_output_buffer_timestamp_);
 
     return kSuccess;
   }
 

[wolenetz, 2015/05/12 20:36:23]

nit: probably good for protection against later regression to add
DCHECK(track_buffer_.empty()) here now.

[landell, 2015/05/13 09:55:56]

Done.

   if (!selected_range_ || !selected_range_->HasNextBuffer()) {
-    if (end_of_stream_ && IsEndSelected())
+    if (IsEndOfStreamReached())
       return kEndOfStream;
     DVLOG(3) << __FUNCTION__ << " " << GetStreamTypeName()
              << ": returning kNeedBuffer "
              << (selected_range_ ? "(selected range has no next buffer)"
                                  : "(no selected range)");
     return kNeedBuffer;
   }
 
   if (selected_range_->GetNextConfigId() != current_config_index_) {
     config_change_pending_ = true;
@@ skipping 85 matching lines @@
 void SourceBufferStream::MarkEndOfStream() {
   DCHECK(!end_of_stream_);
   end_of_stream_ = true;
 }
 
 void SourceBufferStream::UnmarkEndOfStream() {
   DCHECK(end_of_stream_);
   end_of_stream_ = false;
 }
 
-bool SourceBufferStream::IsEndSelected() const {
+bool SourceBufferStream::IsEndOfStreamReached() const {
+  if (!end_of_stream_)
+    return false;
+
   if (ranges_.empty())

[wolenetz, 2015/05/12 20:36:23]

nit: I think it might be possible in an edge case (or in later regression) to
have something in track_buffer_, but for ranges_ to be empty. In this case, we
haven't reached end of stream since a non-empty track_buffer_ has more buffers
to be served by GetNextBuffer() still.

Suggestion: change line 1209 to be:
  if (!end_of_stream_ || !track_buffer_.empty())
and remove the track_buffer_.empty() condition from line 1221.

[landell, 2015/05/13 09:55:56]

Done.

     return true;
 
   if (seek_pending_) {
     base::TimeDelta last_range_end_time =
         ranges_.back()->GetBufferedEndTimestamp().ToPresentationTime();
     return seek_buffer_timestamp_ >= last_range_end_time;
   }
 
+  if (!selected_range_ && track_buffer_.empty())

[wolenetz, 2015/05/12 20:36:24]

nit: see my comment, above.

[landell, 2015/05/13 09:55:56]

Done.

+    return true;
+
   return selected_range_ == ranges_.back();
 }
 
 const AudioDecoderConfig& SourceBufferStream::GetCurrentAudioDecoderConfig() {
   if (config_change_pending_)
     CompleteConfigChange();
   return audio_configs_[current_config_index_];
 }
 
 const VideoDecoderConfig& SourceBufferStream::GetCurrentVideoDecoderConfig() {
@@ skipping 347 matching lines @@
     return false;
 
   DCHECK_NE(have_splice_buffers, have_preroll_buffer);
   splice_buffers_index_ = 0;
   pending_buffer_.swap(*out_buffer);
   pending_buffers_complete_ = false;
   return true;
 }
 
 }  // namespace media