[mythtv] DVD playback AV sync issues: all solved (?)

Davin McCall 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()
245         {
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);
251         }

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 
quick succession.

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 
refresh intervals...).

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 
this regard.

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 
this round.

* (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 mailing list