[MPlayer-dev-eng] Updated nailfps filter patches

[Uoti Urpala]

On Mon, 2007-11-12 at 17:11 -0500, xiphmont at xiph.org wrote:
> 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:
> > > 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'm not sure which output method you're talking about here (which one
cares about -fps value?). I'm saying that you don't always want the side
effects of setting -fps when using the filter.

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

I disagree about it achieving any real abstraction or separation from
the internals of other code.

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

Most demuxers aren't that broken, at least not with perfect files.
However in the perfect case the timing adjustment functionality of your
filter isn't needed either. In the case where some intermediate packets
are missing because of file damage for example the timestamps you feed
to the filters do not correctly show where the jump from one packet to
the next happens.

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

Adding -af nailsync has no effect on the resulting audio whatsoever but
changes the generated video stream. Do you disagree with that statement?

>   If I move the code into dec-audio, it is no different
> and it still must alter a global.

Moving the functionality would allow implementing it with a fraction of
the code lines. There's no reason why using a global would be
unavoidable.

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

Actually I think it's not correct if you have a weird/broken file and
hit audio EOF immediately. Even for normal files you had to change
mplayer.c to make it true.

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

Are you implying that nothing has happened during the last six years
which would have affected the filter? I think that's false - I'm pretty
sure that at least the addition of the -correct-pts mode would have
either broken the filter or required extra work to keep it functioning
had it been added before. Probably some other changes too.

> > 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 haven't said anything about your indentation... And besides bugs I
think things like adding a lot more code than necessary to achieve the
functionality are certainly valid complaints.

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

Making a version that is maintainable, not likely to cause problems to
other functionality and correct in all use cases takes more effort than
creating a version which works well enough for most test cases. No big
surprise there...

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

:)