[FFmpeg-devel] [PATCH 3/4] lavfi/delogo: option show shouldn't affect band

[Jean Delvare]

Hi Stefano,

Thanks again for the fast and complete review.

On Fri, 5 Jul 2013 11:39:51 +0200, Stefano Sabatini wrote:
> On date Friday 2013-07-05 10:34:10 +0200, Jean Delvare encoded:
> > Enabling option show (to see the borders of the logo area) forcibly
> > sets the band option to 4. This is documented, but still surprising
> > and confusing IMHO. Both options should be independent.
> 
> The subject and this paragraph are prescribing a behavior rather than
> describing the change, and thus is not very clear what the patch is
> about.

Good point, I'll reword it.

> > Also document exactly where the rectangle is drawn.
> > 
> > Signed-off-by: Jean Delvare <khali at linux-fr.org>
> > ---
> >  doc/filters.texi        |    8 ++++++--
> >  libavfilter/vf_delogo.c |    4 +---
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > --- ffmpeg.orig/libavfilter/vf_delogo.c	2013-07-05 09:10:16.425572220 +0200
> > +++ ffmpeg/libavfilter/vf_delogo.c	2013-07-05 09:10:22.032572303 +0200
> > @@ -195,10 +195,8 @@ static av_cold int init(AVFilterContext
> >      CHECK_UNSET_OPT(w);
> >      CHECK_UNSET_OPT(h);
> >  
> > -    if (s->band < 0 || s->show) {
> > -        s->show = 1;
> > +    if (s->band < 0)
> >          s->band = 4;
> > -    }
> 
> Looks good. This is technically a backward compatibiliy break but I
> don't think people would care,

Agreed.

> bumping micro may be useful nonetheless.

OK, will do.

> You could probably set default band value to 4, change the range to
> 0..MAX and remove this check altogether (this code was written when we
> still lack proper option handling).

Done in a subsequent patch, as you must have already noticed meanwhile.

> >      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);
> > --- ffmpeg.orig/doc/filters.texi	2013-07-05 09:01:05.040695940 +0200
> > +++ ffmpeg/doc/filters.texi	2013-07-05 09:50:13.998651184 +0200
> > @@ -2780,8 +2780,12 @@ Specify the thickness of the fuzzy edge
> >  
> >  @item show
> 
> >  When set to 1, a green rectangle is drawn on the screen to simplify
> > -finding the right @var{x}, @var{y}, @var{w}, @var{h} parameters, and
> > - at var{band} is set to 4. The default value is 0.
> > +finding the right @var{x}, @var{y}, @var{w}, and @var{h} parameters.
> > +The default value is 0.
> 
> > The rectangle is drawn on the outermost
> > +pixels which will be (partly) replaced with interpolated values.
> > +The values of the next pixels immediately outside this rectangle in
> > +each direction will be used to compute the interpolated pixel values
> > +inside the rectangle.
> 
> I'd prefer to put this change in a separate patch, since it made more
> hard for me to understand what was the patch was about.

You are right, I will do that.

> Note: I wouldn't be against removing the show option altogether, since
> what it does can be better achieved by using a separate filter
> (drawbox).

This was my initial though as well. However two reasons made me change
my mind:

 * The "show" option doesn't use the x, y, w and h parameters directly
   to draw the box where the drawbox filter would. Instead the box is
   drawn at (x-t+1, y-t+1) with width w+2*(t-1) and height h+2*(t-1). So
   we can't sell drawbox as a drop-in replacement for option "show",
   the user would have to compute the proper parameters (or write the
   expressions down and let the drawbox filter compute them, as I see
   this is supported now.)

 * The "show" option has helped me understand how the algorithm works
   and spot a number of bugs in the implementation. In particular for
   subsampled chroma planes, the area is not exactly the same as for
   the luma plane because of the lesser resolution. The drawbox filter
   wouldn't show these details.

That being said, I don't have a so strong opinion on keeping the "show"
option or removing it. It does add some complexity to the code and
slows it down a bit, but probably not that much (I admit I did not try
to measure it.) So there is some value in removing it, but probably not
much. Just like there is value in keeping it, but not much either.

I must admit that I have been using "show=1" for debugging only, _not_
for finding the x, y, w and h parameters. I find it much easier to open
a snapshot in an image editor and locate the borders of the logo area
that way. You need to do that to get a first idea for the x, y, w and h
parameters anyway. "show=1" can only be used for fine tuning afterward.

So if there is a consensus that the "show" option should be removed, I
won't object.

Thanks,
-- 
Jean Delvare