[FFmpeg-devel] policy on "necro-bumping" patches

Wed Sep 16 00:05:35 CEST 2015

On Tue, Sep 15, 2015 at 05:35:53PM -0400, Ganesh Ajjanagadde wrote:
> On Tue, Sep 15, 2015 at 3:03 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Tue, Sep 15, 2015 at 02:00:27PM -0400, Ganesh Ajjanagadde wrote:
> >> On Tue, Sep 15, 2015 at 1:47 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > On Tue, Sep 15, 2015 at 12:47:35PM -0400, Ganesh Ajjanagadde wrote:
> >> >> On Tue, Sep 15, 2015 at 10:54 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >> > On Tue, Sep 15, 2015 at 08:48:33AM -0400, Ganesh Ajjanagadde wrote:
> >> >> >> On Tue, Sep 15, 2015 at 6:54 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> >> >> >> > Hi Ganesh,
> >> >> >> >
> >> >> >> > On Mon, Sep 14, 2015 at 10:27 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> >> >> >> > wrote:
> >> >> >> >
> >> >> >> >> Hi all,
> >> >> >> >>
> >> >> >> >> What is ffmpeg's policy on "necro-bumping" old patches? Or more
> >> >> >> >> precisely, what is the policy of requesting a patch to be merged where
> >> >> >> >> all objections raised have been addressed via discussion/updated
> >> >> >> >> patches, and which have not been merged in over 2 weeks due to unknown
> >> >> >> >> reasons?
> >> >> >> >>
> >> >> >> >> In particular, there are 2 patchsets I would like to get merged:
> >> >> >> >> 1. This I consider an important patch, simply because it solves a trac
> >> >> >> >> ticket labelled as "important": https://trac.ffmpeg.org/ticket/2964,
> >> >> >> >> which also contains links to the patches. A lot of discussion went on
> >> >> >> >> around it on the mailing lists, and it is supported strongly by
> >> >> >> >> Nicolas and me. Michael seemed initially hesitant but later became
> >> >> >> >> convinced of (at least one of the set's) utility, and one of the
> >> >> >> >> patches was applied. The only objection I recall was from Hendrik,
> >> >> >> >> which was addressed by Nicolas in a follow-up.
> >> >> >> >>
> >> >> >> >> 2. This I consider much more trivial, but in this case there are no
> >> >> >> >> remaining objections. However, I still consider it important enough
> >> >> >> >> for a request to re-examine, as I am doing here. The patchset is more
> >> >> >> >> recent, https://ffmpeg.org/pipermail/ffmpeg-devel/2015-August/177794.html
> >> >> >> >> and https://ffmpeg.org/pipermail/ffmpeg-devel/2015-September/178700.html.
> >> >> >> >
> >> >> >> >
> >> >> >> > Trivial patches can be merged after 24-48 hours if there's no objections
> >> >> >> > outstanding. For more elaborate patches, poke anyone for review if you feel
> >> >> >> > it would be helpful.
> >> >> >> >
> >> >> >> > In both cases, having push access yourself will hurry this along (i.e. you
> >> >> >> > really should get push access), but in this case I will push later today.
> >> >> >> > If you don't want push access, poke one of us on IRC to do the push for
> >> >> >> > you, or bump the original email with a "poke" or "ping".
> >> >> >>
> >> >> >> Thanks. Patches for 2) needs work, and I will be posting it soon.
> >> >> >
> >> >> >
> >> >> >> Patch for 1) should be ok (it was reviewed by Nicolas, and Michael
> >> >> >> seems ok with it like I mentioned).
> >> >> >
> >> >> > there where a few patches, iam not exactly sure which are left and
> >> >> > what effects they have
> >> >> > What i objected to and still object to is to cause the terminal to
> >> >> > be messed up in the most common default configuration in linux
> >> >> > (that is with bash) when ffmpeg crashes (either in a naturally/naively
> >> >> > written script or from the command line)
> >> >> > Iam not sure th last patches still cause this or not
> >> >>
> >> >> Don't know what you exactly mean by a naturally/naively written
> >> >> script, and an example would be very helpful. I can say for certainty
> >> >
> >> > naturally written == a script that does not contain a explicit
> >> > workaround for this issue
> >> > One would only add a workaround once one has been hit by a bug or knows
> >> > about it and searched and found a workaround.
> >> > The users time and convenience matters, we should not inconveience
> >> > many people by this.
> >> >
> >> >
> >> >> that fate and its associated scripts will not suffer from this issue,
> >> >> simply because of the -nostdin flag that was added in one of the
> >> >> patches (which has been applied). The question that remains is whether
> >> >> to apply: https://ffmpeg.org/pipermail/ffmpeg-devel/2015-July/176481.html
> >> >> or not.
> >> >>
> >> >
> >> >> The point Nicolas raised is that no matter what we do, there exist
> >> >> quite reasonable ffmpeg invocations that can mess up the terminal
> >> >> (patch or no patch).
> >> >
> >> > this is true, but reasonable is not the same as "used alot in practice"
> >> > something can be reasonable and used alot or it can be reasonable and
> >> > used very rarely
> >> >
> >> >
> >> >> All the patch does is remove a heuristic that
> >> >> does not even work in all cases (which is impossible unless shell
> >> >> configuration is changed with just 1-2 lines). It also improves
> >> >> usability by fixing #2964.
> >> >
> >> > no heuristic works in all cases, thats the nature of heuristics
> >> > for example our file format probing is all heuristics, not every
> >> > file starting with "RIFFAVI" is a avi file  ...
> >> >
> >> > if the heuristic doesnt work well enough, then it should be improved
> >> > if that is possible
> >> >
> >> > if the bug is belived to be else where (shell, config whatever)
> >> > it should be fixed there so that future default setups do not need
> >> > this heuristic anymore
> >> > The heuristic should be kept as long as the alternative inconveiences
> >> > more people
> >> > I assume that the heuristic inconveiences fewer than the alternative
> >> > of nothing. Is there a reason to belive that this is not the case ?
> >>
> >> Well, assuming the issue of Ticket 2964 is not regarded as terribly
> >> incovenient...
> >> If you consider a 1-2 line addition to bashrc or zshrc as incovenient
> >> for a user (which we can document in wiki/faq, or wherever you want),
> >> and you feel the inconvenience of the issue of 2964 is less than that
> >> of some messed up terminals (non fate scripts), then yes, I agree with
> >> you. I am not convinced by this myself, so I think we should see what
> >> others think about this
> >
> >> (I think a formal poll is ridiculous for
> >> something this tiny).
> >
> > what we really would need to know is how many users are affected by
> > each of the 2 options, the choice should be made so as to
> > inconveience fewer.
> 
> Such a thing is impossible to determine with any reasonable assurance
> - I presume FFmpeg is used in a variety of places. Who knows, distro
> maintainers in a few years may finally wake up when enough users
> complain to them about terminal trashing (from a variety of programs,
> e.g cat on binaries, gpg without --armor, etc) that they add the 1-2
> lines to the bashrc/zshrc that they ship.
> 
> >
> > also, the goal should be a better heuristic if at all possible
> > before weighting which inconveience is less
> 
> I do not necessarily agree with this: a "better heuristic" is
> essentially a hack built on an existing hack - sometimes "less is
> more", and the simple, clean solution (and which is also "correct",
> since terminal cleanup is shell's responsibility and NOT FFmpeg's; try
> e.g cat /bin/ls) is the current patch.

"cat /bin/ls" will send the binary ls to the terminal and some of it
can get interpreted as various control chars. This behaviour is
correct and not a bug. One may very well intentionally do a
cat file_with_control_chars
to affect the state intentionally

If an application OTOH crashes then something should cleanup after it
that can be the shell

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150916/fa878c74/attachment.sig>