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

Ganesh Ajjanagadde gajjanag at mit.edu
Wed Sep 16 05:17:15 CEST 2015


On Tue, Sep 15, 2015 at 6:36 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Tue, Sep 15, 2015 at 6:05 PM, Michael Niedermayer <michaelni at gmx.at>
> wrote:
>
>> 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
>
>
> The shell wouldn't know the difference. It has to be an atexit() mechanism
> in the application cleaning up after itself. This isn't specific to the
> shell state changing - it applies more generally imo.

In fact, we already have signal handlers for essentially all catchable
signals that do the tty stuff. This is part of the reason why I ask
for a concrete, practical usage scenario (not artificial) where after
applying the patch, terminal trashing occurs in scripts.

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list