Add callback in VideoDecoder and AudioDecoder to return decoded frames.

Add callback in VideoDecoder and AudioDecoder to return decoded frames.

Previously decoded frames were returned using the DecodeCB callback,
which is called in response to Decode(). This didn't work very well,
particularly for GPU decoder, because decode buffers do not always map
to decoded frames 1 to 1.

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

[Ami GONE FROM CHROMIUM, 2014-05-19 23:34:50]

LGTM from a high-level survey; defer detailed review to xhwang@.

Would be nice to have a test for the condition you're fixing :)

https://codereview.chromium.org/297553002/diff/20001/media/filters/opus_audio...
File media/filters/opus_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/20001/media/filters/opus_audio...
media/filters/opus_audio_decoder_unittest.cc:99: // FIXME
???

[Sergey Ulanov, 2014-05-20 00:08:48]

https://codereview.chromium.org/297553002/diff/20001/media/filters/opus_audio...
File media/filters/opus_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/20001/media/filters/opus_audio...
media/filters/opus_audio_decoder_unittest.cc:99: // FIXME
On 2014/05/19 23:34:51, Ami Fischman wrote:
> ???

Removed now.

[xhwang, 2014-05-21 17:02:22]

On 2014/05/20 00:08:48, Sergey Ulanov wrote:
>
https://codereview.chromium.org/297553002/diff/20001/media/filters/opus_audio...
> File media/filters/opus_audio_decoder_unittest.cc (right):
> 
>
https://codereview.chromium.org/297553002/diff/20001/media/filters/opus_audio...
> media/filters/opus_audio_decoder_unittest.cc:99: // FIXME
> On 2014/05/19 23:34:51, Ami Fischman wrote:
> > ???
> 
> Removed now.

I am just back from vacation and have some higher priority task to finish this
week. Since this CL is pretty big, I'll take a look probably next week. Will
that be okay?

[Sergey Ulanov, 2014-05-21 17:49:42]

On 2014/05/21 17:02:22, xhwang wrote:
> I am just back from vacation and have some higher priority task to finish this
> week. Since this CL is pretty big, I'll take a look probably next week. Will
> that be okay?

That's ok, as long as we can still land it in M37. (that's the only CL that
blocks MediaSource from being enabled by default in chromoting, which blocks
PNaCl migration, which blocks many other things)

[Sergey Ulanov, 2014-05-28 17:23:51]

xhwang: ping :)

[xhwang, 2014-05-28 17:31:43]

On 2014/05/28 17:23:51, Sergey Ulanov wrote:
> xhwang: ping :)

Sorry for the delay. I'll start looking at it today.

[xhwang, 2014-05-29 22:15:14]

Thanks for the CL! I like the idea of having a permanent OutputCB which makes a
lot of code simpler.

Here's my first round of comments. I skimmed most implementations and haven't
reviewed any tests.

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

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:37: typedef base::Callback<void(Status)> DecodeCB;
What does this exactly mean? We need a comment here to make the API contract
clear. For example (if I understand it correctly): Called when the input has
been processed and the VideoDecoder can accept more inputs for decode.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:44: // statistics_cb is used to update global
pipeline statistics.
Add a comment on |output_cb|.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:49: // Requests samples to be decoded and returned
via the provided callback.
This needs to be updated.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:62: virtual void Reset(const base::Closure& closure)
= 0;
In this case, shall all decoded frames be simply dropped, or fired via OutputCB?

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:69: virtual void Stop() = 0;
ditto about "shall all decoded frames be simply dropped, or fired via OutputCB"?

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

https://codereview.chromium.org/297553002/diff/70001/media/base/video_decoder...
media/base/video_decoder.h:70: // Otherwise the returned frame must be NULL.
This whole comment block needs to be updated.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:209: return decoder_->CanReadWithoutStalling();
!ready_outputs_.empty() || decoder_->CanReadWithoutStalling() ?

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:342: break;
nit: DECRYPT_ERROR is going to be obsolete. Feel free to handle these two cases
together and always return DECODE_ERROR. Or you can leave it as is and I'll
clean them up.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:344: case Decoder::kAborted:
IIUIC, This should only happen during Reset(), which is covered by l.326. Can we
DCHECK that this should not happen?

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:360: ReinitializeDecoder();
Hmm, we should only ReinitializeDecoder() after the decoder is flushed (reset).
Why don't we do it on line 386 instead of here?

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:386: // ReinitializeDecoder() will be called
from OnDecodeDone().
See comment above about when to ReinitializeDecoder().

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.cc:57: output_cb_ =
BindToCurrentLoop(output_cb);
nit: BindToCurrentLoop() introduce an extra post. We are probably over-posting
the |output_cb| by doing this. I can see that on l.97, in Decode() call, we
should post the |output_cb|. But in EnqueueSamples (called in DeliverFrames(),
which is always posted), we don't really need to post the |output_cb|. Since I
assume most frames are delivered in EnqueueSamples(), it's worth dropping this
extra post.

I notice that in the original code, we always BindToCurrentLoop(decode_cb). So
this is not introduced by your CL. Feel free to keep it and I'll see whether
it's worth to improve it.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (left):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:124: Decryptor::AudioBuffers
queued_audio_frames_;
remove?

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:95: void EnqueueFrames(const
Decryptor::AudioBuffers& frames);
This comment needs to be updated, and the method should probably be renamed.

https://codereview.chromium.org/297553002/diff/70001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:230: output_cb_.Run(video_frame);
You can probably wrap l.223-230 into a helper function since it's the same as
l.212-218.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:221: decode_cb.Run(kDecodeError);
VideoDecoder API says that |decode_cb| should not be fired in the same callstack
of Decode() call. Need a post here.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:241: flush_cb_ = decode_cb;
hmm, now I see why it's called |flush_cb_|.... This is kinda a grey area. My
understanding is that "decode_cb" is a signal that the input has been
accepted/consumed by the real codec so that the caller can feed more. In this
case there's no real input buffer and the caller knows that. Can we just fire
|decode_cb| right away here? The caller (DecoderStream) should be able to easily
handle this case.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:249: decode_cb.Run(kDecodeError);
ditto

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:262: .second;
hmm... this can be improved :) We can do it later though... add a TODO?

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.h:88: struct PendingDecodeBuffer {
Shall this be PendingDecode-r-Buffer?

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.h:100: void DeliverFrame( const
scoped_refptr<VideoFrame>& frame);
nit: drop extra space before "const"

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.h:142: DecodeCB flush_cb_;
Usually people associate "flush" with "Reset". So it's a bit confusing to have
"flush_cb_" and "pending_reset_cb_|. Can we just name it decode_cb_?

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:358:
base::ResetAndReturn(&decode_cb_).Run(kOk);
fix indent

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:369: // If we didn't get a frame we need more
data.
This comment is obsolete.

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:372: }
nit: {} is optional in this case..

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:374:
base::ResetAndReturn(&decode_cb_).Run(kOk);
nit: Move this up to l.368? Personally I feel it a bit cleaner to return
|decode_cb_| as soon as the input has been consumed.

[Ami GONE FROM CHROMIUM, 2014-06-02 22:15:43]

FTR this also fixes crbug.com/332185 (googler-only, sorry).  Yay!

[Sergey Ulanov, 2014-06-03 00:08:10]

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

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:37: typedef base::Callback<void(Status)> DecodeCB;
On 2014/05/29 22:15:14, xhwang wrote:
> What does this exactly mean? We need a comment here to make the API contract
> clear. For example (if I understand it correctly): Called when the input has
> been processed and the VideoDecoder can accept more inputs for decode.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:44: // statistics_cb is used to update global
pipeline statistics.
On 2014/05/29 22:15:14, xhwang wrote:
> Add a comment on |output_cb|.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:49: // Requests samples to be decoded and returned
via the provided callback.
On 2014/05/29 22:15:14, xhwang wrote:
> This needs to be updated.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:62: virtual void Reset(const base::Closure& closure)
= 0;
On 2014/05/29 22:15:14, xhwang wrote:
> In this case, shall all decoded frames be simply dropped, or fired via
OutputCB?

I think the comment is not entirely true. The buffers may not be actually
dropped. Audio decoders post a task for all callbacks, and these tasks are not
canceled by Reset(). Updated it to reflect this issue.

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:69: virtual void Stop() = 0;
On 2014/05/29 22:15:14, xhwang wrote:
> ditto about "shall all decoded frames be simply dropped, or fired via
OutputCB"?

Done.

BTW, Do we really need this method? Given that the object is not usable after
Stop() it looks like decoders can just call Stop() from destructor. What am I
missing?

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

https://codereview.chromium.org/297553002/diff/70001/media/base/video_decoder...
media/base/video_decoder.h:70: // Otherwise the returned frame must be NULL.
On 2014/05/29 22:15:14, xhwang wrote:
> This whole comment block needs to be updated.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:209: return decoder_->CanReadWithoutStalling();
On 2014/05/29 22:15:14, xhwang wrote:
> !ready_outputs_.empty() || decoder_->CanReadWithoutStalling() ?

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:342: break;
On 2014/05/29 22:15:14, xhwang wrote:
> nit: DECRYPT_ERROR is going to be obsolete. Feel free to handle these two
cases
> together and always return DECODE_ERROR. Or you can leave it as is and I'll
> clean them up.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:344: case Decoder::kAborted:
On 2014/05/29 22:15:14, xhwang wrote:
> IIUIC, This should only happen during Reset(), which is covered by l.326. Can
we
> DCHECK that this should not happen?

No. This came up with a previous CL as well:
https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s...
(line 350). There are cases when we get into this state without Reset().

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:360: ReinitializeDecoder();
On 2014/05/29 22:15:14, xhwang wrote:
> Hmm, we should only ReinitializeDecoder() after the decoder is flushed
(reset).
> Why don't we do it on line 386 instead of here?

ReinitializeDecoder() should be called only after all Decode() requests have
been processes when the decoder supports parallel decoding. That's why we need
the !pending_decode_requests_ check above. That condition won't be true in
OnDecodeOutputReady(). When Decoder::Decode(EOS) is called the decoder
implementation must call output_cb for each output frame in the queue, before
decode_cb can be called - I've added comments about it in video_decoder.h .
I.e. the requirements for how EOS buffer should be handled are different from
regular buffers. For regular frames |output_cb| may be called before or after
|decode_cb| (and GPU decoder calls |output_cb| after |decode_cb|), but when
processing EOS buffer decoders must always call output_cb before decode_cb.

I think it would be better if we had VideoDecoder::Flush() instead of passing
EOS buffer to the decoder, but I don't think it has to be in this CL.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:386: // ReinitializeDecoder() will be called
from OnDecodeDone().
On 2014/05/29 22:15:14, xhwang wrote:
> See comment above about when to ReinitializeDecoder().

See my comment - it's better to keep it in OnDecodeDone()

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.cc:57: output_cb_ =
BindToCurrentLoop(output_cb);
On 2014/05/29 22:15:14, xhwang wrote:
> nit: BindToCurrentLoop() introduce an extra post. We are probably over-posting
> the |output_cb| by doing this. I can see that on l.97, in Decode() call, we
> should post the |output_cb|. But in EnqueueSamples (called in DeliverFrames(),
> which is always posted), we don't really need to post the |output_cb|. Since I
> assume most frames are delivered in EnqueueSamples(), it's worth dropping this
> extra post.
> 
> I notice that in the original code, we always BindToCurrentLoop(decode_cb). So
> this is not introduced by your CL. Feel free to keep it and I'll see whether
> it's worth to improve it.

VideoDecoder contract says that |decode_cb| is not called before Decode()
returns, so |decode_cb| must be posted. Similarly DecoderStream contract
requires that Read() is completed asynchronously. It should be possible to allow
output_cb to be called from Decode(), but I think it's better to do it later in
a separate CL.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (left):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:124: Decryptor::AudioBuffers
queued_audio_frames_;
On 2014/05/29 22:15:14, xhwang wrote:
> remove?

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:95: void EnqueueFrames(const
Decryptor::AudioBuffers& frames);
On 2014/05/29 22:15:14, xhwang wrote:
> This comment needs to be updated, and the method should probably be renamed.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:230: output_cb_.Run(video_frame);
On 2014/05/29 22:15:14, xhwang wrote:
> You can probably wrap l.223-230 into a helper function since it's the same as
> l.212-218.

Formatted this code to avoid duplication.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:221: decode_cb.Run(kDecodeError);
On 2014/05/29 22:15:14, xhwang wrote:
> VideoDecoder API says that |decode_cb| should not be fired in the same
callstack
> of Decode() call. Need a post here. 

Thanks for catching it. Added BindToCurrentLoop().

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:241: flush_cb_ = decode_cb;
On 2014/05/29 22:15:14, xhwang wrote:
> hmm, now I see why it's called |flush_cb_|.... This is kinda a grey area. My
> understanding is that "decode_cb" is a signal that the input has been
> accepted/consumed by the real codec so that the caller can feed more. In this
> case there's no real input buffer and the caller knows that. Can we just fire
> |decode_cb| right away here? The caller (DecoderStream) should be able to
easily
> handle this case.

See my previous comments about flushing. When flushing |decode_cb| must be
called after flushing has finished. Otherwise DecodeStream wouldn't know when
the decoder can be reset after flushing.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:249: decode_cb.Run(kDecodeError);
On 2014/05/29 22:15:14, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:262: .second;
On 2014/05/29 22:15:14, xhwang wrote:
> hmm... this can be improved :) We can do it later though... add a TODO?

reformatted this code to make it more readable.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.h:88: struct PendingDecodeBuffer {
On 2014/05/29 22:15:14, xhwang wrote:
> Shall this be PendingDecode-r-Buffer?

Yes

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.h:100: void DeliverFrame( const
scoped_refptr<VideoFrame>& frame);
On 2014/05/29 22:15:14, xhwang wrote:
> nit: drop extra space before "const"

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.h:142: DecodeCB flush_cb_;
On 2014/05/29 22:15:14, xhwang wrote:
> Usually people associate "flush" with "Reset". So it's a bit confusing to have
> "flush_cb_" and "pending_reset_cb_|. Can we just name it decode_cb_?

I think Flush and Reset are distinct operations. VideoDecodeAccelerator does
have Flush() and Reset(). There is no explicit Flush() for VideoDecoder, but as
I mentioned before it might be better to add it - we already have it implicitly
as Decode(EOS). Anyway I renamed this to |eos_decode_cb_| for now..

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:358:
base::ResetAndReturn(&decode_cb_).Run(kOk);
On 2014/05/29 22:15:14, xhwang wrote:
> fix indent

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:369: // If we didn't get a frame we need more
data.
On 2014/05/29 22:15:14, xhwang wrote:
> This comment is obsolete.

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:372: }
On 2014/05/29 22:15:14, xhwang wrote:
> nit: {} is optional in this case..

Done.

https://codereview.chromium.org/297553002/diff/70001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:374:
base::ResetAndReturn(&decode_cb_).Run(kOk);
On 2014/05/29 22:15:14, xhwang wrote:
> nit: Move this up to l.368? Personally I feel it a bit cleaner to return
> |decode_cb_| as soon as the input has been consumed.

Done.

[xhwang, 2014-06-05 21:53:51]

Some of my comments are in PS4. Most of them are in PS5. Sorry for the
inconvenience.

Mostly looking good. Just some discussion/nits.

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

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:62: virtual void Reset(const base::Closure& closure)
= 0;
On 2014/06/03 00:08:11, Sergey Ulanov wrote:
> On 2014/05/29 22:15:14, xhwang wrote:
> > In this case, shall all decoded frames be simply dropped, or fired via
> OutputCB?
> 
> I think the comment is not entirely true. The buffers may not be actually
> dropped. Audio decoders post a task for all callbacks, and these tasks are not
> canceled by Reset(). Updated it to reflect this issue.

To clarify, do you mean when Reset() is called, there may already be DecodeCBs
posted to the message loop but not executed yet? In that case, the AudioDecoders
can always post the reset callback. Callbacks should be executed in the order
they are posted so my previous comment will be valid. Am I missing anything?

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:69: virtual void Stop() = 0;
On 2014/06/03 00:08:11, Sergey Ulanov wrote:
> On 2014/05/29 22:15:14, xhwang wrote:
> > ditto about "shall all decoded frames be simply dropped, or fired via
> OutputCB"?
> 
> Done.
> 
> BTW, Do we really need this method? Given that the object is not usable after
> Stop() it looks like decoders can just call Stop() from destructor. What am I
> missing?

You are absolutely right! We are planning to drop Stop() call entirely. See
crbug/349211

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:344: case Decoder::kAborted:
On 2014/06/03 00:08:11, Sergey Ulanov wrote:
> On 2014/05/29 22:15:14, xhwang wrote:
> > IIUIC, This should only happen during Reset(), which is covered by l.326.
Can
> we
> > DCHECK that this should not happen?
> 
> No. This came up with a previous CL as well:
>
https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s...
> (line 350). There are cases when we get into this state without Reset().

hmm... Thanks for reminding me of that discussion! 

If you look at l.463 (OnBufferReady), if DemuxerStream::kAborted is received,
we'll abort the |read_cb_| immediately with AudioDecoder::kAborted. In other
words, the decoder only takes normal buffers or EOS as inputs. And the only case
it could return kAborted is because of Reset()/Stop().

Now I looked at the tests [Audio|Video]RendererImplTest.AbortPendingRead_*
again. They are testing something that should never happen. (They could happen
previously because decoder calls DemuxerStream::Read().) So I think these tests
are obsolete now and we may just remove them.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:360: ReinitializeDecoder();
On 2014/06/03 00:08:11, Sergey Ulanov wrote:
> On 2014/05/29 22:15:14, xhwang wrote:
> > Hmm, we should only ReinitializeDecoder() after the decoder is flushed
> (reset).
> > Why don't we do it on line 386 instead of here?
> 
> ReinitializeDecoder() should be called only after all Decode() requests have
> been processes when the decoder supports parallel decoding. That's why we need
> the !pending_decode_requests_ check above. That condition won't be true in
> OnDecodeOutputReady(). When Decoder::Decode(EOS) is called the decoder
> implementation must call output_cb for each output frame in the queue, before
> decode_cb can be called - I've added comments about it in video_decoder.h .

Now I understand that comment. Thanks.

> I.e. the requirements for how EOS buffer should be handled are different from
> regular buffers. For regular frames |output_cb| may be called before or after
> |decode_cb| (and GPU decoder calls |output_cb| after |decode_cb|), but when
> processing EOS buffer decoders must always call output_cb before decode_cb.
> 
> I think it would be better if we had VideoDecoder::Flush() instead of passing
> EOS buffer to the decoder, but I don't think it has to be in this CL.

This makes sense. Previously we need multiple Decode(EOS) calls to flush the
decode. Now since we have OutputCB, we only need one call. Agreed that Flush()
will be cleaner.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:464: SatisfyRead(DEMUXER_READ_ABORTED, NULL);
hmm, actually, shall we check whether read_cb_.is_null() before calling
SatisfyRead()?

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

https://codereview.chromium.org/297553002/diff/90001/media/base/audio_decoder...
media/base/audio_decoder.h:62: // Decode().
This should be consistent with the comment for VideoDecoder::Decode().

https://codereview.chromium.org/297553002/diff/90001/media/base/audio_decoder...
media/base/audio_decoder.h:67: // still be called for older buffers if they were
scheduler before this method
s/scheduler/scheduled

https://codereview.chromium.org/297553002/diff/90001/media/base/audio_decoder...
media/base/audio_decoder.h:74: // if they were scheduler before this method  is
called.
s/scheduler/scheduled

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

https://codereview.chromium.org/297553002/diff/90001/media/base/video_decoder...
media/base/video_decoder.h:74: // If |buffer| is an EOS buffer then the decoder
must be flashed, i.e.
s/flash/flush.

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:84:
scoped_refptr<DecoderBuffer>(new DecoderBuffer(0))));
why making this change?

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:300: message_loop_.RunUntilIdle();
Run the message loop in DeliverBuffer()?

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:301: }
why "while" loop? There could be only one pending |decode_cb_|, which will be
reset in DliverBuffer?

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:308: message_loop_.RunUntilIdle();
SatisfyPendingRead() already runs the message loop at the end of the function.

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:477: reset_cb_ = reset_cb;
Add a comment that in this case the |reset_cb| is fired in DeliverBuffer() after
the decoder is flushed.

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (left):

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:126: }
It's really nice no need to FlushDecoder() multiple times!

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:338: case Decoder::kAborted:
Please see my reply in PS4.

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:350: if (CanDecodeMore() && !end_of_stream)
Some random thought. No action required here :)

Now there are two cases we call Decode(EOS):
1, During config change, where we are in STATE_FLUSHING_DECODER state and called
FlushDecoder().
2, When EOS is received from the demuxer, where we are in STATE_NORMAL state and
call Decode(EOS) implicitly.

I wonder whether we could unify these two cases and have a boolean to remember
why we flush the decoder (config change or EOS). Will that simplify a lot of
cases in this file...

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:352: } else if (state_ ==
STATE_FLUSHING_DECODER) {
Can we DCHECK(end_of_stream) here?

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:458: SatisfyRead(DEMUXER_READ_ABORTED, NULL);
Duplicate from a comment in PS4. Do we need to check read_cb_.is_null()?

https://codereview.chromium.org/297553002/diff/90001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/decrypting...
media/filters/decrypting_audio_decoder_unittest.cc:140: void
ReadAndExpectFrameReadyWith(
This should really be renamed to DecodeAndExpect....

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder.cc:43: output_cb_ = output_cb;
This should be okay. But add a comment why we don't need to BindToCurrentLoop()
here?

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
File media/filters/fake_video_decoder.h (left):

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder.h:35: bool supports_get_decode_output,
Yay!

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
File media/filters/fake_video_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:113: LOG(ERROR) << "Dec " <<
num_input_buffers_;
remove?

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:149: LOG(ERROR) << "WTF";
remove :)

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:154: void
SatisfyReadAndExpect(CallbackResult result) {
nit: this should be renamed to SatisfyDecodeAnd...

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:294:
SatisfyReadAndExpect(NOT_ENOUGH_DATA);
hmm, if max_decode_requests > decoding_delay, should we expect OK?

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:183: if (!buffer) {
hmm, should this ever happen?

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:280:
output_cb_.Run(AudioBuffer::CreateEOSBuffer());
should we keep calling FFmpegDecode(EOS) until the codec is completely flushed?

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder_unittest.cc:108: LOG(ERROR) << "Outp";
rm

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder_unittest.cc:113: LOG(ERROR) << "Finished";
rm

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder_unittest.cc:188:
Decode(AudioDecoder::kAborted);
This is probably obsolete. Now DemuxerStream will never call Decode(NULL). So we
may just remove this test.

If we do that, then all results in this test are kOk. We may just remove
|expected_status| in Decode() call.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:176: if (state_ == kDecodeFinished) {
nit: Probably this should never happen (DecoderStream won't call Decode() again
if decode has finished). But we can investigate this later.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:195: //                frames. The input
buffer is ignored at this point.
this is obsolete now?

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:202: //     When EOS buffer is received.
When EOS buffer is received and the codec has been flushed.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:211: if (!FFmpegDecode(buffer,
&produced_frame)) {
|produced_frame| could be read as a noun. How about |has_produced_frame|?

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:218: } while (repeat);
nit: how about just 
while (buffer->end_of_stream() && produced_frame);
?

Felt that's slightly cleaner as it makes it clear that we only call
FFmpegDecode() multiple times when buffer->end_of_stream().

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:255: *produced_frame = false;
DCHECK(!*produced_frame);

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder_unittest.cc:132: output_frames->clear();
DCHECK(output_frames->empty())?

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder_unittest.cc:172: scoped_refptr<VideoFrame>*
video_frame) {
should this be OutputFrames as well? In theory, we could have multiple outputs.

Since we use OutputFrames in all decode related methods, probably we could make
it a class variable, as we did in FFADTest.

https://codereview.chromium.org/297553002/diff/90001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:167: output_cb_ = output_cb;
In some decoders we BindToCurrentLoop, in some we don't. I "think" either will
work (not binding may be more efficient; binding will be safer), but better be
consistent.

https://codereview.chromium.org/297553002/diff/90001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:261: bitstream_buffers_in_decoder_.end());
nit: You can use ContainsKey():
https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&l=195

https://codereview.chromium.org/297553002/diff/90001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:348:
VideoFrameStreamTestParams(false, 7, 1)));
Add a case where parallel_decoding > decoding_delay?

[Sergey Ulanov, 2014-06-06 22:49:41]

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

https://codereview.chromium.org/297553002/diff/70001/media/base/audio_decoder...
media/base/audio_decoder.h:62: virtual void Reset(const base::Closure& closure)
= 0;
On 2014/06/05 21:53:48, xhwang wrote:
> On 2014/06/03 00:08:11, Sergey Ulanov wrote:
> > On 2014/05/29 22:15:14, xhwang wrote:
> > > In this case, shall all decoded frames be simply dropped, or fired via
> > OutputCB?
> > 
> > I think the comment is not entirely true. The buffers may not be actually
> > dropped. Audio decoders post a task for all callbacks, and these tasks are
not
> > canceled by Reset(). Updated it to reflect this issue.
> 
> To clarify, do you mean when Reset() is called, there may already be DecodeCBs
> posted to the message loop but not executed yet? In that case, the
AudioDecoders
> can always post the reset callback. 

Yes, that's right.

> Callbacks should be executed in the order
> they are posted so my previous comment will be valid. Am I missing anything?

My point was that pending Decode requests may still finish with kOk, so from
callers perspective it doesn't look like all data is dropped as the comment was
saying. Reworded the comment as follows:
  // Resets decoder state. All pending Decode() requests will be finished or
  // aborted before |closure| is called.

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/70001/media/filters/decoder_st...
media/filters/decoder_stream.cc:344: case Decoder::kAborted:
On 2014/06/05 21:53:48, xhwang wrote:
> On 2014/06/03 00:08:11, Sergey Ulanov wrote:
> > On 2014/05/29 22:15:14, xhwang wrote:
> > > IIUIC, This should only happen during Reset(), which is covered by l.326.
> Can
> > we
> > > DCHECK that this should not happen?
> > 
> > No. This came up with a previous CL as well:
> >
>
https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s...
> > (line 350). There are cases when we get into this state without Reset().
> 
> hmm... Thanks for reminding me of that discussion! 
> 
> If you look at l.463 (OnBufferReady), if DemuxerStream::kAborted is received,
> we'll abort the |read_cb_| immediately with AudioDecoder::kAborted. In other
> words, the decoder only takes normal buffers or EOS as inputs. And the only
case
> it could return kAborted is because of Reset()/Stop().
> 
> Now I looked at the tests [Audio|Video]RendererImplTest.AbortPendingRead_*
> again. They are testing something that should never happen. (They could happen
> previously because decoder calls DemuxerStream::Read().) So I think these
tests
> are obsolete now and we may just remove them.
> 

Done.

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

https://codereview.chromium.org/297553002/diff/90001/media/base/audio_decoder...
media/base/audio_decoder.h:62: // Decode().
On 2014/06/05 21:53:48, xhwang wrote:
> This should be consistent with the comment for VideoDecoder::Decode().

Done.

https://codereview.chromium.org/297553002/diff/90001/media/base/audio_decoder...
media/base/audio_decoder.h:67: // still be called for older buffers if they were
scheduler before this method
On 2014/06/05 21:53:48, xhwang wrote:
> s/scheduler/scheduled

Done.

https://codereview.chromium.org/297553002/diff/90001/media/base/audio_decoder...
media/base/audio_decoder.h:74: // if they were scheduler before this method  is
called.
On 2014/06/05 21:53:48, xhwang wrote:
> s/scheduler/scheduled

Done.

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

https://codereview.chromium.org/297553002/diff/90001/media/base/video_decoder...
media/base/video_decoder.h:74: // If |buffer| is an EOS buffer then the decoder
must be flashed, i.e.
On 2014/06/05 21:53:49, xhwang wrote:
> s/flash/flush.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:84:
scoped_refptr<DecoderBuffer>(new DecoderBuffer(0))));
On 2014/06/05 21:53:49, xhwang wrote:
> why making this change?

DecoderStream<StreamType>::OnBufferReady() stops reading after EOF packet has
been demuxed. This breaks Underflow_EndOfStream() which calls
WaitForPendingRead() after SatisfyPendingRead().

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:300: message_loop_.RunUntilIdle();
On 2014/06/05 21:53:49, xhwang wrote:
> Run the message loop in DeliverBuffer()?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:301: }
On 2014/06/05 21:53:49, xhwang wrote:
> why "while" loop? There could be only one pending |decode_cb_|, which will be
> reset in DliverBuffer?

DecoderStream::OnDecodeDone() may get another buffer from the stream. and call
the decoder again. This loop fills up the queue in DecodeStream with EOF
buffers. I agree it's ugly.
I would prefer to rewrite tests in this file to use a fake demuxer, then it
would be possible to control what's being read from the demuxer, but I don't
think it's needs to be in this CL.

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:308: message_loop_.RunUntilIdle();
On 2014/06/05 21:53:49, xhwang wrote:
> SatisfyPendingRead() already runs the message loop at the end of the function.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:477: reset_cb_ = reset_cb;
On 2014/06/05 21:53:49, xhwang wrote:
> Add a comment that in this case the |reset_cb| is fired in DeliverBuffer()
after
> the decoder is flushed.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:338: case Decoder::kAborted:
On 2014/06/05 21:53:49, xhwang wrote:
> Please see my reply in PS4.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:352: } else if (state_ ==
STATE_FLUSHING_DECODER) {
On 2014/06/05 21:53:49, xhwang wrote:
> Can we DCHECK(end_of_stream) here?

No. In case there were multiple decode requests we may still get results for
regular frames.

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:458: SatisfyRead(DEMUXER_READ_ABORTED, NULL);
On 2014/06/05 21:53:49, xhwang wrote:
> Duplicate from a comment in PS4. Do we need to check read_cb_.is_null()?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/decrypting...
media/filters/decrypting_audio_decoder_unittest.cc:140: void
ReadAndExpectFrameReadyWith(
On 2014/06/05 21:53:49, xhwang wrote:
> This should really be renamed to DecodeAndExpect....

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder.cc:43: output_cb_ = output_cb;
On 2014/06/05 21:53:49, xhwang wrote:
> This should be okay. But add a comment why we don't need to
BindToCurrentLoop()
> here?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
File media/filters/fake_video_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:113: LOG(ERROR) << "Dec " <<
num_input_buffers_;
On 2014/06/05 21:53:50, xhwang wrote:
> remove?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:149: LOG(ERROR) << "WTF";
On 2014/06/05 21:53:50, xhwang wrote:
> remove :)

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:154: void
SatisfyReadAndExpect(CallbackResult result) {
On 2014/06/05 21:53:50, xhwang wrote:
> nit: this should be renamed to SatisfyDecodeAnd...

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/fake_video...
media/filters/fake_video_decoder_unittest.cc:294:
SatisfyReadAndExpect(NOT_ENOUGH_DATA);
On 2014/06/05 21:53:50, xhwang wrote:
> hmm, if max_decode_requests > decoding_delay, should we expect OK?

Yes, but we didn't have tests with max_decode_requests > decoding_delay. Added
them now.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:183: if (!buffer) {
On 2014/06/05 21:53:50, xhwang wrote:
> hmm, should this ever happen?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:280:
output_cb_.Run(AudioBuffer::CreateEOSBuffer());
On 2014/06/05 21:53:50, xhwang wrote:
> should we keep calling FFmpegDecode(EOS) until the codec is completely
flushed?

There is a loop in FFmpegDecode() which should flush all data.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder_unittest.cc:108: LOG(ERROR) << "Outp";
On 2014/06/05 21:53:50, xhwang wrote:
> rm

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder_unittest.cc:113: LOG(ERROR) << "Finished";
On 2014/06/05 21:53:50, xhwang wrote:
> rm

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder_unittest.cc:188:
Decode(AudioDecoder::kAborted);
On 2014/06/05 21:53:50, xhwang wrote:
> This is probably obsolete. Now DemuxerStream will never call Decode(NULL). So
we
> may just remove this test.
> 
> If we do that, then all results in this test are kOk. We may just remove
> |expected_status| in Decode() call.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:176: if (state_ == kDecodeFinished) {
On 2014/06/05 21:53:51, xhwang wrote:
> nit: Probably this should never happen (DecoderStream won't call Decode()
again
> if decode has finished). But we can investigate this later.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:195: //                frames. The input
buffer is ignored at this point.
On 2014/06/05 21:53:50, xhwang wrote:
> this is obsolete now?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:202: //     When EOS buffer is received.
On 2014/06/05 21:53:50, xhwang wrote:
> When EOS buffer is received and the codec has been flushed.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:211: if (!FFmpegDecode(buffer,
&produced_frame)) {
On 2014/06/05 21:53:50, xhwang wrote:
> |produced_frame| could be read as a noun. How about |has_produced_frame|?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:218: } while (repeat);
On 2014/06/05 21:53:50, xhwang wrote:
> nit: how about just 
> while (buffer->end_of_stream() && produced_frame);
> ?
> 
> Felt that's slightly cleaner as it makes it clear that we only call
> FFmpegDecode() multiple times when buffer->end_of_stream().

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:255: *produced_frame = false;
On 2014/06/05 21:53:51, xhwang wrote:
> DCHECK(!*produced_frame);

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder_unittest.cc:132: output_frames->clear();
On 2014/06/05 21:53:51, xhwang wrote:
> DCHECK(output_frames->empty())?

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder_unittest.cc:172: scoped_refptr<VideoFrame>*
video_frame) {
On 2014/06/05 21:53:51, xhwang wrote:
> should this be OutputFrames as well? In theory, we could have multiple
outputs.
> 
> Since we use OutputFrames in all decode related methods, probably we could
make
> it a class variable, as we did in FFADTest.

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:167: output_cb_ = output_cb;
On 2014/06/05 21:53:51, xhwang wrote:
> In some decoders we BindToCurrentLoop, in some we don't. I "think" either will
> work (not binding may be more efficient; binding will be safer), but better be
> consistent.

I think all VDA implementations should call the callback only asynchronously,
but I'm not sure and it's not documented. added BindToCurrentLoop().

https://codereview.chromium.org/297553002/diff/90001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:261: bitstream_buffers_in_decoder_.end());
On 2014/06/05 21:53:51, xhwang wrote:
> nit: You can use ContainsKey():
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&l=195

Done.

https://codereview.chromium.org/297553002/diff/90001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:348:
VideoFrameStreamTestParams(false, 7, 1)));
On 2014/06/05 21:53:51, xhwang wrote:
> Add a case where parallel_decoding > decoding_delay?

There are some below, line 360.

[Sergey Ulanov, 2014-06-06 23:12:34]

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:176: if (state_ == kDecodeFinished) {
On 2014/06/06 22:49:41, Sergey Ulanov wrote:
> On 2014/06/05 21:53:51, xhwang wrote:
> > nit: Probably this should never happen (DecoderStream won't call Decode()
> again
> > if decode has finished). But we can investigate this later.
> 
> Done.

Actually DecoderStream may call this method after EOS was returned. It should be
possible to fix it by adding EOS state in DecoderStream(), but I don't think it
has to be in this CL.

[xhwang, 2014-06-07 00:35:15]

lgtm % some trivial comments. Thank you so much for taking this!

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/decoder_st...
media/filters/decoder_stream.cc:352: } else if (state_ ==
STATE_FLUSHING_DECODER) {
On 2014/06/06 22:49:40, Sergey Ulanov wrote:
> On 2014/06/05 21:53:49, xhwang wrote:
> > Can we DCHECK(end_of_stream) here?
> 
> No. In case there were multiple decode requests we may still get results for
> regular frames.

I see. Thanks.

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/90001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:176: if (state_ == kDecodeFinished) {
On 2014/06/06 23:12:34, Sergey Ulanov wrote:
> On 2014/06/06 22:49:41, Sergey Ulanov wrote:
> > On 2014/06/05 21:53:51, xhwang wrote:
> > > nit: Probably this should never happen (DecoderStream won't call Decode()
> > again
> > > if decode has finished). But we can investigate this later.
> > 
> > Done.
> 
> Actually DecoderStream may call this method after EOS was returned. It should
be
> possible to fix it by adding EOS state in DecoderStream(), but I don't think
it
> has to be in this CL.

Can you add a TODO here?

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

https://codereview.chromium.org/297553002/diff/130001/media/filters/decoder_s...
media/filters/decoder_stream.cc:458: DCHECK(read_cb_.is_null());
Why DCHECK(read_cb_.is_null())? In my comment I meant something like:
if (!read_cb_.is_null())
  SatisfyRead......

https://codereview.chromium.org/297553002/diff/130001/media/filters/vpx_video...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/130001/media/filters/vpx_video...
media/filters/vpx_video_decoder.cc:233: output_cb_ = output_cb;
Do we need BindToCurrentLoop() here? Please check all AudioDecoder/VideoDecoder
impls to make sure we have consistent behavior.

[Sergey Ulanov, 2014-06-07 01:12:09]

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

https://codereview.chromium.org/297553002/diff/130001/media/filters/decoder_s...
media/filters/decoder_stream.cc:458: DCHECK(read_cb_.is_null());
On 2014/06/07 00:35:15, xhwang wrote:
> Why DCHECK(read_cb_.is_null())? In my comment I meant something like:
> if (!read_cb_.is_null())
>   SatisfyRead......

Yes. You are right. Actually tests were failing with the DCHECK. Fixed now.

https://codereview.chromium.org/297553002/diff/130001/media/filters/vpx_video...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/297553002/diff/130001/media/filters/vpx_video...
media/filters/vpx_video_decoder.cc:233: output_cb_ = output_cb;
On 2014/06/07 00:35:15, xhwang wrote:
> Do we need BindToCurrentLoop() here? Please check all
AudioDecoder/VideoDecoder
> impls to make sure we have consistent behavior.

Yes. Done.

[Sergey Ulanov, 2014-06-09 18:29:08]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-06-09 18:29:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/297553002/210001

[Sergey Ulanov, 2014-06-09 19:46:06]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-06-09 19:47:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/297553002/230001

[Sergey Ulanov, 2014-06-09 20:24:47]

The CQ bit was unchecked by sergeyu@chromium.org

[Sergey Ulanov, 2014-06-09 20:25:50]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-06-09 20:29:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/297553002/250001

[commit-bot: I haz the power, 2014-06-10 09:57:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-10 11:41:32]

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

[commit-bot: I haz the power, 2014-06-10 11:41:33]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Sergey Ulanov, 2014-06-11 00:16:06]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-06-11 00:17:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/297553002/270001

[commit-bot: I haz the power, 2014-06-11 11:11:54]

Change committed as 276344