[MPlayer-dev-eng] Updated nailfps filter patches

[xiphmont at xiph.org]

On Nov 12, 2007 2:09 PM, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> On Mon, 2007-11-12 at 02:28 -0500, xiphmont at xiph.org wrote:
> > Updated version of the patch including requested changes.  manpage
> > tidied, exit()s removed, messages now use mp_msg.
>
> A partial list of issues:
>
> nailfps should not reuse the -fps option which has another meaning and
> in some cases weird effects.

This has been noted a few times in discussion, and there are two very
good reasons why to use the fps value:

one) if the -fps value and nailfps value diasgree, you get output
marked as being the fps framerate, not the nailfps frame rate.  There
is no -ofps to save you.

two)  "Jeez, how many times do I have to specify a framerate on the
command line to make it actually affect anything?" <-- typical user.
The documentation originally read 'effect the framerate set by -fps'.
I changed it due to the chorus of 'oh, this is really -ofps for
mplayer'.

It's easy enough to change the behavior to use a filter option, but
that means the user will be screwed by (one) every time, no way out.

> The way the filters use static variables

To work with -fixed-vo, I need cross-file state.  Comments in the code
explain this.  Static variables are not inherently incorrect, they're
merely overused.  This is a case where the use is warranted.

> and communicate with globals is
> quite ugly.

Just like other filters do, although usually between mencoder and the
filter, not between two filters.

Folks, I'm not using a single pattern I didn't see used numerous times
elsewhere in the code.

> The patch has several screenfuls of code to add a pts field to the
> filter chain and a new audio filter.

Adding a pts field to the audio filter chain was two lines.  The audio
filter length is architecture overhead.

> However it does not make the pts
> really accurate

As accurate as reported by the demuxer.  If the demuxer is accurate to
the sample, so is the pts.

> and doesn't update it according to filter buffering

Fair observation.

> and
> the filter does not really filter audio but rather updates some magic
> globals.

True.  You can make the case for this not really being a filter.
Looking from the inside, it doesn't really belong.  However, what the
user sees is an audio filter that syncs the audio, just like nailfps
syncs the video.  It makes sense from the CLI.  It works.  It is easy
to understand and use.

> I think such half-assed infrastructure work is not worth much,

And your mama is fat.  Oh, wait, that was inappropriate in a technical
discussion.

> and you could get the same functionality with a couple of special-case
> lines in audio decoding.

I'd have done it that way if I could.  The nailfps filter is a real
filter.  *from the user's perspective* it makes sense to have *what
appears the be the same functionality for audio* be an audio filter.

> The way the nailfps filter assumes that MPlayer will buffer audio
> between calls to it

It assumes no such thing.

> however I think that's the wrong order - it would be
> better to generate the first video frame before committing to the timing
> implied by audio

The intentional model I've followed is *the same one used by mplayer*:
that one modifies video, not audio, to maintain sync.  Therefore to
sync video, we need to see audio first.  Or have every transition be a
jump forward/backward mess.

Your scheme means we see a video frame first, blindly commit it to the
stream-- oops!  The audio pts indicates it was a second too late!  Now
what do we do?  Drop the next second of video?  "Hey," says a user,
"why does a different scene flash at the beginning of the video for no
reason."

> What exactly is the reason for this?
> > +    // we always do this the 'old' (non correct-pts) way because
> > +    // the extra frame generation confuses the Hell out of
> > +    // correct-pts's sync algorithm after seeks.  We bang the
> > +    // frames directly and correct-pts is none the wiser,
> > +    // preventing huge sync offsets in playback that take
> > +    // forever to settle out.
> > +    if (flag)
> > +      vf_next_control(vf, VFCTRL_FLIP_PAGE, NULL);
> Generating extra frames this way will at least break timing if you use
> the filter in normal playback. I see no fundamental reason why the
> filter could not work properly.

Well, try it both ways and see what happens.  The light will come on.

The short answer is, when using -correct-pts, the toplevel sync
feedback goes absolutely haywire if it sees generated frames.  You end
up with seeks and transitions being off by several to tens of seconds,
doubling with each seek.

> The man page entry for nailsync claims it would handle discontinuities,
> but I don't see how the code could handle pts resets properly for
> example.

After reset:
nailfps watches the 'af_nailsync_active' flag.  When the flag is set
to 'resyncing' (1), nailfps drops frames and waits for nailsync to see
a new audio pts.

Discontinuities:
nailsync unilaterally alters the 'apparent starting timebase' nailfps
is using to generate frames with its metronome.  The update moves the
starting timebase forward the amount of the discontinuity.  In the
case of a reverse discontinutiy, it moves back.  Audio and video stay
in sync.  Very simple.

> If the audio filter sees the reset first

Neither audio nor video reset does not flush state.  Reset sets a
'resyncing' flag in audio.

> and then a video frame
> still in pre-reset time is filtered wouldn't nailfps consider the frame
> to be very far from the previous one and repeat a frame lots of times?

No.  Try it.

I don't mind explaining rationale.  But so far every review has
started with explaining how the code is obviously wrong, couldn't
possibly work, and is ugly to boot.  *Then* asks questions to try to
inform the opinion.

Monty