Fix video sync error when framerate or playback rate is low...

Fix video sync error when framerate or playback rate is low
BUG=18441
TEST=play a low framerate video or under low rate and verify it stays synced

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

[kylep, 2009-08-05 01:12:20]

Video stutters a little, but I'll file a bug for that after this gets checked
in. The sync for low framerate and/or slow playback should be relatively close
now.

[awong, 2009-08-05 01:18:15]

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

http://codereview.chromium.org/160620/diff/1/2#newcode215
Line 215: // Make sure we're not playing too fast
Period at end of comment.

Explain also why kMaxSleepMilliseconds is a good time to check here.

http://codereview.chromium.org/160620/diff/1/2#newcode251
Line 251: base::TimeDelta sleep = CalculateSleepDuration(next_frame,
playback_rate);
I worry that we're reading the "now" time in two spots in this code.  Does it
matter?

[scherkus (not reviewing), 2009-08-05 04:00:35]

nice catch!

you're touching some of the scariest threading code we have :P

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

http://codereview.chromium.org/160620/diff/1/2#newcode31
Line 31: // TODO(scherkus): what if the native frame rate is 15 or 10 fps?
you should update this comment

http://codereview.chromium.org/160620/diff/1/2#newcode216
Line 216: if ((current_frame_->GetTimestamp() -
host()->GetTime()).InMilliseconds()
since you're basically checking whether we should idle until we're within
kMaxSleepMilliseconds, we address both albert's concern of reading time in two
spots and simplify your change:
1) save a local copy of "now" at the top where we save the state and playback
rate
2) add another condition to the "Sleep(10)" clause that if the current frame is
way ahead we should also idle

What I like about this is we'll sleep for our regular idle amount (10ms) and
keep checking the time, current playback rate and current state (i.e., this
thread stays responsive instead of checking it every kMaxSleepMilliseconds =
60ms)

[kylep, 2009-08-05 18:31:59]

h264 looks great. ogg is still stutters a little, but I think that's related to
the inexactness of the timestamps (and bug 18208)

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

http://codereview.chromium.org/160620/diff/1/2#newcode31
Line 31: // TODO(scherkus): what if the native frame rate is 15 or 10 fps?
On 2009/08/05 04:00:35, scherkus wrote:
> you should update this comment

Done.

http://codereview.chromium.org/160620/diff/1/2#newcode216
Line 216: if ((current_frame_->GetTimestamp() -
host()->GetTime()).InMilliseconds()
On 2009/08/05 04:00:35, scherkus wrote:
> since you're basically checking whether we should idle until we're within
> kMaxSleepMilliseconds, we address both albert's concern of reading time in two
> spots and simplify your change:
> 1) save a local copy of "now" at the top where we save the state and playback
> rate
> 2) add another condition to the "Sleep(10)" clause that if the current frame
is
> way ahead we should also idle
> 
> What I like about this is we'll sleep for our regular idle amount (10ms) and
> keep checking the time, current playback rate and current state (i.e., this
> thread stays responsive instead of checking it every kMaxSleepMilliseconds =
> 60ms)

Done.

[scherkus (not reviewing), 2009-08-05 19:10:36]

LGTM

you can also consider pulling the "10" into a constant like kIdleMilliseconds or
something

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

http://codereview.chromium.org/160620/diff/5/2001#newcode202
Line 202: // Sleeping for 10 ms if we're ahead by more than that.
comment inline like this is bad, you can just remove it

[kylep, 2009-08-05 21:53:56]

video_renderer_base_unittest.cc changes turn out to be just cosmetic/svn
properties. I had to change where I check the time to keep it from failing
instead of fixing the test ><

[scherkus (not reviewing), 2009-08-06 04:14:32]

LGTM

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

http://codereview.chromium.org/160620/diff/3007/4006#newcode216
Line 216: // Make sure we're not too far ahead.
I would reword this to be "Idle if we are too far ahead"

readability nit: maybe save the delta or milliseconds to a local variable so the
if statement doesn't break up -- whatever looks good

[fbarchard, 2009-08-06 04:31:23]

LGTM
I saw this running and it fixes a blatent bug where rate was somewhat ignored.
But it introduces a new issue of lurching.  Its not clear where the new bug
comes from.  In order of suspicion: 
1. muxing
2. inaccurate timers
3. rate not applied properly to timing

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

http://codereview.chromium.org/160620/diff/3007/4006#newcode201
Line 201: PlatformThread::Sleep(kIdleMilliseconds);
nit I've come to fear Sleep(), because its often a sign that a busy loop with
polling is nearby.  Its okay to Sleep(0) to give up time cooperatively.
If you do enough sleeps, it can add to latency between threads.  Signals are
usually the answer, I'm told.

http://codereview.chromium.org/160620/diff/3007/4006#newcode216
Line 216: // Make sure we're not too far ahead.
nit some compilers don't like ' in comments.  ie HighC.  So you may want to use
different wording.  Its not about 'we', the people anyway.  Wrong pronoun.  Its
the video Frame or whatever it is.

[kylep, 2009-08-06 17:38:03]

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

http://codereview.chromium.org/160620/diff/3007/4006#newcode201
Line 201: PlatformThread::Sleep(kIdleMilliseconds);
On 2009/08/06 04:31:23, fbarchard wrote:
> nit I've come to fear Sleep(), because its often a sign that a busy loop with
> polling is nearby.  Its okay to Sleep(0) to give up time cooperatively.
> If you do enough sleeps, it can add to latency between threads.  Signals are
> usually the answer, I'm told.

I don't feel comfortable changing this. I only extracted the constant because I
was using it somewhere else.

http://codereview.chromium.org/160620/diff/3007/4006#newcode216
Line 216: // Make sure we're not too far ahead.
On 2009/08/06 04:31:23, fbarchard wrote:
> nit some compilers don't like ' in comments.  ie HighC.  So you may want to
use
> different wording.  Its not about 'we', the people anyway.  Wrong pronoun. 
Its
> the video Frame or whatever it is.

Done.

http://codereview.chromium.org/160620/diff/3007/4006#newcode216
Line 216: // Make sure we're not too far ahead.
On 2009/08/06 04:14:32, scherkus wrote:
> I would reword this to be "Idle if we are too far ahead"
> 
> readability nit: maybe save the delta or milliseconds to a local variable so
the
> if statement doesn't break up -- whatever looks good 

Done.

[awong, 2009-08-06 18:34:41]

LGTM

Did we add a new bug for the lurching?

[scherkus (not reviewing), 2009-08-06 19:16:16]

If this change causes the lurching in regular videos (i.e., not ogg sync video),
can we debug it a little more?

[kylep, 2009-08-06 19:50:54]

I'm only seeing the lurch on ogg videos.

On Thu, Aug 6, 2009 at 12:16 PM, <scherkus@chromium.org> wrote:

> If this change causes the lurching in regular videos (i.e., not ogg sync
> video), can we debug it a little more?
>
>
> http://codereview.chromium.org/160620
>