Message ID | 1559948038-47078-1-git-send-email-lance.lmwang@gmail.com |
---|---|
State | Superseded |
Headers | show |
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] [...]
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 ... [...]
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".
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 --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[] = {