No EOS frame in {Audio|Video}Decoder::OutputCB.

No EOS frame in {Audio|Video}Decoder::OutputCB.

This is a follow-up CL of r276344.

Currently, when audio and video decoders receive an EOS buffer, they do the following in order:
1, Flush the codec, return all internally buffered frames through the OutputCB.
2, Return a EOS frame through the OutputCB.
3, Return the DecodeCB.

Since the DecoderStream knows when an input buffer is an EOS, when the DecodeCB is returned, DecoderStream knows that decoding has finished. Therefore, step (2) is redundant.

This CL drops step (2) which simplifies a lot of code.

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

[xhwang, 2014-06-16 16:48:11]

I still need to fix the audio_renderer_impl_unittests.cc. But I think it's
better to get a first round of comments before I go too far.

PTAL

[DaleCurtis, 2014-06-16 18:18:08]

https://codereview.chromium.org/339653003/diff/1/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:233: // kNormal -> kFlushCodec:
Is this still a valid state?

[Sergey Ulanov, 2014-06-16 23:49:32]

only some nits

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

https://codereview.chromium.org/339653003/diff/1/media/base/audio_decoder.h#n...
media/base/audio_decoder.h:35: // frames should not be returned via this
callback.
nit: the last two sentences can be reduced to one. E.g. "Only non-EOS frames
should be returned via this callback."

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

https://codereview.chromium.org/339653003/diff/1/media/base/video_decoder.h#n...
media/base/video_decoder.h:34: // frames should not be returned via this
callback.
same here as in audio_decoder.h

https://codereview.chromium.org/339653003/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl_unittest.cc:293: LOG(ERROR) << __FUNCTION__;
remove?

https://codereview.chromium.org/339653003/diff/1/media/filters/decoder_stream...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream_traits.cc:106: return OutputType::CreateEOSFrame();
Do you really need CreateEOSOutput() in DecoderStreamTraits? I think you should
be able to call OutputType::CreateEOSFrame() directly in DecoderStream.

https://codereview.chromium.org/339653003/diff/1/media/filters/decrypting_vid...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:115: if (video_frame.get())
nit: don't need .get() here.

[xhwang, 2014-06-17 18:56:35]

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

https://codereview.chromium.org/339653003/diff/1/media/base/audio_decoder.h#n...
media/base/audio_decoder.h:35: // frames should not be returned via this
callback.
On 2014/06/16 23:49:32, Sergey Ulanov wrote:
> nit: the last two sentences can be reduced to one. E.g. "Only non-EOS frames
> should be returned via this callback."

Done.

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

https://codereview.chromium.org/339653003/diff/1/media/base/video_decoder.h#n...
media/base/video_decoder.h:34: // frames should not be returned via this
callback.
On 2014/06/16 23:49:32, Sergey Ulanov wrote:
> same here as in audio_decoder.h

Done.

https://codereview.chromium.org/339653003/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl_unittest.cc:293: LOG(ERROR) << __FUNCTION__;
On 2014/06/16 23:49:32, Sergey Ulanov wrote:
> remove?

Updated this function and removed the logs.

https://codereview.chromium.org/339653003/diff/1/media/filters/decoder_stream...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream_traits.cc:106: return OutputType::CreateEOSFrame();
On 2014/06/16 23:49:32, Sergey Ulanov wrote:
> Do you really need CreateEOSOutput() in DecoderStreamTraits? I think you
should
> be able to call OutputType::CreateEOSFrame() directly in DecoderStream.

For audio, it's called CreateEOSBuffer(). CreateEOSOutput() basically wraps the
naming difference b/w audio and video.

https://codereview.chromium.org/339653003/diff/1/media/filters/decrypting_vid...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:115: if (video_frame.get())
On 2014/06/16 23:49:32, Sergey Ulanov wrote:
> nit: don't need .get() here.

This code is obsolete now.

https://codereview.chromium.org/339653003/diff/1/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/339653003/diff/1/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:233: // kNormal -> kFlushCodec:
On 2014/06/16 18:18:08, DaleCurtis wrote:
> Is this still a valid state?

Thanks for catching this. These comments are super old and are in the wrong
spot. Updated the comments and move them to the header file.

[xhwang, 2014-06-17 18:57:58]

Rebased, AudioRendererImplTest fixed and comments addressed.

PTAL again!

dalecurtis@: Since you commented, please take a look as well :)

[xhwang, 2014-06-17 20:17:28]

rebase only

[DaleCurtis, 2014-06-17 20:48:19]

lgtm % nit

https://codereview.chromium.org/339653003/diff/80001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.h (right):

https://codereview.chromium.org/339653003/diff/80001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.h:59: // kkUninitialized -> kNormal:
Extra kk

[xhwang, 2014-06-17 22:42:56]

comments addressed

[xhwang, 2014-06-17 22:43:09]

https://codereview.chromium.org/339653003/diff/80001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.h (right):

https://codereview.chromium.org/339653003/diff/80001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.h:59: // kkUninitialized -> kNormal:
On 2014/06/17 20:48:19, DaleCurtis wrote:
> Extra kk

Done.

[xhwang, 2014-06-17 23:50:47]

comments addressed

[DaleCurtis, 2014-06-17 23:57:51]

lgtm w/ ffmpeg audio decoder fix.

[xhwang, 2014-06-18 01:21:06]

comments addressed

[xhwang, 2014-06-18 07:07:08]

dalecurtis: Please review PS9 again. I added tests to cover the fix in PS8.
Thanks!

[xhwang, 2014-06-18 16:53:22]

rebase only

[DaleCurtis, 2014-06-18 19:50:41]

lgtm

[xhwang, 2014-06-18 19:58:04]

sergeyu@: Could you please take a look?

[Sergey Ulanov, 2014-06-18 21:54:32]

lgtm

[xhwang, 2014-06-18 22:53:56]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-06-18 22:55:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/339653003/180001

[commit-bot: I haz the power, 2014-06-19 01:42:27]

Change committed as 278232