[FFmpeg-devel] [PATCH] telecine filter

[Derek Buitenhuis]

On 2013-04-08 5:19 AM, Paul B Mahol wrote:
> + at item pattern
> +String representing for how many fields a frame is to be displayed.
> +The default value is @code{23}.
> + at end table

This is a woefully inadequate explanation. Not only does it not explain
the format the string should be it, it breaks from the standard ordering
convention (which is calling it, e.g. "3:2 Pulldown" ). Examples are not
a substitute for an actual explanation.

Also, it is a grammatically invalid English sentence.

> +/**
> + * @file telecine filter, heavily based from mvp-player:TOOLS/vf_dlopen/telecine.c by
> + * Rudolf Polzer.
> + */

s/mvp/mpv/

> +        av_log(ctx, AV_LOG_ERROR, "empty pattern\n");
> +        return AVERROR(EINVAL);

"No pattern provided.\n"

Also, use AVERROR_INVALIDDATA.

> +            av_log(ctx, AV_LOG_ERROR, "invalid pattern\n");
> +            return AVERROR(EINVAL);

Misleading.

Try: "Provided pattern includes non-numeric characters.\n"

Also, use AVERROR_INVALIDDATA.

> +    if (len == 0) { // do not output any field from this frame

!len is the convention used in FFmpeg.

> +            // fill in the EARLIER field from the buffered pic
> +            av_image_copy_plane(
> +                &tc->frame[nout]->data[i][tc->frame[nout]->linesize[i] * tc->first_field],
> +                tc->frame[nout]->linesize[i] * 2,
> +                &tc->temp->data[i][inpicref->linesize[i] * tc->first_field],
> +                inpicref->linesize[i] * 2,
> +                tc->stride[i],
> +                (tc->planeheight[i] - tc->first_field + 1) / 2);
> +            // fill in the LATER field from the new pic
> +            av_image_copy_plane(
> +                &tc->frame[nout]->data[i][tc->frame[nout]->linesize[i] * !tc->first_field],
> +                tc->frame[nout]->linesize[i] * 2,
> +                &inpicref->data[i][inpicref->linesize[i] * !tc->first_field],
> +                inpicref->linesize[i] * 2,
> +                tc->stride[i],
> +                (tc->planeheight[i] - !tc->first_field + 1) / 2);

Not to be "that guy", but we don't indent like this in FFmpeg. This follows for
all calls to av_image_copy_plane.

> +        tc->frame[nout]->pts = AV_NOPTS_VALUE;

This seems broken behavior at best.

> +    .description   = NULL_IF_CONFIG_SMALL("Apply telecine process."),

"Apply a telecine pattern.\n"

- Derek