[FFmpeg-devel] [PATCH v5] Add lensfun filter

Moritz Barsnick barsnick at gmx.net
Thu Jul 12 17:29:00 EEST 2018


On Thu, Jul 12, 2018 at 21:17:42 +0900, Stephen Seo wrote:
> +  --enable-lensfun         enable lensfun lens correction [no]

I may be mistaken, but aren't (new) dependencies to external libraries
coded as "libfoo"? So "--enable-liblensfun"?

>  EXTERNAL_LIBRARY_VERSION3_LIST="
>      gmp
> +    lensfun

Same here then of course.

> +The focal length of the image/video (zoom; expected constant for video). For
                                              ^^^^

I now what you mean, but the wording does not seem clear enough.
Perhaps note that either constant value is assumed throughout the
video, or a formula has to be provided via timeline support.

> + at table @samp
> + at item nearest
> + at item linear
> +(default)

Unwrap that last line (though it shouldn't matter).

>  OBJS-$(CONFIG_ZSCALE_FILTER)                 += vf_zscale.o
> +OBJS-$(CONFIG_LENSFUN_FILTER)                += vf_lensfun.o

These are meant to be alphabetical.

> +#define LANCZOS_RESOLUTION 256

> +        av_log(ctx, AV_LOG_INFO, "Using camera %s\n", lensfun->camera->Model);
[...]
> +        av_log(ctx, AV_LOG_FATAL, "Failed to find camera in lensfun database\n");

If you're logging the model on success, you might as well also log it
on failure.

> +        av_log(ctx, AV_LOG_INFO, "Using lens %s\n", lensfun->lens->Model);
[...]
> +        av_log(ctx, AV_LOG_FATAL, "Failed to find lens in lensfun database\n");

Same here.

> +    if (!lensfun->modifier) {
> +        if (lensfun->camera && lensfun->lens) {
[...]
> +        } else {
> +            return AVERROR_INVALIDDATA;
> +        }

I'm not sure this is what AVERROR_INVALIDDATA is meant to be used for.

> +                // apply only subpixsel distortion
                                 ^ subpixel

> +            for (index = 0; index < 4 * LANCZOS_RESOLUTION; ++index) {
> +                a = sqrt((float)index / LANCZOS_RESOLUTION);
> +                if (a == 0.0f) {
> +                    lensfun->interpolation[index] = 1.0f;
> +                } else {
> +                    lensfun->interpolation[index] = lanczos_kernel(a);
> +                }
> +            }

Is it not so that a is only == 0.0f if index == 0? Instead of doing a
floating point operation and a sketchy floating point compare (I know,
comparison with zero is okay), just check for index == 0 before
calculating a?

Something like:
            for (index = 0; index < 4 * LANCZOS_RESOLUTION; ++index) {
                if (index == 0) {
                    lensfun->interpolation[index] = 1.0f;
                } else {
                    a = sqrt((float)index / LANCZOS_RESOLUTION);
                    lensfun->interpolation[index] = lanczos_kernel(a);
                }
            }

> +                            thread_data->data_out[x * 3 + rgb_index + y * thread_data->linesize_out] = interpolated < 0.0f ? 0.0f : interpolated > 255.0f ? 255 : interpolated;
                                                                                                                                                               ^ 255.0f
(Assign 255 as float, like you do with the other constants here.)

> +        distortion_correction_thread_data.width = inlink->w;
> +        distortion_correction_thread_data.height = inlink->h;
> +        distortion_correction_thread_data.distortion_coords = lensfun->distortion_coords;
> +        distortion_correction_thread_data.data_in = in->data[0];
> +        distortion_correction_thread_data.data_out = out->data[0];
> +        distortion_correction_thread_data.linesize_in = in->linesize[0];
> +        distortion_correction_thread_data.linesize_out = out->linesize[0];
> +        distortion_correction_thread_data.interpolation = lensfun->interpolation;
> +        distortion_correction_thread_data.mode = lensfun->mode;
> +        distortion_correction_thread_data.interpolation_type = lensfun->interpolation_type;

I think there's a more compact way to do this with a compound literal
assignment or designated intializers, and ffmpeg code allows for these
particular C99 features.

Cheers,
Moritz


More information about the ffmpeg-devel mailing list