Fix for Ticket #153

• MMVD selecting candidate GBI index from incorrect array.

The unchanged code here looks correct. This function (void MergeCtx::setMmvdMergeCandiInfo(PredictionUnit& pu, int candIdx) is only for MMVD and the input parameter candIdx is actually MMVD index. So it is converted to regular merge index fPosBaseIdx. GBI information is stored in GBiIdx with regular merge index.

Please take another look.

In getInterMMVDMergeCandidates(), mmvdBaseMv list is constructed by selecting up to 2 candidates (MMVD_BASE_MV_NUM=2) from the regular merge list (of up to 7 candidates).

In setMmvdMergeCandiInfo, fPosBaseIdx is derived as an index into mmvdBaseMv list. So, it has values 0 or 1. It is not the regular merge index (0-6).

GBiIdx is an array of up to 7 values corresponding to candidates in the regular merge list.

Using fPosBaseIdx (0-1) to lookup a value in GBiIdx array (up to 7 values) is incorrect. The same is true for interDirNeighbours.

In addition, if maxNumMergeCand is set to 1 in the slice header, fPosBaseIdx=1 will exceed the size of the GBiIdx list (only 1 entry) and will access uninitialized memory.

There are certainly other ways to solve the problem. You could map fPosBaseIdx back to a regular merge candidate index, for example. But left as it is today, it is a problem.

Thanks, Brian

MMVD can only use the first two regular merge candidates. Using fPosBaseIdx looks good to me. I didn't get what is wrong. But when maxNumMergeCand is 1, yes, I think we have issue here. In this case, fPosBaseIdx has to be zero.

• "MMVD can only use the first two regular merge candidates. Using fPosBaseIdx looks good to me. I didn't get what is wrong"

For example, if regular merge candidate 0 is of type MRG_TYPE_SUBPU_ATMVP, then MMVD will not use that one and will use regular merge candidate 1 and 2 instead, so fPosBaseIdx will be off by 1.

fPosBaseIdx = 0 --> mergeCandIdx = 1 fPosBaseIdx = 1 --> mergeCandIdx = 2

They don't match up without a proper mapping.

• "But when maxNumMergeCand is 1, yes, I think we have issue here. In this case, fPosBaseIdx has to be zero."

We currently have a problem yes, but I don't see why fPosBasIdx has to be zero? The MMVD list derivation process has a final step to fill out the list with zero MV (up to MMVD_BASE_MV_NUM=2), so MMVD list will always have two vectors and fPosBaseIdx can still be 1.

It shouldn't be a problem if we solve the issue of not using the wrong list to lookup GBI index.

As ATMVP was moved to sub-block merge branch, fPosBaseIdx should align with regular merge index for the first two candidates.

Regarding to MMVD list construction, yes, you are right. Zero MV will be filled if MMVD_BASE_MV_NUM is not reached.

In this case, no issue?

You're right about ATMVP, sorry that example was not correct. Are there any other cases where getInterMMVDMergeCandidates will not find (mrgCtx.mrgTypeNeighbours[k] == MRG_TYPE_DEFAULT_N) during MMVD list construction process? IBC maybe? Or have all those cases been removed now?

If there are any cases like that remaining, then I think the problem could still exist and fPosBaseIdx might not always align with regular merge index.

But if you are sure that all those cases have been removed, then yes the current approach is OK.

As discussed earlier, the current code will still access uninitialized memory if maxNumMergeCand is set to 1 in the slice header, and someone selects fPosBasedIdx=1 to choose the zero-MV MMVD base candidate. So, you might want to fix that. If not with my proposed changes, then whatever approach you prefer would be fine.

In current code, IBC skip/merge must have mmvd related flag off. So we don't have an issue now. It looks people are proposing separated IBC and regular merge lists.

Regarding to maxNumMergeCand=1, yes, we may access uninitialized memory. In this case, it may be more straightforward to fix in function getInterMMVDMergeCandidates as follows, where I put mrgCtx.GBiIdx[k] = GBI_DEFAULT; and a line below it at last. Is it OK?

  if (currBaseNum < MMVD_BASE_MV_NUM)   
  {   
    for (k = currBaseNum; k < MMVD_BASE_MV_NUM; k++)   
    {   
#if JVET_M0068_M0171_MMVD_CLEANUP   
      mrgCtx.mmvdBaseMv[k][0] = MvField(Mv(0, 0), 0);
      const Slice &slice = *pu.cs->slice;
      mrgCtx.mmvdBaseMv[k][1] = MvField(Mv(0, 0), (slice.isInterB() ? 0 : -1));
#else
      mrgCtx.mmvdBaseMv[k][0] = MvField(Mv(0, 0), 0);
      mrgCtx.mmvdBaseMv[k][0] = MvField(Mv(0, 0), 0);
#endif
      mrgCtx.GBiIdx[k] = GBI_DEFAULT;
      mrgCtx.interDirNeighbours[k] = (mrgCtx.mmvdBaseMv[k][0].refIdx >= 0) + (mrgCtx.mmvdBaseMv[k][1].refIdx >= 0) * 2;
    }
  }

@XiangLi That looks good to me. Thanks for taking care of it. - Brian

@bheng I created another merge request !304 (merged) (!304 (merged)) for the fix mentioned above. Please take a look. If it is fine, I will close this one and merge !304 (merged).

@XiangLi Looks good to me. Thanks.

closed