[FFmpeg-devel] [PATCH] libavfilter/thumbnail filter, copy buffers instead of reference

[Chris Kennedy]

I forgot the copy of the original buffer, oddly if I copy it then causes
things to crash at the end when the codec is freed, only for certain .avi
mpeg4 and in the mpeg4 codec where it frees some data there.  I am digging
in to that, but patch at least shows deeper into something odd going on.  I
can just copy the R and G but not B and it doesn't hit the issue for this
test video.  Seems really strange, before it seemed to be having backtraces
in ffmpeg.c and buffer freeing conflicting or some problem, now when
copying the buffers it is mpeg4 codec freeing and only if reading that blue
value into the destination buffer, odd.

diff --git a/libavfilter/vf_thumbnail.c b/libavfilter/vf_thumbnail.c
index 186ac8d..925eaaf 100644
--- a/libavfilter/vf_thumbnail.c
+++ b/libavfilter/vf_thumbnail.c
@@ -137,10 +137,19 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
*frame)
     int *hist = thumb->frames[thumb->n].histogram;
     const uint8_t *p = frame->data[0];

+    // Copy orginal frame into buffer(s)
+    thumb->frames[thumb->n].buf = ff_get_video_buffer(outlink, outlink->w,
outlink->h);
+    if (!thumb->frames[thumb->n].buf) {
+        av_frame_free(&frame);
+        return AVERROR(ENOMEM);
+    }
+    av_frame_copy_props(thumb->frames[thumb->n].buf, frame);
+    if (av_frame_copy(thumb->frames[thumb->n].buf, frame))
+        return AVERROR(ENOMEM);
+
     // update current frame RGB histogram
     for (j = 0; j < inlink->h; j++) {
@@ -148,14 +157,6 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
*frame)
         p += frame->linesize[0];
     }

-    // Copy orginal frame into buffer(s)
-    thumb->frames[thumb->n].buf = ff_get_video_buffer(inlink, inlink->w,
inlink->h);
-    if (!thumb->frames[thumb->n].buf) {
-        av_frame_free(&frame);
-        return AVERROR(ENOMEM);
-    }
-    av_frame_copy_props(thumb->frames[thumb->n].buf, frame);
-
     // Free original frame we have copied
     av_frame_free(&frame);


On Wed, Feb 25, 2015 at 3:40 PM, Chris Kennedy <bitbytebit at gmail.com> wrote:

> This fixes the buffer handling in the thumbnail filter, it seems that the
> referencing buffers it does isn't safe (could be hundreds of them held in
> ref) and the main ffmpeg buffer handling can fail.
>
> It now just grabs the video buffers and copies them, frees the frame
> immediately, and works just the same now, and doesn't use 1.7 Gig of memory
> in my tests with 300 buffers, it uses around 100 meg max now.  There was
> obviously something nasty in how it was doing this.
>
>
>
> diff --git a/libavfilter/vf_thumbnail.c b/libavfilter/vf_thumbnail.c
> index 1883154..82b8ebe 100644
> --- a/libavfilter/vf_thumbnail.c
> +++ b/libavfilter/vf_thumbnail.c
> @@ -137,9 +137,6 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *frame)
>      int *hist = thumb->frames[thumb->n].histogram;
>      const uint8_t *p = frame->data[0];
>
> -    // keep a reference of each frame
> -    thumb->frames[thumb->n].buf = frame;
> -
>      // update current frame RGB histogram
>      for (j = 0; j < inlink->h; j++) {
>          for (i = 0; i < inlink->w; i++) {
> @@ -150,6 +147,17 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *frame)
>          p += frame->linesize[0];
>      }
>
> +    // Copy orginal frame into buffer(s)
> +    thumb->frames[thumb->n].buf = ff_get_video_buffer(inlink, inlink->w,
> inlink->h);
> +    if (!thumb->frames[thumb->n].buf) {
> +        av_frame_free(&frame);
> +        return AVERROR(ENOMEM);
> +    }
> +    av_frame_copy_props(thumb->frames[thumb->n].buf, frame);
> +
> +    // Free original frame we have copied
> +    av_frame_free(&frame);
> +
>      // no selection until the buffer of N frames is filled up
>      thumb->n++;
>      if (thumb->n < thumb->n_frames)
>
>
> I previously sent a patch which was wrong, and just avoided the issue for
> certain media but hit it on others, this should fix it for good now (since
> it also doesn't alter the chosen thumbnail that results)...
>
>
> Thanks,
> Chris
> ---
> Chris Kennedy
> Video Engineer
> CrunchyRoll - http://www.crunchyroll.com
>
>
>
> --
> ---
> Chris Kennedy
>



-- 
---
Chris Kennedy