On Thu, Aug 18, 2016 at 19:26:39 +0000, Davinder Singh wrote:
> +@table @option
> +@item algo
> +Set the algorithm to be used. Accepts one of the following values:
> +
> +@table @samp
> +@item ebma
> +Exhaustive block matching algorithm.
> +@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
