| Message ID | 22E79390-C8BE-487F-A03A-B956C95E3CC8@dericed.com |
|---|---|
| State | New |
| Headers | show |
Dave Rice (2018-01-11): > From 0faa2954010feb8428542fc33aa81e898a280c88 Mon Sep 17 00:00:00 2001 > From: Dave Rice <dave@dericed.com> > Date: Thu, 11 Jan 2018 15:52:36 -0500 > Subject: [PATCH] ffmpeg.c: use sigterm_handler with sigpipe > > Based on a suggestion by Moritz Barsnick at http://ffmpeg.org/pipermail/ffmpeg-user/2018-January/038549.html <http://ffmpeg.org/pipermail/ffmpeg-user/2018-January/038549.html> SIGTERM is there for a reason. It prevents programs whose output was closed from continuing needlessly. Ignoring it unconditionally will lose that property, which is desirable in most cases. Furthermore, the signal handler you are proposing is designed for interactive signals and will probably not give the results you want in this case. Just use your shell to set it to SIG_IGN when necessary. Regards,
On Thu, Jan 11, 2018 at 16:20:04 -0500, Dave Rice wrote: > Based on a suggestion by Moritz Barsnick at http://ffmpeg.org/pipermail/ffmpeg-user/2018-January/038549.html <http://ffmpeg.org/pipermail/ffmpeg-user/2018-January/038549.html> That's me. > signal(SIGINT , sigterm_handler); /* Interrupt (ANSI). */ > signal(SIGTERM, sigterm_handler); /* Termination (ANSI). */ > + signal(SIGPIPE, sigterm_handler); /* Termination (pipe closed). */ Works for me (TM). Background; When outputting to a pipe, when the receiving program (e.g. "ffplay -") is closed, ffmpeg gets killed with SIGPIPE[*]. If ffmpeg has several outputs, the others are not properly closed then. E.g. MOV files don't get their moov atom written. This patch fixes this. [*] On Unix. Behavior on Windows seems sane, must be some other mechanism. Cheers, Moritz
On Thu, Jan 11, 2018 at 22:43:48 +0100, Nicolas George wrote: > SIGTERM is there for a reason. It prevents programs whose output was > closed from continuing needlessly. Ignoring it unconditionally will lose > that property, which is desirable in most cases. This patch doesn't change the handling of SIGTERM, and it doesn't add ignoring of any signal. It avoids "ignoring" of SIGPIPE, where ffmpeg used to interrupt unconditionally. With this patch, ffmpeg will clean up on SIGPIPE, allowing e.g. other outputs to be finalized correctly. > Furthermore, the signal handler you are proposing is designed for > interactive signals and will probably not give the results you want in > this case. Is SIGPIPE an interactive signal? Anything on the other side of output file(name) "-" or "pipe:N" may terminate for some reason. > Just use your shell to set it to SIG_IGN when necessary. This patch does NOT try to ignore anything. ffmpeg won't keep running due to ignoring of SIGPIPE, it will terminate more cleanly due to handling it. The former is not desired. (But yes, shall handing to enforce ignoring it would allow that.) Best regards, Moritz
Moritz Barsnick (2018-01-11): > This patch doesn't change the handling of SIGTERM You should have read SIGPIPE, obviously. > Is SIGPIPE an interactive signal? Of course not. > Anything on the other side of output > file(name) "-" or "pipe:N" may terminate for some reason. Yes, that is exactly what SIGPIPE is for. > This patch does NOT try to ignore anything. ffmpeg won't keep running > due to ignoring of SIGPIPE, it will terminate more cleanly due to > handling it. The former is not desired. (But yes, shall handing to > enforce ignoring it would allow that.) It will terminate less cleanly than if you do the right thing with SIGPIPE. Basic rule of Unix programming: don't mess with signals unless you are very sure of what you are doing. And even in that case, hesitate. Regards,
Thread bump. > On Jan 11, 2018, at 5:51 PM, Nicolas George <george@nsup.org> wrote: > > Moritz Barsnick (2018-01-11): >> This patch doesn't change the handling of SIGTERM > > You should have read SIGPIPE, obviously. > >> Is SIGPIPE an interactive signal? > > Of course not. > >> Anything on the other side of output >> file(name) "-" or "pipe:N" may terminate for some reason. > > Yes, that is exactly what SIGPIPE is for. > >> This patch does NOT try to ignore anything. ffmpeg won't keep running >> due to ignoring of SIGPIPE, it will terminate more cleanly due to >> handling it. The former is not desired. (But yes, shall handing to >> enforce ignoring it would allow that.) > > It will terminate less cleanly than if you do the right thing with > SIGPIPE. This patch has been working for me and ffmpeg terminates cleanly with SIGPIPE with a valid output (moov atom written with mov or cues/seekhead written with mkv). Not sure if I understand well the disadvantage of this patch. Dave Rice
Dave Rice (2018-01-18):
> Not sure if I understand well the disadvantage of this patch.
When dealing with Unix signals, "not sure if I understand" means
"don't".
The same result can be achieved with the shell, without the
corresponding drawbacks.
Regards,
2018-01-18 15:54 GMT+01:00 Nicolas George <george@nsup.org>: > The same result can be achieved with the shell Would you like to elaborate? > without the corresponding drawbacks. Maybe you could explain the drawbacks? Exiting FFmpeg without closing output files seems like a major drawback to me, independently of the output format. Carl Eugen
On Thu, 18 Jan 2018 15:54:46 +0100 Nicolas George <george@nsup.org> wrote: > Dave Rice (2018-01-18): > > Not sure if I understand well the disadvantage of this patch. > > When dealing with Unix signals, "not sure if I understand" means > "don't". Well, he's asking you why. If you refuse to explain, I don't think your opposition against the patch has to count. > The same result can be achieved with the shell, without the > corresponding drawbacks.
Carl Eugen Hoyos (2018-01-18): > Would you like to elaborate? Of course, provided somebody is interested. Issuing the following command: trap "" SIGPIPE will ignore SIGPIPE in the shell and all subsequent programs started from it. It will achieve exactly the result of this patch, in a much cleaner way, without impacting others users. > Maybe you could explain the drawbacks? No, I cannot. I only know they exist, because signals on Unix are like that. In this instance, I consider the burden of the proof to be on the person who proposes the patch. Still, there is always the fact that it changes the behaviour in a significant way, possibly resulting in process running indefinitely where they were stopping previously. Regards,
2018-01-18 16:05 GMT+01:00 Nicolas George <george@nsup.org>: > Carl Eugen Hoyos (2018-01-18): >> Would you like to elaborate? > > Of course, provided somebody is interested. > > Issuing the following command: > > trap "" SIGPIPE > > will ignore SIGPIPE in the shell and all subsequent programs started > from it. > It will achieve exactly the result of this patch Dave's patch and your shell command above behave differently here. > in a much cleaner way, without impacting others users. It now terminates with an error message instead of succeeding. Why is that cleaner? >> Maybe you could explain the drawbacks? > > No, I cannot. I only know they exist, because signals on Unix are like > that. In this instance, I consider the burden of the proof to be on the > person who proposes the patch. > > Still, there is always the fact that it changes the behaviour in a > significant way, possibly resulting in process running indefinitely > where they were stopping previously. Why do you think so? The OP has created a very easy to reproduce case where FFmpeg unexpectedly doesn't close its output files, and a patch that fixes this (broken) behaviour. Why do you believe there is an indefinitely running process involved? Carl Eugen
Carl Eugen Hoyos (2018-01-18): > The OP has created a very easy to reproduce case where > FFmpeg unexpectedly doesn't close its output files, and > a patch that fixes this (broken) behaviour. And the OP neglected to check that the patch does not break other cases. > Why do you believe there is an indefinitely running process > involved? You misread. Regards,
2018-01-18 16:19 GMT+01:00 Nicolas George <george@nsup.org>: > Carl Eugen Hoyos (2018-01-18): >> The OP has created a very easy to reproduce case where >> FFmpeg unexpectedly doesn't close its output files, and >> a patch that fixes this (broken) behaviour. > > And the OP neglected to check that the patch does not > break other cases. Could you please tell us which cases get broken? >> Why do you believe there is an indefinitely running process >> involved? > > You misread. Then please explain, I am not a native speaker. Carl Eugen
Carl Eugen Hoyos (2018-01-18):
> Could you please tell us which cases get broken?
I cannot. But these are Unix signals: there are.
Regards,
On 1/18/18, Nicolas George <george@nsup.org> wrote: > Carl Eugen Hoyos (2018-01-18): >> Could you please tell us which cases get broken? > > I cannot. But these are Unix signals: there are. Just ignore Nicolas, he doesn't know any better.
2018-01-18 16:27 GMT+01:00 Nicolas George <george@nsup.org>: > Carl Eugen Hoyos (2018-01-18): >> Could you please tell us which cases get broken? > > I cannot. Ok. So we both agree that FFmpeg should get rid of Vincent as fast as possible. We have disagreed in the past about pkg-config, you don't want me to test FFmpeg on less common systems, fine. Now you are again behaving childishly, be it because you don't like a patch, be it because you are in a bad mood. How do you expect we find more support for our common goal? Carl Eugen
Carl Eugen Hoyos (2018-01-18): > We have disagreed in the past about pkg-config, > you don't want me to test FFmpeg on less common > systems, fine. This is untrue. I have offered you help to find a work-flow to test FFmpeg on less common systems while playing nice with pkg-config, but you never accepted. > Now you are again behaving childishly, be it because > you don't like a patch, be it because you are in a > bad mood. Neither. Let me explain one last time. Unix signals are an abomination, their behaviour is barely portable, made mostly of undefined or implementation-defined behaviours, with a lot of strange corner cases. They are also a very fragile edifice, with some signals playing a very subtle but absolutely necessary role in the correct workings of the system. Programming with Unix signals should be reserved to very skilled programmers. I usually do not even trust myself to touch them, except in the simplest of cases; yet, if I say so myself, I think when it comes to Unix system programming, I am rather ahead here. I would not oppose a patch from a skilled Unix programmer who explains the details of the behaviour changes and why they are good. But if somebody just copy-paste a bit of code that looks vaguely similar and says "I do not know what I am doing but it seems to fix my particular use case", then I oppose. It would make things worse. Regards,
On 18/01/18 14:49, Dave Rice wrote: > Thread bump. > >> On Jan 11, 2018, at 5:51 PM, Nicolas George <george@nsup.org> wrote: >> >> Moritz Barsnick (2018-01-11): >>> This patch doesn't change the handling of SIGTERM >> >> You should have read SIGPIPE, obviously. >> >>> Is SIGPIPE an interactive signal? >> >> Of course not. >> >>> Anything on the other side of output >>> file(name) "-" or "pipe:N" may terminate for some reason. >> >> Yes, that is exactly what SIGPIPE is for. >> >>> This patch does NOT try to ignore anything. ffmpeg won't keep running >>> due to ignoring of SIGPIPE, it will terminate more cleanly due to >>> handling it. The former is not desired. (But yes, shall handing to >>> enforce ignoring it would allow that.) >> >> It will terminate less cleanly than if you do the right thing with >> SIGPIPE. > > This patch has been working for me and ffmpeg terminates cleanly with SIGPIPE with a valid output (moov atom written with mov or cues/seekhead written with mkv). Not sure if I understand well the disadvantage of this patch. Making SIGPIPE have an effect like this seems highly dubious to me, but I admit that since it always killed the process before it feels like an improvement. Changing it to ignore the signal (set it to SIG_IGN) instead sounds like a safer option? I think that would move the code to having the same behaviour on Unices as on Windows, since Windows has no SIGPIPE. Setting the termination option in the ffmpeg program or not shouldn't make a difference to the behaviour inside the muxer - the failing call is still going to return and the error propagated to its caller like it is already on Windows, so that handling has to work anyway. If there is a case where a subsequent call causes problems which are somehow different between Unix/Windows such that ignoring the signal does not make them act similarly then that is a bug which should be fixed. - Mark
Mark Thompson (2018-01-18): > Changing it to ignore the signal (set it to SIG_IGN) instead sounds > like a safer option? Yes, I think it would be somewhat safer. And it can be done without modification to FFmpeg. What is missing, though, is better control on error conditions: when muxing in one output fails, choosing the behaviour for other outputs (continue, abort, close cleanly). Regards,
2018-01-18 16:55 GMT+01:00 Mark Thompson <sw@jkqxz.net>: > Setting the termination option in the ffmpeg program or not shouldn't > make a difference to the behaviour inside the muxer How did you test this? Carl Eugen
On 18/01/18 15:58, Nicolas George wrote: > Mark Thompson (2018-01-18): >> Changing it to ignore the signal (set it to SIG_IGN) instead sounds >> like a safer option? > > Yes, I think it would be somewhat safer. And it can be done without > modification to FFmpeg. To clarify: I was definitely proposing that the change be made in the ffmpeg program. The appearance of SIGPIPE is a surprise to many people, and keeping the default disposition here does cause ffmpeg to behave significantly differently on Unix and Windows without any obvious reason why to those not familiar with it. > What is missing, though, is better control on error conditions: when > muxing in one output fails, choosing the behaviour for other outputs > (continue, abort, close cleanly). Sure. That can be independent of any change to signal behaviour, though. - Mark
On Thu, 18 Jan 2018 16:30:22 +0100 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-01-18 16:27 GMT+01:00 Nicolas George <george@nsup.org>: > > Carl Eugen Hoyos (2018-01-18): > >> Could you please tell us which cases get broken? > > > > I cannot. > > Ok. > > So we both agree that FFmpeg should get rid of > Vincent as fast as possible. Please cease your uncalled for childish attacks against me. I'm really having trouble what outraged you this time, just like I did last time. Maybe we should try to enforce the Code of Conduct for once?
On 18/01/18 16:02, Carl Eugen Hoyos wrote: > 2018-01-18 16:55 GMT+01:00 Mark Thompson <sw@jkqxz.net>: > >> Setting the termination option in the ffmpeg program or not shouldn't >> make a difference to the behaviour inside the muxer > > How did you test this? The signal is received by a write call deep inside lavf, and it runs the signal handler which sets the termination option but doesn't actually act on it immediately. The signal handler returns and the write call returns EPIPE, which then has to be handled correctly by all callers up the stack before the ffmpeg program even gets a chance to check the termination option at all. From a little bit of testing with named pipes ignoring the signal does appear to have the right effect (fails in one muxer and closes the other cleanly), so I think that's probably the right answer. - Mark
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 0c16e75ab0..dfcc865dcf 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -406,6 +406,7 @@ void term_init(void) signal(SIGINT , sigterm_handler); /* Interrupt (ANSI). */ signal(SIGTERM, sigterm_handler); /* Termination (ANSI). */ + signal(SIGPIPE, sigterm_handler); /* Termination (pipe closed). */ #ifdef SIGXCPU signal(SIGXCPU, sigterm_handler); #endif
From 0faa2954010feb8428542fc33aa81e898a280c88 Mon Sep 17 00:00:00 2001 From: Dave Rice <dave@dericed.com> Date: Thu, 11 Jan 2018 15:52:36 -0500 Subject: [PATCH] ffmpeg.c: use sigterm_handler with sigpipe Based on a suggestion by Moritz Barsnick at http://ffmpeg.org/pipermail/ffmpeg-user/2018-January/038549.html <http://ffmpeg.org/pipermail/ffmpeg-user/2018-January/038549.html> --- fftools/ffmpeg.c | 1 + 1 file changed, 1 insertion(+)