[FFmpeg-devel,2/2] RFC ffmpeg: exit demuxers earlier after signal received

From: Andriy Gelman <andriy.gelman@gmail.com>

We currently use the same interrupt_callback function for both muxers
and demuxers to break out of potential infinite loops.

The function decode_interrupt_cb() checks for how many SIGINT/SIGTERM
interrupts have been received, and (usually) two interrupts are needed to
break out of an infinite loop.

If this condition is seen on the muxer, however, we will fail to flush the
avio buffers (see retry_transfer_wrapper()).
An example of this issue is seen in Ticket #9009 (which would be fixed
by this patch).

A more robust alternative maybe to break out of demuxers with one
interrupt. The error should propagate through, close the muxers and
allow them to flush properly.
(If the infinite loop is present on the muxer then a second interrupt would
cause it break out and have the same behavior as before.)

This patch adds this behavior. I've labelled it RFC because it
potentially touches many demuxers.
---
 fftools/ffmpeg.c     | 6 ++++++
 fftools/ffmpeg.h     | 1 +
 fftools/ffmpeg_opt.c | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

On Sat, 28. Nov 14:46, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> We currently use the same interrupt_callback function for both muxers
> and demuxers to break out of potential infinite loops.
> 
> The function decode_interrupt_cb() checks for how many SIGINT/SIGTERM
> interrupts have been received, and (usually) two interrupts are needed to
> break out of an infinite loop.
> 
> If this condition is seen on the muxer, however, we will fail to flush the
> avio buffers (see retry_transfer_wrapper()).
> An example of this issue is seen in Ticket #9009 (which would be fixed
> by this patch).
> 
> A more robust alternative maybe to break out of demuxers with one
> interrupt. The error should propagate through, close the muxers and
> allow them to flush properly.
> (If the infinite loop is present on the muxer then a second interrupt would
> cause it break out and have the same behavior as before.)
> 
> This patch adds this behavior. I've labelled it RFC because it
> potentially touches many demuxers.
> ---
>  fftools/ffmpeg.c     | 6 ++++++
>  fftools/ffmpeg.h     | 1 +
>  fftools/ffmpeg_opt.c | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f4ef15d8..bb27eec879 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -510,7 +510,13 @@ static int decode_interrupt_cb(void *ctx)
>      return received_nb_signals > atomic_load(&transcode_init_done);
>  }
>  
> +static int decode_interrupt_one_sig_cb(void *ctx)
> +{
> +    return received_nb_signals > 0;
> +}
> +
>  const AVIOInterruptCB int_cb = { decode_interrupt_cb, NULL };
> +const AVIOInterruptCB int_one_sig_cb = { decode_interrupt_one_sig_cb, NULL };
>  
>  static void ffmpeg_cleanup(int ret)
>  {
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 3b54dab7fc..c49af30009 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -627,6 +627,7 @@ extern int vstats_version;
>  extern int auto_conversion_filters;
>  
>  extern const AVIOInterruptCB int_cb;
> +extern const AVIOInterruptCB int_one_sig_cb;
>  
>  extern const OptionDef options[];
>  extern const HWAccel hwaccels[];
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index 7ee034c9c9..2908f23010 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -1156,7 +1156,7 @@ static int open_input_file(OptionsContext *o, const char *filename)
>      ic->flags |= AVFMT_FLAG_NONBLOCK;
>      if (o->bitexact)
>          ic->flags |= AVFMT_FLAG_BITEXACT;
> -    ic->interrupt_callback = int_cb;
> +    ic->interrupt_callback = int_one_sig_cb;
>  
>      if (!av_dict_get(o->g->format_opts, "scan_all_pmts", NULL, AV_DICT_MATCH_CASE)) {
>          av_dict_set(&o->g->format_opts, "scan_all_pmts", "1", AV_DICT_DONT_OVERWRITE);
> -- 
> 2.28.0
> 

ping

On Sun, 07. Feb 15:04, Andriy Gelman wrote:
> On Sat, 28. Nov 14:46, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > We currently use the same interrupt_callback function for both muxers
> > and demuxers to break out of potential infinite loops.
> > 
> > The function decode_interrupt_cb() checks for how many SIGINT/SIGTERM
> > interrupts have been received, and (usually) two interrupts are needed to
> > break out of an infinite loop.
> > 
> > If this condition is seen on the muxer, however, we will fail to flush the
> > avio buffers (see retry_transfer_wrapper()).
> > An example of this issue is seen in Ticket #9009 (which would be fixed
> > by this patch).
> > 
> > A more robust alternative maybe to break out of demuxers with one
> > interrupt. The error should propagate through, close the muxers and
> > allow them to flush properly.
> > (If the infinite loop is present on the muxer then a second interrupt would
> > cause it break out and have the same behavior as before.)
> > 
> > This patch adds this behavior. I've labelled it RFC because it
> > potentially touches many demuxers.
> > ---
> >  fftools/ffmpeg.c     | 6 ++++++
> >  fftools/ffmpeg.h     | 1 +
> >  fftools/ffmpeg_opt.c | 2 +-
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 01f4ef15d8..bb27eec879 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -510,7 +510,13 @@ static int decode_interrupt_cb(void *ctx)
> >      return received_nb_signals > atomic_load(&transcode_init_done);
> >  }
> >  
> > +static int decode_interrupt_one_sig_cb(void *ctx)
> > +{
> > +    return received_nb_signals > 0;
> > +}
> > +
> >  const AVIOInterruptCB int_cb = { decode_interrupt_cb, NULL };
> > +const AVIOInterruptCB int_one_sig_cb = { decode_interrupt_one_sig_cb, NULL };
> >  
> >  static void ffmpeg_cleanup(int ret)
> >  {
> > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> > index 3b54dab7fc..c49af30009 100644
> > --- a/fftools/ffmpeg.h
> > +++ b/fftools/ffmpeg.h
> > @@ -627,6 +627,7 @@ extern int vstats_version;
> >  extern int auto_conversion_filters;
> >  
> >  extern const AVIOInterruptCB int_cb;
> > +extern const AVIOInterruptCB int_one_sig_cb;
> >  
> >  extern const OptionDef options[];
> >  extern const HWAccel hwaccels[];
> > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> > index 7ee034c9c9..2908f23010 100644
> > --- a/fftools/ffmpeg_opt.c
> > +++ b/fftools/ffmpeg_opt.c
> > @@ -1156,7 +1156,7 @@ static int open_input_file(OptionsContext *o, const char *filename)
> >      ic->flags |= AVFMT_FLAG_NONBLOCK;
> >      if (o->bitexact)
> >          ic->flags |= AVFMT_FLAG_BITEXACT;
> > -    ic->interrupt_callback = int_cb;
> > +    ic->interrupt_callback = int_one_sig_cb;
> >  
> >      if (!av_dict_get(o->g->format_opts, "scan_all_pmts", NULL, AV_DICT_MATCH_CASE)) {
> >          av_dict_set(&o->g->format_opts, "scan_all_pmts", "1", AV_DICT_DONT_OVERWRITE);
> > -- 
> > 2.28.0
> > 
> 
> ping
> 

ping

On Sat, Nov 28, 2020 at 02:46:54PM -0500, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> We currently use the same interrupt_callback function for both muxers
> and demuxers to break out of potential infinite loops.
> 
> The function decode_interrupt_cb() checks for how many SIGINT/SIGTERM
> interrupts have been received, and (usually) two interrupts are needed to
> break out of an infinite loop.
> 
> If this condition is seen on the muxer, however, we will fail to flush the
> avio buffers (see retry_transfer_wrapper()).
> An example of this issue is seen in Ticket #9009 (which would be fixed
> by this patch).
> 
> A more robust alternative maybe to break out of demuxers with one
> interrupt. The error should propagate through, close the muxers and
> allow them to flush properly.
> (If the infinite loop is present on the muxer then a second interrupt would
> cause it break out and have the same behavior as before.)
> 
> This patch adds this behavior. I've labelled it RFC because it
> potentially touches many demuxers.
> ---
>  fftools/ffmpeg.c     | 6 ++++++
>  fftools/ffmpeg.h     | 1 +
>  fftools/ffmpeg_opt.c | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)

The idea is that a single SIG* would try to exit normally
if you on a single one do interrupt the IO this could result in 
some corruption on the input side.

If the goal is to improve the user experience and only
requirte a single SIGINT even in light of infinite loops
then some sort of automated staged interruption could be done.

that is keep trak of progress being made.
If the input is stuck and makes no progress for a period of
time then a signal could enable interruption immedeatly
similar for the output side.
otherwise a signal would only break the main loop cleanly

Thanks

[...]