Report VideoDecoder status through ReadCB instead of through FilterHost.

Report VideoDecoder status through ReadCB instead of through FilterHost.

BUG=108340
TEST=media_unittests

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

[xhwang, 2012-04-26 23:35:55]

Hello fischman & scherkus,

Second patch towards removing VideoDecoder out of Filter hierarchy. Errors are
passed via ReadCB instead of FilterHost. Please review.

xhwang

[Ami GONE FROM CHROMIUM, 2012-04-27 16:49:13]

I like it!

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h#newcode110
media/base/filters.h:110: class MEDIA_EXPORT VideoDecoder : public Filter {
Can you override Filter::set_host() here to LOG(FATAL) to ensure all callers
have been caught?
(with a TODO pointing at the "make a VideoDecoder not a Filter" crbug saying the
override should be removed when this class stops being a Filter).

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h#newcode135
media/base/filters.h:135: typedef base::Callback<void(scoped_refptr<VideoFrame>,
Status)> ReadCB;
Typically dependent args follow their dependencies, so I'd make Status the first
param, not second.

http://codereview.chromium.org/10248002/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:125: EXPECT_EQ(VideoDecoder::kOk,
status);
order should be: actual,expected
(to match thousands of years of mathematical notation, instead of ~10 years of
junit notation, which thankfully gtest moved away from a few years ago).
(here and below)

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:196: CHECK_GE(pending_error_count_, 0);
unnecessary, IMO

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:553: CHECK_GE(pending_error_count_, 0);
unnecessary.

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base.cc:380: }
DCHECK_EQ(status, VideoDecoder::kOk);
?

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:211: CHECK(!read_cb_.is_null()) <<
"Can't deliver a frame without a callback";
unnecessary - l.215 will crash anyway.

[scherkus (not reviewing), 2012-04-27 18:27:55]

very nice!

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h#newcode135
media/base/filters.h:135: typedef base::Callback<void(scoped_refptr<VideoFrame>,
Status)> ReadCB;
On 2012/04/27 16:49:14, Ami Fischman wrote:
> Typically dependent args follow their dependencies, so I'd make Status the
first
> param, not second.

Building on top of that -- the docs should be updated to mention what to expect
of VideoFrame given the value of Status

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:199: pending_error_count_--;
fischman: is there a case where we'd report an error on one read, but have
subsequent reads successfully complete?

in other words, should we replace pending_error_count_ with a flag that puts
GpuVD into lame duck mode and always reports read errors?

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base.h:80: // should be NULL.
nit: this documentation should be in the definition of VideoDecoder::ReadCB
isntead of being duplicated here

[Ami GONE FROM CHROMIUM, 2012-04-27 18:31:16]

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:199: pending_error_count_--;
On 2012/04/27 18:27:56, scherkus wrote:
> fischman: is there a case where we'd report an error on one read, but have
> subsequent reads successfully complete?

No; pending_error_count_ only goes positive when NotifyError is called, past
which vda_ is NULL.

> in other words, should we replace pending_error_count_ with a flag that puts
> GpuVD into lame duck mode and always reports read errors?

I like it!

[xhwang, 2012-04-27 23:22:30]

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h#newcode110
media/base/filters.h:110: class MEDIA_EXPORT VideoDecoder : public Filter {
On 2012/04/27 16:49:14, Ami Fischman wrote:
> Can you override Filter::set_host() here to LOG(FATAL) to ensure all callers
> have been caught?
> (with a TODO pointing at the "make a VideoDecoder not a Filter" crbug saying
the
> override should be removed when this class stops being a Filter).

Done.

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h#newcode135
media/base/filters.h:135: typedef base::Callback<void(scoped_refptr<VideoFrame>,
Status)> ReadCB;
On 2012/04/27 16:49:14, Ami Fischman wrote:
> Typically dependent args follow their dependencies, so I'd make Status the
first
> param, not second.

Done.

http://codereview.chromium.org/10248002/diff/1/media/base/filters.h#newcode135
media/base/filters.h:135: typedef base::Callback<void(scoped_refptr<VideoFrame>,
Status)> ReadCB;
On 2012/04/27 18:27:56, scherkus wrote:
> On 2012/04/27 16:49:14, Ami Fischman wrote:
> > Typically dependent args follow their dependencies, so I'd make Status the
> first
> > param, not second.
> 
> Building on top of that -- the docs should be updated to mention what to
expect
> of VideoFrame given the value of Status

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder_unittest.cc:125: EXPECT_EQ(VideoDecoder::kOk,
status);
On 2012/04/27 16:49:14, Ami Fischman wrote:
> order should be: actual,expected
> (to match thousands of years of mathematical notation, instead of ~10 years of
> junit notation, which thankfully gtest moved away from a few years ago).
> (here and below)

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:196: CHECK_GE(pending_error_count_, 0);
On 2012/04/27 16:49:14, Ami Fischman wrote:
> unnecessary, IMO

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:199: pending_error_count_--;
On 2012/04/27 18:27:56, scherkus wrote:
> fischman: is there a case where we'd report an error on one read, but have
> subsequent reads successfully complete?
> 
> in other words, should we replace pending_error_count_ with a flag that puts
> GpuVD into lame duck mode and always reports read errors?

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:199: pending_error_count_--;
On 2012/04/27 18:31:16, Ami Fischman wrote:
> On 2012/04/27 18:27:56, scherkus wrote:
> > fischman: is there a case where we'd report an error on one read, but have
> > subsequent reads successfully complete?
> 
> No; pending_error_count_ only goes positive when NotifyError is called, past
> which vda_ is NULL.
> 
> > in other words, should we replace pending_error_count_ with a flag that puts
> > GpuVD into lame duck mode and always reports read errors?
> 
> I like it!

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:553: CHECK_GE(pending_error_count_, 0);
On 2012/04/27 16:49:14, Ami Fischman wrote:
> unnecessary.

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base.cc:380: }
On 2012/04/27 16:49:14, Ami Fischman wrote:
> DCHECK_EQ(status, VideoDecoder::kOk);
> ?

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base.h:80: // should be NULL.
On 2012/04/27 18:27:56, scherkus wrote:
> nit: this documentation should be in the definition of VideoDecoder::ReadCB
> isntead of being duplicated here

Done.

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10248002/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:211: CHECK(!read_cb_.is_null()) <<
"Can't deliver a frame without a callback";
On 2012/04/27 16:49:14, Ami Fischman wrote:
> unnecessary - l.215 will crash anyway.

Done.

[scherkus (not reviewing), 2012-04-27 23:28:33]

LGTM w/ one nit

thanks for doing this!

http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc
File media/base/filters.cc (right):

http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc#newco...
media/base/filters.cc:78: LOG(FATAL) << "VideoDecoder::host is not supposed to
be called.";
add TODO w/ crbug entry pointing at removing Filter

[Ami GONE FROM CHROMIUM, 2012-04-27 23:32:04]

LGTM

http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc
File media/base/filters.cc (right):

http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc#newco...
media/base/filters.cc:79: return NULL;
I'm surprised this is necessary (since LOG(FATAL) doesn't return).

[xhwang, 2012-04-27 23:32:25]

http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc
File media/base/filters.cc (right):

http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc#newco...
media/base/filters.cc:78: LOG(FATAL) << "VideoDecoder::host is not supposed to
be called.";
On 2012/04/27 23:28:33, scherkus wrote:
> add TODO w/ crbug entry pointing at removing Filter

TODO is in filter.h together with Pause()/Seek()...

[scherkus (not reviewing), 2012-04-27 23:39:50]

On 2012/04/27 23:32:25, xhwang wrote:
> http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc
> File media/base/filters.cc (right):
> 
>
http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc#newco...
> media/base/filters.cc:78: LOG(FATAL) << "VideoDecoder::host is not supposed to
> be called.";
> On 2012/04/27 23:28:33, scherkus wrote:
> > add TODO w/ crbug entry pointing at removing Filter
> 
> TODO is in filter.h together with Pause()/Seek()...

My gut tells me the unfortunate soul that ends up hitting this LOG(FATAL) won't
think to check there :P

[commit-bot: I haz the power, 2012-04-27 23:43:15]

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

[xhwang, 2012-04-28 01:02:09]

On 2012/04/27 23:39:50, scherkus wrote:
> On 2012/04/27 23:32:25, xhwang wrote:
> > http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc
> > File media/base/filters.cc (right):
> > 
> >
>
http://codereview.chromium.org/10248002/diff/7001/media/base/filters.cc#newco...
> > media/base/filters.cc:78: LOG(FATAL) << "VideoDecoder::host is not supposed
to
> > be called.";
> > On 2012/04/27 23:28:33, scherkus wrote:
> > > add TODO w/ crbug entry pointing at removing Filter
> > 
> > TODO is in filter.h together with Pause()/Seek()...
> 
> My gut tells me the unfortunate soul that ends up hitting this LOG(FATAL)
won't
> think to check there :P

Done.

[commit-bot: I haz the power, 2012-04-28 01:55:39]

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

[commit-bot: I haz the power, 2012-04-28 03:12:49]

Change committed as 134435