[FFmpeg-devel] [PATCH] avfilter/delogo: Set default band to 1

[Jean Delvare]

On Wed, 7 Oct 2015 14:19:58 +0200, Stefano Sabatini wrote:
> > > LGTM. BTW, if you want to drop the band option, you could ifdef it so
> > > that it will be dropt at the next lavfi major bump.
> > 
> > Good idea. Something like this?
> > 
> > ---
> >  libavfilter/vf_delogo.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > --- ffmpeg.orig/libavfilter/vf_delogo.c	2015-10-07 08:45:39.037203764 +0200
> > +++ ffmpeg/libavfilter/vf_delogo.c	2015-10-07 13:52:43.716487733 +0200
> > @@ -165,9 +165,11 @@ static const AVOption delogo_options[]=
> >      { "y",    "set logo y position",       OFFSET(y),    AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, FLAGS },
> >      { "w",    "set logo width",            OFFSET(w),    AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, FLAGS },
> >      { "h",    "set logo height",           OFFSET(h),    AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, FLAGS },
> > +#if LIBAVFILTER_VERSION_MAJOR < 7
> >      /* Actual default value for band/t is 1, set in init */
> >      { "band", "set delogo area band size", OFFSET(band), AV_OPT_TYPE_INT, { .i64 =  0 },  0, INT_MAX, FLAGS },
> >      { "t",    "set delogo area band size", OFFSET(band), AV_OPT_TYPE_INT, { .i64 =  0 },  0, INT_MAX, FLAGS },
> > +#endif
> >      { "show", "show delogo area",          OFFSET(show), AV_OPT_TYPE_BOOL,{ .i64 =  0 },  0, 1,       FLAGS },
> >      { NULL }
> >  };
> > @@ -202,12 +204,16 @@ static av_cold int init(AVFilterContext
> >      CHECK_UNSET_OPT(w);
> >      CHECK_UNSET_OPT(h);
> >  
> > +#if LIBAVFILTER_VERSION_MAJOR < 7
> >      if (s->band == 0) { /* Unset, use default */
> >          av_log(ctx, AV_LOG_WARNING, "Note: default band value was changed from 4 to 1.\n");
> >          s->band = 1;
> >      } else if (s->band != 1) {
> >          av_log(ctx, AV_LOG_WARNING, "Option band is deprecated.\n");
> >      }
> > +#else
> > +    s->band = 1;
> > +#endif
> >      av_log(ctx, AV_LOG_VERBOSE, "x:%d y:%d, w:%d h:%d band:%d show:%d\n",
> >             s->x, s->y, s->w, s->h, s->band, s->show);
> 
> Looks fine to me.

Great.

> > Or should I use FF_API_OLD_FILTER_OPTS?
> 
> No, I think that's related to another issue.

I thought so but wasn't sure.

> > Assuming this is what you had in mind, would it go as a separate patch,
> > or should this be folded in the patch I just sent?
> 
> Use a single patch since it involves less work, unless you prefer to
> do it with a separate patch.

I'm fine either way so if a single patch is easier for you, I'll do
that. I'll send a v2 of the patch in a minute.

Thanks for the review,
-- 
Jean Delvare
SUSE L3 Support