Add status parameter to DemuxerStream::ReadCB

Add status parameter to DemuxerStream::ReadCB


BUG=122913
TEST=None

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

[acolwell GONE FROM CHROMIUM, 2012-06-25 23:45:36]

Initial step toward enabling decoder config changes during playback. I just
wanted to get some initial feedback on this change. I'll probably extract the
MockReader & AudioDecoder::Status changes into separate CLs.

[Ami GONE FROM CHROMIUM, 2012-06-26 00:33:21]

IWBN to have at least a prototype config-change handler wired up (which can land
separately) to prove that this API shape is reasonable.

Also, yeah, please split off the splittable bits :)

http://codereview.chromium.org/10669022/diff/2001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

http://codereview.chromium.org/10669022/diff/2001/media/base/audio_decoder.h#...
media/base/audio_decoder.h:26: kDecodeError,  // Decoding error happened.
These comments aren't adding much value.

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:29: enum Status {
Before this CL, ReadCB had 3 possible return scenarios (normal, EOS, abort). 
After this CL, it has 4 possible return scenarios (above + config change).  Does
it not make sense to promote EOS to a Status now that you must pass/inspect
Status anyway?

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:38: //  kOk : Indicates the second parameter is
Non-NULL and contains media data
These comments belong at the enum declaration, not here.

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:43: //             return.
s/\./ed./

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:45: //                  call to
audio_decoder_config() or video_decoder_config()
This phrasing implies that calling [av]_d_c() on a demuxer stream will
reconfigure a decoder or something.  Rephrase?

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:47: //                  parameter MUST be NULL when
this status is return.
ditto

http://codereview.chromium.org/10669022/diff/2001/media/base/mock_reader.h
File media/base/mock_reader.h (right):

http://codereview.chromium.org/10669022/diff/2001/media/base/mock_reader.h#ne...
media/base/mock_reader.h:18: class DemuxerStreamReader
rename file?

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

http://codereview.chromium.org/10669022/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:531:
base::ResetAndReturn(&pause_cb_).Run();
does the error get dropped at this point?

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

http://codereview.chromium.org/10669022/diff/2001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:174: if (status != DemuxerStream::kOk) {
DCHECK(!input) ?

http://codereview.chromium.org/10669022/diff/2001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10669022/diff/2001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:128: it->Run(DemuxerStream::kOk,
I'm surprised that nothing in this file Run()'s a CB with a status *other* than
kOk.
In particular, I expected there to be mention of kConfigChanged here and/or in
chunk_demuxer.cc.

[acolwell GONE FROM CHROMIUM, 2012-07-12 01:19:38]

PTAL

http://codereview.chromium.org/10669022/diff/2001/media/base/audio_decoder.h
File media/base/audio_decoder.h (right):

http://codereview.chromium.org/10669022/diff/2001/media/base/audio_decoder.h#...
media/base/audio_decoder.h:26: kDecodeError,  // Decoding error happened.
On 2012/06/26 00:33:21, Ami Fischman wrote:
> These comments aren't adding much value.

Done.

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:29: enum Status {
On 2012/06/26 00:33:21, Ami Fischman wrote:
> Before this CL, ReadCB had 3 possible return scenarios (normal, EOS, abort). 
> After this CL, it has 4 possible return scenarios (above + config change). 
Does
> it not make sense to promote EOS to a Status now that you must pass/inspect
> Status anyway?
I believe it does make sense. Mind if I defer this to a follow-on CL?

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:38: //  kOk : Indicates the second parameter is
Non-NULL and contains media data
On 2012/06/26 00:33:21, Ami Fischman wrote:
> These comments belong at the enum declaration, not here.

Done.

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:43: //             return.
On 2012/06/26 00:33:21, Ami Fischman wrote:
> s/\./ed./

Done.

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:45: //                  call to
audio_decoder_config() or video_decoder_config()
On 2012/06/26 00:33:21, Ami Fischman wrote:
> This phrasing implies that calling [av]_d_c() on a demuxer stream will
> reconfigure a decoder or something.  Rephrase?

Done.

http://codereview.chromium.org/10669022/diff/2001/media/base/demuxer_stream.h...
media/base/demuxer_stream.h:47: //                  parameter MUST be NULL when
this status is return.
On 2012/06/26 00:33:21, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10669022/diff/2001/media/base/mock_reader.h
File media/base/mock_reader.h (right):

http://codereview.chromium.org/10669022/diff/2001/media/base/mock_reader.h#ne...
media/base/mock_reader.h:18: class DemuxerStreamReader
On 2012/06/26 00:33:21, Ami Fischman wrote:
> rename file?
Removed file in a separate CL.

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

http://codereview.chromium.org/10669022/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:531:
base::ResetAndReturn(&pause_cb_).Run();
On 2012/06/26 00:33:21, Ami Fischman wrote:
> does the error get dropped at this point?

Done.

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

http://codereview.chromium.org/10669022/diff/2001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:174: if (status != DemuxerStream::kOk) {
On 2012/06/26 00:33:21, Ami Fischman wrote:
> DCHECK(!input) ?

Done.

http://codereview.chromium.org/10669022/diff/2001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10669022/diff/2001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:128: it->Run(DemuxerStream::kOk,
On 2012/06/26 00:33:21, Ami Fischman wrote:
> I'm surprised that nothing in this file Run()'s a CB with a status *other*
than
> kOk.
FFmpeg can always return a buffer or end of stream which is why it returns kOk
all the time. Config changes won't happen in this demuxer.

> In particular, I expected there to be mention of kConfigChanged here and/or in
> chunk_demuxer.cc.
I've uploaded 2 CLs to show how config changes can be supported.

ChunkDemuxer & SourceBufferStream changes to track config changes and generate
kConfigChange status
https://chromiumcodereview.appspot.com/10696182/

FFmpegVideoDecoder changes to react to kConfigChange
https://chromiumcodereview.appspot.com/10704175

[Ami GONE FROM CHROMIUM, 2012-07-12 17:10:26]

LGTM % nits

http://codereview.chromium.org/10669022/diff/13027/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10669022/diff/13027/media/base/demuxer_stream....
media/base/demuxer_stream.h:53: // The second parameter is Non-NULL and contains
media data
s/Non/non/

http://codereview.chromium.org/10669022/diff/13027/media/base/demuxer_stream....
media/base/demuxer_stream.h:56: ReadCB;
indent isn't supported by style guide.  I'd put a newline after the comma in the
previous line and line up the void() params, instead.

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:244: if (status ==
AudioDecoder::kDecodeError) {
swap order to match enum

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:552: void
AudioRendererImpl::HandleAbortedRead() {
This and the previous method are awfully similar and could be collapsed to
PipelineStatus status = is_decode_error ? PIPELINE_ERROR_DECODE : PIPELINE_OK;
and then only call SetError if !OK in the two places SetError is called.

Or do you expect these to (intentionally) diverge over time?

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
File media/filters/audio_renderer_impl.h (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
media/filters/audio_renderer_impl.h:82: void HandleAbortedRead();
swap order of these two to match enum

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_audio...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_audio...
media/filters/ffmpeg_audio_decoder.cc:254:
base::ResetAndReturn(&read_cb_).Run(kOk, output);
braces not strictly necessary

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_audio...
media/filters/ffmpeg_audio_decoder.cc:269: DCHECK_EQ(status !=
DemuxerStream::kOk, !buffer);
if status!=OK but buffer is non-NULL IWBN to see in the DCHECK error message
what status was.  
s/;/ << status;/ ?

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder.cc:307:
base::ResetAndReturn(&read_cb_).Run(decoder_status, NULL);
IWBN to have a uniform approach to read_cb_.Run.  
Either DeliverFrame() everywhere or nowhere.

http://codereview.chromium.org/10669022/diff/13027/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:205: DCHECK_EQ(status != DemuxerStream::kOk,
!buffer);
ditto << status

[acolwell GONE FROM CHROMIUM, 2012-07-12 21:59:58]

http://codereview.chromium.org/10669022/diff/13027/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

http://codereview.chromium.org/10669022/diff/13027/media/base/demuxer_stream....
media/base/demuxer_stream.h:53: // The second parameter is Non-NULL and contains
media data
On 2012/07/12 17:10:26, Ami Fischman wrote:
> s/Non/non/

Done.

http://codereview.chromium.org/10669022/diff/13027/media/base/demuxer_stream....
media/base/demuxer_stream.h:56: ReadCB;
On 2012/07/12 17:10:26, Ami Fischman wrote:
> indent isn't supported by style guide.  I'd put a newline after the comma in
the
> previous line and line up the void() params, instead.

Done.

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:244: if (status ==
AudioDecoder::kDecodeError) {
On 2012/07/12 17:10:26, Ami Fischman wrote:
> swap order to match enum

Done.

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
media/filters/audio_renderer_impl.cc:552: void
AudioRendererImpl::HandleAbortedRead() {
On 2012/07/12 17:10:26, Ami Fischman wrote:
> This and the previous method are awfully similar and could be collapsed to
> PipelineStatus status = is_decode_error ? PIPELINE_ERROR_DECODE : PIPELINE_OK;
> and then only call SetError if !OK in the two places SetError is called.
> 
> Or do you expect these to (intentionally) diverge over time?

Done. Merged into a single method.

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
File media/filters/audio_renderer_impl.h (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/audio_render...
media/filters/audio_renderer_impl.h:82: void HandleAbortedRead();
On 2012/07/12 17:10:26, Ami Fischman wrote:
> swap order of these two to match enum

Done.

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_audio...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_audio...
media/filters/ffmpeg_audio_decoder.cc:254:
base::ResetAndReturn(&read_cb_).Run(kOk, output);
On 2012/07/12 17:10:26, Ami Fischman wrote:
> braces not strictly necessary

Done.

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_audio...
media/filters/ffmpeg_audio_decoder.cc:269: DCHECK_EQ(status !=
DemuxerStream::kOk, !buffer);
On 2012/07/12 17:10:26, Ami Fischman wrote:
> if status!=OK but buffer is non-NULL IWBN to see in the DCHECK error message
> what status was.  
> s/;/ << status;/ ?

Done.

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_video...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/ffmpeg_video...
media/filters/ffmpeg_video_decoder.cc:307:
base::ResetAndReturn(&read_cb_).Run(decoder_status, NULL);
On 2012/07/12 17:10:26, Ami Fischman wrote:
> IWBN to have a uniform approach to read_cb_.Run.  
> Either DeliverFrame() everywhere or nowhere.

Done. Removed DeliverFrame().

http://codereview.chromium.org/10669022/diff/13027/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10669022/diff/13027/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:205: DCHECK_EQ(status != DemuxerStream::kOk,
!buffer);
On 2012/07/12 17:10:26, Ami Fischman wrote:
> ditto << status

Done.

[commit-bot: I haz the power, 2012-07-12 23:52:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/10669022/21004

[commit-bot: I haz the power, 2012-07-13 00:42:16]

Try job failure for 10669022-21004 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-13 04:20:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/10669022/21004

[commit-bot: I haz the power, 2012-07-13 05:16:53]

Try job failure for 10669022-21004 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...