diff mbox

[FFmpeg-devel,17/17] lavfi/buffersink: move to the new design.

Message ID 20161229143403.2851-17-george@nsup.org
State Accepted
Headers show

Commit Message

Nicolas George Dec. 29, 2016, 2:34 p.m. UTC
Signed-off-by: Nicolas George <george@nsup.org>
---
 libavfilter/buffersink.c | 214 +++++++++++++----------------------------------
 1 file changed, 56 insertions(+), 158 deletions(-)


Minor changes to accomodate the previous changes. Since diff is not very
smart about it, better read the code than the patch.

Comments

Nicolas George Jan. 5, 2017, 10:27 a.m. UTC | #1
Le nonidi 9 nivôse, an CCXXV, Nicolas George a écrit :
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/buffersink.c | 214 +++++++++++++----------------------------------
>  1 file changed, 56 insertions(+), 158 deletions(-)
> 
> 
> Minor changes to accomodate the previous changes. Since diff is not very
> smart about it, better read the code than the patch.

This patch and the previous one still lack a review.

For convenience, here is an excerpt of the file after the patch
containing the parts that are mostly rewritten and unreadable with diff.

Regards,
Rostislav Pehlivanov Jan. 5, 2017, 10:51 a.m. UTC | #2
On 5 January 2017 at 10:27, Nicolas George <george@nsup.org> wrote:

> Le nonidi 9 nivôse, an CCXXV, Nicolas George a écrit :
> > Signed-off-by: Nicolas George <george@nsup.org>
> > ---
> >  libavfilter/buffersink.c | 214 +++++++++++++-----------------
> -----------------
> >  1 file changed, 56 insertions(+), 158 deletions(-)
> >
> >
> > Minor changes to accomodate the previous changes. Since diff is not very
> > smart about it, better read the code than the patch.
>
> This patch and the previous one still lack a review.
>
> For convenience, here is an excerpt of the file after the patch
> containing the parts that are mostly rewritten and unreadable with diff.
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Thanks, the excerpt looks fine to me.

btw, there's apparently been a regression w.r.t. EOF handling which wm4
hasn't told you about yet, not sure which commit introduced it though:

https://github.com/mpv-player/mpv/commit/43386a7c92e14e56abdebfd4ed9335794c795f2b
Nicolas George Jan. 5, 2017, 7:38 p.m. UTC | #3
Le sextidi 16 nivôse, an CCXXV, Rostislav Pehlivanov a écrit :
> Thanks, the excerpt looks fine to me.

Thanks for the review. One to go.

> btw, there's apparently been a regression w.r.t. EOF handling which wm4
> hasn't told you about yet, not sure which commit introduced it though:
> 
> https://github.com/mpv-player/mpv/commit/43386a7c92e14e56abdebfd4ed9335794c795f2b

Could you perhaps test if the current series (which needs also the "make
avfilterlink private series" to apply cleanly) fixes it?

If not, can you tell me exactly how to reproduce the issue?

Regards,
Rostislav Pehlivanov Jan. 8, 2017, 12:38 p.m. UTC | #4
On 5 January 2017 at 19:38, Nicolas George <george@nsup.org> wrote:

> Le sextidi 16 nivôse, an CCXXV, Rostislav Pehlivanov a écrit :
> > Thanks, the excerpt looks fine to me.
>
> Thanks for the review. One to go.
>
> > btw, there's apparently been a regression w.r.t. EOF handling which wm4
> > hasn't told you about yet, not sure which commit introduced it though:
> >
> > https://github.com/mpv-player/mpv/commit/43386a7c92e14e56abdebfd4ed9335
> 794c795f2b
>
> Could you perhaps test if the current series (which needs also the "make
> avfilterlink private series" to apply cleanly) fixes it?
>
> If not, can you tell me exactly how to reproduce the issue?
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
I think it might be related to this infinite loop issue:

ffmpeg -i fT7.wav -af dynaudnorm -loglevel error -f null -

with the file I've attached will get stuck in an infinite loop outside of
the filter somewhere.
Nicolas George Jan. 8, 2017, 6:58 p.m. UTC | #5
Le nonidi 19 nivôse, an CCXXV, Rostislav Pehlivanov a écrit :
> I think it might be related to this infinite loop issue:
> 
> ffmpeg -i fT7.wav -af dynaudnorm -loglevel error -f null -
> 
> with the file I've attached will get stuck in an infinite loop outside of
> the filter somewhere.

Thanks for the report. I am relieved to say it is not my fault, it is a
bug in dynaudionorm: when it detects EOF, it calls flush_buffer(), that
in turns calls filter_frame(). Normally, it should result in the last
frames being pushed to the next filter, but in this instance is results
in dynaudionorm queuing the frame.

Seen from the outside, that means dynaudionorm->request_frame() is doing
nothing. It should either return EOF or cause a frame to be pushed.

I am afraid that is as far as I can go, I do not know the algorithm
dynaudionorm is using.

Regards,
Nicolas George Jan. 8, 2017, 8:58 p.m. UTC | #6
Le nonidi 9 nivôse, an CCXXV, Nicolas George a écrit :
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/buffersink.c | 214 +++++++++++++----------------------------------
>  1 file changed, 56 insertions(+), 158 deletions(-)
> 
> Minor changes to accomodate the previous changes. Since diff is not very
> smart about it, better read the code than the patch.

I will push this series and the non-controversial part of the one about
AVFilterLink sometime during the week when I have time, unless there are
new remarks in the meantime.

Regards,
diff mbox

Patch

diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c
index f2d8872622..6b35b54458 100644
--- a/libavfilter/buffersink.c
+++ b/libavfilter/buffersink.c
@@ -23,12 +23,10 @@ 
  * buffer sink
  */
 
-#include "libavutil/audio_fifo.h"
 #include "libavutil/avassert.h"
 #include "libavutil/channel_layout.h"
 #include "libavutil/common.h"
 #include "libavutil/internal.h"
-#include "libavutil/mathematics.h"
 #include "libavutil/opt.h"
 
 #define FF_INTERNAL_FIELDS 1
@@ -36,11 +34,11 @@ 
 #include "avfilter.h"
 #include "avfilterlink.h"
 #include "buffersink.h"
+#include "filters.h"
 #include "internal.h"
 
 typedef struct BufferSinkContext {
     const AVClass *class;
-    AVFifoBuffer *fifo;                      ///< FIFO buffer of video frame references
     unsigned warning_limit;
 
     /* only used for video */
@@ -58,182 +56,73 @@  typedef struct BufferSinkContext {
     int *sample_rates;                      ///< list of accepted sample rates, terminated by -1
     int sample_rates_size;
 
-    /* only used for compat API */
-    AVAudioFifo *audio_fifo;     ///< FIFO for audio samples
-    int64_t next_pts;            ///< interpolating audio pts
+    AVFrame *peeked_frame;
 } BufferSinkContext;
 
 #define NB_ITEMS(list) (list ## _size / sizeof(*list))
 #define FIFO_INIT_SIZE 8
 #define FIFO_INIT_ELEMENT_SIZE sizeof(void *)
 
-static av_cold void uninit(AVFilterContext *ctx)
-{
-    BufferSinkContext *sink = ctx->priv;
-    AVFrame *frame;
-
-    if (sink->audio_fifo)
-        av_audio_fifo_free(sink->audio_fifo);
-
-    if (sink->fifo) {
-        while (av_fifo_size(sink->fifo) >= FIFO_INIT_ELEMENT_SIZE) {
-            av_fifo_generic_read(sink->fifo, &frame, sizeof(frame), NULL);
-            av_frame_free(&frame);
-        }
-        av_fifo_freep(&sink->fifo);
-    }
-}
-
-static int add_buffer_ref(AVFilterContext *ctx, AVFrame *ref)
+int attribute_align_arg av_buffersink_get_frame(AVFilterContext *ctx, AVFrame *frame)
 {
-    BufferSinkContext *buf = ctx->priv;
-
-    if (av_fifo_space(buf->fifo) < FIFO_INIT_ELEMENT_SIZE) {
-        /* realloc fifo size */
-        if (av_fifo_realloc2(buf->fifo, av_fifo_size(buf->fifo) * 2) < 0) {
-            av_log(ctx, AV_LOG_ERROR,
-                   "Cannot buffer more frames. Consume some available frames "
-                   "before adding new ones.\n");
-            return AVERROR(ENOMEM);
-        }
-    }
-
-    /* cache frame */
-    av_fifo_generic_write(buf->fifo, &ref, FIFO_INIT_ELEMENT_SIZE, NULL);
-    return 0;
+    return av_buffersink_get_frame_flags(ctx, frame, 0);
 }
 
-static int filter_frame(AVFilterLink *link, AVFrame *frame)
+static int return_or_keep_frame(BufferSinkContext *buf, AVFrame *out, AVFrame *in, int flags)
 {
-    AVFilterContext *ctx = link->dst;
-    BufferSinkContext *buf = link->dst->priv;
-    int ret;
-
-    if ((ret = add_buffer_ref(ctx, frame)) < 0)
-        return ret;
-    if (buf->warning_limit &&
-        av_fifo_size(buf->fifo) / FIFO_INIT_ELEMENT_SIZE >= buf->warning_limit) {
-        av_log(ctx, AV_LOG_WARNING,
-               "%d buffers queued in %s, something may be wrong.\n",
-               buf->warning_limit,
-               (char *)av_x_if_null(ctx->name, ctx->filter->name));
-        buf->warning_limit *= 10;
+    if ((flags & AV_BUFFERSINK_FLAG_PEEK)) {
+        buf->peeked_frame = in;
+        return out ? av_frame_ref(out, in) : 0;
+    } else {
+        av_assert1(out);
+        buf->peeked_frame = NULL;
+        av_frame_move_ref(out, in);
+        av_frame_free(&in);
+        return 0;
     }
-    return 0;
-}
-
-int attribute_align_arg av_buffersink_get_frame(AVFilterContext *ctx, AVFrame *frame)
-{
-    return av_buffersink_get_frame_flags(ctx, frame, 0);
 }
 
-int attribute_align_arg av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flags)
+static int get_frame_internal(AVFilterContext *ctx, AVFrame *frame, int flags, int samples)
 {
     BufferSinkContext *buf = ctx->priv;
     AVFilterLink *inlink = ctx->inputs[0];
-    int peek_in_framequeue = 0, ret;
-    int64_t frame_count;
+    int status, ret;
     AVFrame *cur_frame;
 
-    /* no picref available, fetch it from the filterchain */
-    while (!av_fifo_size(buf->fifo)) {
-        /* if peek_in_framequeue is true later, then ff_request_frame() and
-           the ff_filter_graph_run_once() loop will take a frame from it and
-           move it to the internal fifo, ending the global loop */
-        av_assert0(!peek_in_framequeue);
-        if (inlink->status_out)
-            return inlink->status_out;
-        peek_in_framequeue = ff_framequeue_queued_frames(&inlink->fifo) &&
-                             ff_framequeue_queued_samples(&inlink->fifo) >= inlink->min_samples;
-        if ((flags & AV_BUFFERSINK_FLAG_NO_REQUEST) && !peek_in_framequeue)
-            return AVERROR(EAGAIN);
-        if ((ret = ff_request_frame(inlink)) < 0)
+    if (buf->peeked_frame)
+        return return_or_keep_frame(buf, frame, buf->peeked_frame, flags);
+
+    while (1) {
+        ret = samples ? ff_inlink_consume_samples(inlink, samples, samples, &cur_frame) :
+                        ff_inlink_consume_frame(inlink, &cur_frame);
+        if (ret < 0) {
             return ret;
-        frame_count = inlink->frame_count_out;
-        while (frame_count == inlink->frame_count_out) {
+        } else if (ret) {
+            /* TODO return the frame instead of copying it */
+            return return_or_keep_frame(buf, frame, cur_frame, flags);
+        } else if (ff_inlink_acknowledge_status(inlink, &status)) {
+            return status;
+        } else if ((flags & AV_BUFFERSINK_FLAG_NO_REQUEST)) {
+            return AVERROR(EAGAIN);
+        } else if (inlink->frame_wanted_out) {
             ret = ff_filter_graph_run_once(ctx->graph);
             if (ret < 0)
                 return ret;
+        } else {
+            ff_inlink_set_frame_wanted(inlink);
         }
     }
-
-    if (flags & AV_BUFFERSINK_FLAG_PEEK) {
-        cur_frame = *((AVFrame **)av_fifo_peek2(buf->fifo, 0));
-        if ((ret = av_frame_ref(frame, cur_frame)) < 0)
-            return ret;
-    } else {
-        av_fifo_generic_read(buf->fifo, &cur_frame, sizeof(cur_frame), NULL);
-        av_frame_move_ref(frame, cur_frame);
-        av_frame_free(&cur_frame);
-    }
-
-    return 0;
 }
 
-static int read_from_fifo(AVFilterContext *ctx, AVFrame *frame,
-                          int nb_samples)
+int attribute_align_arg av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flags)
 {
-    BufferSinkContext *s = ctx->priv;
-    AVFilterLink   *link = ctx->inputs[0];
-    AVFrame *tmp;
-
-    if (!(tmp = ff_get_audio_buffer(link, nb_samples)))
-        return AVERROR(ENOMEM);
-    av_audio_fifo_read(s->audio_fifo, (void**)tmp->extended_data, nb_samples);
-
-    tmp->pts = s->next_pts;
-    if (s->next_pts != AV_NOPTS_VALUE)
-        s->next_pts += av_rescale_q(nb_samples, (AVRational){1, link->sample_rate},
-                                    link->time_base);
-
-    av_frame_move_ref(frame, tmp);
-    av_frame_free(&tmp);
-
-    return 0;
+    return get_frame_internal(ctx, frame, flags, ctx->inputs[0]->min_samples);
 }
 
 int attribute_align_arg av_buffersink_get_samples(AVFilterContext *ctx,
                                                   AVFrame *frame, int nb_samples)
 {
-    BufferSinkContext *s = ctx->priv;
-    AVFilterLink   *link = ctx->inputs[0];
-    AVFrame *cur_frame;
-    int ret = 0;
-
-    if (!s->audio_fifo) {
-        int nb_channels = link->channels;
-        if (!(s->audio_fifo = av_audio_fifo_alloc(link->format, nb_channels, nb_samples)))
-            return AVERROR(ENOMEM);
-    }
-
-    while (ret >= 0) {
-        if (av_audio_fifo_size(s->audio_fifo) >= nb_samples)
-            return read_from_fifo(ctx, frame, nb_samples);
-
-        if (!(cur_frame = av_frame_alloc()))
-            return AVERROR(ENOMEM);
-        ret = av_buffersink_get_frame_flags(ctx, cur_frame, 0);
-        if (ret == AVERROR_EOF && av_audio_fifo_size(s->audio_fifo)) {
-            av_frame_free(&cur_frame);
-            return read_from_fifo(ctx, frame, av_audio_fifo_size(s->audio_fifo));
-        } else if (ret < 0) {
-            av_frame_free(&cur_frame);
-            return ret;
-        }
-
-        if (cur_frame->pts != AV_NOPTS_VALUE) {
-            s->next_pts = cur_frame->pts -
-                          av_rescale_q(av_audio_fifo_size(s->audio_fifo),
-                                       (AVRational){ 1, link->sample_rate },
-                                       link->time_base);
-        }
-
-        ret = av_audio_fifo_write(s->audio_fifo, (void**)cur_frame->extended_data,
-                                  cur_frame->nb_samples);
-        av_frame_free(&cur_frame);
-    }
-
-    return ret;
+    return get_frame_internal(ctx, frame, 0, nb_samples);
 }
 
 AVBufferSinkParams *av_buffersink_params_alloc(void)
@@ -260,13 +149,24 @@  static av_cold int common_init(AVFilterContext *ctx)
 {
     BufferSinkContext *buf = ctx->priv;
 
-    buf->fifo = av_fifo_alloc_array(FIFO_INIT_SIZE, FIFO_INIT_ELEMENT_SIZE);
-    if (!buf->fifo) {
-        av_log(ctx, AV_LOG_ERROR, "Failed to allocate fifo\n");
-        return AVERROR(ENOMEM);
-    }
     buf->warning_limit = 100;
-    buf->next_pts = AV_NOPTS_VALUE;
+    return 0;
+}
+
+static int activate(AVFilterContext *ctx)
+{
+    BufferSinkContext *buf = ctx->priv;
+
+    if (buf->warning_limit &&
+        ff_framequeue_queued_frames(&ctx->inputs[0]->fifo) >= buf->warning_limit) {
+        av_log(ctx, AV_LOG_WARNING,
+               "%d buffers queued in %s, something may be wrong.\n",
+               buf->warning_limit,
+               (char *)av_x_if_null(ctx->name, ctx->filter->name));
+        buf->warning_limit *= 10;
+    }
+
+    /* The frame is queued, the rest is up to get_frame_internal */
     return 0;
 }
 
@@ -280,7 +180,7 @@  void av_buffersink_set_frame_size(AVFilterContext *ctx, unsigned frame_size)
 
 #define MAKE_AVFILTERLINK_ACCESSOR(type, field) \
 type av_buffersink_get_##field(const AVFilterContext *ctx) { \
-    av_assert0(ctx->filter->uninit == uninit); \
+    av_assert0(ctx->filter->activate == activate); \
     return ctx->inputs[0]->field; \
 }
 
@@ -436,7 +336,6 @@  static const AVFilterPad avfilter_vsink_buffer_inputs[] = {
     {
         .name         = "default",
         .type         = AVMEDIA_TYPE_VIDEO,
-        .filter_frame = filter_frame,
     },
     { NULL }
 };
@@ -447,9 +346,9 @@  AVFilter ff_vsink_buffer = {
     .priv_size   = sizeof(BufferSinkContext),
     .priv_class  = &buffersink_class,
     .init_opaque = vsink_init,
-    .uninit      = uninit,
 
     .query_formats = vsink_query_formats,
+    .activate    = activate,
     .inputs      = avfilter_vsink_buffer_inputs,
     .outputs     = NULL,
 };
@@ -458,7 +357,6 @@  static const AVFilterPad avfilter_asink_abuffer_inputs[] = {
     {
         .name         = "default",
         .type         = AVMEDIA_TYPE_AUDIO,
-        .filter_frame = filter_frame,
     },
     { NULL }
 };
@@ -469,9 +367,9 @@  AVFilter ff_asink_abuffer = {
     .priv_class  = &abuffersink_class,
     .priv_size   = sizeof(BufferSinkContext),
     .init_opaque = asink_init,
-    .uninit      = uninit,
 
     .query_formats = asink_query_formats,
+    .activate    = activate,
     .inputs      = avfilter_asink_abuffer_inputs,
     .outputs     = NULL,
 };