diff mbox series

[FFmpeg-devel,1/6] avfilter/af_aderivative: Don't use audiobuf for only 1 sample, fix leak

Message ID AM7PR03MB6660B6F9915F847B3F2B0F288FEE9@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/6] avfilter/af_aderivative: Don't use audiobuf for only 1 sample, fix leak | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 1, 2021, 9:13 p.m. UTC 
Up until now, the aderivative and aintegrate filters allocated
an audio buffer with exactly one sample per channel via
ff_get_audio_buffer(); if said buffer could not be allocated,
a frame would leak, as freeing it has been forgotten.

This commit instead uses a plain array for the audio buffer; it is
allocated when configuring the filter, removing the error condition
where the frame can leak. This fixes Coverity ticket #1197065.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/af_aderivative.c | 40 ++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

Comments

Paul B Mahol Aug. 1, 2021, 9:17 p.m. UTC | #1 
This patch is too much intrusive to be acceptable.

Isn't there simpler solution?
Andreas Rheinhardt Aug. 1, 2021, 9:47 p.m. UTC | #2 
Paul B Mahol:
> This patch is too much intrusive to be acceptable.
> 
> Isn't there simpler solution?
> 
Intrusive? You call that intrusive?
Anyway, here is a way to make it even less intrusive: Use "type *prv =
&p[c]" and don't modify the inner loop at all (and don't update prev
after the loop). If one used "type *prv = ((type*)p) + c", one could
additionally avoid having to add the p0 variable.
But I actually dislike this one-sample pseudo-array and therefore
intentionally switched away from it.

- Andreas
Paul B Mahol Aug. 2, 2021, 6:50 a.m. UTC | #3 
I do not like that, I prefer AVFrame to keep it as is currently.

frame allocation can be moved elsewhere.
diff mbox series

Patch

diff --git a/libavfilter/af_aderivative.c b/libavfilter/af_aderivative.c
index 6b3e4dd0e4..559223d5ee 100644
--- a/libavfilter/af_aderivative.c
+++ b/libavfilter/af_aderivative.c
@@ -22,8 +22,8 @@ 
 
 typedef struct ADerivativeContext {
     const AVClass *class;
-    AVFrame *prev;
-    void (*filter)(void **dst, void **prv, const void **src,
+    void *prev;
+    void (*filter)(void **dst, void *prv, const void **src,
                    int nb_samples, int channels);
 } ADerivativeContext;
 
@@ -63,22 +63,24 @@  static int query_formats(AVFilterContext *ctx)
 }
 
 #define DERIVATIVE(name, type)                                          \
-static void aderivative_## name ##p(void **d, void **p, const void **s, \
+static void aderivative_## name ##p(void **d, void *p0, const void **s, \
                                     int nb_samples, int channels)       \
 {                                                                       \
+    type *const p = p0;                                                 \
     int n, c;                                                           \
                                                                         \
     for (c = 0; c < channels; c++) {                                    \
         const type *src = s[c];                                         \
         type *dst = d[c];                                               \
-        type *prv = p[c];                                               \
+        type  prv = p[c];                                               \
                                                                         \
         for (n = 0; n < nb_samples; n++) {                              \
             const type current = src[n];                                \
                                                                         \
-            dst[n] = current - prv[0];                                  \
-            prv[0] = current;                                           \
+            dst[n] = current - prv;                                     \
+            prv    = current;                                           \
         }                                                               \
+        p[c] = prv;                                                     \
     }                                                                   \
 }
 
@@ -88,22 +90,24 @@  DERIVATIVE(s16, int16_t)
 DERIVATIVE(s32, int32_t)
 
 #define INTEGRAL(name, type)                                          \
-static void aintegral_## name ##p(void **d, void **p, const void **s, \
+static void aintegral_## name ##p(void **d, void *p0, const void **s, \
                                   int nb_samples, int channels)       \
 {                                                                     \
+    type *const p = p0;                                               \
     int n, c;                                                         \
                                                                       \
     for (c = 0; c < channels; c++) {                                  \
         const type *src = s[c];                                       \
         type *dst = d[c];                                             \
-        type *prv = p[c];                                             \
+        type  prv = p[c];                                             \
                                                                       \
         for (n = 0; n < nb_samples; n++) {                            \
             const type current = src[n];                              \
                                                                       \
-            dst[n] = current + prv[0];                                \
-            prv[0] = dst[n];                                          \
+            dst[n] = current + prv;                                   \
+            prv    = dst[n];                                          \
         }                                                             \
+        p[c] = prv;                                                   \
     }                                                                 \
 }
 
@@ -122,6 +126,10 @@  static int config_input(AVFilterLink *inlink)
     case AV_SAMPLE_FMT_S16P: s->filter = aderivative_s16p; break;
     }
 
+    s->prev = av_calloc(inlink->channels, av_get_bytes_per_sample(inlink->format));
+    if (!s->prev)
+        return AVERROR(ENOMEM);
+
     if (strcmp(ctx->filter->name, "aintegral"))
         return 0;
 
@@ -146,15 +154,7 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *in)
     }
     av_frame_copy_props(out, in);
 
-    if (!s->prev) {
-        s->prev = ff_get_audio_buffer(inlink, 1);
-        if (!s->prev) {
-            av_frame_free(&in);
-            return AVERROR(ENOMEM);
-        }
-    }
-
-    s->filter((void **)out->extended_data, (void **)s->prev->extended_data, (const void **)in->extended_data,
+    s->filter((void **)out->extended_data, s->prev, (const void **)in->extended_data,
               in->nb_samples, in->channels);
 
     av_frame_free(&in);
@@ -165,7 +165,7 @@  static av_cold void uninit(AVFilterContext *ctx)
 {
     ADerivativeContext *s = ctx->priv;
 
-    av_frame_free(&s->prev);
+    av_freep(&s->prev);
 }
 
 static const AVFilterPad aderivative_inputs[] = {