diff mbox

[FFmpeg-devel,v3] libavfilter/vf_find_rect: convert the object image to gray8 format instead of failed directly

Message ID 1559948038-47078-1-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show

Commit Message

Lance Wang June 7, 2019, 10:53 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vf_find_rect.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

Comments

Michael Niedermayer June 8, 2019, 8:38 p.m. UTC | #1 
On Sat, Jun 08, 2019 at 06:53:58AM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavfilter/vf_find_rect.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> index d7e6579..6fe12fe 100644
> --- a/libavfilter/vf_find_rect.c
> +++ b/libavfilter/vf_find_rect.c
> @@ -28,6 +28,7 @@
>  #include "internal.h"
>  
>  #include "lavfutils.h"
> +#include "lswsutils.h"
>  
>  #define MAX_MIPMAPS 5
>  
> @@ -235,8 +236,6 @@ static av_cold void uninit(AVFilterContext *ctx)
>          av_frame_free(&foc->haystack_frame[i]);
>      }
>  
> -    if (foc->obj_frame)
> -        av_freep(&foc->obj_frame->data[0]);
>      av_frame_free(&foc->obj_frame);
>  }
>  
> @@ -244,6 +243,9 @@ static av_cold int init(AVFilterContext *ctx)
>  {
>      FOCContext *foc = ctx->priv;
>      int ret, i;
> +    uint8_t *tmp_data[4];
> +    int tmp_linesize[4], width, height;
> +    enum AVPixelFormat pix_fmt;
>  
>      if (!foc->obj_filename) {
>          av_log(ctx, AV_LOG_ERROR, "object filename not set\n");
> @@ -254,24 +256,36 @@ static av_cold int init(AVFilterContext *ctx)
>      if (!foc->obj_frame)
>          return AVERROR(ENOMEM);
>  
> -    if ((ret = ff_load_image(foc->obj_frame->data, foc->obj_frame->linesize,
> -                             &foc->obj_frame->width, &foc->obj_frame->height,
> -                             &foc->obj_frame->format, foc->obj_filename, ctx)) < 0)
> -        return ret;
> -
> -    if (foc->obj_frame->format != AV_PIX_FMT_GRAY8) {
> -        av_log(ctx, AV_LOG_ERROR, "object image is not a grayscale image\n");
> -        return AVERROR(EINVAL);
> -    }
> +    if ((ret = ff_load_image(&tmp_data, tmp_linesize,
> +                             &width, &height,
> +                             &pix_fmt, foc->obj_filename, ctx)) < 0)

libavfilter/vf_find_rect.c: In function ‘init’:
libavfilter/vf_find_rect.c:261:30: warning: passing argument 1 of ‘ff_load_image’ from incompatible pointer type [enabled by default]


[...]
Michael Niedermayer June 8, 2019, 9:41 p.m. UTC | #2 
On Sat, Jun 08, 2019 at 06:53:58AM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavfilter/vf_find_rect.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> index d7e6579..6fe12fe 100644
> --- a/libavfilter/vf_find_rect.c
> +++ b/libavfilter/vf_find_rect.c
> @@ -28,6 +28,7 @@
>  #include "internal.h"
>  
>  #include "lavfutils.h"
> +#include "lswsutils.h"
>  
>  #define MAX_MIPMAPS 5
>  
> @@ -235,8 +236,6 @@ static av_cold void uninit(AVFilterContext *ctx)
>          av_frame_free(&foc->haystack_frame[i]);
>      }
>  
> -    if (foc->obj_frame)
> -        av_freep(&foc->obj_frame->data[0]);
>      av_frame_free(&foc->obj_frame);
>  }

this alone will leak

something like:
foc->obj_frame->buf[0] =
        av_buffer_create(foc->obj_frame->data[0],
                         foc->obj_frame->linesize[0] * foc->obj_frame->height,
                         av_buffer_default_free,
                         NULL,
                         0);

would be needed, but maybe theres a cleaner way to do this
like having a function that does all this in one step and produces a normal
AVFrame. I think more filters could benefit from this if theres nothing
like it yet ...

[...]
Lance Wang June 8, 2019, 10:44 p.m. UTC | #3 
On Sun, Jun 9, 2019 at 5:41 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sat, Jun 08, 2019 at 06:53:58AM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavfilter/vf_find_rect.c | 40 +++++++++++++++++++++++++++-------------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> > index d7e6579..6fe12fe 100644
> > --- a/libavfilter/vf_find_rect.c
> > +++ b/libavfilter/vf_find_rect.c
> > @@ -28,6 +28,7 @@
> >  #include "internal.h"
> >
> >  #include "lavfutils.h"
> > +#include "lswsutils.h"
> >
> >  #define MAX_MIPMAPS 5
> >
> > @@ -235,8 +236,6 @@ static av_cold void uninit(AVFilterContext *ctx)
> >          av_frame_free(&foc->haystack_frame[i]);
> >      }
> >
> > -    if (foc->obj_frame)
> > -        av_freep(&foc->obj_frame->data[0]);
> >      av_frame_free(&foc->obj_frame);
> >  }
>
> this alone will leak
>
> something like:
> foc->obj_frame->buf[0] =
>         av_buffer_create(foc->obj_frame->data[0],
>                          foc->obj_frame->linesize[0] *
> foc->obj_frame->height,
>                          av_buffer_default_free,
>                          NULL,
>                          0);
>
> would be needed, but maybe theres a cleaner way to do this
> like having a function that does all this in one step and produces a normal
> AVFrame. I think more filters could benefit from this if theres nothing
> like it yet ...
>
>
I misunderstand by another code review and think the buffer is freed by
av_free_frame. I'll revert it back in the update patch.



> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Its not that you shouldnt use gotos but rather that you should write
> readable code and code with gotos often but not always is less readable
> _______________________________________________
> 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".
Lance Wang June 8, 2019, 10:47 p.m. UTC | #4 
On Sun, Jun 9, 2019 at 4:38 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sat, Jun 08, 2019 at 06:53:58AM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavfilter/vf_find_rect.c | 40 +++++++++++++++++++++++++++-------------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> > index d7e6579..6fe12fe 100644
> > --- a/libavfilter/vf_find_rect.c
> > +++ b/libavfilter/vf_find_rect.c
> > @@ -28,6 +28,7 @@
> >  #include "internal.h"
> >
> >  #include "lavfutils.h"
> > +#include "lswsutils.h"
> >
> >  #define MAX_MIPMAPS 5
> >
> > @@ -235,8 +236,6 @@ static av_cold void uninit(AVFilterContext *ctx)
> >          av_frame_free(&foc->haystack_frame[i]);
> >      }
> >
> > -    if (foc->obj_frame)
> > -        av_freep(&foc->obj_frame->data[0]);
> >      av_frame_free(&foc->obj_frame);
> >  }
> >
> > @@ -244,6 +243,9 @@ static av_cold int init(AVFilterContext *ctx)
> >  {
> >      FOCContext *foc = ctx->priv;
> >      int ret, i;
> > +    uint8_t *tmp_data[4];
> > +    int tmp_linesize[4], width, height;
> > +    enum AVPixelFormat pix_fmt;
> >
> >      if (!foc->obj_filename) {
> >          av_log(ctx, AV_LOG_ERROR, "object filename not set\n");
> > @@ -254,24 +256,36 @@ static av_cold int init(AVFilterContext *ctx)
> >      if (!foc->obj_frame)
> >          return AVERROR(ENOMEM);
> >
> > -    if ((ret = ff_load_image(foc->obj_frame->data,
> foc->obj_frame->linesize,
> > -                             &foc->obj_frame->width,
> &foc->obj_frame->height,
> > -                             &foc->obj_frame->format,
> foc->obj_filename, ctx)) < 0)
> > -        return ret;
> > -
> > -    if (foc->obj_frame->format != AV_PIX_FMT_GRAY8) {
> > -        av_log(ctx, AV_LOG_ERROR, "object image is not a grayscale
> image\n");
> > -        return AVERROR(EINVAL);
> > -    }
> > +    if ((ret = ff_load_image(&tmp_data, tmp_linesize,
> > +                             &width, &height,
> > +                             &pix_fmt, foc->obj_filename, ctx)) < 0)
>
> libavfilter/vf_find_rect.c: In function ‘init’:
> libavfilter/vf_find_rect.c:261:30: warning: passing argument 1 of
> ‘ff_load_image’ from incompatible pointer type [enabled by default]
>
>
>
Should be fixed in the update v4 version patch. Have tested with expected
function with below command:
./ffmpeg -i ./input.ts -vf
find_rect=./logo.jpg,cover_rect=./cover.jpg:mode=cover output.ts



> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> During times of universal deceit, telling the truth becomes a
> revolutionary act. -- George Orwell
> _______________________________________________
> 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".
diff mbox

Patch

diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
index d7e6579..6fe12fe 100644
--- a/libavfilter/vf_find_rect.c
+++ b/libavfilter/vf_find_rect.c
@@ -28,6 +28,7 @@ 
 #include "internal.h"
 
 #include "lavfutils.h"
+#include "lswsutils.h"
 
 #define MAX_MIPMAPS 5
 
@@ -235,8 +236,6 @@  static av_cold void uninit(AVFilterContext *ctx)
         av_frame_free(&foc->haystack_frame[i]);
     }
 
-    if (foc->obj_frame)
-        av_freep(&foc->obj_frame->data[0]);
     av_frame_free(&foc->obj_frame);
 }
 
@@ -244,6 +243,9 @@  static av_cold int init(AVFilterContext *ctx)
 {
     FOCContext *foc = ctx->priv;
     int ret, i;
+    uint8_t *tmp_data[4];
+    int tmp_linesize[4], width, height;
+    enum AVPixelFormat pix_fmt;
 
     if (!foc->obj_filename) {
         av_log(ctx, AV_LOG_ERROR, "object filename not set\n");
@@ -254,24 +256,36 @@  static av_cold int init(AVFilterContext *ctx)
     if (!foc->obj_frame)
         return AVERROR(ENOMEM);
 
-    if ((ret = ff_load_image(foc->obj_frame->data, foc->obj_frame->linesize,
-                             &foc->obj_frame->width, &foc->obj_frame->height,
-                             &foc->obj_frame->format, foc->obj_filename, ctx)) < 0)
-        return ret;
-
-    if (foc->obj_frame->format != AV_PIX_FMT_GRAY8) {
-        av_log(ctx, AV_LOG_ERROR, "object image is not a grayscale image\n");
-        return AVERROR(EINVAL);
-    }
+    if ((ret = ff_load_image(&tmp_data, tmp_linesize,
+                             &width, &height,
+                             &pix_fmt, foc->obj_filename, ctx)) < 0)
+        goto error;
+
+    /* convert object image to gray8 format with same width and height */
+    foc->obj_frame->format = AV_PIX_FMT_GRAY8;
+    foc->obj_frame->width  = width;
+    foc->obj_frame->height = height;
+    if ((ret = ff_scale_image(foc->obj_frame->data, foc->obj_frame->linesize,
+                    foc->obj_frame->width, foc->obj_frame->height, foc->obj_frame->format,
+                    tmp_data, tmp_linesize, width, height, pix_fmt, ctx)) < 0)
+        goto error;
+    av_freep(&tmp_data[0]);
 
     foc->needle_frame[0] = av_frame_clone(foc->obj_frame);
     for (i = 1; i < foc->mipmaps; i++) {
         foc->needle_frame[i] = downscale(foc->needle_frame[i-1]);
-        if (!foc->needle_frame[i])
-            return AVERROR(ENOMEM);
+        if (!foc->needle_frame[i]) {
+            ret = AVERROR(ENOMEM);
+            goto error;
+        }
     }
 
     return 0;
+error:
+    if (tmp_data)
+        av_freep(&tmp_data[0]);
+    av_frame_free(&foc->obj_frame);
+    return ret;
 }
 
 static const AVFilterPad foc_inputs[] = {