[FFmpeg-devel] ffmpeg.c: use sigterm_handler with sigpipe

Submitted by Dave Rice on Jan. 11, 2018, 9:20 p.m.

Details

Message ID 22E79390-C8BE-487F-A03A-B956C95E3CC8@dericed.com
State New
Headers show

Commit Message

Dave Rice Jan. 11, 2018, 9:20 p.m.
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(+)

Comments

Nicolas George Jan. 11, 2018, 9:43 p.m.
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,
Moritz Barsnick Jan. 11, 2018, 10:06 p.m.
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
Moritz Barsnick Jan. 11, 2018, 10:45 p.m.
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
Nicolas George Jan. 11, 2018, 10:51 p.m.
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,
Dave Rice Jan. 18, 2018, 2:49 p.m.
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
Nicolas George Jan. 18, 2018, 2:54 p.m.
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,
Carl Eugen Hoyos Jan. 18, 2018, 3 p.m.
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
wm4 Jan. 18, 2018, 3 p.m.
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.
Nicolas George Jan. 18, 2018, 3:05 p.m.
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,
Carl Eugen Hoyos Jan. 18, 2018, 3:16 p.m.
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
Nicolas George Jan. 18, 2018, 3:19 p.m.
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,
Carl Eugen Hoyos Jan. 18, 2018, 3:23 p.m.
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
Nicolas George Jan. 18, 2018, 3:27 p.m.
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,
Paul B Mahol Jan. 18, 2018, 3:29 p.m.
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.
Carl Eugen Hoyos Jan. 18, 2018, 3:30 p.m.
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
Nicolas George Jan. 18, 2018, 3:45 p.m.
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,
Mark Thompson Jan. 18, 2018, 3:55 p.m.
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
Nicolas George Jan. 18, 2018, 3:58 p.m.
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,
Carl Eugen Hoyos Jan. 18, 2018, 4:02 p.m.
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
Mark Thompson Jan. 18, 2018, 4:15 p.m.
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
wm4 Jan. 18, 2018, 4:18 p.m.
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?
Mark Thompson Jan. 18, 2018, 4:30 p.m.
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

Patch hide | download patch | download mbox

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