[MPlayer-dev-eng] [PATCH] smooth playback with NAS and all audio drivers

[Sidik Isani]

Hello -

  Thanks for taking a look,

On Tue, Sep 24, 2002 at 12:01:11AM +0200, Arpi wrote:
> Hi,
> 
> >   o It also has 2 important fixes to the RTC sleep.  Just the minimal
> >     changes were done this time, with everything left in mplayer.c.
> >     The RTC returns a 32-bit number, but the read() was for a long long.
> >     This had the effect of cutting the frequency in half (?), so now it
> >     would probably work as well at 512Hz, causing half the number of
> >     hardware interrupts.  I set it at 1024 anyway since that rate didn't
> >     seem to be bringing anyone's machine to its knees.
> i'll apply this part now
> 
> >   o Definitely no changes to setuid behavior this time.  The new call
> >     to seteuid(0) before /dev/rtc is opened should not break anything
> >     and will allow the RTC code to continue to work even if the line
> >     "seteuid(getuid())" is added some day at the start of main().
> please leave this euid thing until we decide to change it everywhere...

  Ok.

> >   o The patch removes the dapsync code.  But I can generate a version
> >     which leaves it in if you prefer?
> dunno
> i don't like it, dunno if anyone use it. i'l just remove now and we'll see :)
> 
> >   o A new variable "delay_avg" stores either the last delay measurement
> >     from audio_out->get_delay(), or a corrected one calculated by my
> 
> the problem is here.
> you should not store the result of audio_out->get_delay(), it's variable in
> time. your patch gets (asks the audio driver) it after decoding a video
> frame, saves it, and uses it after the audio decoding, ignoring all the
> delay and buffer change caused by the audio decoding process. VERY BAD!
> it may work with your avg'd values, but it will surely break the old timing

  Hmm, no you're right.  It would actually affect my sync code the same
  way.  I will take a closer look tonight: might it be possible to do
  all three things which depend on delay (frame dropping, audio time
  correction, and A/V sync adjustment) at the same point in time to
  avoid this problem?

> code, especially on slow systems.
> 
> also, i don't really understand why did you change this:
> 
> -       if(sh_audio && !d_audio->eof){
> +       if(sh_audio && !d_audio->eof && delay_avg!=0){
> 
> (frame dropping entry contdition)

  Oh, yeah.  This will not be necessary if it turns out to be
  possible to move the whole block of code to a place after the
  first A/V sync adjustment is made.  We don't want to break the
  nice division between audio handling and video handling though.
  Let me see what else I can propose.

> the delay caused by audio buffering has NOTHING with the video
> frame dropping, imho.

  True, it shouldn't.  But since the measurement of this delay factors
  in to the algorithm which decides whether or not to drop, it does.

> >   There were several ways to write this patch, but I chose one which
> >   makes it clear that all it does is adjust the delay that Arpi's sync
> 
> i wouldn't call it clear ...

  Well, except for the problem of storing delay_avg, all code was
  inside a single if{} so that, relative to the old code, we can
  see exactly the effect of turning this block of code on/off.
  That was the ideal anyway ;-)

  There is another way to write it: I noticed that at the start of the
  section of code which sleeps, time_frame contains a non-adjusted time
  to sleep for this frame, is that right?  In -nosound mode, this gets
  used as-is, since there's no audio with which to line up.  Now your
  A/V sync code ignores the predicted value completely and re-assigns
  time_frame to a new value based solely on instantaneous information
  from the sound driver.  I found that with my ensoniq driver, the two
  rarely disagree (i.e., they are almost exactly the same value for
  every single frame.)  In other cases, they disagree enough to cause
  an uneven frame rate, but the _average_ of the difference (not the
  average of get_delay() itself!) is still very stable in all cases.
  So ... the other way to write it would be as a correction to time_frame
  itself, and leave your sync code as a separate case.

  And with that, what do you think about this approach for the frame
  dropping:  It could be simplified to just a couple of lines of code
  which look at the _average_ of time_frame, and could automatically
  drop if this average reaches -frame_time.  (We need another average
  because not all frames will take an equal time to render).  The
  nice thing about that is it makes a reliable way of automatically
  determining if frame dropping is needed.  By the time the average
  reaches one frame behind, there's nothing else to do but drop frames
  to keep the video from falling behind.  If you like the idea, I'll
  code it up tonight and you can have a look?

Be seeing you,

- Sidik