Fix VideoRendererBase to allow the decoder to be flushed with a pending read.

Fix VideoRendererBase to allow the decoder to be flushed with a pending read.


BUG=123134
TEST=media_unittests


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

[acolwell GONE FROM CHROMIUM, 2012-04-24 21:27:15]

Here is a replacement patch for Ronghua's original change. It is a little more
involved, but I believe it solves the problem the WebRTC folks are having and
makes the code a little clearer to understand. 

The code now also enforces that the decoder MUST call the pending read callback
before it calls the flush callback.

[Ronghua Wu (Left Chromium), 2012-04-24 22:44:26]

lgtm

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

http://codereview.chromium.org/10173012/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:592: TEST_F(VideoRendererBaseTest,
AbortPendingRead_Flush) {
Since you no longer abort read, rename the unit test also.

[wjia(left Chromium), 2012-04-24 22:46:39]

lgtm. Thanks for fixing this!

[scherkus (not reviewing), 2012-04-25 03:33:58]

LGTM w/ q -- it'd be nice to not have to worry about re-entrant callbacks!

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

http://codereview.chromium.org/10173012/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base.cc:60: // call.
should we have semantics similar to VideoDecoder::Read() that the cb must be
executed on a separate call stack?

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

http://codereview.chromium.org/10173012/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:592: TEST_F(VideoRendererBaseTest,
AbortPendingRead_Flush) {
On 2012/04/24 22:44:26, Ronghua Wu wrote:
> Since you no longer abort read, rename the unit test also.

it appears the AbortRead() is has been folded into the Flush implementation

[commit-bot: I haz the power, 2012-04-25 17:41:43]

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

[acolwell GONE FROM CHROMIUM, 2012-04-25 17:41:44]

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

http://codereview.chromium.org/10173012/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base.cc:60: // call.
On 2012/04/25 03:33:58, scherkus wrote:
> should we have semantics similar to VideoDecoder::Read() that the cb must be
> executed on a separate call stack?
Yes. I think that is a good idea. I created a bug for this work
(http://crbug.com/125020)

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

http://codereview.chromium.org/10173012/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:592: TEST_F(VideoRendererBaseTest,
AbortPendingRead_Flush) {
On 2012/04/25 03:33:58, scherkus wrote:
> On 2012/04/24 22:44:26, Ronghua Wu wrote:
> > Since you no longer abort read, rename the unit test also.
> 
> it appears the AbortRead() is has been folded into the Flush implementation

Yes the read is aborted in the flush implementation.

[commit-bot: I haz the power, 2012-04-25 19:39:52]

Change committed as 133957