Replace AudioDecoder::ProduceAudioSamples/ConsumeAudioSamples with read callbacks.

media/filters/audio_renderer_base_unittest.cc

Index: media/filters/audio_renderer_base_unittest.cc
diff --git a/media/filters/audio_renderer_base_unittest.cc b/media/filters/audio_renderer_base_unittest.cc
index 6f53c0c7f1bb99f5607d857f0f286bff4310d9d8..fff93a169b194aff0b0f3d7125e4d77488f58295 100644
--- a/media/filters/audio_renderer_base_unittest.cc
+++ b/media/filters/audio_renderer_base_unittest.cc
@@ -14,9 +14,7 @@
 
 using ::testing::_;
 using ::testing::AnyNumber;
-using ::testing::InSequence;
 using ::testing::Invoke;
-using ::testing::NotNull;
 using ::testing::Return;
 using ::testing::StrictMock;
 
@@ -36,15 +34,7 @@ class MockAudioRendererBase : public AudioRendererBase {
   MOCK_METHOD3(OnInitialize, bool(int, ChannelLayout, int));
   MOCK_METHOD0(OnStop, void());
 
-  // Used for verifying check points during tests.
-  MOCK_METHOD1(CheckPoint, void(int id));
-
  private:
-  FRIEND_TEST_ALL_PREFIXES(AudioRendererBaseTest, OneCompleteReadCycle);
-  FRIEND_TEST_ALL_PREFIXES(AudioRendererBaseTest, Underflow);
-  FRIEND_TEST_ALL_PREFIXES(AudioRendererBaseTest, Underflow_EndOfStream);
-  friend class AudioRendererBaseTest;
-
   DISALLOW_COPY_AND_ASSIGN(MockAudioRendererBase);
 };
 
@@ -53,12 +43,11 @@ class AudioRendererBaseTest : public ::testing::Test {
   // Give the decoder some non-garbage media properties.
   AudioRendererBaseTest()
       : renderer_(new MockAudioRendererBase()),
-        decoder_(new MockAudioDecoder()),
-        pending_reads_(0) {
+        decoder_(new MockAudioDecoder()) {
     renderer_->set_host(&host_);
 
     // Queue all reads from the decoder.
-    EXPECT_CALL(*decoder_, ProduceAudioSamples(_))
+    EXPECT_CALL(*decoder_, Read(_))

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

Can this EXPECT_CALL be moved into Preroll, right before Seek?

[scherkus (not reviewing), 2011/12/07 05:25:12]

I don't think it'd help as we're never quite sure when Read() will get executed
-- see my comments below.

I'll change this to an ON_CALL() for now, which at least is more accurate (we're
not expecting reads so much as doing some sort of default action)

         .WillRepeatedly(Invoke(this, &AudioRendererBaseTest::EnqueueCallback));
 
     // Set up audio properties.
@@ -78,337 +67,230 @@ class AudioRendererBaseTest : public ::testing::Test {
   }
 
   virtual ~AudioRendererBaseTest() {
-    // Expect a call into the subclass.
     EXPECT_CALL(*renderer_, OnStop());
     renderer_->Stop(NewExpectedClosure());
   }
 
-  MOCK_METHOD0(OnUnderflow, void());
+  MOCK_METHOD1(OnSeekComplete, void(PipelineStatus));
+  FilterStatusCB NewSeekCB() {
+    return base::Bind(&AudioRendererBaseTest::OnSeekComplete,
+                      base::Unretained(this));
+  }
 
+  MOCK_METHOD0(OnUnderflow, void());
   base::Closure NewUnderflowClosure() {
     return base::Bind(&AudioRendererBaseTest::OnUnderflow,
                       base::Unretained(this));
   }
 
-  scoped_refptr<DataBuffer> CreateBuffer(int data_size, uint8 value) {
-    scoped_refptr<DataBuffer> buffer(new DataBuffer(data_size));
-    buffer->SetDataSize(data_size);
-    memset(buffer->GetWritableData(), value, buffer->GetDataSize());
-    return buffer;
+  void Initialize() {
+    EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
+        .WillOnce(Return(true));
+    renderer_->Initialize(
+        decoder_, NewExpectedClosure(), NewUnderflowClosure());
   }
 
-  void WriteUntilNoPendingReads(int data_size, uint8 value,
-                                uint32* bytes_buffered) {
-    while (pending_reads_ > 0) {
-      --pending_reads_;
-      *bytes_buffered += data_size;
-      decoder_->ConsumeAudioSamplesForTest(CreateBuffer(data_size, value));
-    }
+  void Preroll() {
+    // Seek to trigger prerolling.
+    renderer_->Seek(base::TimeDelta(), NewSeekCB());
+
+    // Satisfy reads until prerolling completes.
+    EXPECT_CALL(*this, OnSeekComplete(PIPELINE_OK));
+    DeliverUntilBuffered(0);
   }
 
-  void ConsumeBufferedData(uint32 data_size, uint8 expected_value,
-                           uint32* bytes_buffered) {
-    DCHECK(bytes_buffered);
-    DCHECK_GT(*bytes_buffered, 0u);
-
-    base::TimeDelta playback_delay(base::TimeDelta::FromSeconds(1));
-    scoped_array<uint8> buffer(new uint8[data_size]);
-    while (*bytes_buffered > 0) {
-      EXPECT_EQ(data_size, renderer_->FillBuffer(buffer.get(), data_size,
-                                                 playback_delay, false));
-      EXPECT_EQ(expected_value, buffer[0]);
-      *bytes_buffered -= data_size;
-    }
+  void Play() {
+    renderer_->Play(NewExpectedClosure());
+    renderer_->SetPlaybackRate(1.0f);
   }
 
-  void ExpectUnderflow(uint32 data_size, int checkpoint_value) {
-    scoped_array<uint8> buffer(new uint8[data_size]);
-    base::TimeDelta playback_delay(base::TimeDelta::FromSeconds(1));
+  // Delivers |size| bytes with |value| to |renderer_|.
+  //
+  // There must be a pending read callback.
+  void DeliverBuffer(size_t size, uint8 value) {

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

I think the Deliver* methods, EnqueueCallback, and the |read_cb_| field should
be moved to MockAudioDecoder. They seem to simulate the Decoder's behavior.

And perhaps you could rename DeliverBuffer to FulfillReadRequest or something?

[scherkus (not reviewing), 2011/12/07 05:25:12]

Name change done.

What's going on here is that we're essentially implementing MockAudioDecoder,
which then makes me wonder why we don't have something like FakeAudioDecoder
which doesn't use gmock at all, which then makes me wonder why we can't use
FFmpegAudioDecoder!

I'll think about it a bit more.

+    CHECK(!read_cb_.is_null());
+    scoped_refptr<DataBuffer> buffer(new DataBuffer(size));
+    buffer->SetDataSize(size);
+    memset(buffer->GetWritableData(), value, buffer->GetDataSize());
+
+    AudioDecoder::ReadCB read_cb;
+    std::swap(read_cb, read_cb_);
+    read_cb.Run(buffer);
+  }
+
+  // Delivers an end of stream buffer to |renderer_|.
+  //
+  // There must be a pending read callback.
+  void DeliverEndOfStream() {
+    DeliverBuffer(0, 0);
+  }
 
-    EXPECT_CALL(*this, OnUnderflow());
-    EXPECT_CALL(*renderer_, CheckPoint(checkpoint_value));
-    EXPECT_EQ(0u, renderer_->FillBuffer(buffer.get(), data_size, playback_delay,
-                                        false));
-    renderer_->CheckPoint(checkpoint_value);
+  // Delivers bytes with |value| until |renderer_|'s internal buffer is
+  // full and no longer has pending reads.
+  void DeliverUntilBuffered(uint8 value) {
+    while (!read_cb_.is_null()) {

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

Why "while"? It looks like this can only be executed once; DeliverBuffer
exhausts the read_cb_ after running.

[scherkus (not reviewing), 2011/12/07 05:25:12]

Since AudioRendererBase doesn't use a message loop algorithm_->EnqueueBuffer()
ends up scheduling another read (and overwriting read_cb_) all in the same
execution stack.

This was sort of when I realized just how messed up ARB + associated classes are
:(

+      DeliverBuffer(1024, value);

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

nit: no {}

[scherkus (not reviewing), 2011/12/07 05:25:12]

not having {} for one-liners give me the creeps

+    }
   }
 
+  // Attempts to read |size| bytes from |renderer_|'s internal buffer,

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

I was surprised to see that this method consumed |renderer_|'s internal buffer,
instead of just reading from it. Mention? Maybe even rename the method to
"ReadAndConsume"

[scherkus (not reviewing), 2011/12/07 05:25:12]

Done.

+  // returning the actual bytes read.
+  //
+  // |value| is optional and if passed will get set with the byte value
+  // of read data.
+  uint32 ReadBufferedData(uint32 size, uint8* value) {

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

The one-byte |value| output parameter is a little strange. Why not copy all the
bytes read/consumed into the output parameter?

[scherkus (not reviewing), 2011/12/07 05:25:12]

what's sad is that we only need this crud because FillBuffer() returns a buffer
of zeros when not playing (i.e., underflow, paused)

I think I can get around this by using a constant byte value for DeliverBuffer()
in order to distinguish between muted byte values and normal byte values

+    scoped_array<uint8> buffer(new uint8[size]);
+    uint32 bytes_read = renderer_->FillBuffer(buffer.get(), size,
+                                              base::TimeDelta(), true);
+    if (bytes_read > 0 && value)
+      *value = buffer[0];
+    return bytes_read;
+  }
 
- protected:
-  static const size_t kMaxQueueSize;
+  uint32 bytes_buffered() {
+    return renderer_->algorithm_->QueueSize();
+  }
 
   // Fixture members.
   scoped_refptr<MockAudioRendererBase> renderer_;
   scoped_refptr<MockAudioDecoder> decoder_;
   StrictMock<MockFilterHost> host_;
-
-  // Number of asynchronous read requests sent to |decoder_|.
-  size_t pending_reads_;
+  AudioDecoder::ReadCB read_cb_;
 
  private:
-  void EnqueueCallback(scoped_refptr<Buffer> buffer) {
-    ++pending_reads_;
+  void EnqueueCallback(const AudioDecoder::ReadCB& callback) {

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

nit: Change method name to something more informative?

[scherkus (not reviewing), 2011/12/07 05:25:12]

Done (I hope!)

+    CHECK(read_cb_.is_null()) << "Overlapping reads are not permitted";
+    read_cb_ = callback;
   }
 
   DISALLOW_COPY_AND_ASSIGN(AudioRendererBaseTest);
 };
 
-const size_t AudioRendererBaseTest::kMaxQueueSize = 1u;
-
 TEST_F(AudioRendererBaseTest, Initialize_Failed) {
-  InSequence s;
-
-  // Our subclass will fail when asked to initialize.
   EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
       .WillOnce(Return(false));
-
-  // We expect to receive an error.
   EXPECT_CALL(host_, SetError(PIPELINE_ERROR_INITIALIZATION_FAILED));
-
-  // Initialize, we expect to have no reads.
   renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
-  EXPECT_EQ(0u, pending_reads_);
+
+  // We should have no reads.
+  EXPECT_TRUE(read_cb_.is_null());
 }
 
 TEST_F(AudioRendererBaseTest, Initialize_Successful) {
-  InSequence s;
-
-  // Then our subclass will be asked to initialize.
   EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
       .WillOnce(Return(true));
-
-  // Initialize, we shouldn't have any reads.
   renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
 
-  EXPECT_EQ(0u, pending_reads_);
-
-  // Now seek to trigger prerolling, verifying the callback hasn't been
-  // executed yet.
-  EXPECT_CALL(*renderer_, CheckPoint(0));
-  renderer_->Seek(base::TimeDelta(), NewExpectedStatusCB(PIPELINE_OK));
-  EXPECT_EQ(kMaxQueueSize, pending_reads_);
-  renderer_->CheckPoint(0);
-
-  // Now satisfy the read requests.  Our callback should be executed after
-  // exiting this loop.
-  while (pending_reads_) {
-    scoped_refptr<DataBuffer> buffer(new DataBuffer(1024));
-    buffer->SetDataSize(1024);
-    --pending_reads_;
-    decoder_->ConsumeAudioSamplesForTest(buffer);
-  }
+  // We should have no reads.
+  EXPECT_TRUE(read_cb_.is_null());
 }
 
-TEST_F(AudioRendererBaseTest, OneCompleteReadCycle) {
-  InSequence s;
-
-  // Then our subclass will be asked to initialize.
-  EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
-      .WillOnce(Return(true));
-
-  // Initialize, we shouldn't have any reads.
-  renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
-  EXPECT_EQ(0u, pending_reads_);
-
-  // Now seek to trigger prerolling, verifying the callback hasn't been
-  // executed yet.
-  EXPECT_CALL(*renderer_, CheckPoint(0));
-  renderer_->Seek(base::TimeDelta(), NewExpectedStatusCB(PIPELINE_OK));
-  EXPECT_EQ(kMaxQueueSize, pending_reads_);
-  renderer_->CheckPoint(0);
-
-  // Now satisfy the read requests.  Our callback should be executed after
-  // exiting this loop.
-  const uint32 kDataSize = 1024;
-  uint32 bytes_buffered = 0;
+TEST_F(AudioRendererBaseTest, Preroll) {
+  Initialize();
+  Preroll();
+}
 
-  WriteUntilNoPendingReads(kDataSize, 1, &bytes_buffered);
+TEST_F(AudioRendererBaseTest, Play) {
+  Initialize();
+  Preroll();
 
   // Then set the renderer to play state.
-  renderer_->Play(NewExpectedClosure());
-  renderer_->SetPlaybackRate(1.0f);
-  EXPECT_EQ(1.0f, renderer_->GetPlaybackRate());
-
-  // Then flush the data in the renderer by reading from it.
-  uint8 buffer[kDataSize];
-  for (size_t i = 0; i < kMaxQueueSize; ++i) {
-    EXPECT_EQ(kDataSize,
-              renderer_->FillBuffer(buffer, kDataSize,
-                                    base::TimeDelta(), true));
-    bytes_buffered -= kDataSize;
-  }
+  Play();
+  EXPECT_TRUE(read_cb_.is_null());

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

This expectation confuses me... what is it trying to check?

[scherkus (not reviewing), 2011/12/07 05:25:12]

expecting that we don't have any pending reads as a result of setting our state
to Play()

 
-  // Make sure the read request queue is full.
-  EXPECT_EQ(kMaxQueueSize, pending_reads_);
+  // Drain internal buffer, we should have a pending read.
+  uint8 value;
+  EXPECT_EQ(bytes_buffered(), ReadBufferedData(bytes_buffered(), &value));

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

Immediately after this expectation, would bytes_buffered() == 0?

[scherkus (not reviewing), 2011/12/07 05:25:12]

Yup!

+  EXPECT_FALSE(read_cb_.is_null());
+}
 
-  // Fulfill the read with an end-of-stream packet.
-  scoped_refptr<DataBuffer> last_buffer(new DataBuffer(0));
-  decoder_->ConsumeAudioSamplesForTest(last_buffer);
-  --pending_reads_;
+TEST_F(AudioRendererBaseTest, EndOfStream) {
+  Initialize();
+  Preroll();
 
-  // We shouldn't report ended until all data has been flushed out.
-  EXPECT_FALSE(renderer_->HasEnded());
+  // Then set the renderer to play state.
+  Play();
+  EXPECT_TRUE(read_cb_.is_null());
 
-  // We should have one less read request in the queue.
-  EXPECT_EQ(kMaxQueueSize - 1, pending_reads_);
-
-  // Flush the entire internal buffer and verify NotifyEnded() isn't called
-  // right away.
-  EXPECT_CALL(*renderer_, CheckPoint(1));
-  EXPECT_EQ(0u, bytes_buffered % kDataSize);
-  while (bytes_buffered > 0) {
-    EXPECT_EQ(kDataSize,
-              renderer_->FillBuffer(buffer, kDataSize,
-                                    base::TimeDelta(), true));
-    bytes_buffered -= kDataSize;
-  }
+  // Drain internal buffer, we should have a pending read.

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

Explain why there would be a pending read?

Again, I think the confusion is because it's not clear that ReadBufferedData()
is actually consuming the |renderer_|'s internal buffer, and thus reading all of
the buffered data would result in the |renderer_| requesting more data.

+  EXPECT_EQ(bytes_buffered(), ReadBufferedData(bytes_buffered(), NULL));
+  EXPECT_FALSE(read_cb_.is_null());

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

Can you instead EXPECT_CALL the Read() method on the |decoder_|? Or something to
make it more clear that this represents that the |renderer_| has requested more
decoded data.

[scherkus (not reviewing), 2011/12/07 05:25:12]

Done.

 
-  // Although we've emptied the buffer, we don't consider ourselves ended until
-  // we request another buffer.  This way we know the last of the audio has
-  // played.
+  // Fulfill the read with an end-of-stream packet, we shouldn't report ended
+  // nor have a read until we drain the internal buffer.
+  DeliverEndOfStream();
   EXPECT_FALSE(renderer_->HasEnded());
-  renderer_->CheckPoint(1);
+  EXPECT_TRUE(read_cb_.is_null());
 
-  // Do an additional read to trigger NotifyEnded().
+  // Drain internal buffer, now we should report ended.
   EXPECT_CALL(host_, NotifyEnded());
-  EXPECT_EQ(0u, renderer_->FillBuffer(buffer, kDataSize,
-                                      base::TimeDelta(), true));
-
-  // We should now report ended.
+  EXPECT_EQ(bytes_buffered(), ReadBufferedData(bytes_buffered(), NULL));
+  EXPECT_TRUE(read_cb_.is_null());
   EXPECT_TRUE(renderer_->HasEnded());
-
-  // Further reads should return muted audio and not notify any more.
-  EXPECT_EQ(0u, renderer_->FillBuffer(buffer, kDataSize,
-                                      base::TimeDelta(), true));
 }
 
 TEST_F(AudioRendererBaseTest, Underflow) {
-  InSequence s;
-
-  base::TimeDelta playback_delay(base::TimeDelta::FromSeconds(1));
-
-  // Then our subclass will be asked to initialize.
-  EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
-      .WillOnce(Return(true));
-
-  // Initialize, we shouldn't have any reads.
-  renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
-  EXPECT_EQ(0u, pending_reads_);
-
-  // Now seek to trigger prerolling, verifying the callback hasn't been
-  // executed yet.
-  EXPECT_CALL(*renderer_, CheckPoint(0));
-  renderer_->Seek(base::TimeDelta(), NewExpectedStatusCB(PIPELINE_OK));
-  EXPECT_EQ(kMaxQueueSize, pending_reads_);
-  renderer_->CheckPoint(0);
-
-  // Now satisfy the read requests.  Our callback should be executed after
-  // exiting this loop.
-  const uint32 kDataSize = 1024;
-  uint32 bytes_buffered = 0;
+  Initialize();
+  Preroll();
+  Play();
 
-  WriteUntilNoPendingReads(kDataSize, 1, &bytes_buffered);
+  // Drain internal buffer, we should have a pending read.
+  EXPECT_EQ(bytes_buffered(), ReadBufferedData(bytes_buffered(), NULL));
+  EXPECT_FALSE(read_cb_.is_null());
 
-  uint32 bytes_for_preroll = bytes_buffered;
-
-  // Then set the renderer to play state.
-  renderer_->Play(NewExpectedClosure());
-  renderer_->SetPlaybackRate(1.0f);
-  EXPECT_EQ(1.0f, renderer_->GetPlaybackRate());
-
-  // Consume all of the data passed into the renderer.
-  ConsumeBufferedData(kDataSize, 1, &bytes_buffered);
-
-  // Make sure there are read requests pending.
-  EXPECT_GT(pending_reads_, 0u);
-
-  // Verify the next FillBuffer() call triggers calls the underflow callback
+  // Verify the next FillBuffer() call triggers the underflow callback
   // since the queue is empty.

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

nit: "since the queue is empty" -> "since the decoder hasn't delivered any data
after it was drained."

[scherkus (not reviewing), 2011/12/07 05:25:12]

Done.

-  ExpectUnderflow(kDataSize, 1);
-
-  // Verify that zeroed out buffers are being returned during the underflow.
-  uint32 zeros_to_read = 5 * kDataSize;
-  ConsumeBufferedData(kDataSize, 0, &zeros_to_read);
+  const size_t kDataSize = 1024;
+  EXPECT_CALL(*this, OnUnderflow());
+  EXPECT_EQ(0u, ReadBufferedData(kDataSize, NULL));
 
   renderer_->ResumeAfterUnderflow(false);
 
-  // Verify we are still getting zeroed out buffers since no new data has been
-  // pushed to the renderer.
-  zeros_to_read = 5 * kDataSize;
-  ConsumeBufferedData(kDataSize, 0, &zeros_to_read);
-
-  // Satisfy all pending read requests.
-  WriteUntilNoPendingReads(kDataSize, 2, &bytes_buffered);
-
-  EXPECT_GE(bytes_buffered, bytes_for_preroll);
-
-  // Verify that we are now getting the new data.
-  ConsumeBufferedData(kDataSize, 2, &bytes_buffered);
+  // Verify after resuming that we're still not getting data.
+  //
+  // NOTE: FillBuffer() satisfies the read but returns muted audio.
+  uint8 value;
+  EXPECT_EQ(0u, bytes_buffered());
+  EXPECT_EQ(kDataSize, ReadBufferedData(kDataSize, &value));
+  EXPECT_EQ(0u, value);
+
+  // Deliver data, we should get non-muted audio.
+  DeliverUntilBuffered(99);
+  EXPECT_EQ(kDataSize, ReadBufferedData(kDataSize, &value));
+  EXPECT_EQ(99u, value);
 }
 
-
 TEST_F(AudioRendererBaseTest, Underflow_EndOfStream) {
-  InSequence s;
-
-  base::TimeDelta playback_delay(base::TimeDelta::FromSeconds(1));
-
-  // Then our subclass will be asked to initialize.
-  EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
-      .WillOnce(Return(true));
+  Initialize();
+  Preroll();
+  Play();
 
-  // Initialize, we shouldn't have any reads.
-  renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
-  EXPECT_EQ(0u, pending_reads_);
-
-  // Now seek to trigger prerolling, verifying the callback hasn't been
-  // executed yet.
-  EXPECT_CALL(*renderer_, CheckPoint(0));
-  renderer_->Seek(base::TimeDelta(), NewExpectedStatusCB(PIPELINE_OK));
-  EXPECT_EQ(kMaxQueueSize, pending_reads_);
-  renderer_->CheckPoint(0);
-
-  // Now satisfy the read requests.  Our callback should be executed after
-  // exiting this loop.
-  const uint32 kDataSize = 1024;
-  uint32 bytes_buffered = 0;
-
-  WriteUntilNoPendingReads(kDataSize, 1, &bytes_buffered);
-
-  // Then set the renderer to play state.
-  renderer_->Play(NewExpectedClosure());
-  renderer_->SetPlaybackRate(1.0f);
-  EXPECT_EQ(1.0f, renderer_->GetPlaybackRate());
-
-  // Consume all of the data passed into the renderer.
-  ConsumeBufferedData(kDataSize, 1, &bytes_buffered);
-
-  // Make sure there are read requests pending.
-  EXPECT_GT(pending_reads_, 0u);
+  // Drain internal buffer, we should have a pending read.
+  EXPECT_EQ(bytes_buffered(), ReadBufferedData(bytes_buffered(), NULL));
+  EXPECT_FALSE(read_cb_.is_null());
 
   // Verify the next FillBuffer() call triggers calls the underflow callback
   // since the queue is empty.
-  ExpectUnderflow(kDataSize, 1);
-
-  DCHECK_GE(pending_reads_, 2u);
-
-  // Write a buffer.
-  bytes_buffered += kDataSize;
-  --pending_reads_;
-  decoder_->ConsumeAudioSamplesForTest(CreateBuffer(kDataSize, 3));
-
-  // Verify we are getting zeroed out buffers since we haven't pushed enough
-  // data to satisfy preroll.
-  uint32 zeros_to_read = 5 * kDataSize;
-  ConsumeBufferedData(kDataSize, 0, &zeros_to_read);
-
-  // Send end of stream buffers for all pending reads.
-  while (pending_reads_ > 0) {
-    scoped_refptr<DataBuffer> buffer(new DataBuffer(0));
-    --pending_reads_;
-    decoder_->ConsumeAudioSamplesForTest(buffer);
-  }
-
-  // Verify that we are now getting the new data.
-  ConsumeBufferedData(kDataSize, 3, &bytes_buffered);
+  const size_t kDataSize = 1024;
+  EXPECT_CALL(*this, OnUnderflow());
+  EXPECT_EQ(0u, ReadBufferedData(kDataSize, NULL));
+
+  // Deliver a little bit of data.
+  DeliverBuffer(kDataSize, 99);
+
+  // Verify we're getting muted audio during underflow.
+  //
+  // NOTE: FillBuffer() satisfies the read but returns muted audio.
+  uint8 value;
+  EXPECT_EQ(kDataSize, bytes_buffered());
+  EXPECT_EQ(kDataSize, ReadBufferedData(kDataSize, &value));
+  EXPECT_EQ(0u, value);

[vrk (LEFT CHROMIUM), 2011/12/03 03:10:17]

Hmm... I don't get what's going on!
Why is there |kDataSize| of muted audio here?

[scherkus (not reviewing), 2011/12/07 05:25:12]

I had the same moment of confusion. Turns out "it's just the way FillBuffer()
works" and I don't want to mess around with it yet.

I'm going to file a bug w/ some details.

+
+  // Now deliver end of stream, we should get our little bit of data back.
+  DeliverEndOfStream();
+  EXPECT_EQ(kDataSize, bytes_buffered());
+  EXPECT_EQ(kDataSize, ReadBufferedData(kDataSize, &value));
+  EXPECT_EQ(99u, value);
 }
 
 }  // namespace media