[FFmpeg-devel] [PATCH] telecine filter

[Paul B Mahol]

On 4/8/13, Derek Buitenhuis <derek.buitenhuis at gmail.com> wrote:
> 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.

I will need hand for this because I'm not that guy to write valid English
sentence in this case.

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

Ugh, fixed.

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

Fixed.

>
> Also, use AVERROR_INVALIDDATA.

Fixed.

>
>> +            av_log(ctx, AV_LOG_ERROR, "invalid pattern\n");
>> +            return AVERROR(EINVAL);
>
> Misleading.
>
> Try: "Provided pattern includes non-numeric characters.\n"

Fixed.

>
> Also, use AVERROR_INVALIDDATA.

Fixed.

>
>> +    if (len == 0) { // do not output any field from this frame
>
> !len is the convention used in FFmpeg.

Fixed.

>
>> +            // 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.

Fixed.

>
>> +        tc->frame[nout]->pts = AV_NOPTS_VALUE;
>
> This seems broken behavior at best.

Yes, that why there is TODO, note that there is no perfect solution
possible with current lavfi API.
(FYI original code does not have this issue)

So I need fedback in that area.

>
>> +    .description   = NULL_IF_CONFIG_SMALL("Apply telecine process."),
>
> "Apply a telecine pattern.\n"

Fixed.

I can send new version once doc part is filled.

>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>