On Fri, Aug 19, 2016 at 1:50 AM Moritz Barsnick <barsn...@gmx.net> wrote:
> 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.
>
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
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
