Clean up VideoRendererBase tests.

Clean up VideoRendererBase tests.

Previously the tests were relying on locks internal to the class to prevent race conditions. Instead wait for callbacks to be executed by using a combination of synchronization primitives.

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

[acolwell GONE FROM CHROMIUM, 2011-10-31 18:20:10]

looks good. Just a few questions

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (left):

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:234: TEST_F(VideoRendererBaseTest,
Error_Playing) {
Why was this and the next test removed?

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:292: TEST_F(VideoRendererBaseTest,
Error_DuringPaint) {
Why was this and the next test removed? These look like regression tests for
bugs we've fixed.

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:165: Stop();
This sequence doesn't cause a race on event_ because we run WaitForClosure() in
each of these methods right?

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:247: CreateFrame(i * kDuration,
kDuration);
Consider renaming this. CreateFrame() doesn't really convey that it is creating
a frame and sending it to the decoder so it can satisfy a read.
SendDecoderFrame(),  SatisfyFrameRequest(), or SatisfyProduceVideoFrame() maybe?

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:278: if (seeking_ &&
pending_reads_ == 0) {
Why not just make this an else if(pending_reads_ == 0) ?

[scherkus (not reviewing), 2011-10-31 20:01:53]

thanks for review

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (left):

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:234: TEST_F(VideoRendererBaseTest,
Error_Playing) {
On 2011/10/31 18:20:10, acolwell wrote:
> Why was this and the next test removed?

Whoops -- I'll add them back for now!

Currently FFmpegVideoDecoder sends NULL video frames which VRB is supposed to
interpret as a decoder error and fire PIPELINE_ERROR_DECODE

In my refactor, FFVD handles decode errors itself which means VRB will go
through the Pause/Flush/Stop shutdown sequence as opposed to handling errors on
the fly

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:292: TEST_F(VideoRendererBaseTest,
Error_DuringPaint) {
On 2011/10/31 18:20:10, acolwell wrote:
> Why was this and the next test removed? These look like regression tests for
> bugs we've fixed.

Error_DuringPaint added back for reasons explained above

GetCurrentFrame_AfterStop has been renamed GetCurrentFrame_Shutdown

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:165: Stop();
On 2011/10/31 18:20:10, acolwell wrote:
> This sequence doesn't cause a race on event_ because we run WaitForClosure()
in
> each of these methods right?

Correct -- each method is blocking until callback is executed (with timeout, of
course)

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:247: CreateFrame(i * kDuration,
kDuration);
On 2011/10/31 18:20:10, acolwell wrote:
> Consider renaming this. CreateFrame() doesn't really convey that it is
creating
> a frame and sending it to the decoder so it can satisfy a read.
> SendDecoderFrame(),  SatisfyFrameRequest(), or SatisfyProduceVideoFrame()
maybe?

Done.

http://codereview.chromium.org/8414041/diff/4/media/filters/video_renderer_ba...
media/filters/video_renderer_base_unittest.cc:278: if (seeking_ &&
pending_reads_ == 0) {
On 2011/10/31 18:20:10, acolwell wrote:
> Why not just make this an else if(pending_reads_ == 0) ?

Done.

[acolwell GONE FROM CHROMIUM, 2011-10-31 20:15:49]

LGTM