[MPlayer-dev-eng] Updated nailfps filter patches

[Uoti Urpala]

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.

The way the filters use static variables and communicate with globals is
quite ugly.

The patch has several screenfuls of code to add a pts field to the
filter chain and a new audio filter. However it does not make the pts
really accurate and doesn't update it according to filter buffering, and
the filter does not really filter audio but rather updates some magic
globals. I think such half-assed infrastructure work is not worth much,
and you could get the same functionality with a couple of special-case
lines in audio decoding.

The way the nailfps filter assumes that MPlayer will buffer audio
between calls to it, and the changes to mplayer.c to make that happen,
are ugly and likely to interfere with other code changes. If audio
always needs to be started first the code should explicitly do that
after startup/seek; 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 (though you could decode audio without starting
playback). So IMO the filter should not make such assumptions,
especially not implicitly inside the filter code. This and the use of
globals and static variables declared inside the filters can be grouped
together into a larger architectural problem: the implementation hides
everything in the filters, but the operation really involves top-level
state and the parts are not independent so that changes to other code
are likely to break the setup.

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.

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. If the audio filter sees the reset first 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?