Fix VideoRendererBase end of stream logic.

Fix VideoRendererBase end of stream logic to use a default 
last frame duration if the last frame is far away from the clip duration.


BUG=None
TEST=VideoRendererBaseTest.EndOfStream_DefaultFrameDuration, VideoRendererBaseTest.EndOfStream_DefaultFrameDuration

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

[DaleCurtis, 2012-08-06 21:44:35]

Also needs a more complete description and TEST= line.

http://codereview.chromium.org/10829200/diff/2001/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/2001/media/filters/video_rendere...
media/filters/video_renderer_base.cc:214: if
(ready_frames_.front()->IsEndOfStream()) {
Not super familiar with this code, but won't this cause us to signal ended early
sometimes? The old code ensured that current_frame_ had been on screen for the
proper length of time before bailing out.

[DaleCurtis, 2012-08-06 22:49:41]

Discussed offline; lgtm I ran some tests locally and things seem to come out as
well.

+scherkus just in case though, since he wrote the original code.

[scherkus (not reviewing), 2012-08-06 23:36:46]

the CL description is awfully light on details!

is this a regression? do we have a bug/test in place? what change broke things?

http://codereview.chromium.org/10829200/diff/2001/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/2001/media/filters/video_rendere...
media/filters/video_renderer_base.cc:214: if
(ready_frames_.front()->IsEndOfStream()) {
On 2012/08/06 21:44:35, DaleCurtis wrote:
> Not super familiar with this code, but won't this cause us to signal ended
early
> sometimes? The old code ensured that current_frame_ had been on screen for the
> proper length of time before bailing out.

I wondered about that in acolwell's previous change...

How would we handle a 10 frame video that was 1 fps? Would we play for 10
seconds or 9 seconds?

Is this simply the outcome of r149274?

[acolwell GONE FROM CHROMIUM, 2012-08-06 23:48:06]

I'm going to rework this. I'm not happy with this change. Basically the problem
is that the current code waits until we reach the clip duration before
signalling EOS. This slows down Dale's
Cr100543/FFmpegRegressionTest.BasicPlayback test because it plays ~9 seconds of
the video and then triggers EOS. The renderer waits until the clock has reached
the clip duration (~700 seconds) before calling ended_cb_.

[acolwell GONE FROM CHROMIUM, 2012-08-07 19:45:27]

PTAL

http://codereview.chromium.org/10829200/diff/5004/media/filters/video_rendere...
File media/filters/video_renderer_base_unittest.cc (left):

http://codereview.chromium.org/10829200/diff/5004/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:56:
.WillRepeatedly(ReturnRef(kNaturalSize));
I missed this on the VideoDecoder::natural_size() removal CL

http://codereview.chromium.org/10829200/diff/5004/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:142:
renderer_->Preroll(base::TimeDelta::FromMicroseconds(timestamp),
Changed from Micro -> Milli so that these timestamp are on the same scale as the
default last frame duration.

[DaleCurtis, 2012-08-07 20:32:14]

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.cc:502: frame->SetTimestamp(end_timestamp);
Since it wasn't immediately obvious to me: when end_timestamp == kNoTimestamp()
we'll end up sleeping for (kNoTimestamp() - get_time_cb_.Run()), which I assume
is a negative number thus line 236 will skip sleeping => ending immediately.

OOC could all this be be collapsed to:

frame->SetTimestamp(std::min(
    duration, get_time_cb_.Run() + kLastFrameDuration()));

?

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.cc:503: } else  if (frame->GetTimestamp() >
duration) {
nit: extra space.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:165: int preroll_timestamp =
end_of_stream ? 0 : timestamp;
You're down converting from int64->int; did you mean to do this? Here and
elsewhere in the tests below.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:469: // and deliver all the frame
between the start and |duration|. The preroll
s/frame/frames/

[scherkus (not reviewing), 2012-08-07 20:56:47]

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.cc:509: time_cb_.Run(frame->GetTimestamp());
it's now possible to execute this w/ kNoTimestamp if (frame->IsEndOfStream() &&
ready_frames_.empty() && !curent_frame_)

is that OK?

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.h:32: static base::TimeDelta
kLastFrameDuration();
kMaxLastFrameDuration?

[acolwell GONE FROM CHROMIUM, 2012-08-07 21:10:28]

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.cc:502: frame->SetTimestamp(end_timestamp);
On 2012/08/07 20:32:14, DaleCurtis wrote:
> Since it wasn't immediately obvious to me: when end_timestamp ==
kNoTimestamp()
> we'll end up sleeping for (kNoTimestamp() - get_time_cb_.Run()), which I
assume
> is a negative number thus line 236 will skip sleeping => ending immediately.

I'm pretty sure the only way to get kNoTimestamp is if the first frame we get is
an EOS which means current_frame_ will never get set. This will trigger the
logic on line 220 and it get to line 236. Skipping sleeping would also be ok in
this case since we never got any valid frames to display so there is no need to
wait.
> 
> OOC could all this be be collapsed to:
> 
> frame->SetTimestamp(std::min(
>     duration, get_time_cb_.Run() + kLastFrameDuration()));
> 
> ?
You can't use get_time_cb_ in the normal cases because ready_frames_ is
typically 4 frames deep and we shouldn't make any assumptions about how far
apart the frames are.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.cc:503: } else  if (frame->GetTimestamp() >
duration) {
On 2012/08/07 20:32:14, DaleCurtis wrote:
> nit: extra space.

Done.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.cc:509: time_cb_.Run(frame->GetTimestamp());
On 2012/08/07 20:56:47, scherkus wrote:
> it's now possible to execute this w/ kNoTimestamp if (frame->IsEndOfStream()
&&
> ready_frames_.empty() && !curent_frame_)
> 
> is that OK?
Whoops..No it isnt' Changed it to set the time to duration on EOS and added a
DCHECK to make sure a kNoTimestamp() doesn't sneak in.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base.h:32: static base::TimeDelta
kLastFrameDuration();
On 2012/08/07 20:56:47, scherkus wrote:
> kMaxLastFrameDuration?

Done.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:165: int preroll_timestamp =
end_of_stream ? 0 : timestamp;
On 2012/08/07 20:32:14, DaleCurtis wrote:
> You're down converting from int64->int; did you mean to do this? Here and
> elsewhere in the tests below.
Changed all int64s to int since int64 is overkill for these tests.

http://codereview.chromium.org/10829200/diff/3004/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:469: // and deliver all the frame
between the start and |duration|. The preroll
On 2012/08/07 20:32:14, DaleCurtis wrote:
> s/frame/frames/

Done.

[DaleCurtis, 2012-08-07 21:20:45]

lgtm % nits.

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
media/filters/video_renderer_base.cc:509: if (frame->IsEndOfStream()) {
Could collapse into a ternary if you like.

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:7: #include "base/format_macros.h"
Can drop this header now I think.

[scherkus (not reviewing), 2012-08-07 22:01:00]

http://codereview.chromium.org/10829200/diff/10002/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/10002/media/filters/video_render...
media/filters/video_renderer_base.cc:513: time_cb_.Run(maxClockTime);
wait a tick -- what is this time_cb_ used for again? if we're paused and we
preroll 3 frames, doesn't that mean we exec time_cb_.Run(time-of-3rd-frame)?

[acolwell GONE FROM CHROMIUM, 2012-08-07 22:25:06]

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
media/filters/video_renderer_base.cc:509: if (frame->IsEndOfStream()) {
On 2012/08/07 21:20:46, DaleCurtis wrote:
> Could collapse into a ternary if you like.

Done.

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10829200/diff/3007/media/filters/video_rendere...
media/filters/video_renderer_base_unittest.cc:7: #include "base/format_macros.h"
On 2012/08/07 21:20:46, DaleCurtis wrote:
> Can drop this header now I think.

Done.

[scherkus (not reviewing), 2012-08-07 23:03:19]

LGTM w/ nit

http://codereview.chromium.org/10829200/diff/10002/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10829200/diff/10002/media/filters/video_render...
media/filters/video_renderer_base.cc:510: base::TimeDelta maxClockTime =
unix_hacker

http://codereview.chromium.org/10829200/diff/10002/media/filters/video_render...
media/filters/video_renderer_base.cc:513: time_cb_.Run(maxClockTime);
On 2012/08/07 22:01:00, scherkus wrote:
> wait a tick -- what is this time_cb_ used for again? if we're paused and we
> preroll 3 frames, doesn't that mean we exec time_cb_.Run(time-of-3rd-frame)?

Discussed offline -- this is really a max_time_cb_ and not a time_cb_

[commit-bot: I haz the power, 2012-08-07 23:26:09]

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

[commit-bot: I haz the power, 2012-08-08 00:30:34]

Change committed as 150467