[FFmpeg-devel] [PATCH] lavfi/gradfun: fix direct writing in buffer.

[Clément Bœsch]

On Sun, Dec 02, 2012 at 11:48:03AM +0100, Nicolas George wrote:
> Le duodi 12 frimaire, an CCXXI, Clément Bœsch a écrit :
> >  - first, the use of AV_PERM_PRESERVE looks inappropriate, so we can remove it.
> 
> I confirm.
> 
> >  - then it seems assumed that gradfun can do filtering in-place (see the direct
> >    flag). Though, that direct flag isn't passed to the filtering functions, and
> 
> The direct flag is there to check whether the input buffer must be freed.
> 
> >    the filter doesn't seem to assume/support dst=src, so we need to get a new
> >    temporary buffer (allocated with ff_get_video_buffer) all the time.
> 
> If the filter did not work when direct is set, I believe we would probably
> have noticed it. Right now, a quick glance seems to show that it does work.
> 

In that case, we should be able to do something like this:

diff --git a/libavfilter/vf_gradfun.c b/libavfilter/vf_gradfun.c
index 260a44b..0483b6c 100644
--- a/libavfilter/vf_gradfun.c
+++ b/libavfilter/vf_gradfun.c
@@ -186,23 +186,8 @@ static int filter_frame(AVFilterLink *inlink, AVFilterBufferRef *in)
 {
     GradFunContext *gf = inlink->dst->priv;
     AVFilterLink *outlink = inlink->dst->outputs[0];
-    AVFilterBufferRef *out;
-    int p, direct = 0;
-
-    if ((in->perms & AV_PERM_WRITE) && !(in->perms & AV_PERM_PRESERVE)) {
-        direct = 1;
-        out = in;
-    } else {
-        out = ff_get_video_buffer(outlink, AV_PERM_WRITE, outlink->w, outlink->h);
-        if (!out) {
-            avfilter_unref_bufferp(&in);
-            return AVERROR(ENOMEM);
-        }
-
-        avfilter_copy_buffer_ref_props(out, in);
-        out->video->w = outlink->w;
-        out->video->h = outlink->h;
-    }
+    AVFilterBufferRef *out = in;
+    int p;
 
     for (p = 0; p < 4 && in->data[p]; p++) {
         int w = inlink->w;
@@ -216,13 +201,8 @@ static int filter_frame(AVFilterLink *inlink, AVFilterBufferRef *in)
 
         if (FFMIN(w, h) > 2 * r)
             filter(gf, out->data[p], in->data[p], w, h, out->linesize[p], in->linesize[p], r);
-        else if (out->data[p] != in->data[p])
-            av_image_copy_plane(out->data[p], out->linesize[p], in->data[p], in->linesize[p], w, h);
     }
 
-    if (!direct)
-        avfilter_unref_bufferp(&in);
-
     return ff_filter_frame(outlink, out);
 }
 
@@ -232,7 +212,7 @@ static const AVFilterPad avfilter_vf_gradfun_inputs[] = {
         .type         = AVMEDIA_TYPE_VIDEO,
         .config_props = config_input,
         .filter_frame = filter_frame,
-        .min_perms    = AV_PERM_READ,
+        .min_perms    = AV_PERM_READ | AV_PERM_WRITE,
     },
     { NULL }
 };
diff --git a/libavfilter/vf_hqdn3d.c b/libavfilter/vf_hqdn3d.c
index 939a31d..0b7e33c 100644
--- a/libavfilter/vf_hqdn3d.c
+++ b/libavfilter/vf_hqdn3d.c
@@ -325,24 +325,8 @@ static int filter_frame(AVFilterLink *inlink, AVFilterBufferRef *in)
 {
     HQDN3DContext *hqdn3d = inlink->dst->priv;
     AVFilterLink *outlink = inlink->dst->outputs[0];
-
-    AVFilterBufferRef *out;
-    int direct = 0, c;
-
-    if (in->perms & AV_PERM_WRITE) {
-        direct = 1;
-        out = in;
-    } else {
-        out = ff_get_video_buffer(outlink, AV_PERM_WRITE, outlink->w, outlink->h);
-        if (!out) {
-            avfilter_unref_bufferp(&in);
-            return AVERROR(ENOMEM);
-        }
-
-        avfilter_copy_buffer_ref_props(out, in);
-        out->video->w = outlink->w;
-        out->video->h = outlink->h;
-    }
+    int c;
+    AVFilterBufferRef *out = in;
 
     for (c = 0; c < 3; c++) {
         denoise(hqdn3d, in->data[c], out->data[c],
@@ -353,9 +337,6 @@ static int filter_frame(AVFilterLink *inlink, AVFilterBufferRef *in)
                 hqdn3d->coefs[c?2:0], hqdn3d->coefs[c?3:1]);
     }
 
-    if (!direct)
-        avfilter_unref_bufferp(&in);
-
     return ff_filter_frame(outlink, out);
 }
 
@@ -365,6 +346,7 @@ static const AVFilterPad avfilter_vf_hqdn3d_inputs[] = {
         .type         = AVMEDIA_TYPE_VIDEO,
         .config_props = config_input,
         .filter_frame = filter_frame,
+        .min_perms    = AV_PERM_READ | AV_PERM_WRITE,
     },
     { NULL }
 };

The (untested) code will now assume src = dst all the time, and we can
maybe do some more simplification. The min_perms is here to make sure that
a new buffer is allocated when the input is read-only (that was done
manually previously).

currently:
  if inbuf is RO:
    - filter requests a new write buffer
    - processing with src ≠ dst
  else if inbuf is RW:
    - processing with src = dst

after:
  if inbuf is RO:
    - lavfi requests a new write buffer so a new RW inbuf is passed
  - processing with src = dst

Does that make any sense?

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121202/146d2cea/attachment.asc>