corresponding change in CaptureVideoDecoder and RTCVideoDecoder due to pull model used in medi...

corresponding change in CaptureVideoDecoder and RTCVideoDecoder due to pull model used in media VideoRenderer
1. Recent refactoring in media stack went back to pull model between VideoDecoder and VideoRenderer. Necessary change is needed in VideoDecoder subclasses.
2. fix racing condition during video capture shutdown. Now VideoCaptureImpl is guaranteed to be deleted after all events have been processed.

BUG=none
TEST=trybots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110596

[scherkus (not reviewing), 2011-11-16 01:12:35]

tests?

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
File content/renderer/media/capture_video_decoder.cc (right):

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
content/renderer/media/capture_video_decoder.cc:152: if (state_ != kNormal) {
does this actually happen in practice?

I'd be interested in knowing the conditions

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
File content/renderer/media/capture_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
content/renderer/media/capture_video_decoder.h:103: std::queue<ReadCB>
read_cbs_;
take a look at the definition of VideoDecoder::Read() and FFmpegVideoDecoder --
the new contract is that only one pending read can be in progress at any time

this is to help make things easier so we don't have to worry about queues of
callbacks :)

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.h:75: std::queue<ReadCB> read_cbs_;
ditto

[wjia(left Chromium), 2011-11-16 05:25:53]

I'd put new tests in a follow-on patch. This patch is a blocker for webrtc.

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
File content/renderer/media/capture_video_decoder.cc (right):

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
content/renderer/media/capture_video_decoder.cc:152: if (state_ != kNormal) {
On 2011/11/16 01:12:36, scherkus wrote:
> does this actually happen in practice?
> 
> I'd be interested in knowing the conditions
It could happen in two cases:
1. When pipeline put all filters in pause, there could be racing between Pause()
from pipeline and Read() from VideoRenderer.
2. VideoCapture device has an error event. In the future, error handling will be
added.

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
File content/renderer/media/capture_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
content/renderer/media/capture_video_decoder.h:103: std::queue<ReadCB>
read_cbs_;
On 2011/11/16 01:12:36, scherkus wrote:
> take a look at the definition of VideoDecoder::Read() and FFmpegVideoDecoder
--
> the new contract is that only one pending read can be in progress at any time
> 
> this is to help make things easier so we don't have to worry about queues of
> callbacks :)
That sounds scary since single buffer scheme serializes the pipeline operations.
That could slow down the decoding, lead to display jittering, etc.

The callback queue doesn't cost much here and it's more generic.

[acolwell GONE FROM CHROMIUM, 2011-11-16 18:49:21]

I'm also backing scherkus' request for tests. It may be blocking WebRTC, but we
have no idea why or how. A test that shows how the current code is broken and
how your patch fixes the problem would clarify this for us. It would also
provide a way for us to avoid breaking your code in the future.

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
File content/renderer/media/capture_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/captu...
content/renderer/media/capture_video_decoder.h:103: std::queue<ReadCB>
read_cbs_;
On 2011/11/16 05:25:54, wjia wrote:
> That sounds scary since single buffer scheme serializes the pipeline
operations.

> That could slow down the decoding, lead to display jittering, etc.
> 
It does not serialize pipeline operations. Decoding is happening on a different
thread and the rendererer maintains a queue of frames to avoid display jitter.



> The callback queue doesn't cost much here and it's more generic.

The callback queue does have a cost. It makes handling state transitions,
teardown, and ownership more complicated. We have been suffering from this cost
for a while which is why Andrew made the changes he did. The refactoring was
done to simplify the code so it would be less error prone and more predictable. 

Until we start seeing the potential problems you are stating I think it is
better to stick with the simpler solution and avoid callback queues.

[wjia(left Chromium), 2011-11-17 00:54:59]

On Wed, Nov 16, 2011 at 10:49 AM, <acolwell@chromium.org> wrote:

> I'm also backing scherkus' request for tests. It may be blocking WebRTC,
> but we
> have no idea why or how. A test that shows how the current code is broken
> and
> how your patch fixes the problem would clarify this for us. It would also
> provide a way for us to avoid breaking your code in the future.
>
> More tests have been added to the patch. This patch fixes the broken
contract between VideoDecoder and VideoRenderer which is changed recently
during media stack refactoring. The previous contract didn't require
VideoDecoder to fulfill read request from VideoRenderer. Now it does. That
causes VideoRenderer hangs during Flush.

I don't think a test can always signal a broken code if the change is about
contract change, especially when more restrictions are added, like this
refactoring.


>
>
> http://codereview.chromium.**org/8528045/diff/5001/content/**
>
renderer/media/capture_video_**decoder.h<http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/capture_video_decoder.h>
> File content/renderer/media/**capture_video_decoder.h (right):
>
> http://codereview.chromium.**org/8528045/diff/5001/content/**
>
renderer/media/capture_video_**decoder.h#newcode103<http://codereview.chromium.org/8528045/diff/5001/content/renderer/media/capture_video_decoder.h#newcode103>
> content/renderer/media/**capture_video_decoder.h:103: std::queue<ReadCB>
> read_cbs_;
> On 2011/11/16 05:25:54, wjia wrote:
>
>> That sounds scary since single buffer scheme serializes the pipeline
>>
> operations.
>
>  That could slow down the decoding, lead to display jittering, etc.
>>
>
> It does not serialize pipeline operations. Decoding is happening on a
> different thread and the rendererer maintains a queue of frames to avoid
> display jitter.
>
> Take a look at how decoder and demuxer work. They are serialized. When
demuxer is working on extracting bit stream, decoder is waiting. When
decoder is working on decompressing the bit stream, demuxer is waiting. The
reason is that there is only one read request.


>
>
>
>  The callback queue doesn't cost much here and it's more generic.
>>
>
> The callback queue does have a cost. It makes handling state
> transitions, teardown, and ownership more complicated. We have been
> suffering from this cost for a while which is why Andrew made the
> changes he did. The refactoring was done to simplify the code so it
> would be less error prone and more predictable.


> Until we start seeing the potential problems you are stating I think it
> is better to stick with the simpler solution and avoid callback queues.
>

Simplifying code is good only when it doesn't sacrifice performance.
There is difference between API design and how to implement API. Using
callback queue in VideoDecoder doesn't violate the API which says there is
only one read request. In CaptureVideoDecoder and RTCVideoDecoder, I don't
see much overhead with using callback queue. Every time when it needs to
check read_cb, it will check the callback queue. That's the only difference.


>

http://codereview.chromium.**org/8528045/<http://codereview.chromium.org/8528...
>

[scherkus (not reviewing), 2011-11-17 02:05:21]

I'm sure we all agree that performance is important however I don't believe in
prematurely optimizing a system especially without having measurements to back
it up.

With respect to tests I understand that a unit test is a poor fit for catching
changes in API contracts. Do you know of any integration tests that could be
used to catch contractual changes?

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/capt...
File content/renderer/media/capture_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/capt...
content/renderer/media/capture_video_decoder.h:95: // Helper.
this comment isn't useful -- please either write an informative comment or
remove the comment

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/capt...
content/renderer/media/capture_video_decoder.h:103: std::queue<ReadCB>
read_cbs_;
please remove queues

I'm not sure I understand your reason to complicate the implementation

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:54: // Helper.
ditto

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:55: void DeliverBlackFrame(const
ReadCB& read_cb);
should this be private?

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:76: std::queue<ReadCB> read_cbs_;
ditto

[wjia(left Chromium), 2011-11-17 19:14:45]

patch has been updated.

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/capt...
File content/renderer/media/capture_video_decoder.h (right):

http://codereview.chromium.org/8528045/diff/11001/content/renderer/media/capt...
content/renderer/media/capture_video_decoder.h:95: // Helper.
On 2011/11/17 02:05:22, scherkus wrote:
> this comment isn't useful -- please either write an informative comment or
> remove the comment

Done.

[scherkus (not reviewing), 2011-11-17 22:57:03]

LGTM

thanks wei!

we should discuss medium-sized / integration tests for WebRTC so we don't let
these types of regressions slip in