[FFmpeg-devel] [PATCH v4] libavfilter/vf_cover_rect: support for cover image with more pixel format and different width and height

[Michael Niedermayer]

On Fri, Jun 14, 2019 at 11:52:47PM +0800, Lance Wang wrote:
> On Fri, Jun 14, 2019 at 5:31 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Wed, Jun 12, 2019 at 06:50:18PM +0800, lance.lmwang at gmail.com wrote:
> > > From: Limin Wang <lance.lmwang at gmail.com>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> > > ---
> > >  doc/filters.texi            |  6 ++--
> > >  libavfilter/vf_cover_rect.c | 56 +++++++++++++++++++++++++++----------
> > >  2 files changed, 44 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/doc/filters.texi b/doc/filters.texi
> > > index ec1c7c7591..4594a61c13 100644
> > > --- a/doc/filters.texi
> > > +++ b/doc/filters.texi
> > > @@ -10166,7 +10166,7 @@ Specifies the rectangle in which to search.
> > >
> > >  @itemize
> > >  @item
> > > -Generate a representative palette of a given video using
> > @command{ffmpeg}:
> > > +Cover a rectangular object by the supplied image of a given video using
> > @command{ffmpeg}:
> > >  @example
> > >  ffmpeg -i file.ts -vf
> > find_rect=newref.pgm,cover_rect=cover.jpg:mode=cover new.mkv
> > >  @end example
> > > @@ -10180,7 +10180,7 @@ It accepts the following options:
> > >
> > >  @table @option
> > >  @item cover
> > > -Filepath of the optional cover image, needs to be in yuv420.
> > > +Filepath of the optional cover image.
> > >
> > >  @item mode
> > >  Set covering mode.
> > > @@ -10200,7 +10200,7 @@ Default value is @var{blur}.
> > >
> > >  @itemize
> > >  @item
> > > -Generate a representative palette of a given video using
> > @command{ffmpeg}:
> > > +Cover a rectangular object by the supplied image of a given video using
> > @command{ffmpeg}:
> > >  @example
> > >  ffmpeg -i file.ts -vf
> > find_rect=newref.pgm,cover_rect=cover.jpg:mode=cover new.mkv
> > >  @end example
> > > diff --git a/libavfilter/vf_cover_rect.c b/libavfilter/vf_cover_rect.c
> > > index 898debf09d..36c650dd21 100644
> > > --- a/libavfilter/vf_cover_rect.c
> > > +++ b/libavfilter/vf_cover_rect.c
> > > @@ -28,6 +28,7 @@
> > >  #include "internal.h"
> > >
> > >  #include "lavfutils.h"
> > > +#include "lswsutils.h"
> > >
> > >  enum mode {
> > >      MODE_COVER,
> > > @@ -40,6 +41,7 @@ typedef struct CoverContext {
> > >      int mode;
> > >      char *cover_filename;
> > >      AVFrame *cover_frame;
> > > +    AVFrame *match_frame;
> > >      int width, height;
> > >  } CoverContext;
> > >
> >
> > > @@ -71,21 +73,21 @@ static int config_input(AVFilterLink *inlink)
> > >      return 0;
> > >  }
> > >
> > > -static void cover_rect(CoverContext *cover, AVFrame *in, int offx, int
> > offy)
> > > +static void cover_rect(AVFrame *cover_frame, AVFrame *in, int offx, int
> > offy)
> > >  {
> > >      int x, y, p;
> > >
> > >      for (p = 0; p < 3; p++) {
> > >          uint8_t *data = in->data[p] + (offx>>!!p) + (offy>>!!p) *
> > in->linesize[p];
> > > -        const uint8_t *src = cover->cover_frame->data[p];
> > > -        int w = AV_CEIL_RSHIFT(cover->cover_frame->width , !!p);
> > > -        int h = AV_CEIL_RSHIFT(cover->cover_frame->height, !!p);
> > > +        const uint8_t *src = cover_frame->data[p];
> > > +        int w = AV_CEIL_RSHIFT(cover_frame->width , !!p);
> > > +        int h = AV_CEIL_RSHIFT(cover_frame->height, !!p);
> > >          for (y = 0; y < h; y++) {
> > >              for (x = 0; x < w; x++) {
> > >                  data[x] = src[x];
> > >              }
> > >              data += in->linesize[p];
> > > -            src += cover->cover_frame->linesize[p];
> > > +            src += cover_frame->linesize[p];
> > >          }
> > >      }
> > >  }
> >
> > This hunk is independant and can be done in a seperate patch before
> >
> >
> OK, I'll split the patch.
> 
> 
> >
> > > @@ -138,7 +140,10 @@ static int filter_frame(AVFilterLink *inlink,
> > AVFrame *in)
> > >      CoverContext *cover = ctx->priv;
> > >      AVDictionaryEntry *ex, *ey, *ew, *eh;
> > >      int x = -1, y = -1, w = -1, h = -1;
> > > +    enum AVPixelFormat in_format;
> > >      char *xendptr = NULL, *yendptr = NULL, *wendptr = NULL, *hendptr =
> > NULL;
> > > +    AVFrame *cover_frame = NULL;
> > > +    int ret;
> > >
> > >      ex = av_dict_get(in->metadata, "lavfi.rect.x", NULL,
> > AV_DICT_MATCH_CASE);
> > >      ey = av_dict_get(in->metadata, "lavfi.rect.y", NULL,
> > AV_DICT_MATCH_CASE);
> >
> > > @@ -167,14 +172,34 @@ static int filter_frame(AVFilterLink *inlink,
> > AVFrame *in)
> > >      }
> > >      w = FFMIN(w, in->width  - x);
> > >      h = FFMIN(h, in->height - y);
> > > +    in_format = in->format;
> > >
> > >      if (w > in->width || h > in->height || w <= 0 || h <= 0)
> > >          return AVERROR(EINVAL);
> > >
> > > -    if (cover->cover_frame) {
> > > -        if (w != cover->cover_frame->width || h !=
> > cover->cover_frame->height)
> > > -            return AVERROR(EINVAL);
> > > -    }
> > > +    if (w != cover->cover_frame->width || h !=
> > cover->cover_frame->height ||
> > > +            in_format != cover->cover_frame->format) {
> >
> > This segfaults
> >
> 
> cover->cover_frame is checked in the init function, if it's null, it'll
> failed already, so I remove the checking and haven't got any segments issue.
> anyway, I'll add it for the updated patch.

there are 2 modes, blur doesnt need an image


> 
> 
> >
> >
> > > +        if (!cover->match_frame || (w != cover->match_frame->width || h
> > != cover->match_frame->height
> > > +                    || in_format != cover->match_frame->format)) {
> > > +            if (cover->match_frame)
> > > +                av_freep(&cover->match_frame->data[0]);
> > > +            else if (!(cover->match_frame = av_frame_alloc()))
> > > +                return AVERROR(ENOMEM);
> > > +
> >
> > > +            if ((ret = ff_scale_image(cover->match_frame->data,
> > cover->match_frame->linesize,
> > > +                            w, h, in_format, cover->cover_frame->data,
> > cover->cover_frame->linesize,
> > > +                            cover->cover_frame->width,
> > cover->cover_frame->height,
> > > +                            cover->cover_frame->format, ctx)) < 0)
> > > +                return AVERROR(ENOMEM);
> >
> > This sort of reimplements the scale filter and it
> > doesnt consider some parameters like AVCOL_RANGE*
> >
> 
> the ff_scale_image is implemented and used by other alike place,  I try to
> reuse the function anyway.
> If we need other parameters for scale, we may improve the function later,
> now it's OK for my testing as
> the cover image is logo mostly.

I think one could argue that the scale filter should be used for converting
this way the cover image would become one externally vissible input. (which
then subsequently also could change over time and not just be a static
image ...)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Elect your leaders based on what they did after the last election, not
based on what they say before an election.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190616/7c9b9764/attachment.sig>