[FFmpeg-devel] [GSoC] Motion Interpolation

[Moritz Barsnick]

On Thu, Aug 18, 2016 at 19:26:39 +0000, Davinder Singh wrote:

> + at table @option
> + at item algo
> +Set the algorithm to be used. Accepts one of the following values:
> +
> + at table @samp
> + at item ebma
> +Exhaustive block matching algorithm.
> + at end table
> +Default value is @samp{ebma}.
[...]
> +    { "method", "specify motion estimation method", OFFSET(method), AV_OPT_TYPE_INT, {.i64 = ME_METHOD_ESA}, ME_METHOD_ESA, ME_METHOD_UMH, FLAGS, "method" },
> +        CONST("esa",   "exhaustive search",                  ME_METHOD_ESA,   "method"),
> +        CONST("tss",   "three step search",                  ME_METHOD_TSS,   "method"),
> +        CONST("tdls",  "two dimensional logarithmic search", ME_METHOD_TDLS,  "method"),
> +        CONST("ntss",  "new three step search",              ME_METHOD_NTSS,  "method"),
> +        CONST("fss",   "four step search",                   ME_METHOD_FSS,   "method"),
> +        CONST("ds",    "diamond search",                     ME_METHOD_DS,    "method"),
> +        CONST("hexbs", "hexagon-based search",               ME_METHOD_HEXBS, "method"),
> +        CONST("epzs",  "enhanced predictive zonal search",   ME_METHOD_EPZS,  "method"),
> +        CONST("umh",   "uneven multi-hexagon search",        ME_METHOD_UMH,   "method"),

Documentation mismatches implementation. I think you forgot to adapt
the former to your modifications.
a) It's not "algo", it's "method".
b) Default is "esa", not the non-existent "ebma".
c) You should actually list all possible values.

Furthermore, documentation for minterpolate is missing.


> +#define COST_MV(x, y)\
> +{\
> +    cost = me_ctx->get_cost(me_ctx, x_mb, y_mb, x, y);\
> +    if (cost < cost_min) {\
> +        cost_min = cost;\
> +        mv[0] = x;\
> +        mv[1] = y;\
> +    }\
> +}

The recommendation for function macros is to wrap the definition into a
"do { } while (0)". You do do that in other places.

> +    if (!(cost_min = me_ctx->get_cost(me_ctx, x_mb, y_mb, x_mb, y_mb)))

Why not
       if (cost_min != me_ctx->get_cost(me_ctx, x_mb, y_mb, x_mb, y_mb))
??

> +    if (!(cost_min = me_ctx->get_cost(me_ctx, x_mb, y_mb, x_mb, y_mb)))
> +        return cost_min;

Same here and many other places. "!=" is a valid operator. ;)

> +    #if 1
> +        for (i = 0; i < 8; i++)
> +            COST_P_MV(x + dia[i][0], y + dia[i][1]);
> +    #else

These checks will disappear in the final version?


> +    { "fps", "specify the frame rate", OFFSET(frame_rate), AV_OPT_TYPE_RATIONAL, {.dbl = 60}, 0, INT_MAX, FLAGS },

Could you handle this with an AV_OPT_TYPE_VIDEO_RATE, made specially
for cases such as this?

> +    { "mb_size", "specify the macroblock size", OFFSET(mb_size), AV_OPT_TYPE_INT, {.i64 = 16}, 4, 16, FLAGS },
> +    { "search_param", "specify search parameter", OFFSET(search_param), AV_OPT_TYPE_INT, {.i64 = 32}, 4, INT_MAX, FLAGS },

You can drop the "specify the" part. Every option lets you specify
something. ;-)

> +//int term = (mv_x * mv_x + mv_y * mv_y);
> +//int term = (FFABS(mv_x - me_ctx->pred_x) + FFABS(mv_y - me_ctx->pred_y));
> +//fprintf(stdout, "sbad: %llu, term: %d\n", sbad, term);
> +    return sbad;// + term;

Needs to be fixed?

> +    avcodec_get_chroma_sub_sample(inlink->format, &mi_ctx->chroma_h_shift, &mi_ctx->chroma_v_shift); //TODO remove

To do.

> +            if (!(mi_ctx->int_blocks = av_mallocz_array(mi_ctx->b_count, sizeof(Block))))

!=

> +    if (mi_ctx->me_method == ME_METHOD_ESA)
> +        ff_me_search_esa(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_TSS)
> +        ff_me_search_tss(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_TDLS)
> +        ff_me_search_tdls(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_NTSS)
> +        ff_me_search_ntss(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_FSS)
> +        ff_me_search_fss(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_DS)
> +        ff_me_search_ds(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_HEXBS)
> +        ff_me_search_hexbs(me_ctx, x_mb, y_mb, mv);
> +    else if (mi_ctx->me_method == ME_METHOD_EPZS) {

This calls for a switch/case. (There was another place in the code
which I haven't quoted.) Readability wouldn't improve significantly,
but the advantage is that the compiler can check whether you forgot to
add code for certain values.

> +        #if CACHE_MVS
Will this stay in?

> +#if !CACHE_MVS
Ditto.

Sorry if I missed the fact that this patch isn't ready for production
yet, and I'm nitpicking lots of stuff.

Moritz