Refactor inter prediction core in RDO into a function#5071
Conversation
Extracts the innermost loop body of handle_inter_mode (which evaluates inter predictors, interpolation filters, and motion modes) into a dedicated helper function `evaluate_inter_predictor`. This refactoring preserves the exact loop nesting and data structures of Phase 2.3 to maintain perfect bit-exactness (zero stats difference). TAG=agy CONV=61238dcb-d4aa-4e6e-bc92-4f27ea3f6109
| inter_mode_info (*mode_info)[BAWP_OPTION_CNT][NUM_MV_PRECISIONS] | ||
| [MAX_REF_MV_SEARCH * MAX_REF_MV_SEARCH]; | ||
| int_mv ( | ||
| *save_mv)[NUM_MV_PRECISIONS][MAX_REF_MV_SEARCH * MAX_REF_MV_SEARCH][2]; |
There was a problem hiding this comment.
MAX_REF_MV_SEARCH * MAX_REF_MV_SEARCH appears in many places. How about defining a new macro for it so we can avoid repeating this long expression? I refactored this in one of my changes, but they sit in lossy changes now.
| inter_mode_info (*mode_info)[BAWP_OPTION_CNT][NUM_MV_PRECISIONS] | ||
| [MAX_REF_MV_SEARCH * MAX_REF_MV_SEARCH], | ||
| int_mv (*save_mv)[NUM_MV_PRECISIONS][MAX_REF_MV_SEARCH * MAX_REF_MV_SEARCH] | ||
| [2]) { |
There was a problem hiding this comment.
I noticed many functions take a dozen or more parameters, which really hurts readability. Should we pack them into a struct to cut down the parameter count? I think we should simplify these function APIs going forward — not for this commit, but something to address later. Just flagging it here.
| const TxfmSearchInfo *txfm_info, int num_planes, int tmp_rate_mv, | ||
| int rate2_nocoeff, motion_mode_candidate *motion_mode_cand) { | ||
| MACROBLOCKD *const xd = &x->e_mbd; | ||
| if (tmp_rd < *search_state->best_rd) { |
There was a problem hiding this comment.
How about change to
if (tmp_rd >= *search_state->best_rd) return;
In this way, it can avoid the length issue caused by indent?
|
|
||
| *mbmi = *it_ctx->base_mbmi; | ||
| int_mv tmp_cur_mv[2]; | ||
| int i; |
There was a problem hiding this comment.
can we not define the global i? Only define i in the for loop?
| const MB_MODE_INFO base_mbmi = *mbmi; | ||
| PredictorIterationContext it_ctx; | ||
| init_predictor_iteration_context( | ||
| &it_ctx, bsize, 0, ref_mv_idx[0], ref_mv_idx[1], |
There was a problem hiding this comment.
if use_optflow is always 0, then why do we need to define an item in the data structure for it?
| // including interpolation filter search, motion mode search, and updating | ||
| // the best search state. | ||
| static void evaluate_inter_predictor(AV2_COMP *const cpi, | ||
| TileDataEnc *tile_data, MACROBLOCK *x, |
There was a problem hiding this comment.
I think you've only simplified the workflow to avoid the deeply nested for-loops — you haven't had a chance to clean up the code inside the function itself yet, right?
| &best_rd_stats_uv, &best_mbmi, &best_xskip_txfm, best_blk_skip, | ||
| best_tx_type_map, best_cctx_type_map, best_cwp_costs, best_cwp_idxs, | ||
| &ref_best_rd, best_ref_mv_idx, &mode_info, &save_mv); | ||
|
|
There was a problem hiding this comment.
I have refactored the code before this scale_index for loop, just let you know so that you do not have to repeat the work
| // 3.) Do interpolation filter search | ||
| // 4.) Build the inter predictor | ||
| // 5.) Pick the motion mode | ||
| // 6.) Update stats if best so far |
There was a problem hiding this comment.
this comment is out of date now, could you please update them?
3 participants
Move the inter prediction evaluation core into a helper function. This allows unnesting of inter search loops, with sequential mode evaluation instead. No coding stats difference.