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

[Taylor Ralph]

Davin,

On Tue, Feb 2, 2010 at 2:39 AM, Davin McCall <davmac at davmac.org> wrote:
> 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.)
>

Sounds reasonable but I'll have to think about it more deeply when I
have some time. So we need to keep in for now but it can be improved
after the release.

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

Yes, I guess the goal is to smooth out the reaction so we don't need
to make such large adjustments. However, from my experience, with most
content the drift is quite small and these adjustments are only needed
once every few seconds at most.

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

Actually, I'm sure a lot of people would be very thankful if you
provided a new patch without these 2 items included. Since it's a bug
fix it still has time to be included with the release and worst case
back-ported after the release when it's had some time to soak in
trunk. Then after the release the other two issues can be looked at
hopefully in separate patches to ease integration. If you can provide
something for the current problem I'll review it ASAP this time.

Thanks for the detailed analysis and for digging into this.

--
Taylor