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

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

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138161

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

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:402: //   1) Algorithm has no audio data
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.)

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:426: DCHECK_GT(frames_written, 0u);
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]

thanks for the review!

given that we're trying to clean up things here I've made a few other
suggestions

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:402: //   1) Algorithm has no audio data
On 2012/05/15 17:04:34, Victoria Kirst wrote:
> 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.)

Updated comment.

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

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:426: DCHECK_GT(frames_written, 0u);
On 2012/05/15 17:04:34, Victoria Kirst wrote:
> 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.

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]

sorry for delay!!

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:402: //   1) Algorithm has no audio data
On 2012/05/15 23:41:33, scherkus wrote:
> On 2012/05/15 17:04:34, Victoria Kirst wrote:
> > 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.)
> 
> Updated comment.
> 
> Does this also mean that we will drop the last bit of sound and fire ended
when
> playback rate is > 1.0?

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!

http://codereview.chromium.org/10389138/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:426: DCHECK_GT(frames_written, 0u);
On 2012/05/15 23:41:33, scherkus wrote:
> On 2012/05/15 17:04:34, Victoria Kirst wrote:
> > 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.
> 
> 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?

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

http://codereview.chromium.org/10389138/diff/2003/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10389138/diff/2003/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:430: // even if NeedsMoreData() returns
false.
Hmmm... 

Since all this code is crazy, I think keeping the empty else statement + comment
is the best thing to do. It's not elegant, but it's explicit -- so that way when
someone tries to rewrite this class (someday!!), they'll understand what was
happening originally.

[scherkus (not reviewing), 2012-05-19 03:13:08]

thanks!

http://codereview.chromium.org/10389138/diff/2003/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10389138/diff/2003/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:430: // even if NeedsMoreData() returns
false.
On 2012/05/17 04:40:52, Victoria Kirst wrote:
> Hmmm... 
> 
> Since all this code is crazy, I think keeping the empty else statement +
comment
> is the best thing to do. It's not elegant, but it's explicit -- so that way
when
> someone tries to rewrite this class (someday!!), they'll understand what was
> happening originally.

Done.

http://codereview.chromium.org/10389138/diff/9001/media/filters/audio_rendere...
File media/filters/audio_renderer_algorithm.h (right):

http://codereview.chromium.org/10389138/diff/9001/media/filters/audio_rendere...
media/filters/audio_renderer_algorithm.h:80: bool CanFillBuffer();
NOTE: logic was reversed here so please double check I got everything right both
here + at call sites

[vrk (LEFT CHROMIUM), 2012-05-21 18:04:47]

LGTM

Thanks for doing this!

[commit-bot: I haz the power, 2012-05-21 18:26:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10389138/9001

[commit-bot: I haz the power, 2012-05-21 18:46:34]

Try job failure for 10389138-9001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...