[FFmpeg-devel] [PATCH] Unsharp filter

Bobby Bingham uhmmmm
Mon Apr 5 05:29:16 CEST 2010


On Fri, 26 Mar 2010 11:21:39 -0400
"Daniel G. Taylor" <dan at programmer-art.org> wrote:

> On 03/25/2010 10:18 PM, Bobby Bingham wrote:
> >> [...]
> >> static void set_filter_param(FilterParam *fp, int msize_x, int msize_y, double amount)
> >> {
> >>      fp->msize_x = msize_x;
> >>      fp->msize_y = msize_y;
> >>      fp->amount = amount * 65536.0;
> >>
> >>      fp->steps_x = msize_x / 2;
> >>      fp->steps_y = msize_y / 2;
> >>      fp->scalebits = (fp->steps_x + fp->steps_y) * 2;
> >>      fp->halfscale = 1<<  ((fp->steps_x + fp->steps_y) * 2 - 1);
> >
> > I think this would be easier to read if you either aligned the
> > (fp->steps_x + fp->steps_y) * 2 or simply used fp->scalebits in the
> > calculation of fp->halfscale.
> 
> Fixed.
> 
> >> }
> >>
> >> [...]
> >>
> >>          msize_x = FFMIN(MAX_SIZE, FFMAX(MIN_SIZE, msize_x));
> >>          msize_y = FFMIN(MAX_SIZE, FFMAX(MIN_SIZE, msize_y));
> >
> > av_clip
> 
> Fixed.
> 
> >>      }
> >>
> >>      if (type == 'l' || type == 'b')
> >>          set_filter_param(&unsharp->luma, msize_x, msize_y, amount);
> >>
> >>      if (type == 'c' || type == 'b')
> >>          set_filter_param(&unsharp->chroma, msize_x, msize_y, amount);
> >>
> >
> > The context stores separate filter parameters for luma and chroma.  Why
> > not let the user specify them independently as well?  Something like
> > luma:5:5:1:chroma:3:3:2.  I could see this being useful where chroma is
> > subsampled, so you have to average a smaller number of values together
> > to cover the same number of pixels in the image.
> 
> Fixed to allow you to specify separate luma and chroma parameters, but 
> the C string processing to allow your suggestion above proved to be 
> quite difficult. I've changed it to now take six positional parameters 
> to define luma and chroma sizes and amounts, in the form of:
> 
>  
> luma_width:luma_height:luma_amount:chroma_width:chroma_height:chroma_amount
> 
> If you really want the string "luma" or "chroma" in the filter options 
> then how would you suggest the best way to parse that is? sscanf doesn't 
> seem to parse return the number of characters read and I'm pretty rusty 
> on my C string processing.
> 
> Another thought I had here was just to support a single width/height and 
> amount, and to automatically scale the internal values based on the 
> chroma subsampling size. That way we could also easily support grayscale 
> and RGB formats in the future, as they wouldn't use a chroma/luma setup 
> (correct me if I'm wrong). It gives you less control overall and I'm not 
> sure what's required to support RGB and such or if it's best to keep the 
> filter only processing YUV data, so comments welcome.
> 
> >>      return 0;
> >> }
> >>
> >> static int query_formats(AVFilterContext *ctx)
> >> {
> >>      enum PixelFormat pix_fmts[] = {
> >>          PIX_FMT_YUV420P, PIX_FMT_NONE
> >
> > This code (with the change suggested below) should work for any of
> > these planar YUV formats:
> > YUV420P, YUV422P, YUV444P, YUV410P, YUV411P, YUVJ420P, YUVJ422P,
> > YUVJ444P, YUV440P, YUVJ440P
> 
> Fixed, tested with the above formats and things work well.
> 
> >>     [...]
> >>
> >>      init_filter_param(link->dst,&unsharp->luma,   "luma",   link->w);
> >>      init_filter_param(link->dst,&unsharp->chroma, "chroma", link->w);
> >
> > The chroma planes aren't necessarily full width.  See below.
> 
> Fixed.
> 
> >> [...]
> >>
> >>      unsharpen(out->data[0], in->data[0], out->linesize[0], in->linesize[0], link->w,     link->h,&unsharp->luma);
> >>      unsharpen(out->data[1], in->data[1], out->linesize[1], in->linesize[1], link->w / 2, link->h / 2,&unsharp->chroma);
> >>      unsharpen(out->data[2], in->data[2], out->linesize[2], in->linesize[2], link->w / 2, link->h / 2,&unsharp->chroma);
> >
> >  From libavutil/pixdesc.h:
> >
> >      /**
> >       * Amount to shift the luma width right to find the chroma width.
> >       * For YV12 this is 1 for example.
> >       * chroma_width = -((-luma_width)>>  log2_chroma_w)
> >       * The note above is needed to ensure rounding up.
> >       * This value only refers to the chroma components.
> >       */
> >      uint8_t log2_chroma_w;      ///<  chroma_width = -((-luma_width )>>log2_chroma_w)
> >
> >      /**
> >       * Amount to shift the luma height right to find the chroma height.
> >       * For YV12 this is 1 for example.
> >       * chroma_height= -((-luma_height)>>  log2_chroma_h)
> >       * The note above is needed to ensure rounding up.
> >       * This value only refers to the chroma components.
> >       */
> >      uint8_t log2_chroma_h;
> >
> > So, the width of your chroma plane here will be:
> > -((-link->w)>>  av_pix_fmt_descriptors[link->format].log2_chroma_w)
> >
> > Similar for the height.  This should allow it to work with any of the
> > pixel formats I listed above.
> 
> Thanks, this was very helpful. Fixed.
> 
> Take care,

Just one comment from me in addition to what Stefano said.

> +static void init_filter_param(AVFilterContext *ctx, FilterParam *fp, const char *effect_type, int width)
> +{
> +    int z;
> +    const char *effect;
> +
> +    effect = fp->amount == 0 ? "none" : fp->amount < 0 ? "blur" : "sharpen";
> +
> +    av_log(ctx, AV_LOG_INFO, "effect:%s type:%s msize_x:%d msize_y:%d amount:%0.2f\n", effect, effect_type, fp->msize_x, fp->msize_y, fp->amount / 65535.0);
> +
> +    memset(fp->sc, 0, sizeof(fp->sc));

This memset doesn't look necessary.

> +    for (z = 0; z < 2 * fp->steps_y; z++)
> +        fp->sc[z] = av_malloc(sizeof(*(fp->sc[z])) * (width + 2 * fp->steps_x));
> +}

-- 
Bobby Bingham
??????????????????????



More information about the ffmpeg-devel mailing list