[FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Anton Khirnov]

This hack is used to limit the visibility of some AVFilterLink fields to
only certain files. Replace it with the same pattern that is used e.g.
in lavf AVStream/FFstream and avoid exposing these internal fields in a
public header completely.
---
 libavfilter/avfilter.c       | 191 +++++++++++++++++++++--------------
 libavfilter/avfilter.h       |  45 ---------
 libavfilter/avfiltergraph.c  |   9 +-
 libavfilter/buffersink.c     |   8 +-
 libavfilter/link_internal.h  |  69 +++++++++++++
 libavfilter/tests/filtfmts.c |   9 +-
 6 files changed, 196 insertions(+), 135 deletions(-)
 create mode 100644 libavfilter/link_internal.h

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index c2ecdffa6f5..2a1f99bd656 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -34,15 +34,14 @@
 #include "libavutil/rational.h"
 #include "libavutil/samplefmt.h"
 
-#define FF_INTERNAL_FIELDS 1
-#include "framequeue.h"
-
 #include "audio.h"
 #include "avfilter.h"
 #include "filters.h"
 #include "formats.h"
+#include "framequeue.h"
 #include "framepool.h"
 #include "internal.h"
+#include "link_internal.h"
 
 static void tlog_ref(void *ctx, AVFrame *ref, int end)
 {
@@ -148,6 +147,7 @@ int ff_append_outpad_free_name(AVFilterContext *f, AVFilterPad *p)
 int avfilter_link(AVFilterContext *src, unsigned srcpad,
                   AVFilterContext *dst, unsigned dstpad)
 {
+    FilterLinkInternal *li;
     AVFilterLink *link;
 
     av_assert0(src->graph);
@@ -166,9 +166,10 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
         return AVERROR(EINVAL);
     }
 
-    link = av_mallocz(sizeof(*link));
-    if (!link)
+    li = av_mallocz(sizeof(*li));
+    if (!li)
         return AVERROR(ENOMEM);
+    link = &li->l;
 
     src->outputs[srcpad] = dst->inputs[dstpad] = link;
 
@@ -179,17 +180,20 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
     link->type    = src->output_pads[srcpad].type;
     av_assert0(AV_PIX_FMT_NONE == -1 && AV_SAMPLE_FMT_NONE == -1);
     link->format  = -1;
-    ff_framequeue_init(&link->fifo, &src->graph->internal->frame_queues);
+    ff_framequeue_init(&li->fifo, &src->graph->internal->frame_queues);
 
     return 0;
 }
 
 void avfilter_link_free(AVFilterLink **link)
 {
+    FilterLinkInternal *li;
+
     if (!*link)
         return;
+    li = ff_link_internal(*link);
 
-    ff_framequeue_free(&(*link)->fifo);
+    ff_framequeue_free(&li->fifo);
     ff_frame_pool_uninit((FFFramePool**)&(*link)->frame_pool);
     av_channel_layout_uninit(&(*link)->ch_layout);
 
@@ -209,29 +213,35 @@ static void filter_unblock(AVFilterContext *filter)
 {
     unsigned i;
 
-    for (i = 0; i < filter->nb_outputs; i++)
-        filter->outputs[i]->frame_blocked_in = 0;
+    for (i = 0; i < filter->nb_outputs; i++) {
+        FilterLinkInternal * const li = ff_link_internal(filter->outputs[i]);
+        li->frame_blocked_in = 0;
+    }
 }
 
 
 void ff_avfilter_link_set_in_status(AVFilterLink *link, int status, int64_t pts)
 {
-    if (link->status_in == status)
+    FilterLinkInternal * const li = ff_link_internal(link);
+
+    if (li->status_in == status)
         return;
-    av_assert0(!link->status_in);
-    link->status_in = status;
-    link->status_in_pts = pts;
+    av_assert0(!li->status_in);
+    li->status_in = status;
+    li->status_in_pts = pts;
     link->frame_wanted_out = 0;
-    link->frame_blocked_in = 0;
+    li->frame_blocked_in = 0;
     filter_unblock(link->dst);
     ff_filter_set_ready(link->dst, 200);
 }
 
 void ff_avfilter_link_set_out_status(AVFilterLink *link, int status, int64_t pts)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
+
     av_assert0(!link->frame_wanted_out);
-    av_assert0(!link->status_out);
-    link->status_out = status;
+    av_assert0(!li->status_out);
+    li->status_out = status;
     if (pts != AV_NOPTS_VALUE)
         ff_update_link_current_pts(link, pts);
     filter_unblock(link->dst);
@@ -409,13 +419,15 @@ void ff_tlog_link(void *ctx, AVFilterLink *link, int end)
 
 int ff_request_frame(AVFilterLink *link)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
+
     FF_TPRINTF_START(NULL, request_frame); ff_tlog_link(NULL, link, 1);
 
     av_assert1(!link->dst->filter->activate);
-    if (link->status_out)
-        return link->status_out;
-    if (link->status_in) {
-        if (ff_framequeue_queued_frames(&link->fifo)) {
+    if (li->status_out)
+        return li->status_out;
+    if (li->status_in) {
+        if (ff_framequeue_queued_frames(&li->fifo)) {
             av_assert1(!link->frame_wanted_out);
             av_assert1(link->dst->ready >= 300);
             return 0;
@@ -423,8 +435,8 @@ int ff_request_frame(AVFilterLink *link)
             /* Acknowledge status change. Filters using ff_request_frame() will
                handle the change automatically. Filters can also check the
                status directly but none do yet. */
-            ff_avfilter_link_set_out_status(link, link->status_in, link->status_in_pts);
-            return link->status_out;
+            ff_avfilter_link_set_out_status(link, li->status_in, li->status_in_pts);
+            return li->status_out;
         }
     }
     link->frame_wanted_out = 1;
@@ -437,14 +449,18 @@ static int64_t guess_status_pts(AVFilterContext *ctx, int status, AVRational lin
     unsigned i;
     int64_t r = INT64_MAX;
 
-    for (i = 0; i < ctx->nb_inputs; i++)
-        if (ctx->inputs[i]->status_out == status)
+    for (i = 0; i < ctx->nb_inputs; i++) {
+        FilterLinkInternal * const li = ff_link_internal(ctx->inputs[i]);
+        if (li->status_out == status)
             r = FFMIN(r, av_rescale_q(ctx->inputs[i]->current_pts, ctx->inputs[i]->time_base, link_time_base));
+    }
     if (r < INT64_MAX)
         return r;
     av_log(ctx, AV_LOG_WARNING, "EOF timestamp not reliable\n");
-    for (i = 0; i < ctx->nb_inputs; i++)
-        r = FFMIN(r, av_rescale_q(ctx->inputs[i]->status_in_pts, ctx->inputs[i]->time_base, link_time_base));
+    for (i = 0; i < ctx->nb_inputs; i++) {
+        FilterLinkInternal * const li = ff_link_internal(ctx->inputs[i]);
+        r = FFMIN(r, av_rescale_q(li->status_in_pts, ctx->inputs[i]->time_base, link_time_base));
+    }
     if (r < INT64_MAX)
         return r;
     return AV_NOPTS_VALUE;
@@ -452,17 +468,18 @@ static int64_t guess_status_pts(AVFilterContext *ctx, int status, AVRational lin
 
 static int ff_request_frame_to_filter(AVFilterLink *link)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
     int ret = -1;
 
     FF_TPRINTF_START(NULL, request_frame_to_filter); ff_tlog_link(NULL, link, 1);
     /* Assume the filter is blocked, let the method clear it if not */
-    link->frame_blocked_in = 1;
+    li->frame_blocked_in = 1;
     if (link->srcpad->request_frame)
         ret = link->srcpad->request_frame(link);
     else if (link->src->inputs[0])
         ret = ff_request_frame(link->src->inputs[0]);
     if (ret < 0) {
-        if (ret != AVERROR(EAGAIN) && ret != link->status_in)
+        if (ret != AVERROR(EAGAIN) && ret != li->status_in)
             ff_avfilter_link_set_in_status(link, ret, guess_status_pts(link->src, ret, link->time_base));
         if (ret == AVERROR_EOF)
             ret = 0;
@@ -977,6 +994,7 @@ fail:
 
 int ff_filter_frame(AVFilterLink *link, AVFrame *frame)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
     int ret;
     FF_TPRINTF_START(NULL, filter_frame); ff_tlog_link(NULL, link, 1); ff_tlog(NULL, " "); tlog_ref(NULL, frame, 1);
 
@@ -1006,11 +1024,11 @@ int ff_filter_frame(AVFilterLink *link, AVFrame *frame)
         }
     }
 
-    link->frame_blocked_in = link->frame_wanted_out = 0;
+    li->frame_blocked_in = link->frame_wanted_out = 0;
     link->frame_count_in++;
     link->sample_count_in += frame->nb_samples;
     filter_unblock(link->dst);
-    ret = ff_framequeue_add(&link->fifo, frame);
+    ret = ff_framequeue_add(&li->fifo, frame);
     if (ret < 0) {
         av_frame_free(&frame);
         return ret;
@@ -1023,16 +1041,17 @@ error:
     return AVERROR_PATCHWELCOME;
 }
 
-static int samples_ready(AVFilterLink *link, unsigned min)
+static int samples_ready(FilterLinkInternal *link, unsigned min)
 {
     return ff_framequeue_queued_frames(&link->fifo) &&
            (ff_framequeue_queued_samples(&link->fifo) >= min ||
             link->status_in);
 }
 
-static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
+static int take_samples(FilterLinkInternal *li, unsigned min, unsigned max,
                         AVFrame **rframe)
 {
+    AVFilterLink *link = &li->l;
     AVFrame *frame0, *frame, *buf;
     unsigned nb_samples, nb_frames, i, p;
     int ret;
@@ -1040,9 +1059,9 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
     /* Note: this function relies on no format changes and must only be
        called with enough samples. */
     av_assert1(samples_ready(link, link->min_samples));
-    frame0 = frame = ff_framequeue_peek(&link->fifo, 0);
-    if (!link->fifo.samples_skipped && frame->nb_samples >= min && frame->nb_samples <= max) {
-        *rframe = ff_framequeue_take(&link->fifo);
+    frame0 = frame = ff_framequeue_peek(&li->fifo, 0);
+    if (!li->fifo.samples_skipped && frame->nb_samples >= min && frame->nb_samples <= max) {
+        *rframe = ff_framequeue_take(&li->fifo);
         return 0;
     }
     nb_frames = 0;
@@ -1055,9 +1074,9 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
         }
         nb_samples += frame->nb_samples;
         nb_frames++;
-        if (nb_frames == ff_framequeue_queued_frames(&link->fifo))
+        if (nb_frames == ff_framequeue_queued_frames(&li->fifo))
             break;
-        frame = ff_framequeue_peek(&link->fifo, nb_frames);
+        frame = ff_framequeue_peek(&li->fifo, nb_frames);
     }
 
     buf = ff_get_audio_buffer(link, nb_samples);
@@ -1071,7 +1090,7 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
 
     p = 0;
     for (i = 0; i < nb_frames; i++) {
-        frame = ff_framequeue_take(&link->fifo);
+        frame = ff_framequeue_take(&li->fifo);
         av_samples_copy(buf->extended_data, frame->extended_data, p, 0,
                         frame->nb_samples, link->ch_layout.nb_channels, link->format);
         p += frame->nb_samples;
@@ -1079,10 +1098,10 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
     }
     if (p < nb_samples) {
         unsigned n = nb_samples - p;
-        frame = ff_framequeue_peek(&link->fifo, 0);
+        frame = ff_framequeue_peek(&li->fifo, 0);
         av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n,
                         link->ch_layout.nb_channels, link->format);
-        ff_framequeue_skip_samples(&link->fifo, n, link->time_base);
+        ff_framequeue_skip_samples(&li->fifo, n, link->time_base);
     }
 
     *rframe = buf;
@@ -1091,6 +1110,7 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max,
 
 static int ff_filter_frame_to_filter(AVFilterLink *link)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
     AVFrame *frame = NULL;
     AVFilterContext *dst = link->dst;
     int ret;
@@ -1111,7 +1131,7 @@ static int ff_filter_frame_to_filter(AVFilterLink *link)
        before the frame; ff_filter_frame_framed() will re-increment it. */
     link->frame_count_out--;
     ret = ff_filter_frame_framed(link, frame);
-    if (ret < 0 && ret != link->status_out) {
+    if (ret < 0 && ret != li->status_out) {
         ff_avfilter_link_set_out_status(link, ret, AV_NOPTS_VALUE);
     } else {
         /* Run once again, to see if several frames were available, or if
@@ -1121,18 +1141,21 @@ static int ff_filter_frame_to_filter(AVFilterLink *link)
     return ret;
 }
 
-static int forward_status_change(AVFilterContext *filter, AVFilterLink *in)
+static int forward_status_change(AVFilterContext *filter, FilterLinkInternal *li_in)
 {
+    AVFilterLink *in = &li_in->l;
     unsigned out = 0, progress = 0;
     int ret;
 
-    av_assert0(!in->status_out);
+    av_assert0(!li_in->status_out);
     if (!filter->nb_outputs) {
         /* not necessary with the current API and sinks */
         return 0;
     }
-    while (!in->status_out) {
-        if (!filter->outputs[out]->status_in) {
+    while (!li_in->status_out) {
+        FilterLinkInternal *li_out = ff_link_internal(filter->outputs[out]);
+
+        if (!li_out->status_in) {
             progress++;
             ret = ff_request_frame_to_filter(filter->outputs[out]);
             if (ret < 0)
@@ -1142,7 +1165,7 @@ static int forward_status_change(AVFilterContext *filter, AVFilterLink *in)
             if (!progress) {
                 /* Every output already closed: input no longer interesting
                    (example: overlay in shortest mode, other input closed). */
-                ff_avfilter_link_set_out_status(in, in->status_in, in->status_in_pts);
+                ff_avfilter_link_set_out_status(in, li_in->status_in, li_in->status_in_pts);
                 return 0;
             }
             progress = 0;
@@ -1158,19 +1181,22 @@ static int ff_filter_activate_default(AVFilterContext *filter)
     unsigned i;
 
     for (i = 0; i < filter->nb_inputs; i++) {
-        if (samples_ready(filter->inputs[i], filter->inputs[i]->min_samples)) {
+        if (samples_ready(ff_link_internal(filter->inputs[i]),
+                          filter->inputs[i]->min_samples)) {
             return ff_filter_frame_to_filter(filter->inputs[i]);
         }
     }
     for (i = 0; i < filter->nb_inputs; i++) {
-        if (filter->inputs[i]->status_in && !filter->inputs[i]->status_out) {
-            av_assert1(!ff_framequeue_queued_frames(&filter->inputs[i]->fifo));
-            return forward_status_change(filter, filter->inputs[i]);
+        FilterLinkInternal * const li = ff_link_internal(filter->inputs[i]);
+        if (li->status_in && !li->status_out) {
+            av_assert1(!ff_framequeue_queued_frames(&li->fifo));
+            return forward_status_change(filter, li);
         }
     }
     for (i = 0; i < filter->nb_outputs; i++) {
+        FilterLinkInternal * const li = ff_link_internal(filter->outputs[i]);
         if (filter->outputs[i]->frame_wanted_out &&
-            !filter->outputs[i]->frame_blocked_in) {
+            !li->frame_blocked_in) {
             return ff_request_frame_to_filter(filter->outputs[i]);
         }
     }
@@ -1326,39 +1352,44 @@ int ff_filter_activate(AVFilterContext *filter)
 
 int ff_inlink_acknowledge_status(AVFilterLink *link, int *rstatus, int64_t *rpts)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
     *rpts = link->current_pts;
-    if (ff_framequeue_queued_frames(&link->fifo))
+    if (ff_framequeue_queued_frames(&li->fifo))
         return *rstatus = 0;
-    if (link->status_out)
-        return *rstatus = link->status_out;
-    if (!link->status_in)
+    if (li->status_out)
+        return *rstatus = li->status_out;
+    if (!li->status_in)
         return *rstatus = 0;
-    *rstatus = link->status_out = link->status_in;
-    ff_update_link_current_pts(link, link->status_in_pts);
+    *rstatus = li->status_out = li->status_in;
+    ff_update_link_current_pts(link, li->status_in_pts);
     *rpts = link->current_pts;
     return 1;
 }
 
 size_t ff_inlink_queued_frames(AVFilterLink *link)
 {
-    return ff_framequeue_queued_frames(&link->fifo);
+    FilterLinkInternal * const li = ff_link_internal(link);
+    return ff_framequeue_queued_frames(&li->fifo);
 }
 
 int ff_inlink_check_available_frame(AVFilterLink *link)
 {
-    return ff_framequeue_queued_frames(&link->fifo) > 0;
+    FilterLinkInternal * const li = ff_link_internal(link);
+    return ff_framequeue_queued_frames(&li->fifo) > 0;
 }
 
 int ff_inlink_queued_samples(AVFilterLink *link)
 {
-    return ff_framequeue_queued_samples(&link->fifo);
+    FilterLinkInternal * const li = ff_link_internal(link);
+    return ff_framequeue_queued_samples(&li->fifo);
 }
 
 int ff_inlink_check_available_samples(AVFilterLink *link, unsigned min)
 {
-    uint64_t samples = ff_framequeue_queued_samples(&link->fifo);
+    FilterLinkInternal * const li = ff_link_internal(link);
+    uint64_t samples = ff_framequeue_queued_samples(&li->fifo);
     av_assert1(min);
-    return samples >= min || (link->status_in && samples);
+    return samples >= min || (li->status_in && samples);
 }
 
 static void consume_update(AVFilterLink *link, const AVFrame *frame)
@@ -1372,18 +1403,19 @@ static void consume_update(AVFilterLink *link, const AVFrame *frame)
 
 int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
     AVFrame *frame;
 
     *rframe = NULL;
     if (!ff_inlink_check_available_frame(link))
         return 0;
 
-    if (link->fifo.samples_skipped) {
-        frame = ff_framequeue_peek(&link->fifo, 0);
+    if (li->fifo.samples_skipped) {
+        frame = ff_framequeue_peek(&li->fifo, 0);
         return ff_inlink_consume_samples(link, frame->nb_samples, frame->nb_samples, rframe);
     }
 
-    frame = ff_framequeue_take(&link->fifo);
+    frame = ff_framequeue_take(&li->fifo);
     consume_update(link, frame);
     *rframe = frame;
     return 1;
@@ -1392,6 +1424,7 @@ int ff_inlink_consume_frame(AVFilterLink *link, AVFrame **rframe)
 int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
                             AVFrame **rframe)
 {
+    FilterLinkInternal * const li = ff_link_internal(link);
     AVFrame *frame;
     int ret;
 
@@ -1399,9 +1432,9 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
     *rframe = NULL;
     if (!ff_inlink_check_available_samples(link, min))
         return 0;
-    if (link->status_in)
-        min = FFMIN(min, ff_framequeue_queued_samples(&link->fifo));
-    ret = take_samples(link, min, max, &frame);
+    if (li->status_in)
+        min = FFMIN(min, ff_framequeue_queued_samples(&li->fifo));
+    ret = take_samples(li, min, max, &frame);
     if (ret < 0)
         return ret;
     consume_update(link, frame);
@@ -1411,7 +1444,8 @@ int ff_inlink_consume_samples(AVFilterLink *link, unsigned min, unsigned max,
 
 AVFrame *ff_inlink_peek_frame(AVFilterLink *link, size_t idx)
 {
-    return ff_framequeue_peek(&link->fifo, idx);
+    FilterLinkInternal * const li = ff_link_internal(link);
+    return ff_framequeue_peek(&li->fifo, idx);
 }
 
 int ff_inlink_make_frame_writable(AVFilterLink *link, AVFrame **rframe)
@@ -1497,29 +1531,32 @@ void ff_inlink_request_frame(AVFilterLink *link)
 
 void ff_inlink_set_status(AVFilterLink *link, int status)
 {
-    if (link->status_out)
+    FilterLinkInternal * const li = ff_link_internal(link);
+    if (li->status_out)
         return;
     link->frame_wanted_out = 0;
-    link->frame_blocked_in = 0;
+    li->frame_blocked_in = 0;
     ff_avfilter_link_set_out_status(link, status, AV_NOPTS_VALUE);
-    while (ff_framequeue_queued_frames(&link->fifo)) {
-           AVFrame *frame = ff_framequeue_take(&link->fifo);
+    while (ff_framequeue_queued_frames(&li->fifo)) {
+           AVFrame *frame = ff_framequeue_take(&li->fifo);
            av_frame_free(&frame);
     }
-    if (!link->status_in)
-        link->status_in = status;
+    if (!li->status_in)
+        li->status_in = status;
 }
 
 int ff_outlink_get_status(AVFilterLink *link)
 {
-    return link->status_in;
+    FilterLinkInternal * const li = ff_link_internal(link);
+    return li->status_in;
 }
 
 int ff_inoutlink_check_flow(AVFilterLink *inlink, AVFilterLink *outlink)
 {
+    FilterLinkInternal * const li_in = ff_link_internal(inlink);
     return ff_outlink_frame_wanted(outlink) ||
            ff_inlink_check_available_frame(inlink) ||
-           inlink->status_out;
+           li_in->status_out;
 }
 
 
diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
index c2ec7a4b5fc..5ccab4a3f8f 100644
--- a/libavfilter/avfilter.h
+++ b/libavfilter/avfilter.h
@@ -668,51 +668,6 @@ struct AVFilterLink {
      * AVHWFramesContext describing the frames.
      */
     AVBufferRef *hw_frames_ctx;
-
-#ifndef FF_INTERNAL_FIELDS
-
-    /**
-     * Internal structure members.
-     * The fields below this limit are internal for libavfilter's use
-     * and must in no way be accessed by applications.
-     */
-    char reserved[0xF000];
-
-#else /* FF_INTERNAL_FIELDS */
-
-    /**
-     * Queue of frames waiting to be filtered.
-     */
-    FFFrameQueue fifo;
-
-    /**
-     * If set, the source filter can not generate a frame as is.
-     * The goal is to avoid repeatedly calling the request_frame() method on
-     * the same link.
-     */
-    int frame_blocked_in;
-
-    /**
-     * Link input status.
-     * If not zero, all attempts of filter_frame will fail with the
-     * corresponding code.
-     */
-    int status_in;
-
-    /**
-     * Timestamp of the input status change.
-     */
-    int64_t status_in_pts;
-
-    /**
-     * Link output status.
-     * If not zero, all attempts of request_frame will fail with the
-     * corresponding code.
-     */
-    int status_out;
-
-#endif /* FF_INTERNAL_FIELDS */
-
 };
 
 /**
diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
index 53f468494d3..c3280f5f8e7 100644
--- a/libavfilter/avfiltergraph.c
+++ b/libavfilter/avfiltergraph.c
@@ -31,13 +31,13 @@
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 
-#define FF_INTERNAL_FIELDS 1
-#include "framequeue.h"
 
 #include "avfilter.h"
 #include "buffersink.h"
 #include "formats.h"
+#include "framequeue.h"
 #include "internal.h"
+#include "link_internal.h"
 #include "thread.h"
 
 #define OFFSET(x) offsetof(AVFilterGraph, x)
@@ -1326,10 +1326,11 @@ int avfilter_graph_request_oldest(AVFilterGraph *graph)
     av_assert1(oldest->age_index >= 0);
     frame_count = oldest->frame_count_out;
     while (frame_count == oldest->frame_count_out) {
+        FilterLinkInternal * const li = ff_link_internal(oldest);
         r = ff_filter_graph_run_once(graph);
         if (r == AVERROR(EAGAIN) &&
-            !oldest->frame_wanted_out && !oldest->frame_blocked_in &&
-            !oldest->status_in)
+            !oldest->frame_wanted_out && !li->frame_blocked_in &&
+            !li->status_in)
             ff_request_frame(oldest);
         else if (r < 0)
             return r;
diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c
index e269cf72d1b..e102bd22f38 100644
--- a/libavfilter/buffersink.c
+++ b/libavfilter/buffersink.c
@@ -30,14 +30,13 @@
 #include "libavutil/internal.h"
 #include "libavutil/opt.h"
 
-#define FF_INTERNAL_FIELDS 1
-#include "framequeue.h"
-
 #include "audio.h"
 #include "avfilter.h"
 #include "buffersink.h"
 #include "filters.h"
+#include "framequeue.h"
 #include "internal.h"
+#include "link_internal.h"
 
 typedef struct BufferSinkContext {
     const AVClass *class;
@@ -187,9 +186,10 @@ static av_cold int common_init(AVFilterContext *ctx)
 static int activate(AVFilterContext *ctx)
 {
     BufferSinkContext *buf = ctx->priv;
+    FilterLinkInternal * const li = ff_link_internal(ctx->inputs[0]);
 
     if (buf->warning_limit &&
-        ff_framequeue_queued_frames(&ctx->inputs[0]->fifo) >= buf->warning_limit) {
+        ff_framequeue_queued_frames(&li->fifo) >= buf->warning_limit) {
         av_log(ctx, AV_LOG_WARNING,
                "%d buffers queued in %s, something may be wrong.\n",
                buf->warning_limit,
diff --git a/libavfilter/link_internal.h b/libavfilter/link_internal.h
new file mode 100644
index 00000000000..b5a8ac89ec2
--- /dev/null
+++ b/libavfilter/link_internal.h
@@ -0,0 +1,69 @@
+/*
+ * Internal filter link API
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVFILTER_LINK_INTERNAL_H
+#define AVFILTER_LINK_INTERNAL_H
+
+#include <stdint.h>
+
+#include "avfilter.h"
+#include "framequeue.h"
+
+typedef struct FilterLinkInternal {
+    AVFilterLink l;
+
+    /**
+     * Queue of frames waiting to be filtered.
+     */
+    FFFrameQueue fifo;
+
+    /**
+     * If set, the source filter can not generate a frame as is.
+     * The goal is to avoid repeatedly calling the request_frame() method on
+     * the same link.
+     */
+    int frame_blocked_in;
+
+    /**
+     * Link input status.
+     * If not zero, all attempts of filter_frame will fail with the
+     * corresponding code.
+     */
+    int status_in;
+
+    /**
+     * Timestamp of the input status change.
+     */
+    int64_t status_in_pts;
+
+    /**
+     * Link output status.
+     * If not zero, all attempts of request_frame will fail with the
+     * corresponding code.
+     */
+    int status_out;
+} FilterLinkInternal;
+
+static inline FilterLinkInternal *ff_link_internal(AVFilterLink *link)
+{
+    return (FilterLinkInternal*)link;
+}
+
+#endif /* AVFILTER_LINK_INTERNAL_H */
diff --git a/libavfilter/tests/filtfmts.c b/libavfilter/tests/filtfmts.c
index 909c1e8dc96..3451ed891e5 100644
--- a/libavfilter/tests/filtfmts.c
+++ b/libavfilter/tests/filtfmts.c
@@ -25,12 +25,11 @@
 #include "libavutil/pixdesc.h"
 #include "libavutil/samplefmt.h"
 
-#define FF_INTERNAL_FIELDS 1
-#include "libavfilter/framequeue.h"
-
 #include "libavfilter/avfilter.h"
 #include "libavfilter/formats.h"
+#include "libavfilter/framequeue.h"
 #include "libavfilter/internal.h"
+#include "libavfilter/link_internal.h"
 
 static void print_formats_internal(AVFilterLink **links, const AVFilterPad *pads,
                                    unsigned nb, size_t fmts_cfg_offset,
@@ -123,7 +122,7 @@ int main(int argc, char **argv)
 
     /* create a link for each of the input pads */
     for (i = 0; i < filter_ctx->nb_inputs; i++) {
-        AVFilterLink *link = av_mallocz(sizeof(AVFilterLink));
+        AVFilterLink *link = av_mallocz(sizeof(FilterLinkInternal));
         if (!link) {
             fprintf(stderr, "Unable to allocate memory for filter input link\n");
             ret = 1;
@@ -133,7 +132,7 @@ int main(int argc, char **argv)
         filter_ctx->inputs[i] = link;
     }
     for (i = 0; i < filter_ctx->nb_outputs; i++) {
-        AVFilterLink *link = av_mallocz(sizeof(AVFilterLink));
+        AVFilterLink *link = av_mallocz(sizeof(FilterLinkInternal));
         if (!link) {
             fprintf(stderr, "Unable to allocate memory for filter output link\n");
             ret = 1;
-- 
2.35.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Paul B Mahol]

On 1/30/23, Anton Khirnov <anton@khirnov.net> wrote:
> This hack is used to limit the visibility of some AVFilterLink fields to
> only certain files. Replace it with the same pattern that is used e.g.
> in lavf AVStream/FFstream and avoid exposing these internal fields in a
> public header completely.

Looks fine on quick look, but wait for Nicolas comment.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Anton Khirnov (12023-01-30):
> This hack is used to limit the visibility of some AVFilterLink fields to
> only certain files. Replace it with the same pattern that is used e.g.
> in lavf AVStream/FFstream and avoid exposing these internal fields in a
> public header completely.
> ---
>  libavfilter/avfilter.c       | 191 +++++++++++++++++++++--------------
>  libavfilter/avfilter.h       |  45 ---------
>  libavfilter/avfiltergraph.c  |   9 +-
>  libavfilter/buffersink.c     |   8 +-
>  libavfilter/link_internal.h  |  69 +++++++++++++
>  libavfilter/tests/filtfmts.c |   9 +-
>  6 files changed, 196 insertions(+), 135 deletions(-)
>  create mode 100644 libavfilter/link_internal.h

This makes the code more verbose, less readable and harder to maintain,
so no thanks.

If you find a solution that does not require us to remember which field
is private and with field is public to prefix them with link-> or li->,
it would not have this issue; avoiding this requirement was a prime goal
of the current implementation. At least you did not add an indirection
like on some other places.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Anton Khirnov]

Quoting Nicolas George (2023-01-30 13:40:06)
> Anton Khirnov (12023-01-30):
> > This hack is used to limit the visibility of some AVFilterLink fields to
> > only certain files. Replace it with the same pattern that is used e.g.
> > in lavf AVStream/FFstream and avoid exposing these internal fields in a
> > public header completely.
> > ---
> >  libavfilter/avfilter.c       | 191 +++++++++++++++++++++--------------
> >  libavfilter/avfilter.h       |  45 ---------
> >  libavfilter/avfiltergraph.c  |   9 +-
> >  libavfilter/buffersink.c     |   8 +-
> >  libavfilter/link_internal.h  |  69 +++++++++++++
> >  libavfilter/tests/filtfmts.c |   9 +-
> >  6 files changed, 196 insertions(+), 135 deletions(-)
> >  create mode 100644 libavfilter/link_internal.h
> 
> This makes the code more verbose, less readable and harder to maintain,

I find this a significant improvement in code quality, making it easier
to maintain.

> so no thanks.
> 
> If you find a solution that does not require us to remember which field
> is private and with field is public to prefix them with link-> or li->,
> it would not have this issue; avoiding this requirement was a prime goal
> of the current implementation. At least you did not add an indirection
> like on some other places.

Making it obvious which field is private and which is public is a
feature, not a bug.

I'll also note that
- we've been switching to this precise pattern everywhere else in the
  project
- the most prolific lavfi contributor recently (Paul) has no objections
  to this patch
- the second most prolific lavfi contributor recently (Andreas) told me
  on IRC he planned to write this same patch himself

So if you insist on objecting to this patch, it seems to me that a vote
would be in order.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Anton Khirnov (12023-01-31):
> I find this a significant improvement in code quality, making it easier
> to maintain.

You can say that, you do not maintain it.

> Making it obvious which field is private and which is public is a
> feature, not a bug.

Then I do not want this feature. Making people expend effort for no
reason at all.

> I'll also note that
> - we've been switching to this precise pattern everywhere else in the
>   project

Well, too bad.

> - the most prolific lavfi contributor recently (Paul) has no objections
>   to this patch
> - the second most prolific lavfi contributor recently (Andreas) told me
>   on IRC he planned to write this same patch himself

It is not a matter of volume, it is a matter of complexity. Since
Stefano is no longer involved in the coding, I am the only one who knows
how the tricky bits of libavfilter work.

> So if you insist on objecting to this patch, it seems to me that a vote
> would be in order.

Well, go ahead, but please be aware that a vote cannot force me to
maintain that code.

If this project goes over my objections “that patch makes my maintenance
work harder”, then be very careful to involve in your plans “find
somebody else willing to debug the code of libavfilter”. Good luck.

In fact, I think I will take it very easy with maintenance duty until
further notice. I am really fed up with the direction this project is
taking. Authoritarianism, that was the other side of the fork, and we
all know what happened there. And who is to blame.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Paul B Mahol]

On 1/31/23, Nicolas George <george@nsup.org> wrote:
> Anton Khirnov (12023-01-31):
>> I find this a significant improvement in code quality, making it easier
>> to maintain.
>
> You can say that, you do not maintain it.
>
>> Making it obvious which field is private and which is public is a
>> feature, not a bug.
>
> Then I do not want this feature. Making people expend effort for no
> reason at all.
>
>> I'll also note that
>> - we've been switching to this precise pattern everywhere else in the
>>   project
>
> Well, too bad.
>
>> - the most prolific lavfi contributor recently (Paul) has no objections
>>   to this patch
>> - the second most prolific lavfi contributor recently (Andreas) told me
>>   on IRC he planned to write this same patch himself
>
> It is not a matter of volume, it is a matter of complexity. Since
> Stefano is no longer involved in the coding, I am the only one who knows
> how the tricky bits of libavfilter work.

No, you do not. Calling your libavfilter framework code "complex" is disgrace
to really complex code in non-framework part of libavfilter or else in
FFmpeg libraries.

libavfilter framework needs serious overhaul.
It have numerous limitations and other stuff too much exposed to user,
that really should
not be.

You do not maintain code at all, you just block patches and never
contribute anymore anything useful besides blocking patches.

>
>> So if you insist on objecting to this patch, it seems to me that a vote
>> would be in order.
>
> Well, go ahead, but please be aware that a vote cannot force me to
> maintain that code.
>
> If this project goes over my objections “that patch makes my maintenance
> work harder”, then be very careful to involve in your plans “find
> somebody else willing to debug the code of libavfilter”. Good luck.
>
> In fact, I think I will take it very easy with maintenance duty until
> further notice. I am really fed up with the direction this project is
> taking. Authoritarianism, that was the other side of the fork, and we
> all know what happened there. And who is to blame.
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Paul B Mahol (12023-01-31):
> No, you do not. Calling your libavfilter framework code "complex" is disgrace
> to really complex code in non-framework part of libavfilter or else in
> FFmpeg libraries.
> 
> libavfilter framework needs serious overhaul.
> It have numerous limitations and other stuff too much exposed to user,
> that really should not be.
> 
> You do not maintain code at all, you just block patches and never
> contribute anymore anything useful besides blocking patches.

… says the person who does not know that the duration between pts=3 and
pts=5 must be 2.

This is a bad joke.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Paul B Mahol]

On 1/31/23, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12023-01-31):
>> No, you do not. Calling your libavfilter framework code "complex" is
>> disgrace
>> to really complex code in non-framework part of libavfilter or else in
>> FFmpeg libraries.
>>
>> libavfilter framework needs serious overhaul.
>> It have numerous limitations and other stuff too much exposed to user,
>> that really should not be.
>>
>> You do not maintain code at all, you just block patches and never
>> contribute anymore anything useful besides blocking patches.
>
> … says the person who does not know that the duration between pts=3 and
> pts=5 must be 2.
>
> This is a bad joke.

The patch just returned duration of frame from framesync and not
actual duration of frame.
And I have not tested it extensively at all or even thought about it much.

Why? Because I find whole API including libavfilter very silly and
broken and limited.

At least I contributed numerous filters used in production, otherwise
libavfilter would probably not exist today at all in this form at
least.
It would be already rewritten several times and/or whole idea
abandoned completely because of lack of interest to develop it or poor
usage.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Anton Khirnov]

Quoting Nicolas George (2023-01-31 15:03:34)
> Anton Khirnov (12023-01-31):
> > I find this a significant improvement in code quality, making it easier
> > to maintain.
> 
> You can say that, you do not maintain it.

Do you?

You keep implying in your emails that you are the authority on lavfi,
but I see no objective support for this claim. MAINTAINERS only lists
you as the maintainer for graphdump.c and two filters.
And taken e.g. by the number of commits touching libavfilter/, mine is
similar to yours, and both are far behind other people.

> > - the most prolific lavfi contributor recently (Paul) has no objections
> >   to this patch
> > - the second most prolific lavfi contributor recently (Andreas) told me
> >   on IRC he planned to write this same patch himself
> 
> It is not a matter of volume, it is a matter of complexity. Since
> Stefano is no longer involved in the coding, I am the only one who knows
> how the tricky bits of libavfilter work.

Maybe someone should replace those tricky bits with something simpler
then.

> > So if you insist on objecting to this patch, it seems to me that a vote
> > would be in order.
> 
> Well, go ahead, but please be aware that a vote cannot force me to
> maintain that code.
> 
> If this project goes over my objections “that patch makes my maintenance
> work harder”, then be very careful to involve in your plans “find
> somebody else willing to debug the code of libavfilter”. Good luck.
> 
> In fact, I think I will take it very easy with maintenance duty until
> further notice. I am really fed up with the direction this project is
> taking. Authoritarianism, that was the other side of the fork, and we
> all know what happened there. And who is to blame.

Ah yes, preventing one person from enforcing his opinion over those of
multiple other people is "authoritarianism". I don't think that word
means what you think it means.

So take your pick - either stop opposing this patch or we have a vote
over it and see what other people think.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Anton Khirnov (12023-01-31):
> Do you?
> 
> You keep implying in your emails that you are the authority on lavfi,
> but I see no objective support for this claim. MAINTAINERS only lists
> you as the maintainer for graphdump.c and two filters.
> And taken e.g. by the number of commits touching libavfilter/, mine is
> similar to yours, and both are far behind other people.

I know the code, how it works.

I know how the code needs to evolve to achieve new features without
breaking existing.

I review and apply patches to fix bugs when they come; fortunately it
does not happen frequently.

> Maybe someone should replace those tricky bits with something simpler
> then.

Yes, do that, and I will laugh. The code is tricky because the needs are
tricky. “Something simpler” is precisely what came from libav and I
needed to fix a decade ago.

> Ah yes, preventing one person from enforcing his opinion over those of
> multiple other people is "authoritarianism". I don't think that word
> means what you think it means.
> 
> So take your pick - either stop opposing this patch or we have a vote
> over it and see what other people think.

Go ahead, have a vote.

I no longer care. If it goes against me, that will make one less thing
to consider my responsibility.

We should never have asked ourselves “how should we change to entice
forkers back”, we should have asked “how must the forkers change if they
want to be accepted back”. You did not change.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Anton Khirnov]

Quoting Nicolas George (2023-01-31 15:31:44)
> Anton Khirnov (12023-01-31):
> > Do you?
> > 
> > You keep implying in your emails that you are the authority on lavfi,
> > but I see no objective support for this claim. MAINTAINERS only lists
> > you as the maintainer for graphdump.c and two filters.
> > And taken e.g. by the number of commits touching libavfilter/, mine is
> > similar to yours, and both are far behind other people.
> 
> I know the code, how it works.
> 
> I know how the code needs to evolve to achieve new features without
> breaking existing.
> 
> I review and apply patches to fix bugs when they come; fortunately it
> does not happen frequently.

I still see no objective facts supporting your claim of exclusive
maintainership over the entirety of lavfi generic code and public API.
Considering yourself the maintainer in your own head is not enough.

So to avoid any further pointless bickering, I'm hereby asking the TC to
resolve this.

To summarize my view, this patch is an improvement because:
* it prevents filterlink internals from being visible in a
  public header, where they have no business being
* it is a step towards hiding more of lavfi internals from public
  headers
* the same pattern is already and ever more widely used in the other
  libraries and ffmpeg CLI
* it is supported by Andreas (who submitted a more general analogue of
  this patch over a year ago) and Paul
* I am not aware of anyone other than Nicolas being against it

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 1734 bytes --]

Anton Khirnov (12023-01-31):
> I still see no objective facts supporting your claim of exclusive
> maintainership over the entirety of lavfi generic code and public API.

The fact is very simple: I am the only one who understand how this code
works.

> So to avoid any further pointless bickering, I'm hereby asking the TC to
> resolve this.

Just so we are clear: you are a party in this, you can therefore not be
a judge.

> To summarize my view, this patch is an improvement because:
> * it prevents filterlink internals from being visible in a
>   public header, where they have no business being
> * it is a step towards hiding more of lavfi internals from public
>   headers
> * the same pattern is already and ever more widely used in the other
>   libraries and ffmpeg CLI
> * it is supported by Andreas (who submitted a more general analogue of
>   this patch over a year ago) and Paul
> * I am not aware of anyone other than Nicolas being against it

It is a worsening because:

* It requires the developers to remember which field is public and which
  field is private, which is not something relevant here (is is relevant
  elsewhere).

Of course, if you think about it two seconds, you realize it affects the
person who knows the code very well and used/wants to work on it
intensively more than the developers who move from one part of the code
to another and have to re-learn everything. But thinking two seconds how
your changes affects other people does not seem like your forte either.

Therefore, I add this point:

* If this change is applied, FFmpeg needs to find somebody else to
  maintain the core of libavfilter. (And I predict you will not.)

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Nicolas George (12023-01-31):
> > * it prevents filterlink internals from being visible in a
> >   public header, where they have no business being
> > * it is a step towards hiding more of lavfi internals from public
> >   headers
> > * the same pattern is already and ever more widely used in the other

Note to the TC who will decide: I do not oppose the efforts mentioned in
these two points (that are actually the same points twice), I only
oppose this particular solution because of this drawback:

> * It requires the developers to remember which field is public and which
>   field is private, which is not something relevant here (is is relevant
>   elsewhere).

Without looking very far, I can think of several different ways of
hiding the internal fields better without requiring changes to the
implementation. I would not oppose such a change.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Andreas Rheinhardt]

Nicolas George:
> Nicolas George (12023-01-31):
>>> * it prevents filterlink internals from being visible in a
>>>   public header, where they have no business being
>>> * it is a step towards hiding more of lavfi internals from public
>>>   headers
>>> * the same pattern is already and ever more widely used in the other
> 
> Note to the TC who will decide: I do not oppose the efforts mentioned in
> these two points (that are actually the same points twice), I only
> oppose this particular solution because of this drawback:
> 
>> * It requires the developers to remember which field is public and which
>>   field is private, which is not something relevant here (is is relevant
>>   elsewhere).
> 
> Without looking very far, I can think of several different ways of
> hiding the internal fields better without requiring changes to the
> implementation. I would not oppose such a change.
> 

Details please. I can only think of the following:
a) Allow the use of -fms-extensions. This allows structs with tags and
typedefs thereof to be used as unnamed struct members with the members
of the unnamed structure being treated as members of the enclosing
structure. One could then use a pointer to the big internal structure
internally and one does not need to remember whether a field is internal
or not. There is still a problem, though: One needs to cast from
AVFilterContext.(inputs|outputs). This should be done via dedicated
inlined getters, but this is a bit more typing. E.g.
"input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it
might also be shorter if someone has a short name.
GCC, Clang, MSVC and the IIRC the intel compilers support this.

b) Add a big #define AVFILTERLINK in avfilter.h that expands to the
public part of AVFilterLink and change the declaration of AVFilterLink
to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal
struct via
"struct FilterLinkInternal {
    AVFILTERLINK
    /* actual internal fields */
};"
This has the downside of actually being an aliasing violation and of
adding considerable ugliness to avfilter.h, in particular during
deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via
#if in the implementation of a macro). I also don't know whether it
plays nicely with tools that deal with source code annotations.
c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally
using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter).
d) Same as c), but strip this stuff from installed headers.

I consider b)-d) as inferior to a), which I consider superior to Anton's
proposal, but the big drawback is its reliance on a compiler extension.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 3000 bytes --]

Andreas Rheinhardt (12023-01-31):
> Details please.

I was not going to spend time explaining unless I had assurance it was
because the issue of requiring changes in the internal implementation
and developers habits just to hide some fields was taken into account.
Coming from somebody else, I would have expected the question to be only
to find the first excuses to shoot the proposal down.

But you more or less found the same ideas I did anyway.

> I can only think of the following:
> a) Allow the use of -fms-extensions. This allows structs with tags and
> typedefs thereof to be used as unnamed struct members with the members
> of the unnamed structure being treated as members of the enclosing
> structure. One could then use a pointer to the big internal structure
> internally and one does not need to remember whether a field is internal
> or not. There is still a problem, though: One needs to cast from
> AVFilterContext.(inputs|outputs). This should be done via dedicated
> inlined getters, but this is a bit more typing. E.g.
> "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it
> might also be shorter if someone has a short name.
> GCC, Clang, MSVC and the IIRC the intel compilers support this.
> 
> b) Add a big #define AVFILTERLINK in avfilter.h that expands to the
> public part of AVFilterLink and change the declaration of AVFilterLink
> to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal
> struct via
> "struct FilterLinkInternal {
>     AVFILTERLINK
>     /* actual internal fields */
> };"
> This has the downside of actually being an aliasing violation and of
> adding considerable ugliness to avfilter.h, in particular during
> deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via
> #if in the implementation of a macro). I also don't know whether it
> plays nicely with tools that deal with source code annotations.

Or use a “#include "avfilter_link_internal_fields.h" instead of a macro.

> c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally
> using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter).
> d) Same as c), but strip this stuff from installed headers.
> 
> I consider b)-d) as inferior to a), which I consider superior to Anton's
> proposal, but the big drawback is its reliance on a compiler extension.

Once and for all: If the solution requires changing things in the
internal implementation C files and changing the habits of the people
who work on these files (that goes together), then it is inferior to a
solution that does not require that.

Your solution (a) has this flaw, plus relies on an extension that is
probably not available on all supported platforms (Microsoft I am
looking at you).

My favor goes to (d). Probably with a stricter test for the (c) part
because the internal fields are not necessary for the filters and
therefore could be hidden there too.

Regards,

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12023-01-31):
>> Details please.
> 
> I was not going to spend time explaining unless I had assurance it was
> because the issue of requiring changes in the internal implementation
> and developers habits just to hide some fields was taken into account.
> Coming from somebody else, I would have expected the question to be only
> to find the first excuses to shoot the proposal down.
> 
> But you more or less found the same ideas I did anyway.
> 
>> I can only think of the following:
>> a) Allow the use of -fms-extensions. This allows structs with tags and
>> typedefs thereof to be used as unnamed struct members with the members
>> of the unnamed structure being treated as members of the enclosing
>> structure. One could then use a pointer to the big internal structure
>> internally and one does not need to remember whether a field is internal
>> or not. There is still a problem, though: One needs to cast from
>> AVFilterContext.(inputs|outputs). This should be done via dedicated
>> inlined getters, but this is a bit more typing. E.g.
>> "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it
>> might also be shorter if someone has a short name.
>> GCC, Clang, MSVC and the IIRC the intel compilers support this.
>>
>> b) Add a big #define AVFILTERLINK in avfilter.h that expands to the
>> public part of AVFilterLink and change the declaration of AVFilterLink
>> to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal
>> struct via
>> "struct FilterLinkInternal {
>>     AVFILTERLINK
>>     /* actual internal fields */
>> };"
>> This has the downside of actually being an aliasing violation and of
>> adding considerable ugliness to avfilter.h, in particular during
>> deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via
>> #if in the implementation of a macro). I also don't know whether it
>> plays nicely with tools that deal with source code annotations.
> 
> Or use a “#include "avfilter_link_internal_fields.h" instead of a macro.
> 
>> c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally
>> using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter).
>> d) Same as c), but strip this stuff from installed headers.
>>
>> I consider b)-d) as inferior to a), which I consider superior to Anton's
>> proposal, but the big drawback is its reliance on a compiler extension.
> 
> Once and for all: If the solution requires changing things in the
> internal implementation C files and changing the habits of the people
> who work on these files (that goes together), then it is inferior to a
> solution that does not require that.
> 
> Your solution (a) has this flaw, plus relies on an extension that is
> probably not available on all supported platforms (Microsoft I am
> looking at you).
> 

It's called -fms-extensions for a reason.

> My favor goes to (d). Probably with a stricter test for the (c) part
> because the internal fields are not necessary for the filters and
> therefore could be hidden there too.
> 

This can be accomplished with a), too. All one has to do is use multiple
levels of extensions.

- Andreas

PS: Upon rethinking, it is not only b) that contains undefined
behaviour; it is b)-d) as well as the current code. The reason is that
the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is
not compatible with the type of AVFilterLink from the files without
FF_INTERNAL_FIELDS. This also makes derived types, like the types of
function declarations containing pointers to AVFilterLink incompatible
and this is a violation of 6.2.7(2) of C11. I wonder if this will become
a real problem with lto someday.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Andreas Rheinhardt (12023-02-01):
> PS: Upon rethinking, it is not only b) that contains undefined
> behaviour; it is b)-d) as well as the current code. The reason is that
> the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is
> not compatible with the type of AVFilterLink from the files without
> FF_INTERNAL_FIELDS. This also makes derived types, like the types of
> function declarations containing pointers to AVFilterLink incompatible
> and this is a violation of 6.2.7(2) of C11. I wonder if this will become
> a real problem with lto someday.

No: read the second half of the previous paragraph: two structures with
common first fields are compatible types. What we have been using is a
deliberately supported construct.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12023-02-01):
>> PS: Upon rethinking, it is not only b) that contains undefined
>> behaviour; it is b)-d) as well as the current code. The reason is that
>> the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is
>> not compatible with the type of AVFilterLink from the files without
>> FF_INTERNAL_FIELDS. This also makes derived types, like the types of
>> function declarations containing pointers to AVFilterLink incompatible
>> and this is a violation of 6.2.7(2) of C11. I wonder if this will become
>> a real problem with lto someday.
> 
> No: read the second half of the previous paragraph: two structures with
> common first fields are compatible types. What we have been using is a
> deliberately supported construct.
> 

"Moreover, two structure, union, or enumerated types declared in
separate translation units are compatible if their tags and members
satisfy the following requirements: If one is declared with a tag, the
other shall be declared with the same tag. If both are completed
anywhere within their respective translation units, then the following
additional requirements apply: there shall be a one-to-one
correspondence between their members such that each pair of
corresponding members are declared with compatible types; if one member
of the pair is declared with an alignment specifier, the other is
declared with an equivalent alignment specifier; and if one member of
the pair is declared with a name, the other is declared with the same
name. For two structures, corresponding members shall be declared in the
same order."

1. There is no one-to-one correspondence (aka a bijection) between the
elements of the complete and of the public structure.
2. In the case of AVFilterLink, there is not even an injection from the
public to the FF_INTERNAL_FIELDS structure (due to the former having a
big reserved array).
3. The fact that these structures share a common initial sequence does
not mean that this is legal. There are some guarantees regarding common
initial sequences in 6.5.2.3 (6):

"One special guarantee is made in order to simplify the use of unions:
if a union contains several structures that share a common initial
sequence (see below), and if the union object currently contains one of
these structures, it is permitted to inspect the common initial part of
any of them anywhere that a declaration of the completed type of the
union is visible. Two structures share a common initial sequence if
corresponding members have compatible types (and, for bit-fields, the
same widths) for a sequence of one or more initial members."

But there just is no union between the FF_INTERNAL_FIELDS
!defined(FF_INTERNAL_FIELDS) structures in the whole codebase.
Furthermore, said guarantee is only for inspecting, i.e. reading. For
example, for the following two structs sharing a common initial sequence,

struct Small {
    uint64_t foo;
    uint32_t bar;
};
struct Big {
    uint64_t foo;
    uint32_t bar;
    uint32_t baz;
};

if one had a union { struct Small; struct Big; }, a write to Small.bar
may clobber Big.baz.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George]

Andreas Rheinhardt (12023-02-01):
> "One special guarantee is made in order to simplify the use of unions:
> if a union contains several structures that share a common initial
> sequence (see below), and if the union object currently contains one of
> these structures, it is permitted to inspect the common initial part of
> any of them anywhere that a declaration of the completed type of the
> union is visible. Two structures share a common initial sequence if
> corresponding members have compatible types (and, for bit-fields, the
> same widths) for a sequence of one or more initial members."
> 
> But there just is no union between the FF_INTERNAL_FIELDS
> !defined(FF_INTERNAL_FIELDS) structures in the whole codebase.

It is not necessary that there is one: it is enough that there could be
one in another compilation unit, the code that handles these structures
does not know what exists in the rest of the code base. This guarantee
was made FOR unions, but it does not REQUIRE unions, it applies to
anything that is semantically equivalent to a union.

> Furthermore, said guarantee is only for inspecting, i.e. reading. For
> example, for the following two structs sharing a common initial sequence,
> 
> struct Small {
>     uint64_t foo;
>     uint32_t bar;
> };
> struct Big {
>     uint64_t foo;
>     uint32_t bar;
>     uint32_t baz;
> };
> 
> if one had a union { struct Small; struct Big; }, a write to Small.bar
> may clobber Big.baz.

That is a good point. It does not apply when the first field of Big is
Small itself, which is the case that I absolutely know is supported,
because in that case Big will embed the padding of Small. I had not
thought of this.

Fortunately, even if we are not 100% in the case that is officially
supported, there are multiple reasons that each should be enough to
guarantee that our code will work:

- Between the public part of the structure and the #ifede
  FF_INTERNAL_FIELDS, there are a lot of "not part of the public API"
  fields", changing ch_layout will not clobber incfg because incfg is
  known at this point.

- Even if there was no fields in between, reserved[] offers the same
  protection.

- And even if there was no gap, we are good because applications, and
  even filters, are not supposed to modify AVFilterLink, only the
  framework and the framework knows the whole structure.

In fact, that last remark makes me think of another solution, for the
slightly longer term: make AVFilterLink entirely opaque. The only
documented reason for application to access AVFilterLink was to get the
parameters of buffersink, but buffersink has a real API to get this
since 2016.

Anyway, if you prefer using a compilation option, I have no objection.
My only concern is that when a developer writes:

	if (link->x == ... &&
	    link->y == ... &&
	    link->status_in == ... &&
	    link->min_samples == ..)

they have to remember that no, status_in is different from all the
others.

And it is especially bad in this particular case because the distinction
is not public / private, which makes some semantic sense and might be
easier to remember, the distinction is actually
public-or-private-because-of-a-comment / private-because-of-a-ifdef.

But even if the distinction really was public / private, I consider it
unacceptable if we can do otherwise. And we can.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Uoti Urpala]

On Fri, 2023-02-03 at 15:45 +0100, Nicolas George wrote:
> Andreas Rheinhardt (12023-02-01):
> > "One special guarantee is made in order to simplify the use of unions:
> > if a union contains several structures that share a common initial
> > sequence (see below), and if the union object currently contains one of
> > these structures, it is permitted to inspect the common initial part of
> > any of them anywhere that a declaration of the completed type of the
> > union is visible. Two structures share a common initial sequence if
> > corresponding members have compatible types (and, for bit-fields, the
> > same widths) for a sequence of one or more initial members."
> > 
> > But there just is no union between the FF_INTERNAL_FIELDS
> > !defined(FF_INTERNAL_FIELDS) structures in the whole codebase.
> 
> It is not necessary that there is one: it is enough that there could be
> one in another compilation unit, the code that handles these structures
> does not know what exists in the rest of the code base. This guarantee
> was made FOR unions, but it does not REQUIRE unions, it applies to
> anything that is semantically equivalent to a union.

This is not valid reasoning. Consider code such as

int f(struct s1 *p1, struct s2 *p2) {
    p1->abc = 0;
    p2->abc = 42;
    return p1->abc;
}

where the field abc belongs to a shared initial sequence in s1 and s2.
If there is no common union visible when this code is compiled, it
seems valid for the compiler to hardcode this to return 0, even if it
may be called with p1 and p2 pointing to the same address. LTO may
generate the equivalent of this code when compiling a combination of
assignments and reads from files using different definitions for a
struct.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12023-02-01):
>> "One special guarantee is made in order to simplify the use of unions:
>> if a union contains several structures that share a common initial
>> sequence (see below), and if the union object currently contains one of
>> these structures, it is permitted to inspect the common initial part of
>> any of them anywhere that a declaration of the completed type of the
>> union is visible. Two structures share a common initial sequence if
>> corresponding members have compatible types (and, for bit-fields, the
>> same widths) for a sequence of one or more initial members."
>>
>> But there just is no union between the FF_INTERNAL_FIELDS
>> !defined(FF_INTERNAL_FIELDS) structures in the whole codebase.
> 
> It is not necessary that there is one: it is enough that there could be
> one in another compilation unit, the code that handles these structures
> does not know what exists in the rest of the code base. This guarantee
> was made FOR unions, but it does not REQUIRE unions, it applies to
> anything that is semantically equivalent to a union.
> 

1. The common initial sequence rule indeed forced traditional compilers
to use the same offsets for the members of the common initial sequence
of two arbitrary structs, because there might be a union of these two in
another translation unit. But this is no longer true for lto compilers
that see the whole program at once (although I don't think that they
will deviate from the behaviour of traditional compilers in this regard).
2. But even if the offsets are fine, does not mean that the accesses are
fine. Notice the clause "anywhere that a declaration of the completed
type of the union is visible" above? It is accompanied with the
following example:

"The following is not a valid fragment (because the union type is not
visible within function f):
struct t1 { int m; };
struct t2 { int m; };
int f(struct t1 *p1, struct t2 *p2)
{
    if (p1->m < 0)
        p2->m = -p2->m;
    return p1->m;
}
int g()
{
    union {
        struct t1 s1;
        struct t2 s2;
    } u;
    /* ... */
    return f(&u.s1, &u.s2);
}"


>> Furthermore, said guarantee is only for inspecting, i.e. reading. For
>> example, for the following two structs sharing a common initial sequence,
>>
>> struct Small {
>>     uint64_t foo;
>>     uint32_t bar;
>> };
>> struct Big {
>>     uint64_t foo;
>>     uint32_t bar;
>>     uint32_t baz;
>> };
>>
>> if one had a union { struct Small; struct Big; }, a write to Small.bar
>> may clobber Big.baz.
> 
> That is a good point. It does not apply when the first field of Big is
> Small itself, which is the case that I absolutely know is supported,
> because in that case Big will embed the padding of Small. I had not
> thought of this.
> 
> Fortunately, even if we are not 100% in the case that is officially
> supported, there are multiple reasons that each should be enough to
> guarantee that our code will work:
> 

I'd rather have something that is actually supported and spec-compliant.
Anyway, I worry more about lto-compilers than about traditional
compilers accidentally clobbering something (after all, most of our
fields are sizeof(int)-sized or a multiple (double) thereof, so CPU
instruction sets can write this natively and there is no advantage in
touching padding).

> - Between the public part of the structure and the #ifede
>   FF_INTERNAL_FIELDS, there are a lot of "not part of the public API"
>   fields", changing ch_layout will not clobber incfg because incfg is
>   known at this point.
> 
> - Even if there was no fields in between, reserved[] offers the same
>   protection.
> 
> - And even if there was no gap, we are good because applications, and
>   even filters, are not supposed to modify AVFilterLink, only the
>   framework and the framework knows the whole structure.
> 
> In fact, that last remark makes me think of another solution, for the
> slightly longer term: make AVFilterLink entirely opaque. The only
> documented reason for application to access AVFilterLink was to get the
> parameters of buffersink, but buffersink has a real API to get this
> since 2016.
> 
> Anyway, if you prefer using a compilation option, I have no objection.
> My only concern is that when a developer writes:
> 
> 	if (link->x == ... &&
> 	    link->y == ... &&
> 	    link->status_in == ... &&
> 	    link->min_samples == ..)
> 
> they have to remember that no, status_in is different from all the
> others.
> 
> And it is especially bad in this particular case because the distinction
> is not public / private, which makes some semantic sense and might be
> easier to remember, the distinction is actually
> public-or-private-because-of-a-comment / private-because-of-a-ifdef.
> 
> But even if the distinction really was public / private, I consider it
> unacceptable if we can do otherwise. And we can.
> 
> Regards,
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".