[MPlayer-dev-eng] [PATCH] convol (general convolution filter)

[Michael Niedermayer]

On Sun, Aug 02, 2009 at 10:56:02PM +0000, Adam M. Costello wrote:
> The attached patch adds a new filter, convol, which does general
> convolution.  The scale filter already uses convolution for scaling
> and image-format conversion, but provides very limited control over
> the filter coefficients.  The convol filter does no scaling and no
> image-format conversion but provides full control over the convolution
> coefficients, with (optionally) separate filters for luma-x, luma-y,
> chroma-x, and chroma-y.
> 
> The geq filter can do most of what convol can do (and more), but is
> about 20x slower for similar operations and has a more cumbersome option
> syntax.
> 
> I use convol for attenuating very noisy high-frequency "information" in
> the luma channel of over-compressed MJPEG source material from a pocket
> camera before recoding as H.264.
> 
> AMC

first, i must say this patch is alot of code for what it does and its
completely devoid of optimizations
also something called (general) convolution should support that, your
filter seems to only support seperable convolutions, not the general 2d
case.
In the end i cant see what this filter does that swscale does not already
support, maybe sws lacks access to these things from the command line but
writing a page of code for vf_scale to parse the filter coeffs from the cmd
line is obviously the better choice than this

some incomplete review below that i did until i realized that your filter
adds nothing new


[...]

> +#if UINT_MAX < 0xFFFFFFFF
> +#error Sorry, this code depends on int being at least 32 bits.
> +#endif

useless


> +
> +
> +struct sequence_filter {
> +  int *taps;     /* Filter coefficients. */
> +  int num_taps;  /* Must be odd. */
> +  int sum_taps;  /* Sum of taps, must be > 0. */
> +};

not doxygen compatible comments


[...]

> +static inline int min(int a, int b) {
> +  return a >= b ? b : a;
> +}
> +
> +static inline int max(int a, int b) {
> +  return a >= b ? a : b;
> +}

FFMIN/FFMAX


> +
> +
> +/* Clamp sample to the range [0..sample_max]. */
> +static inline int clamp(int sample, int sample_max) {
> +  return min(sample_max, max(0, sample));
> +}

av_clip()


[...]

> +/* Copy a channel. */
> +
> +static void copy_channel(

iam sure there is code in mplayer that does this


[...]
> +/* Apply a convolution filter to one channel. */
> +
> +static void convolve_channel(
> +  const struct channel_filter *filter,
> +  int sample_stride,
> +  int in_row_stride,
> +  int out_row_stride,
> +  const uint8_t *in_channel,
> +  uint8_t *out_channel)
> +{
> +  const uint8_t *in_row = in_channel;
> +  uint8_t *out_row = out_channel;
> +  int row, col, vertical_margin;
> +
> +  /* Optimization:  If the filter is a no-op, just copy the channel. */
> +  if (filter->horizontal.num_taps == 1 && filter->vertical.num_taps == 1) {
> +    copy_channel(filter->samples_per_row,
> +                 filter->num_rows,
> +                 sample_stride,
> +                 in_row_stride,
> +                 out_row_stride,
> +                 in_channel,
> +                 out_channel);
> +    return;
> +  }
> +
> +  /* We do not vertically filter a few rows at the top and */
> +  /* bottom, where the filter would extend out of bounds.  */

sounds wrong
you have to decide how to define the out of frame pixels and then
filter that too


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090818/4d18f12b/attachment.pgp>