[FFmpeg-devel] [PATCH] Unsharp filter

[Daniel G. Taylor]

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,
-- 
Daniel G. Taylor
http://programmer-art.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unsharp.diff
Type: text/x-diff
Size: 11661 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100326/7564614e/attachment.diff>