Allow Preroll() call on VideoRendererImpl without a Flush().

Allow Preroll() call on VideoRendererImpl without a Flush().

This is the first patch in a sequence of changes to enable Pipeline
to properly manage underflows. This change allows the VideoRendererImpl
to be prerolled in non-seek situations.

BUG=144683
TEST=VideoRendererImplTest.Rebuffer, VideoRendererImplTest.Rebuffer_AlreadyHaveEnoughFrames

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238513

[xhwang, 2013-11-27 21:22:10]

This CL looks good, but I feel the fact that we need to pass in a timestamp in
kFlushed state and kNoTimeStamp() in a kPaused state is a bit confusing. Since
this defines the API of VRI, at least we need some documentation. Or, can we
make this generic that we always pass in A timestamp, which will be checked
against when a frame is ready. Then it's up to the called (pipeline) to decide
what time to preroll to, e.g. kNoTimeStamp() after pause, which means that
preroll frames with any timestamps.

https://codereview.chromium.org/89673002/diff/1/media/filters/video_renderer_...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/89673002/diff/1/media/filters/video_renderer_...
media/filters/video_renderer_impl.cc:139: DCHECK_EQ(state_, kPaused) << "state_
" << state_;
How about 

DCHECK(state_ == kFlushed || state_ == kPaused) << state_;

on line 133 to emphasis that we only allow preroll in these two states? Then we
don't need this DCHECK here.

[acolwell GONE FROM CHROMIUM, 2013-12-02 23:17:43]

PTAL.

On 2013/11/27 21:22:10, xhwang wrote:
> This CL looks good, but I feel the fact that we need to pass in a timestamp in
> kFlushed state and kNoTimeStamp() in a kPaused state is a bit confusing. Since
> this defines the API of VRI, at least we need some documentation. Or, can we
> make this generic that we always pass in A timestamp, which will be checked
> against when a frame is ready. Then it's up to the called (pipeline) to decide
> what time to preroll to, e.g. kNoTimeStamp() after pause, which means that
> preroll frames with any timestamps.

I added docs to VideoRenderer::Preroll delcaration. I think it addresses your
concerns.

https://codereview.chromium.org/89673002/diff/1/media/filters/video_renderer_...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/89673002/diff/1/media/filters/video_renderer_...
media/filters/video_renderer_impl.cc:139: DCHECK_EQ(state_, kPaused) << "state_
" << state_;
On 2013/11/27 21:22:10, xhwang wrote:
> How about 
> 
> DCHECK(state_ == kFlushed || state_ == kPaused) << state_;
> 
> on line 133 to emphasis that we only allow preroll in these two states? Then
we
> don't need this DCHECK here.

Done.

[xhwang, 2013-12-03 16:55:19]

lgtm

[commit-bot: I haz the power, 2013-12-03 18:40:09]

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

[commit-bot: I haz the power, 2013-12-03 19:14:01]

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

[commit-bot: I haz the power, 2013-12-03 23:42:31]

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

[commit-bot: I haz the power, 2013-12-04 01:11:13]

Change committed as 238513