Add AudioBufferStream.

Like the VideoFrameStream, AudioBufferStream wraps a DemuxerStream and an AudioDecoder, providing AudioBuffers to its client.

This moves DemuxerStream reading logic out of all of the AudioDecoders.

AudioBufferStream is really just a typedef of DecoderStream<DemuxerStream::AUDIO>, just as VideoFrameStream is for DemuxerStream::VIDEO.

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

[rileya (GONE FROM CHROMIUM), 2014-02-19 22:37:51]

Still somewhat work-in-progress, (I'm still finding little fixes and things that
need cleanup here and there...) and I'll need to rework/rewrite a good number of
unit tests.

https://codereview.chromium.org/141243003/diff/200001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/200001/media/base/audio_decode...
media/base/audio_decoder.h:70: virtual bool CanReadWithoutStalling() const;
Borrowed this from VideoDecoder, this will be always false for now, but this
makes the Decoder interfaces match better, and may one day be useful for some
AudioDecoder... (the alternative is having a few more template specializations
elsewhere).

https://codereview.chromium.org/141243003/diff/200001/media/base/audio_decode...
media/base/audio_decoder.h:80: virtual bool HasQueuedData() const;
This seems like a clunky way of doing this... Another way of doing this that I
considered was returning a kHasMoreData status code from Decode, to signify that
we have queued data that needs to be exhausted before we want another
DecoderBuffer (but that seemed even worse than this, since the DecoderStream
would have to remember that for the next Read() call...).

I'm open to suggestions here...

https://codereview.chromium.org/141243003/diff/200001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/200001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:1: /*
I'll need to rewrite a bunch of tests... but I'd like some feedback on the
implementation before I do so.

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:283: // getters from AudioDecoder, and
adjust this accordingly.
This will require some plumbing that's beyond the scope of this already
large-ish CL... we'll also need to properly update the |algorithm_| upon config
changes.

[DaleCurtis, 2014-02-21 21:48:47]

Overall looking really good! Nice work.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:24: enum Status {
Since both AudioDecoder and VideoDecoder now have the same errors, should we put
these in a decoder_status.h file or something?

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:70: virtual bool CanReadWithoutStalling() const;
Seems this and HasQueuedData are pretty similar, but I'm not super familiar with
why we have this.  Looking through the code it seems to relate to available
allocations for GPU video decoding.

Is it possible to reuse this to do the same thing for audio?  I suspect the
answer is no, but it seems worth considering.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:72: // Returns various information about the decoded
audio format.
Can these be removed in this CL or are you saving them for the next one?

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:62: base::AutoLock auto_lock(lock_);
What's this for?  To ensure outstanding calls have completed?  ARI is
ref-counted, so nothing should be in flight at destruction time.

I see VRI does this, but it also seems wrong to me there. xhwang@/scherkus@ do
you know why it's done there?

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide...

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:283: // getters from AudioDecoder, and
adjust this accordingly.
On 2014/02/19 22:37:51, rileya wrote:
> This will require some plumbing that's beyond the scope of this already
> large-ish CL... we'll also need to properly update the |algorithm_| upon
config
> changes.

My plan was for ARI and ARA to be agnostic to config changes, the resampling and
channel remixing will happen in some wrapper around AudioBuffers received from
the AudioBufferStream.  We can discuss more offline.

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:123: void OnStatistics(const
PipelineStatistics& statistics) {}
Why?

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:411: scoped_refptr<DecoderBuffer>
buffer = new DecoderBuffer(8);
Seems like this would just cause decoder errors.  Just return an EOS buffer like
the VRI test?

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
media/filters/decoder_stream.cc:354: Decode(NULL);
Won't this happen naturally as the decoder is drained at EOS / seek?  I'm not
sure I understand why this needs to be special cased (since it wasn't before
that I can see).  Can you elaborate?

https://codereview.chromium.org/141243003/diff/300001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:1: /*
Broke? :)

[rileya (GONE FROM CHROMIUM), 2014-02-21 23:04:02]

No actual changes to the code yet, just responses to comments.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:24: enum Status {
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Since both AudioDecoder and VideoDecoder now have the same errors, should we
put
> these in a decoder_status.h file or something?

Sounds good, that'd also remove a DecoderStreamTraits member.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:70: virtual bool CanReadWithoutStalling() const;
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Seems this and HasQueuedData are pretty similar, but I'm not super familiar
with
> why we have this.  Looking through the code it seems to relate to available
> allocations for GPU video decoding.
> 
> Is it possible to reuse this to do the same thing for audio?  I suspect the
> answer is no, but it seems worth considering.

I'm unsure, I'm not really familiar with the gpu decoder...

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:72: // Returns various information about the decoded
audio format.
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Can these be removed in this CL or are you saving them for the next one?

I'm saving that for the next one, it didn't seem completely trivial to remove
them and I figured this CL was large enough as-is.

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:62: base::AutoLock auto_lock(lock_);
On 2014/02/21 21:48:48, DaleCurtis wrote:
> What's this for?  To ensure outstanding calls have completed?  ARI is
> ref-counted, so nothing should be in flight at destruction time.
> 
> I see VRI does this, but it also seems wrong to me there. xhwang@/scherkus@ do
> you know why it's done there?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide...

Yeah, I added this because of VRI doing it, I wasn't sure...

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:123: void OnStatistics(const
PipelineStatistics& statistics) {}
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Why?

I think I was getting uninteresting mock warnings and that was my lazy way of
shutting it up. Previously it never got called because the decoder was mocked
(whereas now the AudioBuferStream reports statistics and isn't mocked (it
probably should be now though)).

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:411: scoped_refptr<DecoderBuffer>
buffer = new DecoderBuffer(8);
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Seems like this would just cause decoder errors.  Just return an EOS buffer
like
> the VRI test?

This was another quick hack to make things compile that I forgot to clean up,
sounds good.

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
media/filters/decoder_stream.cc:354: Decode(NULL);
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Won't this happen naturally as the decoder is drained at EOS / seek?  I'm not
> sure I understand why this needs to be special cased (since it wasn't before
> that I can see).  Can you elaborate?

I just realized how unintuitive this looks now, haha. This logic shouldn't be in
here, it was just easy to put it here, basically if the Decoder has queued data
still, we can return that instead of actually doing a Demuxer read. Passing NULL
(since we're just getting something from the queue, we don't have a
DecoderBuffer) makes it look like an EOS though, which didn't occur to me until
now.

This is the problem we discussed at some point of one DecoderBuffer potentially
decoding into multiple AudioBuffers (for the ffmpeg and decrypting decoders
anyway). For one call to AudioBufferStream::Read, the caller expects a single
AudioBuffer. So unlike VideoFrames we may not need to initiate a Demuxer read
for every Read() call.

Previously the Decoder managed a queue of AudioBuffers and only when it ran out
did it initiate a Demuxer read. Now that the Decoder no longer controls the
DemuxerStream we have this awkward situation where the queue of additional
AudioBuffers lives in the Decoder still, and the DemuxerStream is controlled a
level above in the ABS.

So here I added HasQueuedData() and made the ABS keep calling Decode() with NULL
DecoderBuffers after the first call with the DecoderBuffer until HasQueuedData()
is false. This seems clunky.

If we're willing to change the interface of Decode() vs VideoDecoder, then maybe
we could return a vector of AudioBuffers and then move the queue of AudioBuffers
up into the ABS. This would however clutter DecoderStream with more
specialization for audio only (we could make VideoDecoder match I suppose, but
none of the current decoders require this behavior).

https://codereview.chromium.org/141243003/diff/300001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.h:114: // Pointer to the demuxer stream
that will feed us compressed buffers.
This comment needs an update.

https://codereview.chromium.org/141243003/diff/300001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:1: /*
On 2014/02/21 21:48:48, DaleCurtis wrote:
> Broke? :)

For the moment yes, I wanted feedback on the implementation before digging into
the tests.

[xhwang, 2014-02-21 23:57:56]

Didn't review the whole CL. Mainly replying comments.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:70: virtual bool CanReadWithoutStalling() const;
On 2014/02/21 23:04:02, rileya wrote:
> On 2014/02/21 21:48:48, DaleCurtis wrote:
> > Seems this and HasQueuedData are pretty similar, but I'm not super familiar
> with
> > why we have this.  Looking through the code it seems to relate to available
> > allocations for GPU video decoding.
> > 
> > Is it possible to reuse this to do the same thing for audio?  I suspect the
> > answer is no, but it seems worth considering.
> 
> I'm unsure, I'm not really familiar with the gpu decoder...

Yeah, this is a hacky and confusing function. For GVD, it wraps decoded frames
into picture buffers. A GVD only allocate M pictures buffers, i.e. there can be
at most M decoded frames outstanding. Now if the media pipeline wants to preroll
N frames, with N > M, the (M+1)'th decode will never finish, because the GVD
cannot grab the (M+1)'th picture buffer. Then the preroll never finishes,
because the VideoRendererImpl cannot preroll N frames.

This function basically let us know the the decoder cannot do any decoding
anymore, so that we don't get into this mud in the first place.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:80: virtual bool HasQueuedData() const;
Instead of
if (HasQueuedData())
  Decode(NULL);

Can we just have
scoped_refptr<AudioBuffer> GetAudioBuffer();
which return NULL if no data queued internally.

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:62: base::AutoLock auto_lock(lock_);
On 2014/02/21 21:48:48, DaleCurtis wrote:
> What's this for?  To ensure outstanding calls have completed?  ARI is
> ref-counted, so nothing should be in flight at destruction time.
> 
> I see VRI does this, but it also seems wrong to me there. xhwang@/scherkus@
do
> you know why it's done there?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide...

Yeah, this shouldn't be necessary unless someone is holding a raw pointer to
|this|, which doesn't seem to be the case.

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
media/filters/decoder_stream.cc:354: Decode(NULL);
On 2014/02/21 23:04:02, rileya wrote:
> On 2014/02/21 21:48:48, DaleCurtis wrote:
> > Won't this happen naturally as the decoder is drained at EOS / seek?  I'm
not
> > sure I understand why this needs to be special cased (since it wasn't before
> > that I can see).  Can you elaborate?
> 
> I just realized how unintuitive this looks now, haha. This logic shouldn't be
in
> here, it was just easy to put it here, basically if the Decoder has queued
data
> still, we can return that instead of actually doing a Demuxer read. Passing
NULL
> (since we're just getting something from the queue, we don't have a
> DecoderBuffer) makes it look like an EOS though, which didn't occur to me
until
> now.
> 
> This is the problem we discussed at some point of one DecoderBuffer
potentially
> decoding into multiple AudioBuffers (for the ffmpeg and decrypting decoders
> anyway). For one call to AudioBufferStream::Read, the caller expects a single
> AudioBuffer. So unlike VideoFrames we may not need to initiate a Demuxer read
> for every Read() call.
> 
> Previously the Decoder managed a queue of AudioBuffers and only when it ran
out
> did it initiate a Demuxer read. Now that the Decoder no longer controls the
> DemuxerStream we have this awkward situation where the queue of additional
> AudioBuffers lives in the Decoder still, and the DemuxerStream is controlled a
> level above in the ABS.
> 
> So here I added HasQueuedData() and made the ABS keep calling Decode() with
NULL
> DecoderBuffers after the first call with the DecoderBuffer until
HasQueuedData()
> is false. This seems clunky.
> 
> If we're willing to change the interface of Decode() vs VideoDecoder, then
maybe
> we could return a vector of AudioBuffers and then move the queue of
AudioBuffers
> up into the ABS. This would however clutter DecoderStream with more
> specialization for audio only (we could make VideoDecoder match I suppose, but
> none of the current decoders require this behavior).

Some random ideas:

1, See comment above, if you have
scoped_refptr<AudioBuffer> GetAudioBuffer();
function, you can avoid this Decode(NULL) call.

2, Shouldn't this be checked/called in DS::Read() instead of in
DS::ReadFromDemuxerStream()?

3, Currently we don't async audio decoders, so (1) + (2) sgtm.

4, This can actually also be helpful for a problem in the video pipeline:
- GVD accumulates multiple decoded frames internally.
- Demuxer runs out of data (in the MSE case)
- VideoRendererImpl calls Read(). Since we only call into the decoder after the
demxuer read is returned (which never happens), the decoded frames in GVD is
stuck there.
We need to think more about this for video. But that's our of the scope of this
CL.

[rileya (GONE FROM CHROMIUM), 2014-02-24 21:04:11]

Addressed most of the comments. Various unit tests still need reworking.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:24: enum Status {
On 2014/02/21 23:04:02, rileya wrote:
> On 2014/02/21 21:48:48, DaleCurtis wrote:
> > Since both AudioDecoder and VideoDecoder now have the same errors, should we
> put
> > these in a decoder_status.h file or something?
> 
> Sounds good, that'd also remove a DecoderStreamTraits member.

I'll save this for a future CL, as this one is quite large already, and this
change would add a fair bit of extra noise.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:70: virtual bool CanReadWithoutStalling() const;
On 2014/02/21 23:57:56, xhwang wrote:
> On 2014/02/21 23:04:02, rileya wrote:
> > On 2014/02/21 21:48:48, DaleCurtis wrote:
> > > Seems this and HasQueuedData are pretty similar, but I'm not super
familiar
> > with
> > > why we have this.  Looking through the code it seems to relate to
available
> > > allocations for GPU video decoding.
> > > 
> > > Is it possible to reuse this to do the same thing for audio?  I suspect
the
> > > answer is no, but it seems worth considering.
> > 
> > I'm unsure, I'm not really familiar with the gpu decoder...
> 
> Yeah, this is a hacky and confusing function. For GVD, it wraps decoded frames
> into picture buffers. A GVD only allocate M pictures buffers, i.e. there can
be
> at most M decoded frames outstanding. Now if the media pipeline wants to
preroll
> N frames, with N > M, the (M+1)'th decode will never finish, because the GVD
> cannot grab the (M+1)'th picture buffer. Then the preroll never finishes,
> because the VideoRendererImpl cannot preroll N frames.
> 
> This function basically let us know the the decoder cannot do any decoding
> anymore, so that we don't get into this mud in the first place.

Alright, I removed this from AudioDecoder in favor of a specialization in
DecoderStream. It sounds like ideally it should be removed from VideoDecoder, so
no sense polluting AudioDecoder with it.

https://codereview.chromium.org/141243003/diff/300001/media/base/audio_decode...
media/base/audio_decoder.h:80: virtual bool HasQueuedData() const;
On 2014/02/21 23:57:56, xhwang wrote:
> Instead of
> if (HasQueuedData())
>   Decode(NULL);
> 
> Can we just have
> scoped_refptr<AudioBuffer> GetAudioBuffer();
> which return NULL if no data queued internally.

I did this, although we also need to pass back an AudioDecoder::Status (see line
61), this is not necessary for the DecryptingAudioDecoder since we only enqueue
if we're kOk, but this is not true of FFmpegAudioDecoder.

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl.cc:62: base::AutoLock auto_lock(lock_);
On 2014/02/21 23:57:56, xhwang wrote:
> On 2014/02/21 21:48:48, DaleCurtis wrote:
> > What's this for?  To ensure outstanding calls have completed?  ARI is
> > ref-counted, so nothing should be in flight at destruction time.
> > 
> > I see VRI does this, but it also seems wrong to me there. xhwang@/scherkus@
do
> > you know why it's done there?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide...
> 
> Yeah, this shouldn't be necessary unless someone is holding a raw pointer to
> |this|, which doesn't seem to be the case.

Alright, removed it. The one in VRI should probably be nuked too, but I won't
clutter this CL any more...

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:411: scoped_refptr<DecoderBuffer>
buffer = new DecoderBuffer(8);
On 2014/02/21 23:04:02, rileya wrote:
> On 2014/02/21 21:48:48, DaleCurtis wrote:
> > Seems like this would just cause decoder errors.  Just return an EOS buffer
> like
> > the VRI test?
> 
> This was another quick hack to make things compile that I forgot to clean up,
> sounds good.

Fixed.

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
media/filters/decoder_stream.cc:354: Decode(NULL);
On 2014/02/21 23:57:56, xhwang wrote:
> On 2014/02/21 23:04:02, rileya wrote:
> > On 2014/02/21 21:48:48, DaleCurtis wrote:
> > > Won't this happen naturally as the decoder is drained at EOS / seek?  I'm
> not
> > > sure I understand why this needs to be special cased (since it wasn't
before
> > > that I can see).  Can you elaborate?
> > 
> > I just realized how unintuitive this looks now, haha. This logic shouldn't
be
> in
> > here, it was just easy to put it here, basically if the Decoder has queued
> data
> > still, we can return that instead of actually doing a Demuxer read. Passing
> NULL
> > (since we're just getting something from the queue, we don't have a
> > DecoderBuffer) makes it look like an EOS though, which didn't occur to me
> until
> > now.
> > 
> > This is the problem we discussed at some point of one DecoderBuffer
> potentially
> > decoding into multiple AudioBuffers (for the ffmpeg and decrypting decoders
> > anyway). For one call to AudioBufferStream::Read, the caller expects a
single
> > AudioBuffer. So unlike VideoFrames we may not need to initiate a Demuxer
read
> > for every Read() call.
> > 
> > Previously the Decoder managed a queue of AudioBuffers and only when it ran
> out
> > did it initiate a Demuxer read. Now that the Decoder no longer controls the
> > DemuxerStream we have this awkward situation where the queue of additional
> > AudioBuffers lives in the Decoder still, and the DemuxerStream is controlled
a
> > level above in the ABS.
> > 
> > So here I added HasQueuedData() and made the ABS keep calling Decode() with
> NULL
> > DecoderBuffers after the first call with the DecoderBuffer until
> HasQueuedData()
> > is false. This seems clunky.
> > 
> > If we're willing to change the interface of Decode() vs VideoDecoder, then
> maybe
> > we could return a vector of AudioBuffers and then move the queue of
> AudioBuffers
> > up into the ABS. This would however clutter DecoderStream with more
> > specialization for audio only (we could make VideoDecoder match I suppose,
but
> > none of the current decoders require this behavior).
> 
> Some random ideas:
> 
> 1, See comment above, if you have
> scoped_refptr<AudioBuffer> GetAudioBuffer();
> function, you can avoid this Decode(NULL) call.
> 
> 2, Shouldn't this be checked/called in DS::Read() instead of in
> DS::ReadFromDemuxerStream()?
> 
> 3, Currently we don't async audio decoders, so (1) + (2) sgtm.
> 
> 4, This can actually also be helpful for a problem in the video pipeline:
> - GVD accumulates multiple decoded frames internally.
> - Demuxer runs out of data (in the MSE case)
> - VideoRendererImpl calls Read(). Since we only call into the decoder after
the
> demxuer read is returned (which never happens), the decoded frames in GVD is
> stuck there.
> We need to think more about this for video. But that's our of the scope of
this
> CL.

1) Sounds good, done.

2) Yup, this was originally placed here as a hack to avoid a huge specialization
of Read(), but I added a helper instead.

4) It sounds like maybe moving the queue of decoded output (plus status codes)
into the DecoderStream would help get rid of the VRD problem as well as this
one. I added a TODO to the header and I'll look into it for another CL.

[rileya (GONE FROM CHROMIUM), 2014-02-24 21:07:50]

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/300001/media/filters/decoder_s...
media/filters/decoder_stream.cc:354: Decode(NULL);
On 2014/02/24 21:04:11, rileya wrote:
> On 2014/02/21 23:57:56, xhwang wrote:
> > On 2014/02/21 23:04:02, rileya wrote:
> > > On 2014/02/21 21:48:48, DaleCurtis wrote:
> > > > Won't this happen naturally as the decoder is drained at EOS / seek? 
I'm
> > not
> > > > sure I understand why this needs to be special cased (since it wasn't
> before
> > > > that I can see).  Can you elaborate?
> > > 
> > > I just realized how unintuitive this looks now, haha. This logic shouldn't
> be
> > in
> > > here, it was just easy to put it here, basically if the Decoder has queued
> > data
> > > still, we can return that instead of actually doing a Demuxer read.
Passing
> > NULL
> > > (since we're just getting something from the queue, we don't have a
> > > DecoderBuffer) makes it look like an EOS though, which didn't occur to me
> > until
> > > now.
> > > 
> > > This is the problem we discussed at some point of one DecoderBuffer
> > potentially
> > > decoding into multiple AudioBuffers (for the ffmpeg and decrypting
decoders
> > > anyway). For one call to AudioBufferStream::Read, the caller expects a
> single
> > > AudioBuffer. So unlike VideoFrames we may not need to initiate a Demuxer
> read
> > > for every Read() call.
> > > 
> > > Previously the Decoder managed a queue of AudioBuffers and only when it
ran
> > out
> > > did it initiate a Demuxer read. Now that the Decoder no longer controls
the
> > > DemuxerStream we have this awkward situation where the queue of additional
> > > AudioBuffers lives in the Decoder still, and the DemuxerStream is
controlled
> a
> > > level above in the ABS.
> > > 
> > > So here I added HasQueuedData() and made the ABS keep calling Decode()
with
> > NULL
> > > DecoderBuffers after the first call with the DecoderBuffer until
> > HasQueuedData()
> > > is false. This seems clunky.
> > > 
> > > If we're willing to change the interface of Decode() vs VideoDecoder, then
> > maybe
> > > we could return a vector of AudioBuffers and then move the queue of
> > AudioBuffers
> > > up into the ABS. This would however clutter DecoderStream with more
> > > specialization for audio only (we could make VideoDecoder match I suppose,
> but
> > > none of the current decoders require this behavior).
> > 
> > Some random ideas:
> > 
> > 1, See comment above, if you have
> > scoped_refptr<AudioBuffer> GetAudioBuffer();
> > function, you can avoid this Decode(NULL) call.
> > 
> > 2, Shouldn't this be checked/called in DS::Read() instead of in
> > DS::ReadFromDemuxerStream()?
> > 
> > 3, Currently we don't async audio decoders, so (1) + (2) sgtm.
> > 
> > 4, This can actually also be helpful for a problem in the video pipeline:
> > - GVD accumulates multiple decoded frames internally.
> > - Demuxer runs out of data (in the MSE case)
> > - VideoRendererImpl calls Read(). Since we only call into the decoder after
> the
> > demxuer read is returned (which never happens), the decoded frames in GVD is
> > stuck there.
> > We need to think more about this for video. But that's our of the scope of
> this
> > CL.
> 
> 1) Sounds good, done.
> 
> 2) Yup, this was originally placed here as a hack to avoid a huge
specialization
> of Read(), but I added a helper instead.
> 
> 4) It sounds like maybe moving the queue of decoded output (plus status codes)
> into the DecoderStream would help get rid of the VRD problem as well as this
> one. I added a TODO to the header and I'll look into it for another CL.

s/VRD/GVD/

Not sure how I messed that up haha... must've been thinking of VRI...

[DaleCurtis, 2014-02-26 23:00:49]

General idea looks sound to me. I'll reserve nitty feedback until your done
polishing it. xhwang@ and I talked about allowing the decode_cb to be called
more than once (a la a permanent callback), but that's a larger change for a
follow up cl.

[xhwang, 2014-02-27 19:52:27]

Sorry for the delay. The CL looks good in general, just having some suggestions
which may make the code a bit cleaner. I'll review again when you clean up this
CL more :)

https://codereview.chromium.org/141243003/diff/700001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/700001/media/base/audio_decode...
media/base/audio_decoder.h:61: virtual scoped_refptr<AudioBuffer>
GetAudioBuffer(Status* status);
I wonder shouldn't this |status| always be kOk? We shouldn't call this function
if the decoder is pending reset or stop in the first place. I may miss something
here though...

https://codereview.chromium.org/141243003/diff/700001/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/141243003/diff/700001/media/base/video_decode...
media/base/video_decoder.h:23: // TODO(rileya): Now that  both AudioDecoder and
VideoDecoder Status enums
double space after "that"

https://codereview.chromium.org/141243003/diff/700001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/700001/media/filters/decoder_s...
media/filters/decoder_stream.cc:357: bool
DecoderStream<DemuxerStream::AUDIO>::SendQueuedOutput() {
The name "SendQueuedOutput" is a bit odd. How about adding GetDecodeOutput() to
both AudioDecoder and VideoDecoder, and having VideoDecoder::GetDecodeOutput()
always return NULL, then we don't need this specialization here.

https://codereview.chromium.org/141243003/diff/700001/media/filters/decoder_s...
media/filters/decoder_stream.cc:360: OnDecodeOutputReady(0, status, buffer);
since the first parameter is always 0, how about calling SatisfyRead() directly?

[rileya (GONE FROM CHROMIUM), 2014-03-04 00:14:26]

Alright, I did a bunch of cleanup, and fixed the FFmpegAudioDecoder and
DecryptingAudioDecoder unit tests.

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
media/filters/decoder_stream.cc:104: if (scoped_refptr<Output> output =
decoder_->GetDecodeOutput()) {
Is assignment in a conditional allowed in chromium?

https://codereview.chromium.org/141243003/diff/940001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (left):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:489:
TEST_F(DecryptingAudioDecoderTest, Reset_DuringDemuxerRead_Ok) {
All of these PendingDemuxerRead tests are no longer meaningful without a
DemuxerStream.

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:420: queued_audio_.clear();
Previously we could queue up buffers which were not kOk here, meaning we needed
to return a struct with the buffer and the status.

Instead I empty the queue and return an error, in theory there might be some
case where we throw out a valid buffer because the next buffer in the same
packet has a config change, but we'll support config changes soon enough
anyway...

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.h (left):

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:98: ReadCB read_cb_;
Conveniently, all of init/decode/reset/stop functions in FFmpegAudioDecoder now
post their callback immediately, so we don't have to deal with the headache of
reset/stop during a pending read/decode and so forth. This allowed a good bit of
cleanup (and probably more is possible).

https://codereview.chromium.org/141243003/diff/940001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.h (left):

https://codereview.chromium.org/141243003/diff/940001/media/filters/opus_audi...
media/filters/opus_audio_decoder.h:78: ReadCB read_cb_;
As with FFmpeg the removal of DemuxerStream reading logic means we no longer
have to worry about async headaches with Decode/Stop/Reset, whoo!

[DaleCurtis, 2014-03-04 01:01:32]

Looking great!

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
media/filters/decoder_stream.cc:104: if (scoped_refptr<Output> output =
decoder_->GetDecodeOutput()) {
On 2014/03/04 00:14:27, rileya wrote:
> Is assignment in a conditional allowed in chromium?

Generally frowned upon, but not unheard of.

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:28: void
DecoderStreamTraits<DemuxerStream::AUDIO>::ReportStatistics(
Both AUDIO and VIDEO seem to have the same function?  Remove the specialization?

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:177: // Callback called from within FFmpeg
to allocate a buffer based on
We put method comments in the .h file generally.  It doesn't look like this is
actually using any class members though, so you could just move it to the top of
the file as an internal static declaration.

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:255: // During decode, because reads are
issued asynchronously, it is possible to
Instead of having this similar / same comment in both the Audio and Video
decoders, is there a place we can put in the DecoderStream?

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:320: scoped_refptr<AudioBuffer>
audio_buffer;
Unused? You could just set result/audio_buffer= output and have one
decode_cb.Run() call below.

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:493: // TODO(rileya) Remove this check
once we properly support midstream audio
Note: Some of these were added to avoid ffmpeg security issues, so be sure to
run ffmpeg_regression_tests when you do remove them.

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:56: 
Remove extra line.

[rileya (GONE FROM CHROMIUM), 2014-03-04 02:03:00]

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
media/filters/decoder_stream.cc:104: if (scoped_refptr<Output> output =
decoder_->GetDecodeOutput()) {
On 2014/03/04 01:01:33, DaleCurtis wrote:
> On 2014/03/04 00:14:27, rileya wrote:
> > Is assignment in a conditional allowed in chromium?
> 
> Generally frowned upon, but not unheard of.

Is it acceptable in this case, or am I better off getting rid of this?

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:28: void
DecoderStreamTraits<DemuxerStream::AUDIO>::ReportStatistics(
On 2014/03/04 01:01:33, DaleCurtis wrote:
> Both AUDIO and VIDEO seem to have the same function?  Remove the
specialization?

Unfortunately AUDIO uses "statistics.audio_bytes_decoded" and VIDEO uses
"statistics.video_bytes_decoded".

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:177: // Callback called from within FFmpeg
to allocate a buffer based on
On 2014/03/04 01:01:33, DaleCurtis wrote:
> We put method comments in the .h file generally.  It doesn't look like this is
> actually using any class members though, so you could just move it to the top
of
> the file as an internal static declaration.

Oh wow, I saw that the original version did this and didn't look closely, but
looks like this has no reason to be a member of FFmpegAudioDecoder. I hoisted
all of this up into the already-existing static function above (which previously
just called this on the decoder...).

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:255: // During decode, because reads are
issued asynchronously, it is possible to
On 2014/03/04 01:01:33, DaleCurtis wrote:
> Instead of having this similar / same comment in both the Audio and Video
> decoders, is there a place we can put in the DecoderStream?

The problem is that this logic is only shared by the FFmpeg Audio/VideoDecoders,
Opus doesn't even track states, and DecryptingAudioDecoder has a somewhat
different set of states.

There's probably some refactoring potential in here somewhere (FFmpeg audio and
video decoders are pretty similar for one, and most decoders use similar but
subtly different state machines... perhaps a common base for the various
Decoders to keep some of the state transitions and stuff consistent would be
useful...).

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:320: scoped_refptr<AudioBuffer>
audio_buffer;
On 2014/03/04 01:01:33, DaleCurtis wrote:
> Unused? You could just set result/audio_buffer= output and have one
> decode_cb.Run() call below.

Whoops, yup. I ended up just removing it, moving the callback call to the bottom
made the control flow harder to follow.

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.h (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.h:56: 
On 2014/03/04 01:01:33, DaleCurtis wrote:
> Remove extra line.

Done.

[DaleCurtis, 2014-03-04 22:22:16]

lgtm.  Great work! \o/

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/940001/media/filters/decoder_s...
media/filters/decoder_stream.cc:104: if (scoped_refptr<Output> output =
decoder_->GetDecodeOutput()) {
On 2014/03/04 02:03:01, rileya wrote:
> On 2014/03/04 01:01:33, DaleCurtis wrote:
> > On 2014/03/04 00:14:27, rileya wrote:
> > > Is assignment in a conditional allowed in chromium?
> > 
> > Generally frowned upon, but not unheard of.
> 
> Is it acceptable in this case, or am I better off getting rid of this?

I'd move it out for clarity.

[xhwang, 2014-03-05 00:40:46]

It's looking really good. Thanks for the hard work and patience. Just a few more
comments (mostly nits).

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl.h:182: // Stops the |decoder_| if present.
Ensures |stop_cb_| is called.
Is this from rebasing? Anyway, where is |stop_cb_|?

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:253: // We should have no reads.
s/read/decode

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:284: bool IsReadPending() const {
s/Read/Decode

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:522: // Used for satisfying reads.
ditto

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
media/filters/decoder_stream.cc:104: if (scoped_refptr<Output> output =
decoder_->GetDecodeOutput()) {
This is neat, but I am not sure whether it's more readable... See offline chats.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
media/filters/decoder_stream.cc:105: SatisfyRead(OK, output);
Can we add a unit test to cover this logic?

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:84: state_ = kPendingConfigChange;
In DVD, we use kPendingDecoderInit for this status. Is there any reason to use
kPendingConfigChange here?

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:129: }
nit: no need for {}

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:146:
decoder_->GetDecodeOutput()) {
ditto about this style :)

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:163:
decoder_->Decode(ProvideDecoderBuffer(),
It's odd that in a lot of tests we call PDB(), then we expect PDB() to be
called. Most of them can be replaced with decoder_->Decode(encrypted_buffer_,
...)

The only exception is on line 174, where PDB() will return multiple buffers. But
we should be able to do what DVDTest is doing:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr...

https://codereview.chromium.org/141243003/diff/960001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:58: AVFrame* frame,
indent is off

https://codereview.chromium.org/141243003/diff/960001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:323: task_runner_->PostTask(FROM_HERE,
closure);
Nice!

[rileya (GONE FROM CHROMIUM), 2014-03-05 08:08:27]

Responses to comments and fixes to the simpler ones. Various unit test
fixes/additions will come in the next patchset.

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl.h:182: // Stops the |decoder_| if present.
Ensures |stop_cb_| is called.
On 2014/03/05 00:40:46, xhwang wrote:
> Is this from rebasing? Anyway, where is |stop_cb_|?

Must have been (or this declaration managed to survive when the definition was
thrown out).

stop_cb_ was removed since it is always ResetAndReturn-ed within Stop().

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:253: // We should have no reads.
On 2014/03/05 00:40:46, xhwang wrote:
> s/read/decode

To this and the other comments in this file, I think "Read" is generally more
appropriate than "Decode" (AudioRendererImpl calls "Read()" on
AudioBufferStream). I think it probably makes the most sense to mock
AudioBufferStream and remove most of the references to  AudioDecoder/decoding.
The tests here also probably need some more general cleanup too. I'll work on
this.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
media/filters/decoder_stream.cc:104: if (scoped_refptr<Output> output =
decoder_->GetDecodeOutput()) {
On 2014/03/05 00:40:46, xhwang wrote:
> This is neat, but I am not sure whether it's more readable... See offline
chats.

Done.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
media/filters/decoder_stream.cc:105: SatisfyRead(OK, output);
On 2014/03/05 00:40:46, xhwang wrote:
> Can we add a unit test to cover this logic?

Sounds good, will do.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:84: state_ = kPendingConfigChange;
On 2014/03/05 00:40:46, xhwang wrote:
> In DVD, we use kPendingDecoderInit for this status. Is there any reason to use
> kPendingConfigChange here?

Not that I can see. Removed kPendingConfigChange entirely.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:129: }
On 2014/03/05 00:40:46, xhwang wrote:
> nit: no need for {}

Removed.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:146:
decoder_->GetDecodeOutput()) {
On 2014/03/05 00:40:46, xhwang wrote:
> ditto about this style :)

Fixed.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:163:
decoder_->Decode(ProvideDecoderBuffer(),
On 2014/03/05 00:40:46, xhwang wrote:
> It's odd that in a lot of tests we call PDB(), then we expect PDB() to be
> called. Most of them can be replaced with decoder_->Decode(encrypted_buffer_,
> ...)
> 
> The only exception is on line 174, where PDB() will return multiple buffers.
But
> we should be able to do what DVDTest is doing:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/decr...

Good point. I'd pretty much just replaced all the Demuxer::Read mock
expectations with PDB(). I'll rework this.

https://codereview.chromium.org/141243003/diff/960001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:58: AVFrame* frame,
On 2014/03/05 00:40:46, xhwang wrote:
> indent is off

Fixed.

[xhwang, 2014-03-06 16:41:12]

We are almost there! I'll take another look after you update the tests then
we'll be good to go!

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/audio_ren...
media/filters/audio_renderer_impl_unittest.cc:253: // We should have no reads.
On 2014/03/05 08:08:28, rileya wrote:
> On 2014/03/05 00:40:46, xhwang wrote:
> > s/read/decode
> 
> To this and the other comments in this file, I think "Read" is generally more
> appropriate than "Decode" (AudioRendererImpl calls "Read()" on
> AudioBufferStream). I think it probably makes the most sense to mock
> AudioBufferStream and remove most of the references to  AudioDecoder/decoding.
> The tests here also probably need some more general cleanup too. I'll work on
> this.

I see. "read" sgtm. You can do the cleanup later.

[rileya (GONE FROM CHROMIUM), 2014-03-06 22:11:55]

Alright, did the unit test cleanup, and added one new test.

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/960001/media/filters/decoder_s...
media/filters/decoder_stream.cc:105: SatisfyRead(OK, output);
On 2014/03/05 08:08:28, rileya wrote:
> On 2014/03/05 00:40:46, xhwang wrote:
> > Can we add a unit test to cover this logic?
> 
> Sounds good, will do.

Done. There's currently no tests for DecoderStream, so I added it to
VideoFrameStream's tests (and filed http://crbug.com/350072).

[xhwang, 2014-03-06 23:32:39]

https://codereview.chromium.org/141243003/diff/1010001/media/filters/fake_vid...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/141243003/diff/1010001/media/filters/fake_vid...
media/filters/fake_video_decoder.cc:111: }
nit: no need for braces.

https://codereview.chromium.org/141243003/diff/1010001/media/filters/fake_vid...
media/filters/fake_video_decoder.cc:113: return VideoFrame::CreateEOSFrame();
This isn't quite right. Even though this decoder is a fake, it tries to be as
real as possible. An EOS frame in the middle of the decoding process is not real
:)

Actually we may not need to do anything to "queue read". FakeVideoDecoder
supports decoding delay, which will actually queue decoded frames internally.
GetDecodeOutput() can directly return those frames.

One problem with this is that we'll always drain the decoder before trying new
decodes. In reality, all of our video decoders don't support GetDecodeOutput().
So it's still worth testing the case where FakeVideoDecoder::GetDecodeOutput()
always return NULL. 

Can we change the ctor of FakeVideoDecoder to
FakeVideoDecoder(int decoding_delay, bool supportsGetDecodeOutput), so that in
VFSTest, we can add another test param to test both cases?

Am example of using multiple test parameters can be found here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...

https://codereview.chromium.org/141243003/diff/1010001/media/filters/fake_vid...
File media/filters/fake_video_decoder.h (right):

https://codereview.chromium.org/141243003/diff/1010001/media/filters/fake_vid...
media/filters/fake_video_decoder.h:46: void QueueRead();
You probably also need to update fake_video_decoder_unittest.cc.

https://codereview.chromium.org/141243003/diff/1010001/media/filters/fake_vid...
media/filters/fake_video_decoder.h:57: void SatisfyRead();
We should replace all "read" with "decode" in another CL. It seems I forgot to
do that :)

[rileya (GONE FROM CHROMIUM), 2014-03-07 18:40:20]

Okay, I added a switch for enabling GetDecodeOutput() in FakeVideoDecoder's ctor
and added a new test param for that in VideoFrameStreamTest, I also added a test
of FakeVideoDecoder::GetDecodeOutput().

[xhwang, 2014-03-07 18:47:37]

Thanks for the great work!

LGTM!

[rileya (GONE FROM CHROMIUM), 2014-03-10 18:22:24]

I just ran this against ffmpeg_regression_tests and fixed a couple issues, can I
get a quick sanity check? One was just a matter of cleaning up ffmpeg resources
to satisfy a DCHECK, the other was BTCL-ing a callback in DecoderSelector (see
comment below).

https://codereview.chromium.org/141243003/diff/1070001/media/filters/decoder_...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/1070001/media/filters/decoder_...
media/filters/decoder_stream.cc:96: read_cb_ = BindToCurrentLoop(read_cb);
Is this reasonable? This fixes a failure in ffmpeg_regression_tests where if we
return immediately due to the decoder having queued output, we attempt to
acquire a lock in read_cb that was already held by our caller, BTCL-ing ensures
we don't run until after our caller has released the lock.

[xhwang, 2014-03-10 18:36:30]

https://codereview.chromium.org/141243003/diff/1070001/media/filters/decoder_...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/141243003/diff/1070001/media/filters/decoder_...
media/filters/decoder_stream.cc:96: read_cb_ = BindToCurrentLoop(read_cb);
On 2014/03/10 18:22:25, rileya wrote:
> Is this reasonable? This fixes a failure in ffmpeg_regression_tests where if
we
> return immediately due to the decoder having queued output, we attempt to
> acquire a lock in read_cb that was already held by our caller, BTCL-ing
ensures
> we don't run until after our caller has released the lock.

In general we want to avoid unnecessary posts. For the majority case we are
doing ReadFromDemuxerStream which doesn't need BindToCurrentLoop(). Maybe on
line 107 we can post read_cb_ directly?

[rileya (GONE FROM CHROMIUM), 2014-03-10 18:52:38]

Sounds good, PostTask-ing only in that case works.

[DaleCurtis, 2014-03-10 19:04:25]

lgtm

[rileya (GONE FROM CHROMIUM), 2014-03-17 17:30:43]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-03-17 17:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/141243003/1110001

[commit-bot: I haz the power, 2014-03-17 19:10:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-17 19:10:12]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[rileya (GONE FROM CHROMIUM), 2014-03-17 19:18:42]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-03-17 19:19:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/141243003/1110001

[commit-bot: I haz the power, 2014-03-17 21:45:08]

Change committed as 257500