[FFmpeg-devel] [PATCH] telecine filter

Clément Bœsch ubitux at gmail.com
Mon Apr 8 03:11:02 CEST 2013


On Mon, Apr 08, 2013 at 12:33:00AM +0000, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  Changelog                 |   1 +
>  doc/filters.texi          |  38 +++++++
>  libavfilter/Makefile      |   1 +
>  libavfilter/allfilters.c  |   1 +
>  libavfilter/version.h     |   2 +-
>  libavfilter/vf_telecine.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 289 insertions(+), 1 deletion(-)
>  create mode 100644 libavfilter/vf_telecine.c
> 
> diff --git a/Changelog b/Changelog
> index 5faa414..bee29c3 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -16,6 +16,7 @@ version <next>:
>    filtergraph description to be read from a file
>  - OpenCL support
>  - audio phaser filter
> +- telecine filter
>  
>  
>  version 1.2:
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 483d8a1..9093bf9 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -428,6 +428,44 @@ slope
>  Determine how steep is the filter's shelf transition.
>  @end table
>  
> + at section telecine
> +
> +Apply telecine process to the video.
> +
> +The filter accepts parameters as a list of @var{key}=@var{value}
> +pairs, separated by ":".
> +
> +A description of the accepted parameters follows.
> +
> + at table @option
> + at item first_field

Maybe "Select the first field. Available values are:"

> + at table @option

@table @samp

> + at item t
> +top field first
> + at item b
> +bottom field first
> + at end table
> +

What is the default?

Also, is it really problematic to use "top" and "bottom" instead of one
letter?

> + at item pattern
> +String representing for how many fields a frame is to be displayed.
> + at end table
> +
> +Some typical patterns:
> +
> +NTSC output (30i):
> +27.5p: 32222
> +24p: 23 (classic)
> +24p: 2332 (preferred)
> +20p: 33
> +18p: 334
> +16p: 3444
> +
> +PAL output (25i):
> +27.5p: 12222
> +24p: 222222222223 ("Euro pulldown")
> +16.67p: 33
> +16p: 33333334
> +

A sentence or two about the pattern would be welcome.

I also see no note about the pts you are resetting. BTW, I still don't get
why you are not setting them in the filter itself.

[...]
> +#define OFFSET(x) offsetof(TelecineContext, x)
> +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> +
> +static const AVOption telecine_options[] = {
> +    {"first_field", "", OFFSET(first_field), AV_OPT_TYPE_INT, {.i64=0}, 0, 1, FLAGS, "field"},

"select first field" or something as description.

> +        {"t", "top", 0, AV_OPT_TYPE_CONST, {.i64=0}, 0, 0, FLAGS, "field"},
> +        {"b", "bottom", 0, AV_OPT_TYPE_CONST, {.i64=1}, 0, 0, FLAGS, "field"},

"top", "top field first"
"bottom", "bottom field first'

> +    {"pattern", "pattern that describe for how many fields a frame is to be displayed", OFFSET(pattern), AV_OPT_TYPE_STRING, {.str="23"}, 0, 0, FLAGS},
> +    {NULL}
> +};
> +
> +AVFILTER_DEFINE_CLASS(telecine);
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args)
> +{
> +    TelecineContext *tc = ctx->priv;
> +    const char *p;
> +    int max = 0;
> +
> +    for (p = tc->pattern; *p; p++)
> +        if (*p < '0' || *p > '9') {

av_isdigit()

Also, you can include this check in the following loop.

> +            av_log(ctx, AV_LOG_ERROR, "invalid pattern\n");
> +            return AVERROR(EINVAL);
> +        }
> +
> +    for (p = tc->pattern; *p; p++) {
> +        max = FFMAX(*p - '0', max);
> +        tc->pts.num += 2;
> +        tc->pts.den += *p - '0';
> +    }
> +

What if the string is empty?

Now, what if p is "0"? Isn't this going to /0 somewhere?

> +    tc->out_cnt = (max + 1) / 2;
> +    av_log(ctx, AV_LOG_INFO,
> +        "telecine pattern %s yields up to %d frames per frame, pts advance factor: %d/%d\n",
> +        tc->pattern, tc->out_cnt, tc->pts.num, tc->pts.den);
> +

nit: align

> +    return 0;
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    AVFilterFormats *pix_fmts = NULL;
> +    int fmt;
> +
> +    for (fmt = 0; fmt < AV_PIX_FMT_NB; fmt++) {
> +        const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(fmt);
> +        if (!(desc->flags & PIX_FMT_HWACCEL))
> +            ff_add_format(&pix_fmts, fmt);
> +    }
> +

So gray, rgb, alpha variants, 10-bits yuv, ... are supported?

> +    ff_set_common_formats(ctx, pix_fmts);
> +    return 0;
> +}
> +
> +static int config_input(AVFilterLink *inlink)
> +{
> +    TelecineContext *tc = inlink->dst->priv;
> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> +    int i, ret;
> +
> +    tc->temp = ff_get_video_buffer(inlink, inlink->w, inlink->h);
> +    if (!tc->temp)
> +            return AVERROR(ENOMEM);
> +    for (i = 0; i < tc->out_cnt; i++) {
> +        tc->frame[i] = ff_get_video_buffer(inlink, inlink->w, inlink->h);
> +        if (!tc->frame[i])
> +            return AVERROR(ENOMEM);
> +    }
> +
> +    if ((ret = av_image_fill_linesizes(tc->stride, inlink->format, inlink->w)) < 0)
> +        return ret;
> +
> +    tc->planeheight[1] = tc->planeheight[2] = inlink->h >> desc->log2_chroma_h;
> +    tc->planeheight[0] = tc->planeheight[3] = inlink->h;
> +
> +    tc->nb_planes = av_pix_fmt_count_planes(inlink->format);
> +
> +    return 0;
> +}
> +
> +static int filter_frame(AVFilterLink *inlink, AVFrame *inpicref)
> +{
> +    AVFilterContext *ctx = inlink->dst;
> +    AVFilterLink *outlink = ctx->outputs[0];
> +    TelecineContext *tc = ctx->priv;
> +    int i, len, ret = 0, nout = 0;
> +
> +    len = tc->pattern[tc->pattern_pos] - '0';
> +
> +    tc->pattern_pos++;
> +    if (!tc->pattern[tc->pattern_pos])
> +        tc->pattern_pos = 0;
> +
> +    if (len == 0) // do not output any field from this frame
> +        return 0;
> +
> +    if (tc->occupied) {
> +        for (i = 0; i < tc->nb_planes; i++) {
> +            // 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);
> +        }
> +        tc->frame[nout]->pts = AV_NOPTS_VALUE;
> +        nout++;
> +        len--;
> +        tc->occupied = 0;
> +    }
> +
> +    while (len >= 2) {
> +        // output THIS image as-is
> +        for (i = 0; i < tc->nb_planes; i++)
> +            av_image_copy_plane(
> +                tc->frame[nout]->data[i], tc->frame[nout]->linesize[i],
> +                inpicref->data[i], inpicref->linesize[i],
> +                tc->stride[i],
> +                tc->planeheight[i]);
> +        tc->frame[nout]->pts = AV_NOPTS_VALUE;
> +        nout++;
> +        len -= 2;
> +    }
> +
> +    if (len >= 1) {
> +        // copy THIS image to the buffer, we need it later
> +        for (i = 0; i < tc->nb_planes; i++)
> +            av_image_copy_plane(
> +                &tc->temp->data[i][0], inpicref->linesize[i],

Are you sure temp is going to have inpicref linesize all the time?

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130408/5295f11b/attachment.asc>


More information about the ffmpeg-devel mailing list