Revert 264766 "Simplify AudioSplicer logic which slots buffers b..."

Possible candidate for broken media_unittests:

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/30043/steps/media_unittests/logs/MediaSource_MP3_TimestampOffset_0


> Simplify AudioSplicer logic which slots buffers before or after a splice point.
> 
> Since the first post splice buffer after the config change has a splice_timestamp()
> of kNoTimestamp() we can definitively say when we have the first post splice
> buffer instead of having to relying on a problematic timestamp match.
> 
> The new code makes it so that clients must always call SetSpliceTimestamp() with
> kNoTimestamp() once the first post splice buffer is received.
> 
> BUG=334493
> TEST=existing tests pass.
> 
> Review URL: https://codereview.chromium.org/240123004

TBR=dalecurtis@chromium.org

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

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

6 files changed, 156 insertions, 62 deletions

diff --git a/media/base/audio_splicer.cc b/media/base/audio_splicer.cc
index b83765e..5ce45f3 100644
--- a/media/base/audio_splicer.cc
+++ b/media/base/audio_splicer.cc
@@ -92,6 +92,9 @@ class AudioStreamSanitizer {
   // Returns the total frame count of all buffers available for output.
   int GetFrameCount() const;
 
+  // Returns the duration of all buffers added to the output queue thus far.
+  base::TimeDelta GetDuration() const;
+
   const AudioTimestampHelper& timestamp_helper() {
     return output_timestamp_helper_;
   }
@@ -241,6 +244,12 @@ int AudioStreamSanitizer::GetFrameCount() const {
   return frame_count;
 }
 
+base::TimeDelta AudioStreamSanitizer::GetDuration() const {
+  DCHECK(output_timestamp_helper_.base_timestamp() != kNoTimestamp());
+  return output_timestamp_helper_.GetTimestamp() -
+         output_timestamp_helper_.base_timestamp();
+}
+
 bool AudioStreamSanitizer::DrainInto(AudioStreamSanitizer* output) {
   while (HasNextBuffer()) {
     if (!output->AddInput(GetNextBuffer()))
@@ -256,8 +265,7 @@ AudioSplicer::AudioSplicer(int samples_per_second)
       max_splice_end_timestamp_(kNoTimestamp()),
       output_sanitizer_(new AudioStreamSanitizer(samples_per_second)),
       pre_splice_sanitizer_(new AudioStreamSanitizer(samples_per_second)),
-      post_splice_sanitizer_(new AudioStreamSanitizer(samples_per_second)),
-      have_all_pre_splice_buffers_(false) {}
+      post_splice_sanitizer_(new AudioStreamSanitizer(samples_per_second)) {}
 
 AudioSplicer::~AudioSplicer() {}
 
@@ -265,7 +273,6 @@ void AudioSplicer::Reset() {
   output_sanitizer_->Reset();
   pre_splice_sanitizer_->Reset();
   post_splice_sanitizer_->Reset();
-  have_all_pre_splice_buffers_ = false;
   reset_splice_timestamps();
 }
 
@@ -280,8 +287,17 @@ bool AudioSplicer::AddInput(const scoped_refptr<AudioBuffer>& input) {
   const AudioTimestampHelper& output_ts_helper =
       output_sanitizer_->timestamp_helper();
 
-  if (!have_all_pre_splice_buffers_) {
-    DCHECK(!input->end_of_stream());
+  if (!post_splice_sanitizer_->HasNextBuffer()) {
+    // Due to inaccurate demuxer timestamps a splice may have been incorrectly
+    // marked such that the "pre splice buffers" were all entirely before the
+    // splice point.  Here we check for end of stream or buffers which couldn't
+    // possibly be part of the splice.
+    if (input->end_of_stream() ||
+        input->timestamp() >= max_splice_end_timestamp_) {
+      CHECK(pre_splice_sanitizer_->DrainInto(output_sanitizer_.get()));
+      reset_splice_timestamps();
+      return output_sanitizer_->AddInput(input);
+    }
 
     // If the provided buffer is entirely before the splice point it can also be
     // added to the output queue.
@@ -299,12 +315,23 @@ bool AudioSplicer::AddInput(const scoped_refptr<AudioBuffer>& input) {
           output_ts_helper.frame_count(), output_ts_helper.base_timestamp());
     }
 
-    return pre_splice_sanitizer_->AddInput(input);
-  }
+    // The first overlapping buffer is expected to have a timestamp of exactly
+    // |splice_timestamp_|.  Until that timestamp is seen, all buffers go into
+    // |pre_splice_sanitizer_|.
+    if (!pre_splice_sanitizer_->HasNextBuffer() ||
+        input->timestamp() != splice_timestamp_) {
+      return pre_splice_sanitizer_->AddInput(input);
+    }
 
-  // The first post splice buffer is expected to match |splice_timestamp_|.
-  if (!post_splice_sanitizer_->HasNextBuffer())
-    DCHECK(splice_timestamp_ == input->timestamp());
+    // We've received the first overlapping buffer.
+  } else if (!input->end_of_stream() &&
+             input->timestamp() == splice_timestamp_) {
+    // In this case we accidentally put a pre splice buffer in the post splice
+    // sanitizer, so we need to recover by transferring all current post splice
+    // buffers into the pre splice sanitizer and continuing with the splice.
+    post_splice_sanitizer_->DrainInto(pre_splice_sanitizer_.get());
+    post_splice_sanitizer_->Reset();
+  }
 
   // At this point we have all the fade out preroll buffers from the decoder.
   // We now need to wait until we have enough data to perform the crossfade (or
@@ -330,11 +357,12 @@ bool AudioSplicer::AddInput(const scoped_refptr<AudioBuffer>& input) {
     return true;
   }
 
-  // Wait until we have enough data to crossfade or end of stream.
-  if (!input->end_of_stream() &&
-      input->timestamp() + input->duration() < max_splice_end_timestamp_) {
+  // Since it's possible that a pre splice buffer after the first might have its
+  // timestamp fuzzed to be |splice_timestamp_| we need to always wait until we
+  // have seen all possible post splice buffers to ensure we didn't mistakenly
+  // put a pre splice buffer into the post splice sanitizer.
+  if (!input->end_of_stream() && input->timestamp() < max_splice_end_timestamp_)
     return true;
-  }
 
   scoped_refptr<AudioBuffer> crossfade_buffer;
   scoped_ptr<AudioBus> pre_splice =
@@ -358,13 +386,7 @@ scoped_refptr<AudioBuffer> AudioSplicer::GetNextBuffer() {
 }
 
 void AudioSplicer::SetSpliceTimestamp(base::TimeDelta splice_timestamp) {
-  if (splice_timestamp == kNoTimestamp()) {
-    DCHECK(splice_timestamp_ != kNoTimestamp());
-    DCHECK(!have_all_pre_splice_buffers_);
-    have_all_pre_splice_buffers_ = true;
-    return;
-  }
-
+  DCHECK(splice_timestamp != kNoTimestamp());
   if (splice_timestamp_ == splice_timestamp)
     return;
 
@@ -377,7 +399,6 @@ void AudioSplicer::SetSpliceTimestamp(base::TimeDelta splice_timestamp) {
   max_splice_end_timestamp_ = splice_timestamp_ + max_crossfade_duration_;
   pre_splice_sanitizer_->Reset();
   post_splice_sanitizer_->Reset();
-  have_all_pre_splice_buffers_ = false;
 }
 
 scoped_ptr<AudioBus> AudioSplicer::ExtractCrossfadeFromPreSplice(
diff --git a/media/base/audio_splicer.h b/media/base/audio_splicer.h
index 389607a..4d484d6 100644
--- a/media/base/audio_splicer.h
+++ b/media/base/audio_splicer.h
@@ -45,15 +45,10 @@ class MEDIA_EXPORT AudioSplicer {
   // should only be called if HasNextBuffer() returns true.
   scoped_refptr<AudioBuffer> GetNextBuffer();
 
-  // Indicates an upcoming splice point.  All buffers overlapping or after the
-  // |splice_timestamp| will be considered as "before the splice."  Clients must
-  // then call SetSpliceTimestamp(kNoTimestamp()) to signal that future buffers
-  // should be considered as "after the splice."
-  //
-  // Once |kCrossfadeDurationInMilliseconds| of buffers "after the splice" or
-  // end of stream has been received, the "after" buffers will be crossfaded
-  // with all "before" buffers which overlap them.  "before" buffers outside
-  // of the overlap range will be discarded.
+  // Indicates that overlapping buffers are coming up and should be crossfaded.
+  // Once set, all buffers encountered after |splice_timestamp| will be queued
+  // internally until at least 5ms of overlapping buffers are received (or end
+  // of stream, whichever comes first).
   void SetSpliceTimestamp(base::TimeDelta splice_timestamp);
 
  private:
@@ -106,10 +101,6 @@ class MEDIA_EXPORT AudioSplicer {
   scoped_ptr<AudioStreamSanitizer> pre_splice_sanitizer_;
   scoped_ptr<AudioStreamSanitizer> post_splice_sanitizer_;
 
-  // Whether all buffers which should go into |pre_splice_sanitizer_| have been
-  // received.  If true, buffers should now be put in |post_splice_sanitizer_|.
-  bool have_all_pre_splice_buffers_;
-
   DISALLOW_IMPLICIT_CONSTRUCTORS(AudioSplicer);
 };
 
diff --git a/media/base/audio_splicer_unittest.cc b/media/base/audio_splicer_unittest.cc
index 71e1728..2f74a50 100644
--- a/media/base/audio_splicer_unittest.cc
+++ b/media/base/audio_splicer_unittest.cc
@@ -191,6 +191,7 @@ TEST_F(AudioSplicerTest, Reset) {
   // Verify that a new input buffer passes through as expected.
   scoped_refptr<AudioBuffer> input_2 = GetNextInputBuffer(0.2f);
   EXPECT_TRUE(AddInput(input_2));
+  ASSERT_TRUE(splicer_.HasNextBuffer());
   VerifyNextBuffer(input_2);
   EXPECT_FALSE(splicer_.HasNextBuffer());
 }
@@ -332,8 +333,8 @@ TEST_F(AudioSplicerTest, PartialOverlap) {
 
   // Verify that the first input buffer passed through unmodified.
   VerifyNextBuffer(input_1);
-
   ASSERT_TRUE(splicer_.HasNextBuffer());
+
   scoped_refptr<AudioBuffer> output_2 = splicer_.GetNextBuffer();
   EXPECT_FALSE(splicer_.HasNextBuffer());
 
@@ -431,10 +432,10 @@ TEST_F(AudioSplicerTest, PartialOverlapCrossfade) {
   EXPECT_TRUE(AddInput(overlapped_buffer));
   EXPECT_FALSE(splicer_.HasNextBuffer());
 
-  // |overlapping_buffer| completes the splice.
-  splicer_.SetSpliceTimestamp(kNoTimestamp());
+  // Even though |overlapping_buffer| completes the splice, one extra buffer is
+  // required to confirm it's actually a splice.
   EXPECT_TRUE(AddInput(overlapping_buffer));
-  ASSERT_TRUE(splicer_.HasNextBuffer());
+  EXPECT_FALSE(splicer_.HasNextBuffer());
 
   // Add one more buffer to make sure it's passed through untouched.
   scoped_refptr<AudioBuffer> extra_post_splice_buffer =
@@ -508,12 +509,12 @@ TEST_F(AudioSplicerTest, PartialOverlapCrossfadeEndOfStream) {
 
   // |overlapping_buffer| should not have enough data to complete the splice, so
   // ensure output is not available.
-  splicer_.SetSpliceTimestamp(kNoTimestamp());
   EXPECT_TRUE(AddInput(overlapping_buffer));
   EXPECT_FALSE(splicer_.HasNextBuffer());
 
   // Now add an EOS buffer which should complete the splice.
   EXPECT_TRUE(AddInput(AudioBuffer::CreateEOSBuffer()));
+  ASSERT_TRUE(splicer_.HasNextBuffer());
 
   VerifyPreSpliceOutput(overlapped_buffer,
                         overlapping_buffer,
@@ -567,9 +568,14 @@ TEST_F(AudioSplicerTest, PartialOverlapCrossfadeShortPreSplice) {
   EXPECT_TRUE(AddInput(overlapped_buffer));
   EXPECT_FALSE(splicer_.HasNextBuffer());
 
-  // |overlapping_buffer| completes the splice.
-  splicer_.SetSpliceTimestamp(kNoTimestamp());
+  // Even though |overlapping_buffer| completes the splice, one extra buffer is
+  // required to confirm it's actually a splice.
   EXPECT_TRUE(AddInput(overlapping_buffer));
+  EXPECT_FALSE(splicer_.HasNextBuffer());
+
+  // Add an EOS buffer to complete the splice.
+  EXPECT_TRUE(AddInput(AudioBuffer::CreateEOSBuffer()));
+  ASSERT_TRUE(splicer_.HasNextBuffer());
 
   const int kExpectedPreSpliceSize = 55;
   const base::TimeDelta kExpectedPreSpliceDuration =
@@ -596,6 +602,10 @@ TEST_F(AudioSplicerTest, PartialOverlapCrossfadeShortPreSplice) {
             post_splice_output->duration());
 
   EXPECT_TRUE(VerifyData(post_splice_output, GetValue(overlapping_buffer)));
+
+  post_splice_output = splicer_.GetNextBuffer();
+  EXPECT_TRUE(post_splice_output->end_of_stream());
+
   EXPECT_FALSE(splicer_.HasNextBuffer());
 }
 
@@ -632,8 +642,8 @@ TEST_F(AudioSplicerTest, IncorrectlyMarkedSplice) {
 
   // |second_buffer| should complete the supposed splice, so ensure output is
   // now available.
-  splicer_.SetSpliceTimestamp(kNoTimestamp());
   EXPECT_TRUE(AddInput(second_buffer));
+  ASSERT_TRUE(splicer_.HasNextBuffer());
 
   VerifyNextBuffer(first_buffer);
   VerifyNextBuffer(second_buffer);
@@ -668,16 +678,100 @@ TEST_F(AudioSplicerTest, IncorrectlyMarkedSpliceWithGap) {
   // The splicer should pass through the first buffer since it's not part of the
   // splice.
   EXPECT_TRUE(AddInput(first_buffer));
+  EXPECT_TRUE(splicer_.HasNextBuffer());
   VerifyNextBuffer(first_buffer);
 
   // Do not add |gap_buffer|.
 
-  // |second_buffer| will complete the supposed splice.
-  splicer_.SetSpliceTimestamp(kNoTimestamp());
+  // |second_buffer| will trigger an exact overlap splice check.
   EXPECT_TRUE(AddInput(second_buffer));
+  EXPECT_FALSE(splicer_.HasNextBuffer());
+
+  // When the next buffer is not an exact overlap, the bad splice detection code
+  // will kick in and release the buffers.
+  EXPECT_TRUE(AddInput(AudioBuffer::CreateEOSBuffer()));
+  ASSERT_TRUE(splicer_.HasNextBuffer());
 
   VerifyNextBuffer(gap_buffer);
   VerifyNextBuffer(second_buffer);
+  scoped_refptr<AudioBuffer> eos_buffer = splicer_.GetNextBuffer();
+  EXPECT_TRUE(eos_buffer->end_of_stream());
+  EXPECT_FALSE(splicer_.HasNextBuffer());
+}
+
+// Test behavior when a splice frame gets fuzzed such that there is a pre splice
+// buffer after the first which has a timestamp equal to the splice timestamp.
+// +-----------+
+// |11111111111|
+// +-----------+
+//           +-------+
+//           |2222222|
+//           +-------+
+//           +--------------+
+//           |33333333333333|
+//           +--------------+
+// Results in:
+// +---------+--------------+
+// |111111111|xyyyyyy3333333|
+// +---------+--------------+
+// Where x represents a crossfade between buffers 1 and 3; while y is a
+// crossfade between buffers 2 and 3.
+TEST_F(AudioSplicerTest, SpliceIncorrectlySlotted) {
+  const int kBufferSize =
+      input_timestamp_helper_.GetFramesToTarget(max_crossfade_duration());
+  const int kOverlapSize = 2;
+
+  scoped_refptr<AudioBuffer> buffer_1 =
+      GetNextInputBuffer(1.0f, kBufferSize + kOverlapSize);
+  input_timestamp_helper_.SetBaseTimestamp(buffer_1->timestamp());
+  input_timestamp_helper_.AddFrames(kBufferSize);
+
+  const base::TimeDelta splice_timestamp =
+      input_timestamp_helper_.GetTimestamp();
+  splicer_.SetSpliceTimestamp(splice_timestamp);
+
+  scoped_refptr<AudioBuffer> buffer_2 = GetNextInputBuffer(0.5f, kBufferSize);
+
+  // Create an overlap buffer which is just short of the crossfade size.
+  input_timestamp_helper_.SetBaseTimestamp(splice_timestamp);
+  scoped_refptr<AudioBuffer> buffer_3 =
+      GetNextInputBuffer(0.0f, kBufferSize - kOverlapSize);
+
+  // The splicer should be internally queuing input since |buffer_1| is part of
+  // the supposed splice.
+  EXPECT_TRUE(AddInput(buffer_1));
+  EXPECT_FALSE(splicer_.HasNextBuffer());
+
+  // Adding |buffer_2| should look like a completion of the splice, but still no
+  // buffer should be handed out.
+  EXPECT_TRUE(AddInput(buffer_2));
+  EXPECT_FALSE(splicer_.HasNextBuffer());
+
+  // Adding |buffer_3| should complete the splice correctly, but there is still
+  // not enough data for crossfade, so it shouldn't return yet.
+  EXPECT_TRUE(AddInput(buffer_3));
+  EXPECT_FALSE(splicer_.HasNextBuffer());
+
+  // Add an EOS buffer which should trigger completion of the splice.
+  EXPECT_TRUE(AddInput(AudioBuffer::CreateEOSBuffer()));
+  ASSERT_TRUE(splicer_.HasNextBuffer());
+
+  const int kExpectedPreSpliceSize = kBufferSize;
+  const base::TimeDelta kExpectedPreSpliceDuration = splice_timestamp;
+  const base::TimeDelta kExpectedCrossfadeDuration =
+      base::TimeDelta::FromMicroseconds(4966);
+  VerifyPreSpliceOutput(
+      buffer_1, buffer_3, kExpectedPreSpliceSize, kExpectedPreSpliceDuration);
+  VerifyCrossfadeOutput(buffer_1,
+                        buffer_2,
+                        buffer_3,
+                        kOverlapSize,
+                        buffer_3->frame_count(),
+                        kExpectedCrossfadeDuration);
+
+  scoped_refptr<AudioBuffer> eos_buffer = splicer_.GetNextBuffer();
+  EXPECT_TRUE(eos_buffer->end_of_stream());
+
   EXPECT_FALSE(splicer_.HasNextBuffer());
 }
 
diff --git a/media/base/stream_parser_buffer.h b/media/base/stream_parser_buffer.h
index 9c077c1..0dbb1b3 100644
--- a/media/base/stream_parser_buffer.h
+++ b/media/base/stream_parser_buffer.h
@@ -54,9 +54,8 @@ class MEDIA_EXPORT StreamParserBuffer : public DecoderBuffer {
   // have any  EOS buffers and must not have any nested splice buffers.
   //
   // |pre_splice_buffers| will be deep copied and each copy's splice_timestamp()
-  // will be set to this buffer's splice_timestamp().  A copy of |this|, with a
-  // splice_timestamp() of kNoTimestamp(), will be added to the end of
-  // |splice_buffers_|.
+  // will be set to this buffer's splice_timestamp().  A copy of |this| will be
+  // added to the end of |splice_buffers_|.
   //
   // See the Audio Splice Frame Algorithm in the MSE specification for details.
   typedef StreamParser::BufferQueue BufferQueue;
diff --git a/media/filters/decoder_stream.cc b/media/filters/decoder_stream.cc
index c4adc63..350e47f 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -50,7 +50,6 @@ DecoderStream<StreamType>::DecoderStream(
           new DecoderSelector<StreamType>(task_runner,
                                           decoders.Pass(),
                                           set_decryptor_ready_cb)),
-      active_splice_(false),
       weak_factory_(this) {}
 
 template <DemuxerStream::Type StreamType>
@@ -409,12 +408,9 @@ void DecoderStream<StreamType>::OnBufferReady(
     return;
   }
 
-  if (!splice_observer_cb_.is_null() && !buffer->end_of_stream()) {
-    const bool has_splice_ts = buffer->splice_timestamp() != kNoTimestamp();
-    if (active_splice_ || has_splice_ts) {
-      splice_observer_cb_.Run(buffer->splice_timestamp());
-      active_splice_ = has_splice_ts;
-    }
+  if (!splice_observer_cb_.is_null() && !buffer->end_of_stream() &&
+      buffer->splice_timestamp() != kNoTimestamp()) {
+    splice_observer_cb_.Run(buffer->splice_timestamp());
   }
 
   DCHECK(status == DemuxerStream::kOk) << status;
diff --git a/media/filters/decoder_stream.h b/media/filters/decoder_stream.h
index 4992141..2fd2a55 100644
--- a/media/filters/decoder_stream.h
+++ b/media/filters/decoder_stream.h
@@ -89,9 +89,6 @@ class MEDIA_EXPORT DecoderStream {
 
   // Allows callers to register for notification of splice buffers from the
   // demuxer.  I.e., DecoderBuffer::splice_timestamp() is not kNoTimestamp().
-  //
-  // The observer will be notified of all buffers with a splice_timestamp() and
-  // the first buffer after which has a splice_timestamp() of kNoTimestamp().
   typedef base::Callback<void(base::TimeDelta)> SpliceObserverCB;
   void set_splice_observer(const SpliceObserverCB& splice_observer) {
     splice_observer_cb_ = splice_observer;
@@ -183,10 +180,6 @@ class MEDIA_EXPORT DecoderStream {
   SpliceObserverCB splice_observer_cb_;
   ConfigChangeObserverCB config_change_observer_cb_;
 
-  // If a splice_timestamp() has been seen, this is true until a
-  // splice_timestamp() of kNoTimestamp() is encountered.
-  bool active_splice_;
-
   // NOTE: Weak pointers must be invalidated before all other member variables.
   base::WeakPtrFactory<DecoderStream<StreamType> > weak_factory_;