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

Submitted by lance.lmwang@gmail.com on June 12, 2019, 10:57 a.m.

Details

Message ID 20190612105731.43622-1-lance.lmwang@gmail.com
State New
Headers show

Commit Message

lance.lmwang@gmail.com June 12, 2019, 10:57 a.m.
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/filters.texi           |  2 +-
 libavfilter/vf_find_rect.c | 39 +++++++++++++++++++++++++++-----------
 2 files changed, 29 insertions(+), 12 deletions(-)

Comments

Michael Niedermayer June 14, 2019, 7:15 p.m.
On Wed, Jun 12, 2019 at 06:57:29PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  doc/filters.texi           |  2 +-
>  libavfilter/vf_find_rect.c | 39 +++++++++++++++++++++++++++-----------
>  2 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index ec1c7c7591..90c57430a6 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -10150,7 +10150,7 @@ It accepts the following options:
>  
>  @table @option
>  @item object
> -Filepath of the object image, needs to be in gray8.
> +Filepath of the object image.
>  
>  @item threshold
>  Detection threshold, default is 0.5.
> diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> index d7e6579af7..ee6c3f4b45 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
>  
> @@ -244,6 +245,9 @@ static av_cold int init(AVFilterContext *ctx)
>  {
>      FOCContext *foc = ctx->priv;
>      int ret, i;
> +    uint8_t *tmp_data[4] = { NULL };
> +    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 +258,37 @@ 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]);

Iam not sure i understand what this patch is intending to do

Before a grayscale image was needed, after the patch anything is accepted but
only grayscale is used. This is quite unexpected for a user who might expect
color to be used if color is accpeted.
Generally, doing something that is unexpected by the user is not good

[...]
lance.lmwang@gmail.com June 14, 2019, 10:53 p.m.
On Sat, Jun 15, 2019 at 3:16 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Jun 12, 2019 at 06:57:29PM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  doc/filters.texi           |  2 +-
> >  libavfilter/vf_find_rect.c | 39 +++++++++++++++++++++++++++-----------
> >  2 files changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index ec1c7c7591..90c57430a6 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -10150,7 +10150,7 @@ It accepts the following options:
> >
> >  @table @option
> >  @item object
> > -Filepath of the object image, needs to be in gray8.
> > +Filepath of the object image.
> >
> >  @item threshold
> >  Detection threshold, default is 0.5.
> > diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> > index d7e6579af7..ee6c3f4b45 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
> >
> > @@ -244,6 +245,9 @@ static av_cold int init(AVFilterContext *ctx)
> >  {
> >      FOCContext *foc = ctx->priv;
> >      int ret, i;
> > +    uint8_t *tmp_data[4] = { NULL };
> > +    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 +258,37 @@ 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]);
>
> Iam not sure i understand what this patch is intending to do
>
> Before a grayscale image was needed, after the patch anything is accepted
> but
> only grayscale is used. This is quite unexpected for a user who might
> expect
> color to be used if color is accpeted.
> Generally, doing something that is unexpected by the user is not good
>
> The goal is make the common user to use it more friendly.  When I'm do the
testing,  I have got some object
file isn't gray image,  I had to use ffmpeg convert it to gray every time.
although I don't care for only gray color is used
clearly.

How about to add description to explain we use gray color only for the
compare now.


> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are best at talking, realize last or never when they are wrong.
> _______________________________________________
> 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".
Michael Niedermayer June 15, 2019, 10:16 p.m.
On Sat, Jun 15, 2019 at 06:53:20AM +0800, Lance Wang wrote:
> On Sat, Jun 15, 2019 at 3:16 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Jun 12, 2019 at 06:57:29PM +0800, lance.lmwang@gmail.com wrote:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  doc/filters.texi           |  2 +-
> > >  libavfilter/vf_find_rect.c | 39 +++++++++++++++++++++++++++-----------
> > >  2 files changed, 29 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/doc/filters.texi b/doc/filters.texi
> > > index ec1c7c7591..90c57430a6 100644
> > > --- a/doc/filters.texi
> > > +++ b/doc/filters.texi
> > > @@ -10150,7 +10150,7 @@ It accepts the following options:
> > >
> > >  @table @option
> > >  @item object
> > > -Filepath of the object image, needs to be in gray8.
> > > +Filepath of the object image.
> > >
> > >  @item threshold
> > >  Detection threshold, default is 0.5.
> > > diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> > > index d7e6579af7..ee6c3f4b45 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
> > >
> > > @@ -244,6 +245,9 @@ static av_cold int init(AVFilterContext *ctx)
> > >  {
> > >      FOCContext *foc = ctx->priv;
> > >      int ret, i;
> > > +    uint8_t *tmp_data[4] = { NULL };
> > > +    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 +258,37 @@ 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]);
> >
> > Iam not sure i understand what this patch is intending to do
> >
> > Before a grayscale image was needed, after the patch anything is accepted
> > but
> > only grayscale is used. This is quite unexpected for a user who might
> > expect
> > color to be used if color is accpeted.
> > Generally, doing something that is unexpected by the user is not good
> >
> > The goal is make the common user to use it more friendly.  When I'm do the
> testing,  I have got some object
> file isn't gray image,  I had to use ffmpeg convert it to gray every time.
> although I don't care for only gray color is used
> clearly.
> 

> How about to add description to explain we use gray color only for the
> compare now.

not sure its best but its better, yes

thx

[...]
lance.lmwang@gmail.com June 15, 2019, 11:06 p.m.
On Sun, Jun 16, 2019 at 6:16 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sat, Jun 15, 2019 at 06:53:20AM +0800, Lance Wang wrote:
> > On Sat, Jun 15, 2019 at 3:16 AM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > On Wed, Jun 12, 2019 at 06:57:29PM +0800, lance.lmwang@gmail.com
> wrote:
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > >
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  doc/filters.texi           |  2 +-
> > > >  libavfilter/vf_find_rect.c | 39
> +++++++++++++++++++++++++++-----------
> > > >  2 files changed, 29 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/doc/filters.texi b/doc/filters.texi
> > > > index ec1c7c7591..90c57430a6 100644
> > > > --- a/doc/filters.texi
> > > > +++ b/doc/filters.texi
> > > > @@ -10150,7 +10150,7 @@ It accepts the following options:
> > > >
> > > >  @table @option
> > > >  @item object
> > > > -Filepath of the object image, needs to be in gray8.
> > > > +Filepath of the object image.
> > > >
> > > >  @item threshold
> > > >  Detection threshold, default is 0.5.
> > > > diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
> > > > index d7e6579af7..ee6c3f4b45 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
> > > >
> > > > @@ -244,6 +245,9 @@ static av_cold int init(AVFilterContext *ctx)
> > > >  {
> > > >      FOCContext *foc = ctx->priv;
> > > >      int ret, i;
> > > > +    uint8_t *tmp_data[4] = { NULL };
> > > > +    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 +258,37 @@ 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]);
> > >
> > > Iam not sure i understand what this patch is intending to do
> > >
> > > Before a grayscale image was needed, after the patch anything is
> accepted
> > > but
> > > only grayscale is used. This is quite unexpected for a user who might
> > > expect
> > > color to be used if color is accpeted.
> > > Generally, doing something that is unexpected by the user is not good
> > >
> > > The goal is make the common user to use it more friendly.  When I'm do
> the
> > testing,  I have got some object
> > file isn't gray image,  I had to use ffmpeg convert it to gray every
> time.
> > although I don't care for only gray color is used
> > clearly.
> >
>
> > How about to add description to explain we use gray color only for the
> > compare now.
>
> not sure its best but its better, yes
>
> OK, I'll add one description in the filters.texi.



> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> It is dangerous to be right in matters on which the established authorities
> are wrong. -- Voltaire
> _______________________________________________
> 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".

Patch hide | download patch | download mbox

diff --git a/doc/filters.texi b/doc/filters.texi
index ec1c7c7591..90c57430a6 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -10150,7 +10150,7 @@  It accepts the following options:
 
 @table @option
 @item object
-Filepath of the object image, needs to be in gray8.
+Filepath of the object image.
 
 @item threshold
 Detection threshold, default is 0.5.
diff --git a/libavfilter/vf_find_rect.c b/libavfilter/vf_find_rect.c
index d7e6579af7..ee6c3f4b45 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
 
@@ -244,6 +245,9 @@  static av_cold int init(AVFilterContext *ctx)
 {
     FOCContext *foc = ctx->priv;
     int ret, i;
+    uint8_t *tmp_data[4] = { NULL };
+    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 +258,37 @@  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:
+    av_freep(&tmp_data[0]);
+    if (foc->obj_frame)
+        av_freep(&foc->obj_frame->data[0]);
+    av_frame_free(&foc->obj_frame);
+    return ret;
 }
 
 static const AVFilterPad foc_inputs[] = {