Issue 11316293: Replace WaitableEvents and ConditionalVariables in VideoRendererBase tests with MessageLoop.

Issue 11316293: Replace WaitableEvents and ConditionalVariables in VideoRendererBase tests with MessageLoop. (Closed)

Created:
8 years ago by scherkus (not reviewing)

Modified:
8 years ago

Reviewers:
acolwell GONE FROM CHROMIUM

CC:
chromium-reviews, feature-media-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Replace WaitableEvents and ConditionalVariables in VideoRendererBase tests with MessageLoop. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171438

Patch Set 10 : fix bustage #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+434 lines, -437 lines)			Patch
M	content/renderer/media/rtc_video_decoder_unittest.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	media/base/mock_callback.h	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -36 lines	0 comments	Download
M	media/base/mock_callback.cc	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -43 lines	0 comments	Download
M	media/base/pipeline_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
A	media/base/test_helpers.h	View	1 2 3 4 5 6 7	1 chunk	+57 lines, -0 lines	0 comments	Download
A	media/base/test_helpers.cc	View	1 2 3 4 5 6 7	1 chunk	+99 lines, -0 lines	0 comments	Download
M	media/filters/audio_renderer_impl_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/chunk_demuxer_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/decrypting_audio_decoder_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/decrypting_demuxer_stream_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/decrypting_video_decoder_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/ffmpeg_audio_decoder_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/ffmpeg_demuxer_unittest.cc	View	1 2 3 4 5 6 7	7 chunks	+19 lines, -25 lines	0 comments	Download
M	media/filters/ffmpeg_video_decoder_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/file_data_source_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	media/filters/video_renderer_base_unittest.cc	View	1 2 3 4 5 6 7 8	19 chunks	+246 lines, -320 lines	0 comments	Download
M	media/media.gyp	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -2 lines	0 comments	Download
M	webkit/media/buffered_data_source_unittest.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

scherkus (not reviewing)

I'm not totally happy with this but if we're moving parts of VRB over to ...

8 years ago (2012-12-01 01:54:43 UTC) #1

acolwell GONE FROM CHROMIUM

Here is my initial round of comments. https://codereview.chromium.org/11316293/diff/3002/media/filters/video_renderer_base_unittest.cc File media/filters/video_renderer_base_unittest.cc (right): https://codereview.chromium.org/11316293/diff/3002/media/filters/video_renderer_base_unittest.cc#newcode47 media/filters/video_renderer_base_unittest.cc:47: void Reset() ...

8 years ago (2012-12-03 22:50:43 UTC) #2

scherkus (not reviewing)

PTAL -- I tried to annotate my latest PS w/ notes to help reviewing It's ...

8 years ago (2012-12-05 00:03:45 UTC) #3

acolwell GONE FROM CHROMIUM

LGTM % minor comments https://codereview.chromium.org/11316293/diff/3003/media/base/mock_callback.h File media/base/mock_callback.h (right): https://codereview.chromium.org/11316293/diff/3003/media/base/mock_callback.h#newcode17 media/base/mock_callback.h:17: class MockClosure : public base::RefCountedThreadSafe<MockClosure> ...

8 years ago (2012-12-05 18:15:25 UTC) #4

scherkus (not reviewing)

mind taking another quick look? https://codereview.chromium.org/11316293/diff/3003/media/filters/video_renderer_base_unittest.cc File media/filters/video_renderer_base_unittest.cc (right): https://codereview.chromium.org/11316293/diff/3003/media/filters/video_renderer_base_unittest.cc#newcode49 media/filters/video_renderer_base_unittest.cc:49: void Reset() { signaled_ ...

8 years ago (2012-12-05 21:52:02 UTC) #5

acolwell GONE FROM CHROMIUM

A few more comments. https://codereview.chromium.org/11316293/diff/3003/media/filters/video_renderer_base_unittest.cc File media/filters/video_renderer_base_unittest.cc (right): https://codereview.chromium.org/11316293/diff/3003/media/filters/video_renderer_base_unittest.cc#newcode654 media/filters/video_renderer_base_unittest.cc:654: Stop(); On 2012/12/05 21:52:03, scherkus ...

8 years ago (2012-12-05 23:00:54 UTC) #6

scherkus (not reviewing)

PTAL https://codereview.chromium.org/11316293/diff/19001/media/filters/video_renderer_base_unittest.cc File media/filters/video_renderer_base_unittest.cc (right): https://codereview.chromium.org/11316293/diff/19001/media/filters/video_renderer_base_unittest.cc#newcode247 media/filters/video_renderer_base_unittest.cc:247: message_loop_.PostDelayedTask( On 2012/12/05 23:00:54, acolwell wrote: > On ...

8 years ago (2012-12-06 01:44:08 UTC) #7

PTAL

https://codereview.chromium.org/11316293/diff/19001/media/filters/video_rende...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/11316293/diff/19001/media/filters/video_rende...
media/filters/video_renderer_base_unittest.cc:247:
message_loop_.PostDelayedTask(
On 2012/12/05 23:00:54, acolwell wrote:
> On 2012/12/05 21:52:03, scherkus wrote:
> > FYI this is a bit gross but something like it is necessary as we need to
wait
> > for the internal timing thread to notice time has advanced + kick off a read
> > 
> > open to suggestions!
> I think if you add a bool to this class and
> 
> How about replacing the loop with something like this instead of the loop.
> 
> DCHECK(pending_read_cb_.is_null());
> WaitableMessageLoopEvent event;
> pending_read_cb_ = event.GetClosure();
> RunAndWait();
> EXPECT_TRUE(pending_read_cb_.is_null());

Done.

https://codereview.chromium.org/11316293/diff/19001/media/filters/video_rende...
media/filters/video_renderer_base_unittest.cc:312: return;
On 2012/12/05 23:00:54, acolwell wrote:
> I think if you change this to 
> if (decode_results_.empty()) {
>   if (!pending_read_cb_.is_null())
>     base::ResetAndReturn(&pending_read_cb_).Run();
> 
>   return;
> }
> 
> You can convert WaitForPendingRead() look more like everything else.

Done.

https://codereview.chromium.org/11316293/diff/19001/media/filters/video_rende...
media/filters/video_renderer_base_unittest.cc:557: SetTime(kFrameDuration);
On 2012/12/05 23:00:54, acolwell wrote:
> nit: Shouldn't this be something more like SetTime(GetTime() +
kFrameDuration)?
> This looks more like advancing time to me. Perhaps rename SetTime() to
> AdvanceTime(kFrameDuration)?

Done -- also added Ms suffixes to various variables to make sense of what units
we're dealing with

https://codereview.chromium.org/11316293/diff/19001/media/filters/video_rende...
media/filters/video_renderer_base_unittest.cc:577: QueueAbortedRead();
On 2012/12/05 23:00:54, acolwell wrote:
> If you move this before SetTime() won't this remove the need for
> SatisfyPendingRead() & WaitForPendingRead()?

The order of SetTime -> Wait guarantees the timing thread has seen the clock
update + posted a task to read + we've executed said task

If we did Queue -> SetTime, we race against calling VRB::Pause() and pausing the
timing thread before it has a chance to see the updated time + read our queued
result.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scherkus@chromium.org/11316293/25002

8 years ago (2012-12-06 02:53:24 UTC) #9

Message was sent while issue was closed.

Change committed as 171438

Expand Messages | Collapse Messages