[FFmpeg-devel] [PATCH] avfilter/vf_zoompan: fix shaking when zooming

Moritz Barsnick barsnick at gmx.net
Thu Jan 23 18:19:10 EET 2020


On Thu, Jan 23, 2020 at 13:40:55 +0100, Robert Deibel wrote:
> +/**
> + * Scales n to be a multiple of grid_size and divisible by two but minimally 2 * grid_size.
> + *
> + * Used to scale the width and height of a frame to fit with the subsampling grid.
> + * @param n The number to be scaled.
> + * @param grid_size the size of the grid.
> + * @return The scaled number divisible by 2, minimally 2 * grid_size

For a local static function, your documentation doesn't need to be
doygen, but I don't mind.

> + */
> +static int scale_to_grid(int n, uint8_t grid_size){

This is part of the style Paul meant: ffmpeg uses a newline for a
function's opening curly bracket. And only there (and some structs),
otherwise a space is required. -> ") {"

> +    return (((n + (1 << grid_size) * 2) & ~((1 << grid_size) - 1)) + 1) & ~1;

Even if this works perfectly for your case: Have you validated this
against the above specification? I get peculiar results. For which
sizes of n and grid_size does it work correctly?

> +    w = dw = (double) in->width * (1.0 / *zoom);
> +    h = dh = (double) in->height * (1.0 / *zoom);

Since the right hand expressions are now completely double, you can
probably drop the 1.0. Though I believe the compiler will optimize it
away anyway.

> +    dx_scaled = ((crop_w - dw) * *zoom) / (((double) crop_w - dw) / (*dx - (double) crop_x));
> +    x = ceil(av_clipd(dx_scaled, 0, FFMAX(overscaled_w - outlink->w, 0)));
> +    x &= ~((1 << s->desc->log2_chroma_w) - 1);
> +
> +    dy_scaled = ((crop_h - dh) * *zoom) / (((double) crop_h - dh) / (*dx - (double) crop_y));
> +    y = ceil(av_clipd(dy_scaled, 0, FFMAX(overscaled_h - outlink->h, 0)));
> +    y &= ~((1 << s->desc->log2_chroma_h) - 1);
> +
> +    px[1] = px[2] = AV_CEIL_RSHIFT(x, s->desc->log2_chroma_w);
> +    px[0] = px[3] = x;
> +
> +    py[1] = py[2] = AV_CEIL_RSHIFT(y, s->desc->log2_chroma_h);
> +    py[0] = py[3] = y;

One might be tempted to define a macro function MACRO() for declaring
MACRO(w, x);
MACRO(h, y);
to reduce the code duplication (and aid in maintaining), but I may be
overoptimizing this. ;-)

> +    for (k = 0; k<4; k++)
"k < 4"

Cheers,
Moritz


More information about the ffmpeg-devel mailing list