[mythtv] DVD playback AV sync issues: all solved (?)
davmac at davmac.org
Tue Feb 2 07:39:29 UTC 2010
On 02/02/10 05:12, Taylor Ralph wrote:
> Now that it's committed I have a couple questions:
> 1) could you please explain why KeepPhase() was removed? The comments
> would leave someone to believe it is necessary for HW vsync methods.
> I'm hoping you know cause I don't. ;)
Probably I should have left it in for the moment, but, mainly: Because
it seems broken.
The comment for the method says "Keep our nexttrigger from drifting too
close to the exact retrace." However the code says:
244 void VideoSync::KeepPhase()
246 // cerr << m_delay << endl;
247 if (m_delay < -(m_refresh_interval/2))
248 OffsetTimeval(m_nexttrigger, 200);
249 else if (m_delay > -500)
250 OffsetTimeval(m_nexttrigger, -2000);
This is executed after actually performing the delay, which in theory
leaves m_delay at anything in the range (-m_refresh_interval, 0].
The method is called "KeepPhase()" and yet it specifically alters or
adjusts the phase (phase being the distance between the frame interval
boundary and the refresh interval boundary). It "keeps" phase only if
the frame interval is, or is very nearly, an interger multiple of the
refresh interval. That's not guaranteed.
Let me explain the so-called straddle problem, as I understand it. The
term "straddle" refers to the fact that the frame boundary might fall
either side of a particular refresh boundary. If the frame rate is an
exact multiple of the refresh rate (or very close to it) then you might
see the problem. Despite crazy theories (in the discussion thread which
you pointed out) about it being some kind of hardware issue*, the real
problem is that you can get "short frames" followed by "long frames" in
Eg. if frame interval = 3 * refresh interval +/- small random factors
in the refresh (due to the hardware). In one frame we might only
complete two refresh intervals (a short frame), leaving a large
remainder (m_delay only slightly less than 0). Now, if the next few
refresh intervals are just a teeny bit short, we might find that
remainder is enough to get a whole extra refresh interval in, so we end
up waiting four refresh intervals (the normal three, plus one that we
squeezed out of the large remainder from the previous frame). If this
happens repeatedly you'd probably notice it as a visual problem
(particularly if the "short" frame is only 1 refresh interval, or even 0
So, the KeepPhase method continually tries to shift the notional frame
boundary towards the middle of the refresh interval, to prevent it
straddling the refresh boundary. The code above actually succeeds in
Note however, if there is a slight continual drift then this eventually
brings AV sync out. Consider the case where frame interval is (some
multiple of the refresh interval + 2000 microseconds). If m_delay ever
falls into the > -500 range, then KeepPhase() will actually keep
phase... by bringing the AV sync out by 2 milliseconds every frame. Ok,
it's an artificial scenario, but the point is that "keeping phase" is
the wrong way to solve the straddle problem.
The right way is to regulate the refresh intervals per frame... that is,
if you did three refresh intervals for the last frame, but the current
calculation shows two refresh intervals are needed - with a large
remainder - then just do three again. Likewise if you get four with a
small remainder, just do three. (likewise, if you did 2 last time but
calculate 4 this time, just do 3). I.e. don't stuff around with the AV sync!
Having said all that, I probably should have left KeepPhase() in for
* (Hardware might cause it, by having inconsistent refresh intervals;
but that's a bit different to what was suggested.)
> 2) why did you remove the +/- 1.5 frame allowance before making av adjustments?
That one, I think, was a mistake, however: It doesn't really matter if
the audio timecode reading is accurate. The problem is, I guess, that it
might not be, and that you then end up needlessly adjusting AV sync back
and forth. (Note that setting the avsync adjustment to the average delay
/ 2 cushions the effect. If you upped the divisor enough you probably
wouldn't need the frame "allowance" because you'd be adjusting by a much
smaller amount - by less than a refresh interval for instance. Only once
the sync was out by more than the allowance would you start to see
significant adjustment anyway).
In fact: it's ultimately better to go with that approach, because if you
have an "allowance" then the AV sync potentially bounces around the
allowance edge. (you slowly get more and more out of sync until you hit
the allowance, then you are sharply reigned in by the adjustment, until
you get back within the allowance... and so on).
The patch has been reverted now, anyway; I may re-do it with KeepPhase
and the frame allowance left in. But please, discuss. I would have liked
to get more feedback/questions *before* the dang patch was applied.
More information about the mythtv-dev