[FFmpeg-devel] [PATCH] Adding dejudder filter to remove judder produced by partially telecined material.

[Stefano Sabatini]

Subject nit:
lavfi: add dejudder filter to remove judder ...

On date Saturday 2014-02-08 10:21:49 -0500, Nicholas Robbins encoded:
> Signed-off-by: Nicholas Robbins <nickrobbins at yahoo.com>
> ---
>  Changelog                 |   1 +
>  doc/filters.texi          |  19 ++++++
>  libavfilter/Makefile      |   1 +
>  libavfilter/allfilters.c  |   1 +
>  libavfilter/vf_dejudder.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 184 insertions(+)
>  create mode 100644 libavfilter/vf_dejudder.c
> 
> diff --git a/Changelog b/Changelog
> index 7171c90..2f1b845 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -24,6 +24,7 @@ version <next>
>  - OpenGL device
>  - Use metadata_header_padding to control padding in ID3 tags (currently used in
>    MP3, AIFF, and OMA files), FLAC header, and the AVI "junk" block.
> +- dejudder filter
>  
>  
>  version 2.1:
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 2639f8c..e5a7581 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -3215,6 +3215,24 @@ Set whether or not chroma is considered in the metric calculations. Default is
>  @code{1}.
>  @end table
>  
> + at section dejudder
> +
> +Remove judder. Judder can be introduced, for instance, by @ref{pullup} filter.

Remove judder produced by partially interlaced telecined content.

Judder can be introduced ...

Note: I'm always suprised by the amont of cr at p relared to interlaced
content (it seems most our filters are used to fix one problem or
another related to it...).

> +If the original source was partially telecined content then the output of
> + at code{-vf pullup,dejudder} will have a variable frame rate. May change the

@code{pullup,dejudder}, -vf should not be used since this
documentation is not necessarily read by ffmpeg tool users.

> +recorded frame rate of the container. Aside from that change, this filter will
> +not affect constant frame rate video.
> +

> +This filter accepts the following option:

options:

> + at table @option
> +
> + at item cycle
> +Specify the length of the window over which the judder repeats. If the original
> +was telecined from 24 to 30 fps (Film to NTSC), then use @code{4}. If the original was
> +telecined from 25 to 30 fps (PAL to NTSC), then use @code{5}. If a mixture of the two
> +use @code{20}. The default is @code{4}.
> + at end table
> +
>  @section delogo
>  
>  Suppress a TV station logo by a simple interpolation of the surrounding
> @@ -6637,6 +6655,7 @@ On this example the input file being processed is compared with the
>  reference file @file{ref_movie.mpg}. The PSNR of each individual frame
>  is stored in @file{stats.log}.
>  
> + at anchor{pullup}
>  @section pullup
>  
>  Pulldown reversal (inverse telecine) filter, capable of handling mixed
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 05ec5f2..c36b67e 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -127,6 +127,7 @@ OBJS-$(CONFIG_CROPDETECT_FILTER)             += vf_cropdetect.o
>  OBJS-$(CONFIG_CURVES_FILTER)                 += vf_curves.o
>  OBJS-$(CONFIG_DCTDNOIZ_FILTER)               += vf_dctdnoiz.o
>  OBJS-$(CONFIG_DECIMATE_FILTER)               += vf_decimate.o
> +OBJS-$(CONFIG_DEJUDDER_FILTER)               += vf_dejudder.o
>  OBJS-$(CONFIG_DELOGO_FILTER)                 += vf_delogo.o
>  OBJS-$(CONFIG_DESHAKE_FILTER)                += vf_deshake.o
>  OBJS-$(CONFIG_DRAWBOX_FILTER)                += vf_drawbox.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 8e8df7b..d042b64 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -122,6 +122,7 @@ void avfilter_register_all(void)
>      REGISTER_FILTER(CURVES,         curves,         vf);
>      REGISTER_FILTER(DCTDNOIZ,       dctdnoiz,       vf);
>      REGISTER_FILTER(DECIMATE,       decimate,       vf);
> +    REGISTER_FILTER(DEJUDDER,       dejudder,       vf);
>      REGISTER_FILTER(DELOGO,         delogo,         vf);
>      REGISTER_FILTER(DESHAKE,        deshake,        vf);
>      REGISTER_FILTER(DRAWBOX,        drawbox,        vf);
> diff --git a/libavfilter/vf_dejudder.c b/libavfilter/vf_dejudder.c
> new file mode 100644
> index 0000000..d3fab06
> --- /dev/null
> +++ b/libavfilter/vf_dejudder.c
> @@ -0,0 +1,162 @@
> +/*
> + * Copyright (c) 2014 Nicholas Robbins
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * remove judder in video stream
> + */
> +
> +#include "libavutil/opt.h"
> +#include "libavutil/mathematics.h"
> +#include "avfilter.h"
> +#include "internal.h"
> +#include "video.h"
> +
> +typedef struct {
> +    const AVClass *class;

> +    int64_t *ringbuff;
> +    int a, b, c, d;

comments/doxy are welcome here, also "a, b, c, d" are not very
descriptive variable names.

> +    int64_t new_pts;
> +    int start_count;
> +
> +    /* options */
> +    int cycle;
> +} DejudderContext;
> +
> +#define OFFSET(x) offsetof(DejudderContext, x)
> +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM | AV_OPT_FLAG_VIDEO_PARAM
> +
> +static const AVOption dejudder_options[] = {
> +    {"cycle", "length of the cycle to use for dejuddering",

nit: set length ...

> +        OFFSET(cycle), AV_OPT_TYPE_INT, {.i64 = 4}, 2, 240, .flags = FLAGS},
> +    {NULL}
> +};
> +
> +AVFILTER_DEFINE_CLASS(dejudder);
> +
> +static int config_out_props(AVFilterLink *outlink)
> +{
> +    AVFilterContext *ctx = outlink->src;
> +    DejudderContext *dj = ctx->priv;
> +    AVFilterLink *inlink = outlink->src->inputs[0];
> +
> +    outlink->time_base = av_mul_q(inlink->time_base, av_make_q(1, 2 * dj->cycle));

> +    outlink->frame_rate = av_mul_q(inlink->frame_rate, av_make_q(2 * dj->cycle, 1));

What happens if frame_rate is not defined? Maybe you should abort in
that case.

> +
> +    av_log(ctx, AV_LOG_VERBOSE, "%d cycle dejudder.\n", dj->cycle);

nit: cycle:%d\n

> +
> +    return 0;
> +}
> +
> +

nit++: remove duplicated empty lines, here and below

> +static av_cold int dejudder_init(AVFilterContext *ctx)
> +{
> +    DejudderContext *dj = ctx->priv;
> +

> +    dj->ringbuff = av_mallocz(sizeof(int64_t) * (dj->cycle+2));

nit: sizeof(*dj->ringbuff) * ...)

> +    if (!dj->ringbuff)
> +        return AVERROR(ENOMEM);
> +
> +    dj->new_pts = 0;
> +    dj->a = 0;
> +    dj->b = 1;
> +    dj->c = 2;
> +    dj->d = 3;
> +    dj->start_count = dj->cycle + 2;
> +
> +    return 0;
> +}
> +
> +
> +static av_cold void dejudder_uninit(AVFilterContext *ctx)
> +{
> +    DejudderContext *dj = ctx->priv;
> +
> +    av_freep(&(dj->ringbuff));
> +}
> +
> +
> +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> +{
> +    int i;
> +    AVFilterContext *ctx  = inlink->dst;
> +    AVFilterLink *outlink = ctx->outputs[0];
> +    DejudderContext *dj   = ctx->priv;

> +    int64_t *judbuff      = dj->ringbuff;

you can probably remove the indirection and leave optims to the
compiler

> +    int64_t next_pts      = frame->pts;
> +    int64_t offset;
> +
> +

> +    if (dj->start_count) {
> +        dj->start_count--;
> +        dj->new_pts = next_pts * 2 * dj->cycle;

what happens in case pts == AV_NOPTS_VALUE?

> +    } else {
> +        if (next_pts < judbuff[dj->b]) {
> +            offset = next_pts + judbuff[dj->c] - judbuff[dj->d] - judbuff[dj->a];

> +            for (i = 0; i < dj->cycle + 2; i++) judbuff[i] += offset;

style: judbuff[i] += offset; in a separate line

> +        }
> +        dj->new_pts += (dj->cycle - 1) * (judbuff[dj->c] - judbuff[dj->a])
> +                    + (dj->cycle + 1) * (next_pts - judbuff[dj->d]);
> +    }
> +
> +    judbuff[dj->b] = next_pts;
> +    dj->a = dj->b;
> +    dj->b = dj->c;
> +    dj->c = dj->d;
> +    dj->d = (dj->d + 1) % (dj->cycle + 2);
> +
> +    frame->pts = dj->new_pts;
> +

> +    for (i = 0; i < dj->cycle + 2; i++)
> +        av_log(ctx, AV_LOG_DEBUG, "%"PRId64"\t", judbuff[i]);
> +    av_log(ctx, AV_LOG_DEBUG, "next=%"PRId64", new=%"PRId64"\n", next_pts, frame->pts);
> +
> +    return ff_filter_frame(outlink, frame);
> +}
> +
> +
> +static const AVFilterPad dejudder_inputs[] = {
> +    {
> +        .name         = "default",
> +        .type         = AVMEDIA_TYPE_VIDEO,
> +        .filter_frame = filter_frame,
> +    },
> +    { NULL }
> +};
> +
> +static const AVFilterPad dejudder_outputs[] = {
> +    {
> +        .name = "default",
> +        .type = AVMEDIA_TYPE_VIDEO,
> +        .config_props = config_out_props,
> +    },
> +    { NULL }
> +};
> +
> +AVFilter ff_vf_dejudder = {
> +    .name        = "dejudder",

> +    .description = NULL_IF_CONFIG_SMALL("Remove judder produced by pullup"),

missing ending point

[...]
-- 
FFmpeg = Fundamental and Fancy Mortal Perfectionist Ecletic Game