[FFmpeg-devel] [PATCH] lavfi: Port fspp to FFmpeg

[Stefano Sabatini]

On date Tuesday 2014-12-23 15:52:57 +0530, arwa arif encoded:
> On Tue, Dec 23, 2014 at 12:40 PM, Stefano Sabatini <stefasab at gmail.com>
> wrote:
[...]

Please learn to trim the original message when you reply. I had to
scroll down over 900 lines, and this wastes both bandwidth and time.

> > > +static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> > > +{
> > > +    AVFilterContext *ctx = inlink->dst;
> > > +    FSPPContext *fspp = ctx->priv;
> > > +    AVFilterLink *outlink = ctx->outputs[0];
> > > +    AVFrame *out = in;
> > > +
> > > +    int qp_stride = 0;
> > > +    uint8_t *qp_table = NULL;
> > > +    int i, bias;
> > > +    int custom_threshold_m[64];
> > > +
> > > +    bias = (1 << 4) + fspp->strength;
> > > +

> > > +    for (i = 0; i < 64; i++) //FIXME: tune custom_threshold[] and
> > remove this !
> > > +     custom_threshold_m[i] = (uint64_t)(custom_threshold[i] * (bias /
> > 71) + 0.5);
> >
> > style: indent
> >
> > 71. -> 71
> >
> > Also why are you not using int as in the original code?
> >
> 
> I am using int. Why shouldn't 71. be used?

No it was the other way around, and pointed out by Michael.

[...]
> The output differs a lot with and without -cpuflags 0.
> Updated the patch.

> From 05dc64b0048547221f63824b4158701f8257e15c Mon Sep 17 00:00:00 2001
> From: Arwa Arif <arwaarif1994 at gmail.com>
> Date: Sun, 14 Dec 2014 12:03:31 +0530
> Subject: [PATCH] lavfi: port mp=fspp to a native libavfilter filter
[...]
> diff --git a/libavfilter/version.h b/libavfilter/version.h
> index 4bd18f3..a405591 100644
> --- a/libavfilter/version.h
> +++ b/libavfilter/version.h
> @@ -30,8 +30,8 @@
>  #include "libavutil/version.h"
>  

>  #define LIBAVFILTER_VERSION_MAJOR  5
> -#define LIBAVFILTER_VERSION_MINOR  2
> -#define LIBAVFILTER_VERSION_MICRO 104
> +#define LIBAVFILTER_VERSION_MINOR  5
> +#define LIBAVFILTER_VERSION_MICRO 99

What's this? Please skip this hunk altogether or update against latest
master.

[...]
> +static void mul_thrmat_c(FSPPContext *p, int q)
> +{
> +    int a;
> +    for (a = 0; a < 64; a++)
> +        ((int8_t *)p->threshold_mtx)[a] = q * ((int8_t *)p->threshold_mtx_noq)[a];//ints faster in C
> +}

As pointed out by Michael I was badly sleepy when I suggested to
change short -> int8_t.

Lesson: don't trust me, and don't take reviewer's suggestions
acritically :-)

[...]
> +static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> +{
> +    AVFilterContext *ctx = inlink->dst;
> +    FSPPContext *fspp = ctx->priv;
> +    AVFilterLink *outlink = ctx->outputs[0];
> +    AVFrame *out = in;
> +
> +    int qp_stride = 0;
> +    uint8_t *qp_table = NULL;
> +    int i, bias;
> +    int custom_threshold_m[64];
> +
> +    bias = (1 << 4) + fspp->strength;
> +
> +    for (i = 0; i < 64; i++) //FIXME: tune custom_threshold[] and remove this !
> +        custom_threshold_m[i] = (uint64_t)(custom_threshold[i] * (bias / 71.0) + 0.5);

In the original code:

    for(i=0;i<64;i++) //FIXME: tune custom_threshold[] and remove this !
        custom_threshold_m[i]=(int)(custom_threshold[i]*(bias/71.)+ 0.5);

The cast to uint64_t may create differences (not sure in this case
though). In general I think it's better to keep the same type as the
original, and eventually change them later (possibly after we have a
test).

[...]

Test with a fixed qp value for comparison, as the mp=fspp is broken
with regards to qp passing. Also it should generate the same
output as with -cpuflags 0, but only in case the original filter
did. Report in any case.
-- 
FFmpeg = Friendly Fabulous Meaningless Power Elitarian Gargoyle