Fix incorrect config id being associated with post splice buffer.

The code was using splice_buffers_index_ >= splice_buffer.size()
to indicate that all pre splice buffers have been handed out.

However this meant that the config change after handing out all
pre splice buffers would incorrectly use the config id of the
splice container instead of the last buffer (which is the first
post splice buffer).  See GetConfigId() for more details.

This change fixes several tests and adds a new one to catch this
explicitly.

BUG=334493
TEST=new unittests, fixed existing tests!

Review URL: https://codereview.chromium.org/251903008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267098 0039d316-1c4b-4281-b951-d872f2087c98

content/renderer/media/webmediaplayer_impl.cc

@@ -1167,7 +1167,7 @@ void WebMediaPlayerImpl::StartPipeline() {
         BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnDemuxerOpened),
         BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnNeedKey),
         base::Bind(&LogMediaSourceError, media_log_),
-        false);
+        true);
     demuxer_.reset(chunk_demuxer_);
   }
 

media/filters/chunk_demuxer_unittest.cc

@@ -2569,12 +2569,6 @@ TEST_P(ChunkDemuxerTest, ConfigChange_Audio) {
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
   ASSERT_EQ(status, DemuxerStream::kConfigChanged);
   EXPECT_EQ(last_timestamp.InMilliseconds(), 524);
-  ASSERT_TRUE(audio_config_1.Matches(audio->audio_decoder_config()));
-
-  // The next is due to a typical config difference.
-  ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
-  ASSERT_EQ(status, DemuxerStream::kConfigChanged);
-  EXPECT_EQ(last_timestamp.InMilliseconds(), 527);
 
   // Fetch the new decoder config.
   const AudioDecoderConfig& audio_config_2 = audio->audio_decoder_config();
@@ -2587,14 +2581,6 @@ TEST_P(ChunkDemuxerTest, ConfigChange_Audio) {
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
   ASSERT_EQ(status, DemuxerStream::kConfigChanged);
   EXPECT_EQ(last_timestamp.InMilliseconds(), 782);
-  ASSERT_TRUE(audio_config_2.Matches(audio->audio_decoder_config()));
-
-  ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
-
-  ASSERT_EQ(status, DemuxerStream::kConfigChanged);
-  EXPECT_EQ(last_timestamp.InMilliseconds(), 779);
-
-  // Get the new config and verify that it matches the first one.
   ASSERT_TRUE(audio_config_1.Matches(audio->audio_decoder_config()));
 
   // Read until the end of the stream just to make sure there aren't any other

media/filters/source_buffer_stream.cc

@@ -359,6 +359,7 @@ SourceBufferStream::SourceBufferStream(const AudioDecoderConfig& audio_config,
       memory_limit_(kDefaultAudioMemoryLimit),
       config_change_pending_(false),
       splice_buffers_index_(0),
+      pre_splice_complete_(false),
       splice_frames_enabled_(splice_frames_enabled) {
   DCHECK(audio_config.IsValidConfig());
   audio_configs_.push_back(audio_config);
@@ -384,6 +385,7 @@ SourceBufferStream::SourceBufferStream(const VideoDecoderConfig& video_config,
       memory_limit_(kDefaultVideoMemoryLimit),
       config_change_pending_(false),
       splice_buffers_index_(0),
+      pre_splice_complete_(false),
       splice_frames_enabled_(splice_frames_enabled) {
   DCHECK(video_config.IsValidConfig());
   video_configs_.push_back(video_config);
@@ -410,6 +412,7 @@ SourceBufferStream::SourceBufferStream(const TextTrackConfig& text_config,
       memory_limit_(kDefaultAudioMemoryLimit),
       config_change_pending_(false),
       splice_buffers_index_(0),
+      pre_splice_complete_(false),
       splice_frames_enabled_(splice_frames_enabled) {}
 
 SourceBufferStream::~SourceBufferStream() {
@@ -691,6 +694,7 @@ void SourceBufferStream::ResetSeekState() {
   last_output_buffer_timestamp_ = kNoTimestamp();
   splice_buffers_index_ = 0;
   splice_buffer_ = NULL;
+  pre_splice_complete_ = false;
 }
 
 bool SourceBufferStream::ShouldSeekToStartOfBuffered(
@@ -1170,8 +1174,9 @@ SourceBufferStream::Status SourceBufferStream::GetNextBuffer(
   }
 
   // Did we hand out the last pre-splice buffer on the previous call?
-  if (splice_buffers_index_ == last_splice_buffer_index) {
-    splice_buffers_index_++;
+  if (!pre_splice_complete_) {
+    DCHECK_EQ(splice_buffers_index_, last_splice_buffer_index);
+    pre_splice_complete_ = true;
     config_change_pending_ = true;
     DVLOG(1) << "Config change (forced for fade in of splice frame).";
     return SourceBufferStream::kConfigChange;
@@ -1181,11 +1186,13 @@ SourceBufferStream::Status SourceBufferStream::GetNextBuffer(
   // so hand out the final buffer for fade in.  Because a config change is
   // always issued prior to handing out this buffer, any changes in config id
   // have been inherently handled.
-  DCHECK_GE(splice_buffers_index_, splice_buffers.size());
+  DCHECK(pre_splice_complete_);
+  DCHECK_EQ(splice_buffers_index_, splice_buffers.size() - 1);
   DCHECK(splice_buffers.back()->splice_timestamp() == kNoTimestamp());
   *out_buffer = splice_buffers.back();
   splice_buffer_ = NULL;
   splice_buffers_index_ = 0;
+  pre_splice_complete_ = false;
   return SourceBufferStream::kSuccess;
 }
 

media/filters/source_buffer_stream.h

@@ -398,6 +398,9 @@ class MEDIA_EXPORT SourceBufferStream {
   // handled out next.
   size_t splice_buffers_index_;
 
+  // Indicates that all pre splice buffers have been handed out.
+  bool pre_splice_complete_;
+
   // Indicates that splice frame generation is enabled.
   const bool splice_frames_enabled_;
 

media/filters/source_buffer_stream_unittest.cc

@@ -58,14 +58,17 @@ class SourceBufferStreamTest : public testing::Test {
   void SetAudioStream() {
     video_config_ = TestVideoConfig::Invalid();
     accurate_durations_ = true;
-    AudioDecoderConfig config(kCodecVorbis,
-                              kSampleFormatPlanarF32,
-                              CHANNEL_LAYOUT_STEREO,
-                              1000,
-                              NULL,
-                              0,
-                              false);
-    stream_.reset(new SourceBufferStream(config, LogCB(), true));
+    audio_config_.Initialize(kCodecVorbis,
+                             kSampleFormatPlanarF32,
+                             CHANNEL_LAYOUT_STEREO,
+                             1000,
+                             NULL,
+                             0,
+                             false,
+                             false,
+                             base::TimeDelta(),
+                             0);
+    stream_.reset(new SourceBufferStream(audio_config_, LogCB(), true));
 
     // Equivalent to 2ms per frame.
     SetStreamInfo(500, 500);
@@ -316,6 +319,13 @@ class SourceBufferStreamTest : public testing::Test {
         << "\nActual: " << actual.AsHumanReadableString();
   }
 
+  void CheckAudioConfig(const AudioDecoderConfig& config) {
+    const AudioDecoderConfig& actual = stream_->GetCurrentAudioDecoderConfig();
+    EXPECT_TRUE(actual.Matches(config))
+        << "Expected: " << config.AsHumanReadableString()
+        << "\nActual: " << actual.AsHumanReadableString();
+  }
+
   const LogCB log_cb() {
     return base::Bind(&SourceBufferStreamTest::DebugMediaLog,
                       base::Unretained(this));
@@ -325,6 +335,7 @@ class SourceBufferStreamTest : public testing::Test {
 
   scoped_ptr<SourceBufferStream> stream_;
   VideoDecoderConfig video_config_;
+  AudioDecoderConfig audio_config_;
 
  private:
   base::TimeDelta ConvertToFrameDuration(int frames_per_second) {
@@ -465,6 +476,7 @@ class SourceBufferStreamTest : public testing::Test {
       if (last_splice_frame) {
         // Require at least one additional buffer for a splice.
         CHECK(!pre_splice_buffers.empty());
+        buffer->SetConfigId(splice_config_id);
         buffer->ConvertToSpliceBuffer(pre_splice_buffers);
         pre_splice_buffers.clear();
       }
@@ -3593,8 +3605,9 @@ TEST_F(SourceBufferStreamTest, SpliceFrame_ConfigChangeWithinSplice) {
   CheckExpectedBuffers("0K 3K C");
   CheckVideoConfig(new_config);
   CheckExpectedBuffers("6 9 C");
+  CheckExpectedBuffers("10 C");
   CheckVideoConfig(video_config_);
-  CheckExpectedBuffers("10 15");
+  CheckExpectedBuffers("15");
   CheckNoNextBuffer();
 }
 
@@ -3628,8 +3641,9 @@ TEST_F(SourceBufferStreamTest,
   CheckExpectedBuffers("7K C");
   CheckVideoConfig(new_config);
   CheckExpectedBuffers("8 9 C");
+  CheckExpectedBuffers("10 C");
   CheckVideoConfig(video_config_);
-  CheckExpectedBuffers("10 20");
+  CheckExpectedBuffers("20");
   CheckNoNextBuffer();
 }
 
@@ -3704,6 +3718,28 @@ TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_CorrectMediaSegmentStartTime) {
   CheckNoNextBuffer();
 }
 
+TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_ConfigChange) {
+  SetAudioStream();
+
+  AudioDecoderConfig new_config(kCodecVorbis,
+                                kSampleFormatPlanarF32,
+                                CHANNEL_LAYOUT_MONO,
+                                1000,
+                                NULL,
+                                0,
+                                false);
+  ASSERT_NE(new_config.channel_layout(), audio_config_.channel_layout());
+
+  Seek(0);
+  CheckAudioConfig(audio_config_);
+  NewSegmentAppend("0K 2K 4K 6K");
+  stream_->UpdateAudioConfig(new_config);
+  NewSegmentAppend("5K 8K 12K");
+  CheckExpectedBuffers("0K 2K 4K 6K C 5K 8K 12K");
+  CheckAudioConfig(new_config);
+  CheckNoNextBuffer();
+}
+
 // TODO(vrk): Add unit tests where keyframes are unaligned between streams.
 // (crbug.com/133557)