[FFmpeg-devel] [GSoC] Motion Interpolation

[Davinder Singh]

On Fri, Aug 19, 2016 at 1:50 AM Moritz Barsnick <barsnick at gmx.net> wrote:

> 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.
>

docs are yet to be updated.


> > +#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.
>

will do.


>
> > +    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. ;)
>

yes, that would be in case of == operator, not = operator, no?


> > +    #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?
>
>
yes.


>
> > +    { "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?
>

ok, will look into it.


>
> > +    { "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. ;-)
>

sure. i thought of doing that while updating docs.


>
> > +//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.
>

will do. can you tell which is faster?


>
> > +        #if CACHE_MVS
> Will this stay in?
>

it will be removed.


>
> > +#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
>