[FFmpeg-devel] [PATCH] Unsharp filter

[Daniel G. Taylor]

On 03/26/2010 11:21 AM, Daniel G. Taylor 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,
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Ping?

-- 
Daniel G. Taylor
http://programmer-art.org/