[MPlayer-dev-eng] Updated nailfps filter patches

[Uoti Urpala]

On Mon, 2007-11-12 at 15:00 -0500, xiphmont at xiph.org wrote:
> On Nov 12, 2007 2:09 PM, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> > 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:

But there are also good reasons not to use it: that it has another
meaning, and setting the option can have unwanted effects elsewhere in
the code.

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

I think hiding that cross-file state in static/global variables declared
inside a filter is not such a good idea.

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

There's lots of code that is crap, especially in MEncoder. That doesn't
mean we would want to maintain more of that.

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

Whatever the parts, overall the implementation using an audio filter +
the pts and reset additions for it does have a significant amount of
code.

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

The end position for the data is equally accurate, but it ignores packet
boundaries and pts values not exactly matching packet lengths.

> > 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 don't see why you'd call that "syncing the audio" when what it does is
alter the video track and leave the audio unchanged.

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

So you added all that code to be able to use "-af" syntax? I don't see
why that would be so important, especially when as pointed above it
alters video not audio.

> > The way the nailfps filter assumes that MPlayer will buffer audio
> > between calls to it
> 
> It assumes no such thing.

> +    if(af_nailsync_active!=2){
> +      mp_msg(MSGT_VFILTER, MSGL_INFO,
> +            "\r[nailfps] waiting for audio frame...                                            ");
> +      return 0;
> +    }

That is what I mean - it assumes MPlayer will eventually buffer audio if
the filter keeps returning 0.

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

I didn't mean that to be a suggestion how to implement the filter, but
something which would make sense to do in mplayer.c and which would
conflict with your filter implementation.

My main point with this and the globals issue is that the setup of this
filter system makes things significantly more fragile. It makes several
assumptions about how the other pieces work that aren't required by
anything else. I'm not willing to commit to keeping a patch like this
working when I make other unrelated changes to the timing/sync code.

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

Either the filter does something wrong or there's a bug in other code
which should be fixed. I suspect the filter as the generation of extra
frames happens inside generate_video_frame() and the toplevel sync code
shouldn't even see whether the frame was an "extra" generated by a
filter or not. But in any case there should be no reason not to
implement the generation of extra frames properly in the filter.

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

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

By "pts reset" above I meant pts suddenly dropping back to 0, like what
occurs on some DVDs.

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

Have you tried that?
Suppose audio and video pts both are about 1000. Now audio pts drops to
0. nailsync changes the starting timebase backward by about 1000. Then
nailfps sees a video frame with pts=1000. In the new timebase it's about
1000 seconds after the previous frame, so nailfps will duplicate a frame
lots of times.
How would the behavior of the code differ from the above?

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

I don't claim it to be "obviously wrong", though I don't think it will
handle DVD timestamp resets well. However I do think it is ugly in
several ways, and it makes the code more fragile. Even if the patch
would work for most uses now committing it would very likely cause
noticeable extra work in the future.