[FFmpeg-devel] [PATCH] lavfi: port sab filter from libmpcodecs
Stefano Sabatini
stefasab at gmail.com
Mon Jun 3 15:59:02 CEST 2013
On date Monday 2013-06-03 15:31:07 +0200, Clément Bœsch encoded:
> On Mon, Jun 03, 2013 at 03:02:18PM +0200, Stefano Sabatini wrote:
> [...]
> > +static const AVOption sab_options[] = {
> > + { "luma_radius", "set luma radius", OFFSET(luma.radius), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, RADIUS_MIN, RADIUS_MAX, .flags=FLAGS },
> > + { "lr" , "set luma radius", OFFSET(luma.radius), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, RADIUS_MIN, RADIUS_MAX, .flags=FLAGS },
> > + { "luma_pre_filter_radius", "set luma pre-filter radius", OFFSET(luma.pre_filter_radius), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, PRE_FILTER_RADIUS_MIN, PRE_FILTER_RADIUS_MAX, .flags=FLAGS },
> > + { "lpfr", "set luma pre-filter radius", OFFSET(luma.pre_filter_radius), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, PRE_FILTER_RADIUS_MIN, PRE_FILTER_RADIUS_MAX, .flags=FLAGS },
> > + { "luma_strength", "set luma strength", OFFSET(luma.strength), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, STRENGTH_MIN, STRENGTH_MAX, .flags=FLAGS },
> > + { "ls", "set luma strength", OFFSET(luma.strength), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, STRENGTH_MIN, STRENGTH_MAX, .flags=FLAGS },
> > +
> > + { "chroma_radius", "set chroma radius", OFFSET(chroma.radius), AV_OPT_TYPE_FLOAT, {.dbl=RADIUS_MIN-1}, RADIUS_MIN-1, RADIUS_MAX, .flags=FLAGS },
> > + { "cr", "set chroma radius", OFFSET(chroma.radius), AV_OPT_TYPE_FLOAT, {.dbl=RADIUS_MIN-1}, RADIUS_MIN-1, RADIUS_MAX, .flags=FLAGS },
> > + { "chroma_pre_filter_radius", "set chroma pre-filter radius", OFFSET(chroma.pre_filter_radius), AV_OPT_TYPE_FLOAT, {.dbl=PRE_FILTER_RADIUS_MIN-1},
> > + PRE_FILTER_RADIUS_MIN-1, PRE_FILTER_RADIUS_MAX, .flags=FLAGS },
> > + { "cpfr", "set chroma pre-filter radius", OFFSET(chroma.pre_filter_radius), AV_OPT_TYPE_FLOAT, {.dbl=PRE_FILTER_RADIUS_MIN-1},
> > + PRE_FILTER_RADIUS_MIN-1, PRE_FILTER_RADIUS_MAX, .flags=FLAGS },
> > + { "chroma_strength", "set chroma strength", OFFSET(chroma.strength), AV_OPT_TYPE_FLOAT, {.dbl=STRENGTH_MIN-1}, STRENGTH_MIN-1, STRENGTH_MAX, .flags=FLAGS },
> > + { "cs", "set chroma strength", OFFSET(chroma.strength), AV_OPT_TYPE_FLOAT, {.dbl=STRENGTH_MIN-1}, STRENGTH_MIN-1, STRENGTH_MAX, .flags=FLAGS },
> > +
> > + { NULL }
> > +};
> > +
> > +AVFILTER_DEFINE_CLASS(sab);
> > +
> > +static av_cold int init(AVFilterContext *ctx)
> > +{
> > + SabContext *sab = ctx->priv;
> > +
> > + /* make chroma default to luma values, if not explicitly set */
> > + if (sab->chroma.radius < RADIUS_MIN)
> > + sab->chroma.radius = sab->luma.radius;
> > + if (sab->chroma.pre_filter_radius < PRE_FILTER_RADIUS_MIN)
> > + sab->chroma.pre_filter_radius = sab->luma.pre_filter_radius;
> > + if (sab->chroma.strength < STRENGTH_MIN)
> > + sab->chroma.strength = sab->luma.strength;
> > +
>
> > + sab->luma.quality = sab->chroma.quality = 3.0;
>
> Not configurable?
Sure this can be done by adding new options, but I prefer to have this
in a separate commit.
>
> > + sab->sws_flags = SWS_POINT;
> > +
> > + av_log(ctx, AV_LOG_VERBOSE,
> > + "luma_radius:%f luma_pre_filter_radius::%f luma_strength:%f "
> > + "chroma_radius:%f chroma_pre_filter_radius:%f chroma_strength:%f\n",
> > + sab->luma .radius, sab->luma .pre_filter_radius, sab->luma .strength,
> > + sab->chroma.radius, sab->chroma.pre_filter_radius, sab->chroma.strength);
> > + return 0;
> > +}
> > +
> > +static void free_filter_param(FilterParam *f)
> > +{
> > + if (f->pre_filter_context)
> > + sws_freeContext(f->pre_filter_context);
>
> > + f->pre_filter_context = NULL;
> > +
>
> Can be in the if scope
Why not?
[...]
> > +#define RADIUS_MIN 0.1
> > +#define RADIUS_MAX 4.0
> > +
> > +#define PRE_FILTER_RADIUS_MIN 0.1
> > +#define PRE_FILTER_RADIUS_MAX 2.0
> > +
> > +#define STRENGTH_MIN 0.1
> > +#define STRENGTH_MAX 100.0
> > +
> > +#define OFFSET(x) offsetof(SabContext, x)
> > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> > +
>
> aren't sws_flags int64_t?
They are specified as int in sws_getContext.
> > +{
> > + SwsVector *vec;
> > + SwsFilter sws_f;
> > + int i, x, y;
> > + int linesize = FFALIGN(width, 8);
> > +
> > + f->pre_filter_buf = av_malloc(linesize*height);
>
> nit: spaces
>
> > + if (!f->pre_filter_buf)
> > + return AVERROR(ENOMEM);
> > +
> > + f->pre_filter_linesize = linesize;
> > + vec = sws_getGaussianVec(f->pre_filter_radius, f->quality);
> > + sws_f.lumH = sws_f.lumV = vec;
> > + sws_f.chrH = sws_f.chrV = NULL;
> > + f->pre_filter_context = sws_getContext(width, height, AV_PIX_FMT_GRAY8,
> > + width, height, AV_PIX_FMT_GRAY8,
> > + sws_flags, &sws_f, NULL, NULL);
> > + sws_freeVec(vec);
> > +
> > + vec = sws_getGaussianVec(f->strength, 5.0);
> > + for (i = 0; i < COLOR_DIFF_COEFF_SIZE; i++) {
> > + double d;
> > + int index = i-COLOR_DIFF_COEFF_SIZE/2 + vec->length/2;
> > +
> > + if (index < 0 || index >= vec->length) d = 0.0;
> > + else d = vec->coeff[index];
> > +
> > + f->color_diff_coeff[i] = (int)(d/vec->coeff[vec->length/2]*(1<<12) + 0.5);
> > + }
> > + sws_freeVec(vec);
> > +
> > + vec = sws_getGaussianVec(f->radius, f->quality);
> > + f->dist_width = vec->length;
> > + f->dist_linesize = FFALIGN(vec->length, 8);
>
> > + f->dist_coeff = av_malloc(f->dist_width * f->dist_linesize * sizeof(int32_t));
>
> sizeof(*f->dist_coeff)
Fixed.
> > + if (!f->dist_coeff)
> > + return AVERROR(ENOMEM);
> > +
> > + for (y = 0; y < vec->length; y++) {
> > + for (x = 0; x < vec->length; x++) {
> > + double d = vec->coeff[x] * vec->coeff[y];
> > + f->dist_coeff[x + y*f->dist_linesize] = (int)(d*(1<<10) + 0.5);
> > + }
> > + }
> > + sws_freeVec(vec);
> > +
> > + return 0;
> > +}
> > +
> > +static int config_props(AVFilterLink *inlink)
> > +{
> > + SabContext *sab = inlink->dst->priv;
> > + const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> > + int ret;
> > +
> > + sab->hsub = desc->log2_chroma_w;
> > + sab->vsub = desc->log2_chroma_h;
> > +
> > + ret = alloc_sws_context(&sab->luma, inlink->w, inlink->h, sab->sws_flags);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = alloc_sws_context(&sab->chroma,
> > + FF_CEIL_RSHIFT(inlink->w, sab->hsub),
> > + FF_CEIL_RSHIFT(inlink->h, sab->vsub), sab->sws_flags);
>
> Note: from what I could tell, there was some recent patches for filter
> reconfiguration where config_props was made to behave properly when called
> multiple times. Typically, by free-ing previous buffers (in this case it
> would be pre_filter_buf and f->dist_coeff) so it doesn't leak.
>
> Maybe you want to do that here, but in practice I wonder if a flag such as
> AVFILTER_FLAG_RECONFIG_NEEDS_UNINIT wouldn't be more appropriate than
> duplicating this logic as it was done in other filter.
>
> TL;DR: feel free to call uninit() in config_props() to avoid a theoretical
> leak.
Done in a slightly different way, implementing a sort of open/close
API for filter params.
> > + return ret;
> > +}
> > +
> > +#define NB_PLANES 4
> > +
> > +static void blur(uint8_t *dst, const int dst_linesize,
> > + const uint8_t *src, const int src_linesize,
> > + const int w, const int h, FilterParam *fp)
> > +{
> > + int x, y;
> > + FilterParam f = *fp;
> > + const int radius = f.dist_width/2;
> > +
> > + const uint8_t * const src2[NB_PLANES] = { src };
> > + int src2_linesize[NB_PLANES] = { src_linesize };
> > + uint8_t *dst2[NB_PLANES] = { f.pre_filter_buf };
> > + int dst2_linesize[NB_PLANES] = { f.pre_filter_linesize };
> > +
> > + sws_scale(f.pre_filter_context, src2, src2_linesize, 0, h, dst2, dst2_linesize);
> > +
> > +#define UPDATE_FACTOR do { \
> > + factor = f.color_diff_coeff[COLOR_DIFF_COEFF_SIZE/2 + pre_val - \
> > + f.pre_filter_buf[ix + iy*f.pre_filter_linesize]] * f.dist_coeff[dx + dy*f.dist_linesize]; \
> > + sum += src[ix + iy*src_linesize] * factor; \
> > + div += factor; \
> > + } while (0)
> > +
> > + for (y = 0; y < h; y++) {
> > + for (x = 0; x < w; x++) {
> > + int sum = 0;
> > + int div = 0;
> > + int dy;
> > + const int pre_val = f.pre_filter_buf[x + y*f.pre_filter_linesize];
> > + if (x >= radius && x < w - radius) {
>
> > + for (dy = 0; dy < radius*2+1; dy++) {
>
> nit-space-style here and below
>
> > + int dx;
> > + int iy = y+dy - radius;
> > + if (iy < 0) iy = -iy;
> > + else if (iy >= h) iy = h+h-iy-1;
> > +
> > + for (dx = 0; dx < radius*2+1; dx++) {
> > + const int ix = x+dx - radius;
>
> > + int factor;
> > + UPDATE_FACTOR;
>
> Here and below, you can move the int factor into the do { } while(0)
[...]
> No more comment from me, thanks
Thanks for the review.
--
FFmpeg = Furious and Faithful Mortal Purposeless Erratic Genius
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavfi-port-sab-filter-from-libmpcodecs.patch
Type: text/x-diff
Size: 17336 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130603/a56db5cf/attachment.bin>
More information about the ffmpeg-devel
mailing list