[FFmpeg-devel] avfilter/af_amix: switch to activate

Submitted by Paul B Mahol on Aug. 26, 2017, 3:53 p.m.

Details

Message ID 20170826155318.17408-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Aug. 26, 2017, 3:53 p.m.
Really fixes hangs and infinite loops.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/af_amix.c | 161 +++++++++++++++++++++++++-------------------------
 1 file changed, 81 insertions(+), 80 deletions(-)

Comments

Nicolas George Aug. 26, 2017, 6:02 p.m.
Le nonidi 9 fructidor, an CCXXV, Paul B Mahol a écrit :
> Really fixes hangs and infinite loops.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/af_amix.c | 161 +++++++++++++++++++++++++-------------------------
>  1 file changed, 81 insertions(+), 80 deletions(-)
> 
> diff --git a/libavfilter/af_amix.c b/libavfilter/af_amix.c
> index 78be57a..989fd84 100644
> --- a/libavfilter/af_amix.c
> +++ b/libavfilter/af_amix.c
> @@ -41,6 +41,7 @@

I am sorry to say, but it is not good at all. But it is not your fault
at all, it is because of the existing code.

From the beginning, this filter suffered from the "let's reinvent the
wheel instead of designing a proper framework, and let us make it
square" syndrome: there is an audio FIFO, then a strange FrameList /
FrameInfo structure to keep the frames within the FIFO.

The activate design comes with the framework that was needed: a real
conversion to activate would mean getting rid of all that, keeping only
output_frame() (and the init / format code, of course) and making it
query the links' fifo.

That said, I do not object to the patch if it fixes something. But I
think a big /* FIXME */ comment would be helpful to avoid people taking
this code as an example.

Regards,
Michael Niedermayer Aug. 27, 2017, 12:59 a.m.
On Sat, Aug 26, 2017 at 05:53:18PM +0200, Paul B Mahol wrote:
> Really fixes hangs and infinite loops.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/af_amix.c | 161 +++++++++++++++++++++++++-------------------------
>  1 file changed, 81 insertions(+), 80 deletions(-)

This breaks fate:

stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/  1056768
size: |2116800 - 1056768| >= 0
stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/   704512
size: |2116800 - 704512| >= 0
TEST    filter-hdcd-s16p
Test filter-amix-simple failed. Look at tests/data/fate/filter-amix-simple.err for details.
make: *** [fate-filter-amix-simple] Error 134
make: *** [fate-filter-amix-transition] Error 134

[...]
James Almer Aug. 27, 2017, 1:22 a.m.
On 8/26/2017 9:59 PM, Michael Niedermayer wrote:
> On Sat, Aug 26, 2017 at 05:53:18PM +0200, Paul B Mahol wrote:
>> Really fixes hangs and infinite loops.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavfilter/af_amix.c | 161 +++++++++++++++++++++++++-------------------------
>>  1 file changed, 81 insertions(+), 80 deletions(-)
> 
> This breaks fate:
> 
> stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/  1056768
> size: |2116800 - 1056768| >= 0
> stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/   704512
> size: |2116800 - 704512| >= 0
> TEST    filter-hdcd-s16p
> Test filter-amix-simple failed. Look at tests/data/fate/filter-amix-simple.err for details.
> make: *** [fate-filter-amix-simple] Error 134
> make: *** [fate-filter-amix-transition] Error 134

What system? FATE looks clean.
Michael Niedermayer Aug. 27, 2017, 1:59 a.m.
On Sat, Aug 26, 2017 at 10:22:38PM -0300, James Almer wrote:
> On 8/26/2017 9:59 PM, Michael Niedermayer wrote:
> > On Sat, Aug 26, 2017 at 05:53:18PM +0200, Paul B Mahol wrote:
> >> Really fixes hangs and infinite loops.
> >>
> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> ---
> >>  libavfilter/af_amix.c | 161 +++++++++++++++++++++++++-------------------------
> >>  1 file changed, 81 insertions(+), 80 deletions(-)
> > 
> > This breaks fate:
> > 
> > stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/  1056768
> > size: |2116800 - 1056768| >= 0
> > stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/   704512
> > size: |2116800 - 704512| >= 0
> > TEST    filter-hdcd-s16p
> > Test filter-amix-simple failed. Look at tests/data/fate/filter-amix-simple.err for details.
> > make: *** [fate-filter-amix-simple] Error 134
> > make: *** [fate-filter-amix-transition] Error 134
> 
> What system? FATE looks clean.

nothing special just normal ubuntu x86-64

i tool a closer look and it dies here:
Assertion !link->status_in failed at libavfilter/avfilter.c:1639
Aborted (core dumped)
make: *** [fate-filter-amix-simple] Error 134

thats a av_assert1(), i built with --assert-level=2

[...]
Paul B Mahol Aug. 27, 2017, 7:05 a.m.
On 8/27/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sat, Aug 26, 2017 at 10:22:38PM -0300, James Almer wrote:
>> On 8/26/2017 9:59 PM, Michael Niedermayer wrote:
>> > On Sat, Aug 26, 2017 at 05:53:18PM +0200, Paul B Mahol wrote:
>> >> Really fixes hangs and infinite loops.
>> >>
>> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> >> ---
>> >>  libavfilter/af_amix.c | 161
>> >> +++++++++++++++++++++++++-------------------------
>> >>  1 file changed, 81 insertions(+), 80 deletions(-)
>> >
>> > This breaks fate:
>> >
>> > stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/  1056768
>> > size: |2116800 - 1056768| >= 0
>> > stddev:      0.00 PSNR:inf MAXDIFF:         0 bytes:  2116800/   704512
>> > size: |2116800 - 704512| >= 0
>> > TEST    filter-hdcd-s16p
>> > Test filter-amix-simple failed. Look at
>> > tests/data/fate/filter-amix-simple.err for details.
>> > make: *** [fate-filter-amix-simple] Error 134
>> > make: *** [fate-filter-amix-transition] Error 134
>>
>> What system? FATE looks clean.
>
> nothing special just normal ubuntu x86-64
>
> i tool a closer look and it dies here:
> Assertion !link->status_in failed at libavfilter/avfilter.c:1639
> Aborted (core dumped)
> make: *** [fate-filter-amix-simple] Error 134
>
> thats a av_assert1(), i built with --assert-level=2

Should be fixed.

Patch hide | download patch | download mbox

diff --git a/libavfilter/af_amix.c b/libavfilter/af_amix.c
index 78be57a..989fd84 100644
--- a/libavfilter/af_amix.c
+++ b/libavfilter/af_amix.c
@@ -41,6 +41,7 @@ 
 
 #include "audio.h"
 #include "avfilter.h"
+#include "filters.h"
 #include "formats.h"
 #include "internal.h"
 
@@ -263,21 +264,15 @@  static int config_output(AVFilterLink *outlink)
     return 0;
 }
 
-static int calc_active_inputs(MixContext *s);
-
 /**
  * Read samples from the input FIFOs, mix, and write to the output link.
  */
-static int output_frame(AVFilterLink *outlink, int need_request)
+static int output_frame(AVFilterLink *outlink)
 {
     AVFilterContext *ctx = outlink->src;
     MixContext      *s = ctx->priv;
     AVFrame *out_buf, *in_buf;
-    int nb_samples, ns, ret, i;
-
-    ret = calc_active_inputs(s);
-    if (ret < 0)
-        return ret;
+    int nb_samples, ns, i;
 
     if (s->input_state[0] & INPUT_ON) {
         /* first input live: use the corresponding frame size */
@@ -288,7 +283,7 @@  static int output_frame(AVFilterLink *outlink, int need_request)
                 if (ns < nb_samples) {
                     if (!(s->input_state[i] & INPUT_EOF))
                         /* unclosed input with not enough samples */
-                        return need_request ? ff_request_frame(ctx->inputs[i]) : 0;
+                        return 0;
                     /* closed input to drain */
                     nb_samples = ns;
                 }
@@ -303,8 +298,10 @@  static int output_frame(AVFilterLink *outlink, int need_request)
                 nb_samples = FFMIN(nb_samples, ns);
             }
         }
-        if (nb_samples == INT_MAX)
-            return AVERROR_EOF;
+        if (nb_samples == INT_MAX) {
+            ff_outlink_set_status(outlink, AVERROR_EOF, s->next_pts);
+            return 0;
+        }
     }
 
     s->next_pts = frame_list_next_pts(s->frame_list);
@@ -367,27 +364,18 @@  static int output_frame(AVFilterLink *outlink, int need_request)
 static int request_samples(AVFilterContext *ctx, int min_samples)
 {
     MixContext *s = ctx->priv;
-    int i, ret;
+    int i;
 
     av_assert0(s->nb_inputs > 1);
 
     for (i = 1; i < s->nb_inputs; i++) {
-        ret = 0;
         if (!(s->input_state[i] & INPUT_ON))
             continue;
         if (av_audio_fifo_size(s->fifos[i]) >= min_samples)
             continue;
-        ret = ff_request_frame(ctx->inputs[i]);
-        if (ret == AVERROR_EOF) {
-            s->input_state[i] |= INPUT_EOF;
-            if (av_audio_fifo_size(s->fifos[i]) == 0) {
-                s->input_state[i] = 0;
-                continue;
-            }
-        } else if (ret < 0)
-            return ret;
+        ff_inlink_request_frame(ctx->inputs[i]);
     }
-    return output_frame(ctx->outputs[0], 1);
+    return output_frame(ctx->outputs[0]);
 }
 
 /**
@@ -411,73 +399,87 @@  static int calc_active_inputs(MixContext *s)
     return 0;
 }
 
-static int request_frame(AVFilterLink *outlink)
+static int activate(AVFilterContext *ctx)
 {
-    AVFilterContext *ctx = outlink->src;
-    MixContext      *s = ctx->priv;
-    int ret;
-    int wanted_samples;
-
-    ret = calc_active_inputs(s);
-    if (ret < 0)
-        return ret;
-
-    if (!(s->input_state[0] & INPUT_ON))
-        return request_samples(ctx, 1);
-
-    if (s->frame_list->nb_frames == 0) {
-        ret = ff_request_frame(ctx->inputs[0]);
-        if (ret == AVERROR_EOF) {
-            s->input_state[0] = 0;
-            if (s->nb_inputs == 1)
-                return AVERROR_EOF;
-            return output_frame(ctx->outputs[0], 1);
-        }
-        return ret;
-    }
-    av_assert0(s->frame_list->nb_frames > 0);
+    AVFilterLink *outlink = ctx->outputs[0];
+    MixContext *s = ctx->priv;
+    AVFrame *buf = NULL;
+    int i, ret;
 
-    wanted_samples = frame_list_next_frame_size(s->frame_list);
+    for (i = 0; i < s->nb_inputs; i++) {
+        AVFilterLink *inlink = ctx->inputs[i];
+
+        if ((ret = ff_inlink_consume_frame(ctx->inputs[i], &buf)) > 0) {
+            if (i == 0) {
+                int64_t pts = av_rescale_q(buf->pts, inlink->time_base,
+                                           outlink->time_base);
+                ret = frame_list_add_frame(s->frame_list, buf->nb_samples, pts);
+                if (ret < 0) {
+                    av_frame_free(&buf);
+                    return ret;
+                }
+            }
 
-    return request_samples(ctx, wanted_samples);
-}
+            ret = av_audio_fifo_write(s->fifos[i], (void **)buf->extended_data,
+                                      buf->nb_samples);
+            if (ret < 0) {
+                av_frame_free(&buf);
+                return ret;
+            }
 
-static int filter_frame(AVFilterLink *inlink, AVFrame *buf)
-{
-    AVFilterContext  *ctx = inlink->dst;
-    MixContext       *s = ctx->priv;
-    AVFilterLink *outlink = ctx->outputs[0];
-    int i, ret = 0;
+            av_frame_free(&buf);
 
-    for (i = 0; i < ctx->nb_inputs; i++)
-        if (ctx->inputs[i] == inlink)
-            break;
-    if (i >= ctx->nb_inputs) {
-        av_log(ctx, AV_LOG_ERROR, "unknown input link\n");
-        ret = AVERROR(EINVAL);
-        goto fail;
+            ret = output_frame(outlink);
+            if (ret < 0)
+                return ret;
+        }
     }
 
-    if (i == 0) {
-        int64_t pts = av_rescale_q(buf->pts, inlink->time_base,
-                                   outlink->time_base);
-        ret = frame_list_add_frame(s->frame_list, buf->nb_samples, pts);
-        if (ret < 0)
-            goto fail;
+    for (i = 0; i < s->nb_inputs; i++) {
+        int64_t pts;
+        int status;
+
+        if (ff_inlink_acknowledge_status(ctx->inputs[i], &status, &pts)) {
+            if (status == AVERROR_EOF) {
+                if (i == 0) {
+                    s->input_state[i] = 0;
+                    if (s->nb_inputs == 1) {
+                        ff_outlink_set_status(outlink, status, pts);
+                        return 0;
+                    }
+                } else {
+                    s->input_state[i] |= INPUT_EOF;
+                    if (av_audio_fifo_size(s->fifos[i]) == 0) {
+                        s->input_state[i] = 0;
+                    }
+                }
+            }
+        }
     }
 
-    ret = av_audio_fifo_write(s->fifos[i], (void **)buf->extended_data,
-                              buf->nb_samples);
-    if (ret < 0)
-        goto fail;
+    if (calc_active_inputs(s)) {
+        ff_outlink_set_status(outlink, AVERROR_EOF, s->next_pts);
+        return 0;
+    }
 
-    av_frame_free(&buf);
-    return output_frame(outlink, 0);
+    if (ff_outlink_frame_wanted(outlink)) {
+        int wanted_samples;
 
-fail:
-    av_frame_free(&buf);
+        if (!(s->input_state[0] & INPUT_ON))
+            return request_samples(ctx, 1);
 
-    return ret;
+        if (s->frame_list->nb_frames == 0) {
+            ff_inlink_request_frame(ctx->inputs[0]);
+            return 0;
+        }
+        av_assert0(s->frame_list->nb_frames > 0);
+
+        wanted_samples = frame_list_next_frame_size(s->frame_list);
+
+        return request_samples(ctx, wanted_samples);
+    }
+
+    return 0;
 }
 
 static av_cold int init(AVFilterContext *ctx)
@@ -494,7 +496,6 @@  static av_cold int init(AVFilterContext *ctx)
         pad.name           = av_strdup(name);
         if (!pad.name)
             return AVERROR(ENOMEM);
-        pad.filter_frame   = filter_frame;
 
         if ((ret = ff_insert_inpad(ctx, i, &pad)) < 0) {
             av_freep(&pad.name);
@@ -562,7 +563,6 @@  static const AVFilterPad avfilter_af_amix_outputs[] = {
         .name          = "default",
         .type          = AVMEDIA_TYPE_AUDIO,
         .config_props  = config_output,
-        .request_frame = request_frame
     },
     { NULL }
 };
@@ -574,6 +574,7 @@  AVFilter ff_af_amix = {
     .priv_class     = &amix_class,
     .init           = init,
     .uninit         = uninit,
+    .activate       = activate,
     .query_formats  = query_formats,
     .inputs         = NULL,
     .outputs        = avfilter_af_amix_outputs,