Convert Filter::Seek() to use new callback system.

Convert Filter::Seek() to use new callback system.


BUG=82167
TEST=none. Existing tests still pass.

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

[Ami GONE FROM CHROMIUM, 2011-05-12 20:42:16]

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.cc:283: if (!status_cb_.is_null()) {
DCHECK not both fields are set?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.cc:404: if (!status_cb_.is_null()) {
This duplicates code in DispatchPendingCallback.
Can you avoid this?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.cc:445: return callback_.get() ||
!status_cb_.is_null();
DCHECK !&& ?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter.h
File media/base/composite_filter.h (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.h:110: bool IsOperationPending() const;
doco

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.h:113: // FilterCallback.
TODO to remove when FilterCallback goes away?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.h:122: FilterStatusCB status_cb_;
Yowza; either can be non-null?  Seems like a TODO to remove callback_ is in
order?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:57: // |expected_status| - The expected
status value returned by the status
There is no "status callback" in this comment other than referenced here.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:89: // Verifies |filter_1_callback_| or
|filter_1_status_cb_| set.
"Verifies" seems overly assertive.  Returns whether they are set.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:90: bool HaveFilter1Callback() const;
Doesn't chrome use Has* generally?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:244: case SEEK: {
braces unnecessary

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:331: return filter_1_callback_ != NULL
|| !filter_1_status_cb_.is_null();
CHECK not both?

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:351: return filter_2_callback_ != NULL
|| !filter_2_status_cb_.is_null();
ditto

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

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.cc#newcode66
media/base/filters.cc:66: if (!callback.is_null())
I don't understand the DCHECK+if.
Is there a missing TODO here?

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.cc#newcode67
media/base/filters.cc:67: callback.Run(PIPELINE_OK);
outdent

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

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.h#newcode71
media/base/filters.h:71: FilterStatusCB CopyAndResetCB(FilterStatusCB& cb);
style guide says no non-const reference passing.

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.h#newcode71
media/base/filters.h:71: FilterStatusCB CopyAndResetCB(FilterStatusCB& cb);
I think you only use this to immediately Run() the returned value, so why not
void RunAndResetCB(FilterStatusCB* cb, PipelineStatus status) {
  cb->Run(status);
  cb->Reset();
}
?

http://codereview.chromium.org/6969026/diff/1002/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:582: SetError(status);
This is funky b/c it posts the errorchanged task, and then OFST() below posts a
FSTT task, which depends on the errorchanged having run.  I guess this is ok
(messageloop must guarantee order of execution, right?), but funky.

http://codereview.chromium.org/6969026/diff/1002/media/filters/adaptive_demux...
File media/filters/adaptive_demuxer.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/adaptive_demux...
media/filters/adaptive_demuxer.cc:195: // Helper class that wraps FilterStatusCB
and expects to get called a set
Sad panda that this has to coexist w/ CountingCallback.

http://codereview.chromium.org/6969026/diff/1002/media/filters/adaptive_demux...
media/filters/adaptive_demuxer.cc:210: base::Unretained(this));
I think if you drop the unretained then you can also drop the delete this from
the fire_orig_cb callpath below, no?

http://codereview.chromium.org/6969026/diff/1002/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:67: void
AudioRendererBase::Seek(base::TimeDelta time, const FilterStatusCB&  cb) {
Extra space before cb.

http://codereview.chromium.org/6969026/diff/1002/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:189: NewRunnableMethod(this,
&FFmpegVideoDecoder::Seek,
indent is off (before your CL).

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:588: scoped_ptr<FilterCallback> callback;
Typically the reason we have this sort of stack var holding member state is so
that it can be used outside the lock that has to be held to inspect the member
state.  But in this case that doesn't hold - the entire method body is under
lock, so you could as well just fire seek_cb_ inside the switch.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:607: status_cb = CopyAndResetCB(seek_cb_);
std::swap() fits in better here, esp. if you take my suggestion about
CopyAndResetCB().

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:608: } break;
braces should be unnecessary.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:624: host()->SetError(status);
Is it really right to not set the host's error state if e.g. we were seeking? 
Wouldn't we want to know about the error when doing non-seek operations
following?
I suspect both this and the state_=kError; statement above really belong at the
top of the method, after taking a copy of state_ to switch off of.

[acolwell GONE FROM CHROMIUM, 2011-05-12 22:30:40]

Thanks for the review. Here are the suggested changes.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.cc:283: if (!status_cb_.is_null()) {
On 2011/05/12 20:42:16, Ami Fischman wrote:
> DCHECK not both fields are set?

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.cc:404: if (!status_cb_.is_null()) {
On 2011/05/12 20:42:16, Ami Fischman wrote:
> This duplicates code in DispatchPendingCallback.
> Can you avoid this?
Done. Merge functionality into DispatchPendingCallback.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.cc:445: return callback_.get() ||
!status_cb_.is_null();
On 2011/05/12 20:42:16, Ami Fischman wrote:
> DCHECK !&& ?

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter.h
File media/base/composite_filter.h (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.h:110: bool IsOperationPending() const;
On 2011/05/12 20:42:16, Ami Fischman wrote:
> doco

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.h:113: // FilterCallback.
On 2011/05/12 20:42:16, Ami Fischman wrote:
> TODO to remove when FilterCallback goes away?

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter....
media/base/composite_filter.h:122: FilterStatusCB status_cb_;
On 2011/05/12 20:42:16, Ami Fischman wrote:
> Yowza; either can be non-null?  Seems like a TODO to remove callback_ is in
> order?
Yes. One or the other will be non-null, but not both. Added a TODO

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:57: // |expected_status| - The expected
status value returned by the status
On 2011/05/12 20:42:16, Ami Fischman wrote:
> There is no "status callback" in this comment other than referenced here.

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:89: // Verifies |filter_1_callback_| or
|filter_1_status_cb_| set.
On 2011/05/12 20:42:16, Ami Fischman wrote:
> "Verifies" seems overly assertive.  Returns whether they are set.

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:90: bool HaveFilter1Callback() const;
On 2011/05/12 20:42:16, Ami Fischman wrote:
> Doesn't chrome use Has* generally?

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:244: case SEEK: {
On 2011/05/12 20:42:16, Ami Fischman wrote:
> braces unnecessary

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:331: return filter_1_callback_ != NULL
|| !filter_1_status_cb_.is_null();
On 2011/05/12 20:42:16, Ami Fischman wrote:
> CHECK not both?

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:351: return filter_2_callback_ != NULL
|| !filter_2_status_cb_.is_null();
On 2011/05/12 20:42:16, Ami Fischman wrote:
> ditto

Done.

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

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.cc#newcode66
media/base/filters.cc:66: if (!callback.is_null())
On 2011/05/12 20:42:16, Ami Fischman wrote:
> I don't understand the DCHECK+if.
> Is there a missing TODO here?
I was just making it match the other methods. I'll remove the if here and in the
other methods.

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.cc#newcode67
media/base/filters.cc:67: callback.Run(PIPELINE_OK);
On 2011/05/12 20:42:16, Ami Fischman wrote:
> outdent

Done.

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

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.h#newcode71
media/base/filters.h:71: FilterStatusCB CopyAndResetCB(FilterStatusCB& cb);
On 2011/05/12 20:42:16, Ami Fischman wrote:
> style guide says no non-const reference passing.

Done.

http://codereview.chromium.org/6969026/diff/1002/media/base/filters.h#newcode71
media/base/filters.h:71: FilterStatusCB CopyAndResetCB(FilterStatusCB& cb);
On 2011/05/12 20:42:16, Ami Fischman wrote:
> I think you only use this to immediately Run() the returned value, so why not
> void RunAndResetCB(FilterStatusCB* cb, PipelineStatus status) {
>   cb->Run(status);
>   cb->Reset();
> }
> ?
Created a slightly modified version that uses a temporary. We need to clear cb
before the callback is called.

http://codereview.chromium.org/6969026/diff/1002/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:582: SetError(status);
On 2011/05/12 20:42:16, Ami Fischman wrote:
> This is funky b/c it posts the errorchanged task, and then OFST() below posts
a
> FSTT task, which depends on the errorchanged having run.  I guess this is ok
> (messageloop must guarantee order of execution, right?), but funky.
Yes. This is one of the major reasons why I want to get rid of SetError() in
FilterHost. Code depends on this fact that both the callback AND the error get
put on the message loop. A few months ago, I tried to prevent posting to the
message loop if we were on the pipeline thread, but ran into this assumption.

http://codereview.chromium.org/6969026/diff/1002/media/filters/adaptive_demux...
File media/filters/adaptive_demuxer.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/adaptive_demux...
media/filters/adaptive_demuxer.cc:195: // Helper class that wraps FilterStatusCB
and expects to get called a set
On 2011/05/12 20:42:16, Ami Fischman wrote:
> Sad panda that this has to coexist w/ CountingCallback.
Agreed. It won't last too long. Once I convert Stop() to use FilterStatusCB I
can remove CountingCallback. I've added TODOs to remind me to remove the code
when Stop() is converted.

http://codereview.chromium.org/6969026/diff/1002/media/filters/adaptive_demux...
media/filters/adaptive_demuxer.cc:210: base::Unretained(this));
On 2011/05/12 20:42:16, Ami Fischman wrote:
> I think if you drop the unretained then you can also drop the delete this from
> the fire_orig_cb callpath below, no?
Yes. I had to derive from RefCountedThreadSafe<> to make the compiler happy.

http://codereview.chromium.org/6969026/diff/1002/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:67: void
AudioRendererBase::Seek(base::TimeDelta time, const FilterStatusCB&  cb) {
On 2011/05/12 20:42:16, Ami Fischman wrote:
> Extra space before cb.

Done.

http://codereview.chromium.org/6969026/diff/1002/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:189: NewRunnableMethod(this,
&FFmpegVideoDecoder::Seek,
On 2011/05/12 20:42:16, Ami Fischman wrote:
> indent is off (before your CL).

Done.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:588: scoped_ptr<FilterCallback> callback;
On 2011/05/12 20:42:16, Ami Fischman wrote:
> Typically the reason we have this sort of stack var holding member state is so
> that it can be used outside the lock that has to be held to inspect the member
> state.  But in this case that doesn't hold - the entire method body is under
> lock, so you could as well just fire seek_cb_ inside the switch.

Done.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:607: status_cb = CopyAndResetCB(seek_cb_);
On 2011/05/12 20:42:16, Ami Fischman wrote:
> std::swap() fits in better here, esp. if you take my suggestion about
> CopyAndResetCB().

Done.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:608: } break;
On 2011/05/12 20:42:16, Ami Fischman wrote:
> braces should be unnecessary.

Done.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:624: host()->SetError(status);
On 2011/05/12 20:42:16, Ami Fischman wrote:
> Is it really right to not set the host's error state if e.g. we were seeking? 
> Wouldn't we want to know about the error when doing non-seek operations
> following?

Yes. Calling host()->SetError() is the old style of reporting errors for
FilterCallbacks. The idea is that using FilterStatusCB will replace the
"host()->SetError() + callback->Run()" combo. Once all the methods are converted
over, the SetError() will go away and they will all use RunAndResetCB().

> I suspect both this and the state_=kError; statement above really belong at
the
> top of the method, after taking a copy of state_ to switch off of.
Moved state_ = kError before switch.

[Ami GONE FROM CHROMIUM, 2011-05-12 22:49:25]

Almost there.

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:624: host()->SetError(status);
On 2011/05/12 22:30:40, acolwell wrote:
> On 2011/05/12 20:42:16, Ami Fischman wrote:
> > Is it really right to not set the host's error state if e.g. we were
seeking? 
> > Wouldn't we want to know about the error when doing non-seek operations
> > following?
> 
> Yes. Calling host()->SetError() is the old style of reporting errors for
> FilterCallbacks. The idea is that using FilterStatusCB will replace the
> "host()->SetError() + callback->Run()" combo. Once all the methods are
converted
> over, the SetError() will go away and they will all use RunAndResetCB().

I understand this, but I'm questioning whether the intermediate state is safe.

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

http://codereview.chromium.org/6969026/diff/1004/media/base/filters.h#newcode71
media/base/filters.h:71: void RunAndResetCB(FilterStatusCB* cb, PipelineStatus
status);
In that case, s/RunAndResetCB/ResetAndRunCB/ ?

[acolwell GONE FROM CHROMIUM, 2011-05-13 00:49:10]

bam!

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/6969026/diff/1002/media/filters/video_renderer...
media/filters/video_renderer_base.cc:624: host()->SetError(status);
On 2011/05/12 22:49:25, Ami Fischman wrote:
> I understand this, but I'm questioning whether the intermediate state is safe.
Sorry I wasn't clear. Yes this is safe because in this case calling status_cb
will call CompositeFilter::OnStatusCB(). host()->SetError() here calls
CompositeFilter::FilterHostImpl::SetError() which ends up calling
CompositeFilter::SetError(). OnStatusCB() triggers the same code paths as this
code.

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

http://codereview.chromium.org/6969026/diff/1004/media/base/filters.h#newcode71
media/base/filters.h:71: void RunAndResetCB(FilterStatusCB* cb, PipelineStatus
status);
On 2011/05/12 22:49:26, Ami Fischman wrote:
> In that case, s/RunAndResetCB/ResetAndRunCB/ ?

Done.

[Ami GONE FROM CHROMIUM, 2011-05-13 02:17:02]

LGTM
boom!

[scherkus (not reviewing), 2011-05-13 23:01:04]

sanity check LGTM

loving it!