Remove media::AudioRendererImpl::SignalEndOfStream() and some other minor cleanup.

media/filters/audio_renderer_impl.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/audio_renderer_impl.h"
 
 #include <math.h>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 259 matching lines @@
     case kUnderflow:
     case kRebuffering:
       if (buffer && !buffer->IsEndOfStream())
         algorithm_->EnqueueBuffer(buffer);
       return;
     case kStopped:
       return;
   }
 }
 
-void AudioRendererImpl::SignalEndOfStream() {
-  DCHECK(received_end_of_stream_);
-  if (!rendered_end_of_stream_) {
-    rendered_end_of_stream_ = true;
-    host()->NotifyEnded();
-  }
-}
-
 void AudioRendererImpl::ScheduleRead_Locked() {
   lock_.AssertAcquired();
   if (pending_read_ || state_ == kPaused)
     return;
   pending_read_ = true;
   decoder_->Read(read_cb_);
 }
 
 void AudioRendererImpl::SetPlaybackRate(float playback_rate) {
   DCHECK_LE(0.0f, playback_rate);
@@ skipping 101 matching lines @@
       // the subclass can restart the conversation.
       //
       // This should get handled by the subclass http://crbug.com/106600
       const uint32 kZeroLength = 8192;
       size_t zeros_to_write =
           std::min(kZeroLength, requested_frames * bytes_per_frame_);
       memset(dest, 0, zeros_to_write);
       return zeros_to_write / bytes_per_frame_;
     }
 
-    // Use three conditions to determine the end of playback:
-    // 1. Algorithm needs more audio data.
-    // 2. We've received an end of stream buffer.
-    //    (received_end_of_stream_ == true)
-    // 3. Browser process has no audio data being played.
-    //    There is no way to check that condition that would work for all
-    //    derived classes, so call virtual method that would either render
-    //    end of stream or schedule such rendering.
+    // We use the following conditions to determine end of playback:
+    //   1) Algorithm has no audio data

[vrk (LEFT CHROMIUM), 2012/05/15 17:04:34]

nit: technically, if NeedsMoreData() is true, it doesn't necessarily mean that
the algorithm has no data at all. It could just mean that it has data, but not
enough data to render a frame. (This can happen when playback rate > 1.0, where
the algorithm sometimes needs a lot more data to render the next frame.)

[scherkus (not reviewing), 2012/05/15 23:41:33]

Updated comment.

Does this also mean that we will drop the last bit of sound and fire ended when
playback rate is > 1.0?

[vrk (LEFT CHROMIUM), 2012/05/17 04:40:52]

Yes, in certain cases (namely when the size of the stream % window_size_ != 0) 
there will be an awkward number of bytes left over at the end that get dropped.
The ended event still fires fine...

... except there's a bug where ended gets fired late when playback rate > 1.0
because the playback rate scaling in UpdateEarliestEndTime() is unnecessary.
(crbug.com/122709) Super low hanging fruit, actually I could just write a CL to
fix it tomorrow morning!

+    //   2) We received an end of stream buffer
+    //   3) We haven't already signalled that we've ended
+    //   4) Our estimated earliest end time has expired
     //
-    // Three conditions determine when an underflow occurs:
-    // 1. Algorithm has no audio data.
-    // 2. Currently in the kPlaying state.
-    // 3. Have not received an end of stream buffer.
-    if (algorithm_->NeedsMoreData()) {
-      if (received_end_of_stream_) {
-        // TODO(enal): schedule callback instead of polling.
-        if (base::Time::Now() >= earliest_end_time_)
-          SignalEndOfStream();
-      } else if (state_ == kPlaying) {
-        state_ = kUnderflow;
-        underflow_cb = underflow_cb_;
-      }
+    // TODO(enal): we should replace (4) with a check that the browser has no
+    // more audio data or at least use a delayed callback.
+    //
+    // We use the following conditions to determine underflow:
+    //   1) Algorithm has no audio data
+    //   2) We have NOT received an end of stream buffer
+    //   3) We are in the kPlaying state
+    //
+    // Otherwise we have data we can send to the device.
+    if (algorithm_->NeedsMoreData() && received_end_of_stream_ &&
+        !rendered_end_of_stream_ && base::Time::Now() >= earliest_end_time_) {
+      rendered_end_of_stream_ = true;
+      host()->NotifyEnded();
+    } else if (algorithm_->NeedsMoreData() && !received_end_of_stream_ &&
+               state_ == kPlaying) {
+      state_ = kUnderflow;
+      underflow_cb = underflow_cb_;
     } else {
-      // Otherwise fill the buffer.
       frames_written = algorithm_->FillBuffer(dest, requested_frames);
+      DCHECK_GT(frames_written, 0u);

[vrk (LEFT CHROMIUM), 2012/05/15 17:04:34]

I'm not sure if this is right. This else branch used to only execute when
!algorithm_->NeedsMoreData(). Now it can execute when
algorithm_->NeedsMoreData() but other conditions fail.

For example, if the "end of playback" conditions 1-3 are fulfilled, but 4 is not
fulfilled, then this else statement will execute. Because NeedsMoreData() is
true, algorithm_ should not be able to write any frames, and the DCHECK should
fire.

[scherkus (not reviewing), 2012/05/15 23:41:33]

Fixed condition but this is somewhat confusing.

Why does "needing data" preclude me from filling a buffer? Doesn't algorithm_
always "need data" as long as it's not completely full?

I think I was thrown off here by the name. I notice the comment for
NeedsMoreData() is correct so maybe we can come up with a better name. What
about CanFillBuffer()? If it's true, I can call FillBuffer() and get a non-zero
return value, false otherwise.

WDYT?

[vrk (LEFT CHROMIUM), 2012/05/17 04:40:52]

Yeah, I agree that "NeedsMoreData" is weird/misleading! CanFillBuffer SGTM.

     }
 
     // The |audio_time_buffered_| is the ending timestamp of the last frame
     // buffered at the audio device. |playback_delay| is the amount of time
     // buffered at the audio device. The current time can be computed by their
     // difference.
     if (audio_time_buffered_ != kNoTimestamp()) {
       base::TimeDelta previous_time = current_time_;
       current_time_ = audio_time_buffered_ - playback_delay;
 
@@ skipping 55 matching lines @@
         base::Time::kMicrosecondsPerSecond * bytes / bytes_per_second_);
   }
   return base::TimeDelta();
 }
 
 void AudioRendererImpl::OnRenderError() {
   host()->DisableAudioRenderer();
 }
 
 }  // namespace media