[MPlayer-dev-eng] Updated nailfps filter patches

[xiphmont at xiph.org]

On Nov 12, 2007 4:43 PM, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> 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.

So I should implement it so that it doesn't work (output always has
incorrect framerate set) because that's cleaner?  Excellent choice,
Sir.

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

Nothing is being hidden.

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

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

Yes, abstraction barriers imply more code than would otherwise be used.

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

I can't tell if you're implying that the demuxers are so broken
there's no point to even trying to use them.  If that's the case, why
have audio pts at all?

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

Because that's what it will appear to do to the user.

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

It does not alter the video.  It watches the audio (it sits IN THE
AUDIO STREAM) and adjusts a global sync parameter that the video
filter watches.  If I move the code into dec-audio, it is no different
and it still must alter a global.

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

At the beginning of a file, it drops frames until an audio buffer is
processed.  The assumption it makes is correct.

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

I've been waiting six years for that to happen and finally got off my
ass to do it myself.  Call me when someone comes along and breaks it,
it'll be a while.

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

Fine.  I give up.

There are plenty of things about the design worth arguing about... but
you guys are more worried about the color of the mudflaps on this
dumptruck.  You're rejecting considered decisions out of hand with
vague, irrelevant dogma.  I've seen two legitimate bugs brought up so
far in a mountain of "WAAAH! WE DON'T LIKE YOUR INDEEEEENT."

I have the only mplayer/mencoder on earth that can always dump WMV2
and WMV3 without losing sync.  I just needed it for me.  I thought it
would be nice to submit it for all your users who have been asking for
this for years.  But it just ceased being worth it.

Patch retracted.  You can all go fuck yourselves.  Life is too short
for this asshattery.

Monty