[FFmpeg-devel] [Patch] New filter -- dejudder

[Lukasz Marek]

On 29.01.2014 18:52, Nicholas Robbins wrote:
> This filter removes the judder introduced by -vf pullup into videos that were partially telescened.
>
>
> 0001-Adding-dejudder-filter-to-remove-judder-produced-by-.patch
>
>
>  From 7f0d67bf66ba2f0ee47cd6bdde368e351e075844 Mon Sep 17 00:00:00 2001
> From: Nicholas Robbins<nickrobbins at yahoo.com>
> Date: Wed, 29 Jan 2014 12:47:44 -0500
> Subject: [PATCH] Adding dejudder filter to remove judder produced by partialy
>   telescened material.
>
> Signed-off-by: Nicholas Robbins<nickrobbins at yahoo.com>
> ---
>   libavfilter/Makefile      |   1 +
>   libavfilter/allfilters.c  |   1 +
>   libavfilter/vf_dejudder.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 164 insertions(+)
>   create mode 100644 libavfilter/vf_dejudder.c

Missing doc and change log update
You are inconsistent with spaces around =+-*/ personally I don't care 
much about it, but inconsistency in commit should be fixed.
Also spaces between if/for and ( are missing.

> +typedef struct {
> +    const AVClass *class;
> +    int64_t *ringbuff;
> +    int a,b,c,d;

Can they be more meaningful?

> +    int64_t new,next;

I know this is C, but I try to not use reserved words for C++. new can 
be replaced anyway with something more specific, like new_pts?
And also, new can be moved to local variable in filter_frame, and next 
variable seems to be not used at all.

> +    int startcount;

maybe 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 cycle to use for dejuddering",

I'm not native speaker so I may be wrong, but "length of the cycle" 
seems more natural to me.

> +        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));
> +
> +    av_log(ctx, AV_LOG_VERBOSE, "%d cycle dejudder.\n", dj->cycle );
> +
> +    return 0;
> +}
> +
> +
> +static av_cold int dejudder_init(AVFilterContext *ctx)
> +{
> +    DejudderContext *dj = ctx->priv;
> +
> +    dj->ringbuff=(int64_t *)av_mallocz(sizeof(int64_t)*(dj->cycle+2));

A cast seems to be unneeded.

> +    if (!dj->ringbuff) return AVERROR(ENOMEM);

Moving return to new line makes it a bit more readable.

> +
> +    dj->new=0;
> +    dj->a=0;
> +    dj->b=1;
> +    dj->c=2;
> +    dj->d=3;
> +    dj->startcount=dj->cycle+2;
> +
> +    return 0;
> +}
> +
> +
> +static av_cold void dejudder_uninit(AVFilterContext *ctx)
> +{
> +    DejudderContext *dj = ctx->priv;
> +
> +    av_freep(&(dj->ringbuff));
> +}
> +
> +
> +

no need to make 3 lines break.

> +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;
> +    int64_t next, offset;
> +
> +
> +    next = frame->pts;
> +
> +    if( dj->startcount ){
> +        (dj->startcount)--;
> +        dj->new = next*2*dj->cycle;
> +    } else {
> +        if(next<judbuff[dj->b]){
> +            offset = next+judbuff[dj->c]-judbuff[dj->d]-judbuff[dj->a];
> +            for(i=0;i<dj->cycle+2;i++) judbuff[i] += offset;
> +        }
> +        dj->new += (dj->cycle-1)*( judbuff[dj->c] - judbuff[dj->a] )
> +                    + (dj->cycle+1)*( next - judbuff[dj->d] ) ;
> +    }
> +
> +    judbuff[dj->b] = next;
> +    dj->a=dj->b;
> +    dj->b=dj->c;
> +    dj->c=dj->d;
> +    dj->d = (dj->d+1) % (dj->cycle+2);
> +
> +    frame->pts = dj->new;
> +
> +    for(i=0;i<dj->cycle+2;i++) av_log(ctx, AV_LOG_DEBUG, "%"PRId64"\t", judbuff[i]);

move av_log to new line.

> +    av_log(ctx, AV_LOG_DEBUG, "next=%"PRId64", new=%"PRId64"\n", next, 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"),
> +    .priv_size   = sizeof(DejudderContext),
> +    .priv_class  = &dejudder_class,
> +    .inputs      = dejudder_inputs,
> +    .outputs     = dejudder_outputs,
> +    .init        = dejudder_init,
> +    .uninit      = dejudder_uninit,
> +};

I don't know filters API nor algorithm so can review at this point of view.


-- 
Best Regards,
Lukasz Marek

Microsoft isn't evil, they just make really crappy operating systems. - 
Linus Torvalds