Replace AudioDecoder::ProduceAudioSamples/ConsumeAudioSamples with read callbacks.

Replace AudioDecoder::ProduceAudioSamples/ConsumeAudioSamples with read callbacks.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115148

[scherkus (not reviewing), 2011-12-01 01:13:28]

vrk PTAL

[scherkus (not reviewing), 2011-12-03 01:41:55]

ping vrk

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

Yay, really nice improvement!!

It still took some code spelunking/diagramming to understand how these objects
interact. At some point we should document, but maybe after additional cleanup
(in a later CL).

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base.cc:140: void
AudioRendererBase::SamplesReady(scoped_refptr<Buffer> samples) {
Not sure why SamplesReady/samples is plural? Looks like it's just one sample.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base.cc:270: decoder_->Read(read_cb_);
nit: Looks like it's unnecessary to make |read_cb_| a member. Just call
base::Bind(AudioRendererBase::SamplesReady, ...) here.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:41: class AudioRendererBaseTest :
public ::testing::Test {
Overall comment: It's a bit hard to tell what objects/interactions between
objects the unit test is simulating...

In particular, I think two important but unintuitive parts are:
 - In Preroll(), Seek is triggering the |decoder_| Read() under the covers,
which represents a read request from |renderer_|
 - DeliverUntilBuffered() represents the |decoder_| fulfilling that read request
to the |renderer_|.

I'll put some comments inline below.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:50: EXPECT_CALL(*decoder_,
Read(_))
Can this EXPECT_CALL be moved into Preroll, right before Seek?

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:110: void DeliverBuffer(size_t
size, uint8 value) {
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?

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:131: while (!read_cb_.is_null()) {
Why "while"? It looks like this can only be executed once; DeliverBuffer
exhausts the read_cb_ after running.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:132: DeliverBuffer(1024, value);
nit: no {}

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:136: // Attempts to read |size|
bytes from |renderer_|'s internal buffer,
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"

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:141: uint32
ReadBufferedData(uint32 size, uint8* value) {
The one-byte |value| output parameter is a little strange. Why not copy all the
bytes read/consumed into the output parameter?

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:161: void EnqueueCallback(const
AudioDecoder::ReadCB& callback) {
nit: Change method name to something more informative?

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:199:
EXPECT_TRUE(read_cb_.is_null());
This expectation confuses me... what is it trying to check?

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:203: EXPECT_EQ(bytes_buffered(),
ReadBufferedData(bytes_buffered(), &value));
Immediately after this expectation, would bytes_buffered() == 0?

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:215: // Drain internal buffer, we
should have a pending read.
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.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:217:
EXPECT_FALSE(read_cb_.is_null());
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.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:242: // since the queue is empty.
nit: "since the queue is empty" -> "since the decoder hasn't delivered any data
after it was drained."

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:287: EXPECT_EQ(0u, value);
Hmm... I don't get what's going on!
Why is there |kDataSize| of muted audio here?

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:90: // TODO(scherkus): forced task post
since AudioRendererBase::SamplesReady()
nit: What's the TODO? Should this just be a comment?

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:160: CHECK(!callback.is_null());
CHECK? Do you mean DCHECK?

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:237: DeliverSamples(output);
nit: remove {}

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:250: // TODO(scherkus): forced task post
since FFmpegDemuxerStream::Read() can
nit: ditto above comment re: "TODO"

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

+enal: can you take a look at my change in AudioRendererImpl?

vrk: let me know if you have any other suggestions for the unittest -- if it's
still even a tiny bit confusing we'd do ourselves a favour by fixing it now
rather than later

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base.cc:140: void
AudioRendererBase::SamplesReady(scoped_refptr<Buffer> samples) {
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> Not sure why SamplesReady/samples is plural? Looks like it's just one sample.

Changed to DecodedAudioReady() which I hope will be less ambiguous

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base.cc:270: decoder_->Read(read_cb_);
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: Looks like it's unnecessary to make |read_cb_| a member. Just call
> base::Bind(AudioRendererBase::SamplesReady, ...) here.

somewhat of an optimization to avoid needlessly re-mallocing the base::Bind()
result given we callback remains the same throughout the lifetime of playback

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:50: EXPECT_CALL(*decoder_,
Read(_))
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> Can this EXPECT_CALL be moved into Preroll, right before Seek?

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)

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:110: void DeliverBuffer(size_t
size, uint8 value) {
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> 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?

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.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:131: while (!read_cb_.is_null()) {
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> Why "while"? It looks like this can only be executed once; DeliverBuffer
> exhausts the read_cb_ after running.

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
:(

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:132: DeliverBuffer(1024, value);
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: no {}

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

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:136: // Attempts to read |size|
bytes from |renderer_|'s internal buffer,
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> 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"

Done.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:141: uint32
ReadBufferedData(uint32 size, uint8* value) {
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> The one-byte |value| output parameter is a little strange. Why not copy all
the
> bytes read/consumed into the output parameter?

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

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:161: void EnqueueCallback(const
AudioDecoder::ReadCB& callback) {
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: Change method name to something more informative?

Done (I hope!)

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:199:
EXPECT_TRUE(read_cb_.is_null());
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> This expectation confuses me... what is it trying to check?

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

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:203: EXPECT_EQ(bytes_buffered(),
ReadBufferedData(bytes_buffered(), &value));
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> Immediately after this expectation, would bytes_buffered() == 0?

Yup!

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:217:
EXPECT_FALSE(read_cb_.is_null());
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> 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.

Done.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:242: // since the queue is empty.
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: "since the queue is empty" -> "since the decoder hasn't delivered any
data
> after it was drained."

Done.

http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
media/filters/audio_renderer_base_unittest.cc:287: EXPECT_EQ(0u, value);
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> Hmm... I don't get what's going on!
> Why is there |kDataSize| of muted audio here?

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.

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:90: // TODO(scherkus): forced task post
since AudioRendererBase::SamplesReady()
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: What's the TODO? Should this just be a comment?

done and updated FFmpegVideoDecoder as well

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:160: CHECK(!callback.is_null());
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> CHECK? Do you mean DCHECK?

Done.

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:237: DeliverSamples(output);
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: remove {}

other one-line statements in this file + ffmpegvideodecoder use {} -- going with
consistency here

as a side, I try to use use {} for non-trivial ifs (i.e., there's an else clause
or the statement requires 3+ lines) but that's purely preferential!

http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:250: // TODO(scherkus): forced task post
since FFmpegDemuxerStream::Read() can
On 2011/12/03 03:10:17, Victoria Kirst wrote:
> nit: ditto above comment re: "TODO"

Done + FFmpegVideoDecoder

http://codereview.chromium.org/8763010/diff/11003/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (left):

http://codereview.chromium.org/8763010/diff/11003/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:140: void
AudioRendererImpl::ConsumeAudioSamples(
enal: do you recall why this was needed?

I tested high latency audio path and didn't find a single scenario where this
was actually required.

[enal, 2011-12-15 22:50:04]

On 2011/12/07 05:25:11, scherkus wrote:
> +enal: can you take a look at my change in AudioRendererImpl?
> 
> vrk: let me know if you have any other suggestions for the unittest -- if it's
> still even a tiny bit confusing we'd do ourselves a favour by fixing it now
> rather than later
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> File media/filters/audio_renderer_base.cc (right):
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base.cc:140: void
> AudioRendererBase::SamplesReady(scoped_refptr<Buffer> samples) {
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > Not sure why SamplesReady/samples is plural? Looks like it's just one
sample.
> 
> Changed to DecodedAudioReady() which I hope will be less ambiguous
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base.cc:270: decoder_->Read(read_cb_);
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: Looks like it's unnecessary to make |read_cb_| a member. Just call
> > base::Bind(AudioRendererBase::SamplesReady, ...) here.
> 
> somewhat of an optimization to avoid needlessly re-mallocing the base::Bind()
> result given we callback remains the same throughout the lifetime of playback
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> File media/filters/audio_renderer_base_unittest.cc (right):
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:50: EXPECT_CALL(*decoder_,
> Read(_))
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > Can this EXPECT_CALL be moved into Preroll, right before Seek?
> 
> 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)
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:110: void DeliverBuffer(size_t
> size, uint8 value) {
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > 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?
> 
> 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.
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:131: while (!read_cb_.is_null())
{
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > Why "while"? It looks like this can only be executed once; DeliverBuffer
> > exhausts the read_cb_ after running.
> 
> 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
> :(
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:132: DeliverBuffer(1024, value);
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: no {}
> 
> not having {} for one-liners give me the creeps
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:136: // Attempts to read |size|
> bytes from |renderer_|'s internal buffer,
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > 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"
> 
> Done.
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:141: uint32
> ReadBufferedData(uint32 size, uint8* value) {
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > The one-byte |value| output parameter is a little strange. Why not copy all
> the
> > bytes read/consumed into the output parameter?
> 
> 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
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:161: void EnqueueCallback(const
> AudioDecoder::ReadCB& callback) {
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: Change method name to something more informative?
> 
> Done (I hope!)
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:199:
> EXPECT_TRUE(read_cb_.is_null());
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > This expectation confuses me... what is it trying to check?
> 
> expecting that we don't have any pending reads as a result of setting our
state
> to Play()
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:203: EXPECT_EQ(bytes_buffered(),
> ReadBufferedData(bytes_buffered(), &value));
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > Immediately after this expectation, would bytes_buffered() == 0?
> 
> Yup!
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:217:
> EXPECT_FALSE(read_cb_.is_null());
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:242: // since the queue is
empty.
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: "since the queue is empty" -> "since the decoder hasn't delivered any
> data
> > after it was drained."
> 
> Done.
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/audio_renderer_ba...
> media/filters/audio_renderer_base_unittest.cc:287: EXPECT_EQ(0u, value);
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > Hmm... I don't get what's going on!
> > Why is there |kDataSize| of muted audio here?
> 
> 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.
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
> File media/filters/ffmpeg_audio_decoder.cc (right):
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
> media/filters/ffmpeg_audio_decoder.cc:90: // TODO(scherkus): forced task post
> since AudioRendererBase::SamplesReady()
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: What's the TODO? Should this just be a comment?
> 
> done and updated FFmpegVideoDecoder as well
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
> media/filters/ffmpeg_audio_decoder.cc:160: CHECK(!callback.is_null());
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > CHECK? Do you mean DCHECK?
> 
> Done.
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
> media/filters/ffmpeg_audio_decoder.cc:237: DeliverSamples(output);
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: remove {}
> 
> other one-line statements in this file + ffmpegvideodecoder use {} -- going
with
> consistency here
> 
> as a side, I try to use use {} for non-trivial ifs (i.e., there's an else
clause
> or the statement requires 3+ lines) but that's purely preferential!
> 
>
http://codereview.chromium.org/8763010/diff/1/media/filters/ffmpeg_audio_deco...
> media/filters/ffmpeg_audio_decoder.cc:250: // TODO(scherkus): forced task post
> since FFmpegDemuxerStream::Read() can
> On 2011/12/03 03:10:17, Victoria Kirst wrote:
> > nit: ditto above comment re: "TODO"
> 
> Done + FFmpegVideoDecoder
> 
>
http://codereview.chromium.org/8763010/diff/11003/content/renderer/media/audi...
> File content/renderer/media/audio_renderer_impl.cc (left):
> 
>
http://codereview.chromium.org/8763010/diff/11003/content/renderer/media/audi...
> content/renderer/media/audio_renderer_impl.cc:140: void
> AudioRendererImpl::ConsumeAudioSamples(
> enal: do you recall why this was needed?
> 
> I tested high latency audio path and didn't find a single scenario where this
> was actually required.

lgtm, but I never looked at that code before. Victoria would probably have more
good comments...

[enal, 2011-12-15 22:51:25]

Oops, forgot to answer direct question in previous reply.

http://codereview.chromium.org/8763010/diff/11003/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (left):

http://codereview.chromium.org/8763010/diff/11003/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:140: void
AudioRendererImpl::ConsumeAudioSamples(
On 2011/12/07 05:25:12, scherkus wrote:
> enal: do you recall why this was needed?
> 
> I tested high latency audio path and didn't find a single scenario where this
> was actually required.

No idea, it was before my time...

[vrk (LEFT CHROMIUM), 2011-12-16 00:37:32]

LGTM, I just have a few nits.

The unit tests look a lot clearer!

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:70: DCHECK(!pending_read_) << "Pending
reads must complete before seeking";
teeny tiny nit: s/reads/read

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:182: // No more pending reads!  We're now
officially "paused".
teeny tiny nit: s/reads/read

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:134: // Delivers bytes with
|value| until |renderer_|'s internal buffer is
update comment - |value| no longer valid.

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:136: void FulfillUntilBuffered() {
Bah it's hard to think of a good and concise name for this method!!

What do you think of "DeliverRemainingAudio"? Kind of opaque still but perhaps a
little clearer?

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:139: }
CHECK(read_cb_.is_null())? The result of |read_cb_| should not request more
data.

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:164: uint32 bytes_remaining() {
nit: s/bytes_remaining/\0_in_buffer?

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:285: // since the queue is empty.
nit: change to match comment on line 252

http://codereview.chromium.org/8763010/diff/11003/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/8763010/diff/11003/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:161: CHECK(read_cb_.is_null()) <<
"Overlapping decodes are not supported.";
DCHECK instead of CHECK?

[scherkus (not reviewing), 2011-12-19 19:30:13]

vrk: I've got some questions surrounding pending reads -- could you take a quick
look?

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:70: DCHECK(!pending_read_) << "Pending
reads must complete before seeking";
On 2011/12/16 00:37:33, Victoria Kirst wrote:
> teeny tiny nit: s/reads/read

Done.

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:182: // No more pending reads!  We're now
officially "paused".
On 2011/12/16 00:37:33, Victoria Kirst wrote:
> teeny tiny nit: s/reads/read

Done.

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:136: void FulfillUntilBuffered() {
On 2011/12/16 00:37:33, Victoria Kirst wrote:
> Bah it's hard to think of a good and concise name for this method!!
> 
> What do you think of "DeliverRemainingAudio"? Kind of opaque still but perhaps
a
> little clearer?

Done.

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:164: uint32 bytes_remaining() {
On 2011/12/16 00:37:33, Victoria Kirst wrote:
> nit: s/bytes_remaining/\0_in_buffer?

Done.

http://codereview.chromium.org/8763010/diff/11003/media/filters/audio_rendere...
media/filters/audio_renderer_base_unittest.cc:285: // since the queue is empty.
On 2011/12/16 00:37:33, Victoria Kirst wrote:
> nit: change to match comment on line 252

Done.

http://codereview.chromium.org/8763010/diff/11003/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/8763010/diff/11003/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:161: CHECK(read_cb_.is_null()) <<
"Overlapping decodes are not supported.";
On 2011/12/16 00:37:33, Victoria Kirst wrote:
> DCHECK instead of CHECK?

nope -- I'd rather crash than get really strange bug reports

http://codereview.chromium.org/8763010/diff/22001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (left):

http://codereview.chromium.org/8763010/diff/22001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:189:
renderer_->ConsumeAudioSamples(buffer);
this should never happen if there wasn't a pending read so I'm not sure what
this is supposed to be testing

http://codereview.chromium.org/8763010/diff/22001/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (left):

http://codereview.chromium.org/8763010/diff/22001/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:152: if (!pending_reads_)
vrk: do you know if replacing this with a CHECK() is OK?

under which circumstances did ConsumeAudioSamples() get called when there wasn't
a pending read?

[vrk (LEFT CHROMIUM), 2011-12-20 01:21:59]

http://codereview.chromium.org/8763010/diff/22001/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (left):

http://codereview.chromium.org/8763010/diff/22001/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:152: if (!pending_reads_)
On 2011/12/19 19:30:13, scherkus wrote:
> vrk: do you know if replacing this with a CHECK() is OK?

Bah, spent a long time looking through the code today to try to figure it out.
The best answer I can come up with is: if you're 100% sure it's correct to
replace |pending_reads_| with |pending_read_|, then it is correct to add the
CHECK here.

> under which circumstances did ConsumeAudioSamples() get called when there
wasn't
> a pending read?

I believe the only ways for |pending_read_| to be false at that time is if a)
DecodedAudioReady is called before ScheduleRead_Locked, or b) the
ScheduleRead_Locked and DecodedAudioReady calls are imbalanced -- that is, the
code assumes ScheduleRead_Locked will not be called again until
DecodedAudioReady is executed, but if that assumption is false then you may
CHECK.

Scenario (a) can't happen. DecodedAudioReady is passed in as a CB to the demuxer
via ScheduleRead_Locked(), so it's not possible for it to be executed before
ScheduleRead_Locked() is executed.

Scenario (b) *shouldn't* happen, ... but I'm a little wary because I don't
understand the threading story completely.

The case I'm worried about is the following: say ScheduleRead_Locked() is called
twice in a row. This posts 2 FFmpegAudioDecoder::Read tasks in the decoder
thread. Then when these read tasks are executed, the demuxer stream queues up 2
read callbacks in its |read_queue_|. Then the decoder decodes for both of these
requests, and for each finished read request, DecodeBuffer is executed... the
first time DecodeBuffer executes, |pending_read_| is true, but the 2nd time it
executes it would CHECK.

Buuuuut yeah, I don't know if that's possible!! If there really can't be more
than one pending read at a time, then you should be able to CHECK here... but
then, wouldn't you also be able to replace |read_queue_| in FFmpegDemuxerStream
with just a |read_cb_| or something?

[vrk (LEFT CHROMIUM), 2011-12-20 02:50:14]

On 2011/12/20 01:21:59, Victoria Kirst wrote:
> The case I'm worried about is the following: say ScheduleRead_Locked() is
called
> twice in a row. This posts 2 FFmpegAudioDecoder::Read tasks in the decoder
> thread. Then when these read tasks are executed, the demuxer stream queues up
2
> read callbacks in its |read_queue_|. Then the decoder decodes for both of
these
> requests, and for each finished read request, DecodeBuffer is executed... the
> first time DecodeBuffer executes, |pending_read_| is true, but the 2nd time it
> executes it would CHECK.
> 
> Buuuuut yeah, I don't know if that's possible!! 

D'OH nvm! I forgot that you put "if (pending_read_) return;" in
ScheduleRead_Locked()! That should be enough to prevent the problem I was
thinking of, so I'm pretty sure you're okay to CHECK here then - from
ScheduleRead_Lock() onward, everything looks to be in balance.

On 2011/12/20 01:21:59, Victoria Kirst wrote:
> but then, wouldn't you also be able to replace |read_queue_| in
FFmpegDemuxerStream
> with just a |read_cb_| or something?

I think this should still be true for the audio case, but the other types of
decoders might issue multiple reads at the same time.

[scherkus (not reviewing), 2011-12-20 15:27:57]

http://codereview.chromium.org/8763010/diff/22001/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (left):

http://codereview.chromium.org/8763010/diff/22001/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:152: if (!pending_reads_)
On 2011/12/20 01:22:00, Victoria Kirst wrote:
> On 2011/12/19 19:30:13, scherkus wrote:
> > vrk: do you know if replacing this with a CHECK() is OK?
> 
> Bah, spent a long time looking through the code today to try to figure it out.
> The best answer I can come up with is: if you're 100% sure it's correct to
> replace |pending_reads_| with |pending_read_|, then it is correct to add the
> CHECK here.
> 
> > under which circumstances did ConsumeAudioSamples() get called when there
> wasn't
> > a pending read?
> 
> I believe the only ways for |pending_read_| to be false at that time is if a)
> DecodedAudioReady is called before ScheduleRead_Locked, or b) the
> ScheduleRead_Locked and DecodedAudioReady calls are imbalanced -- that is, the
> code assumes ScheduleRead_Locked will not be called again until
> DecodedAudioReady is executed, but if that assumption is false then you may
> CHECK.
> 
> Scenario (a) can't happen. DecodedAudioReady is passed in as a CB to the
demuxer
> via ScheduleRead_Locked(), so it's not possible for it to be executed before
> ScheduleRead_Locked() is executed.
> 
> Scenario (b) *shouldn't* happen, ... but I'm a little wary because I don't
> understand the threading story completely.
> 
> The case I'm worried about is the following: say ScheduleRead_Locked() is
called
> twice in a row. This posts 2 FFmpegAudioDecoder::Read tasks in the decoder
> thread. Then when these read tasks are executed, the demuxer stream queues up
2
> read callbacks in its |read_queue_|. Then the decoder decodes for both of
these
> requests, and for each finished read request, DecodeBuffer is executed... the
> first time DecodeBuffer executes, |pending_read_| is true, but the 2nd time it
> executes it would CHECK.
> 
> Buuuuut yeah, I don't know if that's possible!! If there really can't be more
> than one pending read at a time, then you should be able to CHECK here... but
> then, wouldn't you also be able to replace |read_queue_| in
FFmpegDemuxerStream
> with just a |read_cb_| or something?

Thanks for digging in!

Yes all read calls are supposed to be balanced and if they're not I'm more than
happy to have CHECK() calls to catch such violations.

You're correct in observing that FFmpegDemuxerStream is still in the old school
mode of expecting concurrent reads but that should go away as well.