[FFmpeg-devel] [PATCH 2/2 v2] delogo: Take SAR into account

[Jean Delvare]

Hi Stefano,

Thanks a lot for the thorough review, I appreciate it very much.

On Fri, 28 Jun 2013 00:51:55 +0200, Stefano Sabatini wrote:
> Note:
> lavfi/delogo: take SAR into account
> 
> as subject would be more consistent with (my own) conventions.

OK, I have updated all my pending patches to follow this convention.

> On date Wednesday 2013-06-26 14:57:34 +0200, Jean Delvare encoded:
> > When interpolating, weights are based on relative distances, which
> > assume square pixels. If a non-1:1 sample aspect ratio is used, it
> > should be taken into account when comparing distances, because the
> > human eye and brain care about the picture as it is displayed, not
> > stored.
> > 
> > Signed-off-by: Jean Delvare <khali at linux-fr.org>
> > ---
> > No functional change since v1, but rebasing was needed.
> > 
> >  libavfilter/version.h   |    2 +-
> >  libavfilter/vf_delogo.c |   14 ++++++++------
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > --- ffmpeg.orig/libavfilter/vf_delogo.c	2013-06-25 21:56:27.049663266 +0200
> > +++ ffmpeg/libavfilter/vf_delogo.c	2013-06-25 22:00:58.615920845 +0200
> > @@ -56,7 +56,7 @@
> >   */
> >  static void apply_delogo(uint8_t *dst, int dst_linesize,
> >                           uint8_t *src, int src_linesize,
> > -                         int w, int h,
> > +                         int w, int h, AVRational sar,
> >                           int logo_x, int logo_y, int logo_w, int logo_h,
> >                           int band, int show, int direct)
> >  {
> > @@ -67,6 +67,7 @@ static void apply_delogo(uint8_t *dst, i
> >      uint8_t *topleft, *botleft, *topright;
> >      int xclipl, xclipr, yclipt, yclipb;
> >      int logo_x1, logo_x2, logo_y1, logo_y2;
> 
> > +    int sar_num = sar.num ? : 1;
> 
> GNU extension, check:
> http://en.wikipedia.org/wiki/%3F:#C
> 
> Potentially non portable.

I wasn't sure, although I admit I should have taken the lack of
occurrence of this construct in the rest of the code as a hint it was
undesirable. I'll update it and resend.

> Also you can modify the sar variable in place, and avoid the
> additional temporary sar_num.

Oh, of course, thanks. Not sure how I missed that, I'll fix it.

I am also realizing that subsampling factors in chroma planes should
ideally be taken into account when computing the relative weights. It
doesn't matter for my own YUV 4:2:0 input but it would matter whenever
vertical and horizontal subsampling factors differ. I'll do some tests
and write a separate patch.

> >  
> >      xclipl = FFMAX(-logo_x, 0);
> >      xclipr = FFMAX(logo_x+logo_w-w, 0);
> > @@ -93,11 +94,11 @@ static void apply_delogo(uint8_t *dst, i
> >               xdst = dst+logo_x1+1,
> >               xsrc = src+logo_x1+1; x < logo_x2-1; x++, xdst++, xsrc++) {
> >  
> > -            /* Weighted interpolation based on relative distances */
> > -            weightl = (uint64_t)              (logo_x2-1-x) * (y-logo_y1) * (logo_y2-1-y);
> > -            weightr = (uint64_t)(x-logo_x1)                 * (y-logo_y1) * (logo_y2-1-y);
> > -            weightt = (uint64_t)(x-logo_x1) * (logo_x2-1-x)               * (logo_y2-1-y);
> > -            weightb = (uint64_t)(x-logo_x1) * (logo_x2-1-x) * (y-logo_y1);
> > +            /* Weighted interpolation based on relative distances, taking SAR into account */
> 
> > +            weightl = (uint64_t)              (logo_x2-1-x) * (y-logo_y1) * (logo_y2-1-y) * sar.den;
> 
> I'd expect
> (logo_x2-1-x)*sar.num * (y-logo_y1)*sar.den * (logo_y2-1-y)*sar.den
> 
> but maybe I'm missing something...

You are right, but yes you are also missing something ;-)

I originally wrote exactly what you wrote above, but then realized that
sar.num * sar.den was a common factor to all weights, so it could be
omitted. Omitting it makes the code faster.

> > +            weightr = (uint64_t)(x-logo_x1)                 * (y-logo_y1) * (logo_y2-1-y) * sar.den;
> > +            weightt = (uint64_t)(x-logo_x1) * (logo_x2-1-x)               * (logo_y2-1-y) * sar_num;
> > +            weightb = (uint64_t)(x-logo_x1) * (logo_x2-1-x) * (y-logo_y1)                 * sar_num;
> >  
> >              interp =
> >                  (topleft[src_linesize*(y-logo_y  -yclipt)]   +
> > @@ -239,6 +240,7 @@ static int filter_frame(AVFilterLink *in
> >                       in ->data[plane], in ->linesize[plane],
> >                       FF_CEIL_RSHIFT(inlink->w, hsub),
> >                       FF_CEIL_RSHIFT(inlink->h, vsub),
> > +                     in->sample_aspect_ratio,
> >                       s->x>>hsub, s->y>>vsub,
> >                       FF_CEIL_RSHIFT(s->w, hsub),
> >                       FF_CEIL_RSHIFT(s->h, vsub),
> > --- ffmpeg.orig/libavfilter/version.h	2013-06-25 18:13:56.702312485 +0200
> > +++ ffmpeg/libavfilter/version.h	2013-06-25 21:59:34.304666042 +0200
> > @@ -31,7 +31,7 @@
> >  
> >  #define LIBAVFILTER_VERSION_MAJOR  3
> >  #define LIBAVFILTER_VERSION_MINOR  77
> > -#define LIBAVFILTER_VERSION_MICRO 101
> > +#define LIBAVFILTER_VERSION_MICRO 102
> 
> No need to bump version for this.

I received contradicting instructions regarding this on #ffmpeg, from
"bump it always" to "bump it on interface change only", to "bump it on
user-visible changes", to "bump it in the last patch of the series." I
guess I'll just leave it alone from now on and let whoever applies my
patches bump it when he/she thinks it is appropriate. This makes my life
easier anyway.

BTW, as I am now very familiar with the delogo filter code, would it be
desirable that I become the maintainer of it? I don't want to step on
anyone toes, only wondering if this would be appreciated to off-load
some work from the core ffmpeg developers. Note though that I do not
intend to become a full time contributor to ffmpeg (I would love to but
it simply doesn't fit in my daily job and family life makes my spare
time a scarce resource) and I am not reading ffmpeg-devel all the time.

Also note that I have several patches in the works for the delogo
filter, including bug fixes and code optimizations. I'll send them over
for review when I am done polishing them.

Thanks again,
-- 
Jean Delvare