[FFmpeg-devel] [PATCH 5/7] avfilter/formats: Factor checking for mergeability out of ff_merge_*
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Sat Aug 15 08:48:14 EEST 2020
The callers of the ff_merge_*() functions fall into two categories with
quite different needs:
One caller is can_merge_formats() which only wants to test for mergeability
without it merging anything. In order to do so, it duplicates the lists
it intends to test and resets their owners so that they are not modified
by ff_merge_*(). It also means that it needs to receive the merged list
(and not only an int containing whether the lists are mergeable) to
properly free it.
The other callers want the lists to be actually merged. But given the
fact that ff_merge_*() automatically updates the owners of the lists,
they only want the information whether the merge succeeded or not; they
don't want a link to the new list.
Therefore this commit splits these functions in two: ff_merge_*() for
the latter callers and ff_can_merge_*() for the former.
ff_merge_*() doesn't need to return a pointer to the combined list at all
and hence these functions have been modified to return an int, which
allows to distinguish between incompability and memory allocation failures.
ff_can_merge_*() meanwhile doesn't modify its arguments at all obviating
the need for copies. This in turn implies that there is no reason to
return a pointer to the new list, as nothing needs to be freed. These
functions therefore return an int as well. This allowed to completely
remove can_merge_formats() in avfiltergraph.c.
Notice that no ff_can_merge_channel_layouts() has been created, because
there is currently no caller for this. It could be added if needed.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
If neither a nor b had references (which happens iff the call came from
can_merge_refs()), the earlier code would allocate a references array
for zero elements which in turn leads to an allocation of one byte.
This commit avoids said allocation completely.
This led to thousands of failing FATE-tests when it was tried in [1]
to return NULL when the allocation of zero bytes is requested.
(The nut muxer also tries to allocate an array of zero elements (for its
chapters) and it is the next biggest source of such allocations.)
[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260430.html
libavfilter/avfiltergraph.c | 129 ++++++++++--------------------------
libavfilter/formats.c | 91 +++++++++++++++++--------
libavfilter/formats.h | 44 ++++++------
3 files changed, 120 insertions(+), 144 deletions(-)
diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
index 681c519ef0..eebaa4c358 100644
--- a/libavfilter/avfiltergraph.c
+++ b/libavfilter/avfiltergraph.c
@@ -372,61 +372,6 @@ static int formats_declared(AVFilterContext *f)
return 1;
}
-static AVFilterFormats *clone_filter_formats(AVFilterFormats *arg)
-{
- AVFilterFormats *a = av_memdup(arg, sizeof(*arg));
- if (a) {
- a->refcount = 0;
- a->refs = NULL;
- a->formats = av_memdup(a->formats, sizeof(*a->formats) * a->nb_formats);
- if (!a->formats && arg->formats)
- av_freep(&a);
- }
- return a;
-}
-
-static int can_merge_formats(AVFilterFormats *a_arg,
- AVFilterFormats *b_arg,
- enum AVMediaType type,
- int is_sample_rate)
-{
- AVFilterFormats *a, *b, *ret;
- if (a_arg == b_arg)
- return 1;
- a = clone_filter_formats(a_arg);
- b = clone_filter_formats(b_arg);
-
- if (!a || !b) {
- if (a)
- av_freep(&a->formats);
- if (b)
- av_freep(&b->formats);
-
- av_freep(&a);
- av_freep(&b);
-
- return 0;
- }
-
- if (is_sample_rate) {
- ret = ff_merge_samplerates(a, b);
- } else {
- ret = ff_merge_formats(a, b, type);
- }
- if (ret) {
- av_freep(&ret->formats);
- av_freep(&ret->refs);
- av_freep(&ret);
- return 1;
- } else {
- av_freep(&a->formats);
- av_freep(&b->formats);
- av_freep(&a);
- av_freep(&b);
- return 0;
- }
-}
-
/**
* Perform one round of query_formats() and merging formats lists on the
* filter graph.
@@ -473,45 +418,40 @@ static int query_formats(AVFilterGraph *graph, AVClass *log_ctx)
if (link->in_formats != link->out_formats
&& link->in_formats && link->out_formats)
- if (!can_merge_formats(link->in_formats, link->out_formats,
- link->type, 0))
+ if (!ff_can_merge_formats(link->in_formats, link->out_formats,
+ link->type))
convert_needed = 1;
if (link->type == AVMEDIA_TYPE_AUDIO) {
if (link->in_samplerates != link->out_samplerates
&& link->in_samplerates && link->out_samplerates)
- if (!can_merge_formats(link->in_samplerates,
- link->out_samplerates,
- 0, 1))
+ if (!ff_can_merge_samplerates(link->in_samplerates,
+ link->out_samplerates))
convert_needed = 1;
}
-#define MERGE_DISPATCH(field, statement) \
+#define CHECKED_MERGE(field, ...) ((ret = ff_merge_ ## field(__VA_ARGS__)) <= 0)
+#define MERGE_DISPATCH(field, ...) \
if (!(link->in_ ## field && link->out_ ## field)) { \
count_delayed++; \
} else if (link->in_ ## field == link->out_ ## field) { \
count_already_merged++; \
} else if (!convert_needed) { \
count_merged++; \
- statement \
+ if (CHECKED_MERGE(field, __VA_ARGS__)) { \
+ if (ret < 0) \
+ return ret; \
+ convert_needed = 1; \
+ } \
}
if (link->type == AVMEDIA_TYPE_AUDIO) {
- MERGE_DISPATCH(channel_layouts,
- if (!ff_merge_channel_layouts(link->in_channel_layouts,
- link->out_channel_layouts))
- convert_needed = 1;
- )
- MERGE_DISPATCH(samplerates,
- if (!ff_merge_samplerates(link->in_samplerates,
- link->out_samplerates))
- convert_needed = 1;
- )
+ MERGE_DISPATCH(channel_layouts, link->in_channel_layouts,
+ link->out_channel_layouts)
+ MERGE_DISPATCH(samplerates, link->in_samplerates,
+ link->out_samplerates)
}
- MERGE_DISPATCH(formats,
- if (!ff_merge_formats(link->in_formats, link->out_formats,
- link->type))
- convert_needed = 1;
- )
+ MERGE_DISPATCH(formats, link->in_formats,
+ link->out_formats, link->type)
#undef MERGE_DISPATCH
if (convert_needed) {
@@ -585,27 +525,26 @@ static int query_formats(AVFilterGraph *graph, AVClass *log_ctx)
av_assert0(outlink-> in_channel_layouts->refcount > 0);
av_assert0(outlink->out_channel_layouts->refcount > 0);
}
- if (!ff_merge_formats( inlink->in_formats, inlink->out_formats, inlink->type) ||
- !ff_merge_formats(outlink->in_formats, outlink->out_formats, outlink->type))
- ret = AVERROR(ENOSYS);
- if (inlink->type == AVMEDIA_TYPE_AUDIO &&
- (!ff_merge_samplerates(inlink->in_samplerates,
- inlink->out_samplerates) ||
- !ff_merge_channel_layouts(inlink->in_channel_layouts,
- inlink->out_channel_layouts)))
- ret = AVERROR(ENOSYS);
- if (outlink->type == AVMEDIA_TYPE_AUDIO &&
- (!ff_merge_samplerates(outlink->in_samplerates,
- outlink->out_samplerates) ||
- !ff_merge_channel_layouts(outlink->in_channel_layouts,
- outlink->out_channel_layouts)))
- ret = AVERROR(ENOSYS);
-
- if (ret < 0) {
+ if (CHECKED_MERGE(formats, inlink->in_formats,
+ inlink->out_formats, inlink->type) ||
+ CHECKED_MERGE(formats, outlink->in_formats,
+ outlink->out_formats, outlink->type) ||
+ inlink->type == AVMEDIA_TYPE_AUDIO &&
+ (CHECKED_MERGE(samplerates, inlink->in_samplerates,
+ inlink->out_samplerates) ||
+ CHECKED_MERGE(channel_layouts, inlink->in_channel_layouts,
+ inlink->out_channel_layouts)) ||
+ outlink->type == AVMEDIA_TYPE_AUDIO &&
+ (CHECKED_MERGE(samplerates, outlink->in_samplerates,
+ outlink->out_samplerates) ||
+ CHECKED_MERGE(channel_layouts, outlink->in_channel_layouts,
+ outlink->out_channel_layouts))) {
+ if (ret < 0)
+ return ret;
av_log(log_ctx, AV_LOG_ERROR,
"Impossible to convert between the formats supported by the filter "
"'%s' and the filter '%s'\n", link->src->name, link->dst->name);
- return ret;
+ return AVERROR(ENOSYS);
}
}
}
diff --git a/libavfilter/formats.c b/libavfilter/formats.c
index e8a43a434d..9788357952 100644
--- a/libavfilter/formats.c
+++ b/libavfilter/formats.c
@@ -56,16 +56,20 @@ do { \
/**
* Add all formats common to a and b to a, add b's refs to a and destroy b.
+ * If check is set, nothing is modified and it is only checked whether
+ * the formats are compatible.
* If empty_allowed is set and one of a,b->nb is zero, the lists are
* merged; otherwise, it is treated as error.
*/
-#define MERGE_FORMATS(a, b, fmts, nb, type, fail_statement, empty_allowed) \
+#define MERGE_FORMATS(a, b, fmts, nb, type, check, empty_allowed) \
do { \
int i, j, k = 0; \
void *tmp; \
\
if (empty_allowed) { \
if (!a->nb || !b->nb) { \
+ if (check) \
+ return 1; \
if (!a->nb) \
FFSWAP(type *, a, b); \
goto merge_ref; \
@@ -74,31 +78,34 @@ do {
for (i = 0; i < a->nb; i++) \
for (j = 0; j < b->nb; j++) \
if (a->fmts[i] == b->fmts[j]) { \
+ if (check) \
+ return 1; \
a->fmts[k++] = a->fmts[i]; \
break; \
} \
/* Check that there was at least one common format. \
* Notice that both a and b are unchanged if not. */ \
if (!k) \
- { fail_statement } \
+ return 0; \
+ av_assert2(!check); \
a->nb = k; \
tmp = av_realloc_array(a->fmts, a->nb, sizeof(*a->fmts)); \
if (tmp) \
a->fmts = tmp; \
\
merge_ref: \
- MERGE_REF(a, b, fmts, type, fail_statement); \
+ MERGE_REF(a, b, fmts, type, return AVERROR(ENOMEM);); \
} while (0)
-AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
- enum AVMediaType type)
+static int merge_formats_internal(AVFilterFormats *a, AVFilterFormats *b,
+ enum AVMediaType type, int check)
{
int i, j;
int alpha1=0, alpha2=0;
int chroma1=0, chroma2=0;
if (a == b)
- return a;
+ return 1;
/* Do not lose chroma or alpha in merging.
It happens if both lists have formats with chroma (resp. alpha), but
@@ -122,31 +129,58 @@ AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
// If chroma or alpha can be lost through merging then do not merge
if (alpha2 > alpha1 || chroma2 > chroma1)
- return NULL;
+ return 0;
+
+ MERGE_FORMATS(a, b, formats, nb_formats, AVFilterFormats, check, 0);
+
+ return 1;
+}
- MERGE_FORMATS(a, b, formats, nb_formats, AVFilterFormats, return NULL;, 0);
+int ff_can_merge_formats(const AVFilterFormats *a, const AVFilterFormats *b,
+ enum AVMediaType type)
+{
+ return merge_formats_internal((AVFilterFormats *)a,
+ (AVFilterFormats *)b, type, 1);
+}
- return a;
+int ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
+ enum AVMediaType type)
+{
+ av_assert2(a->refcount && b->refcount);
+ return merge_formats_internal(a, b, type, 0);
}
-AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a,
- AVFilterFormats *b)
+static int merge_samplerates_internal(AVFilterFormats *a,
+ AVFilterFormats *b, int check)
{
- if (a == b) return a;
+ if (a == b) return 1;
- MERGE_FORMATS(a, b, formats, nb_formats, AVFilterFormats, return NULL;, 1);
- return a;
+ MERGE_FORMATS(a, b, formats, nb_formats, AVFilterFormats, check, 1);
+ return 1;
}
-AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
- AVFilterChannelLayouts *b)
+int ff_can_merge_samplerates(const AVFilterFormats *a, const AVFilterFormats *b)
+{
+ return merge_samplerates_internal((AVFilterFormats *)a, (AVFilterFormats *)b, 1);
+}
+
+int ff_merge_samplerates(AVFilterFormats *a, AVFilterFormats *b)
+{
+ av_assert2(a->refcount && b->refcount);
+ return merge_samplerates_internal(a, b, 0);
+}
+
+int ff_merge_channel_layouts(AVFilterChannelLayouts *a,
+ AVFilterChannelLayouts *b)
{
uint64_t *channel_layouts;
unsigned a_all = a->all_layouts + a->all_counts;
unsigned b_all = b->all_layouts + b->all_counts;
int ret_max, ret_nb = 0, i, j, round;
- if (a == b) return a;
+ av_assert2(a->refcount && b->refcount);
+
+ if (a == b) return 1;
/* Put the most generic set in a, to avoid doing everything twice */
if (a_all < b_all) {
@@ -162,16 +196,16 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
/* Not optimal: the unknown layouts of b may become known after
another merge. */
if (!j)
- return NULL;
+ return 0;
b->nb_channel_layouts = j;
}
- MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, return NULL;);
- return b;
+ MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, return AVERROR(ENOMEM););
+ return 1;
}
ret_max = a->nb_channel_layouts + b->nb_channel_layouts;
if (!(channel_layouts = av_malloc_array(ret_max, sizeof(*channel_layouts))))
- return NULL;
+ return AVERROR(ENOMEM);
/* a[known] intersect b[known] */
for (i = 0; i < a->nb_channel_layouts; i++) {
@@ -209,21 +243,20 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
channel_layouts[ret_nb++] = a->channel_layouts[i];
}
- if (!ret_nb)
- goto fail;
+ if (!ret_nb) {
+ av_free(channel_layouts);
+ return 0;
+ }
if (a->refcount > b->refcount)
FFSWAP(AVFilterChannelLayouts *, a, b);
- MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, goto fail;);
+ MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts,
+ { av_free(channel_layouts); return AVERROR(ENOMEM); });
av_freep(&b->channel_layouts);
b->channel_layouts = channel_layouts;
b->nb_channel_layouts = ret_nb;
- return b;
-
-fail:
- av_free(channel_layouts);
- return NULL;
+ return 1;
}
int ff_fmt_is_in(int fmt, const int *fmts)
diff --git a/libavfilter/formats.h b/libavfilter/formats.h
index ffe7a12d53..dd0cbca6d5 100644
--- a/libavfilter/formats.h
+++ b/libavfilter/formats.h
@@ -110,17 +110,32 @@ typedef struct AVFilterChannelLayouts {
(int)((l) & 0x7FFFFFFF) : 0)
/**
- * Return a channel layouts/samplerates list which contains the intersection of
- * the layouts/samplerates of a and b. Also, all the references of a, all the
- * references of b, and a and b themselves will be deallocated.
+ * Check the formats/samplerates lists for compatibility for merging
+ * without actually merging.
*
- * If a and b do not share any common elements, neither is modified, and NULL
- * is returned.
+ * @return 1 if they are compatible, 0 if not.
*/
-AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
- AVFilterChannelLayouts *b);
-AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a,
- AVFilterFormats *b);
+int ff_can_merge_formats(const AVFilterFormats *a, const AVFilterFormats *b,
+ enum AVMediaType type);
+int ff_can_merge_samplerates(const AVFilterFormats *a, const AVFilterFormats *b);
+
+/**
+ * Merge the formats/channel layouts/samplerates lists if they are compatible
+ * and update all the references of a and b to point to the combined list and
+ * free the old lists as needed. The combined list usually contains the
+ * intersection of the lists of a and b.
+ *
+ * Both a and b must have owners (i.e. refcount > 0) for these functions.
+ *
+ * @return 1 if merging succeeded, 0 if a and b are incompatible
+ * and negative AVERROR code on failure.
+ * a and b are unmodified if 0 is returned.
+ */
+int ff_merge_channel_layouts(AVFilterChannelLayouts *a,
+ AVFilterChannelLayouts *b);
+int ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
+ enum AVMediaType type);
+int ff_merge_samplerates(AVFilterFormats *a, AVFilterFormats *b);
/**
* Construct an empty AVFilterChannelLayouts/AVFilterFormats struct --
@@ -238,17 +253,6 @@ int ff_formats_pixdesc_filter(AVFilterFormats **rfmts, unsigned want, unsigned r
av_warn_unused_result
AVFilterFormats *ff_planar_sample_fmts(void);
-/**
- * Return a format list which contains the intersection of the formats of
- * a and b. Also, all the references of a, all the references of b, and
- * a and b themselves will be deallocated.
- *
- * If a and b do not share any common formats, neither is modified, and NULL
- * is returned.
- */
-AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
- enum AVMediaType type);
-
/**
* Add *ref as a new reference to formats.
* That is the pointers will point like in the ascii art below:
--
2.20.1
More information about the ffmpeg-devel
mailing list