Motion vector wrapping with AMVR

[bheng]

Motion vector wrapping to 18-bit range, like below, occurs in several places.

"uLX[ 0 ] = ( mvpLX[ 0 ] + mvdLX[ 0 ] + 2¹⁸ ) % 2¹⁸
mvLX[ 0 ][ 0 ][ 0 ] = ( uLX[ 0 ] >= 2¹⁷ ) ? ( uLX[ 0 ] − 2¹⁸ ) : uLX[ 0 ]"

The logic used here assumes (mvpLX[ 0 ] + mvdLX[ 0 ]) is always less than or equal to -2¹⁸, otherwise the result is undefined (modulo not defined for negative x).

However, I don't believe this assumption is guaranteed to be true.

The MVD range is 18 bits, from the following restriction:

"The value of lMvd[ compIdx ] shall be in the range of −2¹⁷ to 2¹⁷ − 1, inclusive."

This value is assigned to MvdL0:

"MvdL0[ x0 ][ y0 ][ compIdx ] is set equal to lMvd[ compIdx ]"

then MvdL0 is modified by AmvrShift (which could be 6).

"MvdL0[ x0 ][ y0 ][ 0 ] = MvdL0[ x0 ][ y0 ][ 0 ] << AmvrShift"

and the resulting MvdL0 could be -2²³. In which case, the wrapping logic above would fail to work correctly.

I have not found any further restriction on MvdL0 after AMVR is applied. Just that original restriction on lMvd.

There's a note that says the resulting vectors are in the 18-bit range, but that is just an informational statement, not a guarantee or a conformance restriction.

I believe changing all the "+ 2¹⁸" to "+ 2²⁴" would be one way to solve the problem.

[fbossen]

The same issue seems to occur with the affine mode although in that case it might fail even with AmvrShift set to 0 as two mvds are added in (653). The same suggested solution should work there.

Somewhat related to this, there is a restriction on MvdL1 in 7.4.12.8:
When sym_mvd_flag[ x0 ][ y0 ] is equal to 1, the value of MvdL1[ x0 ][ y0 ][ compIdx ] shall be in the range of −2¹⁷ to 2¹⁷ − 1, inclusive.
However it is not very clear when that restriction applies, if at all, since mvd_coding (the topic of 7.4.12.8) is not invoked with refList=1 when sym_mvd_flag is true. SW checks for that restriction right after setting MvdL1 to -MvdL0.

[bbross]

Thanks for bringing that up. Two things:

• How is the wrap-around done in VTM, is the offset 2¹⁸ as well?
• Can other experts comment on

a) Would the following fix the issue:
uLX[ 1 ] = ( mvpLX[ 1 ] + mvdLX[ 1 ] + 2²⁴ ) % 2¹⁸ (464)
uLX[ 1 ] = ( mvpLX[ 1 ] + mvdLX[ 1 ] + 2²⁴ ) % 2¹⁸ (464)
uLX[ cpIdx ][ 0 ] = ( mvpCpLX[ cpIdx ][ 0 ] + mvdCpLX[ cpIdx ][ 0 ] + 2²⁵ ) % 2¹⁸ (655)
uLX[ cpIdx ][ 1 ] = ( mvpCpLX[ cpIdx ][ 1 ] + mvdCpLX[ cpIdx ][ 1 ] + 2²⁵ ) % 2¹⁸ (657)
b) What about IBC?
u[ 0 ] = ( bvL[ 0 ] + bvd[ 0 ] + 2¹⁸ ) % 2¹⁸ (1083)
u[ 1 ] = ( bvL[ 1 ] + bvd[ 1 ] + 2¹⁸ ) % 2¹⁸ (1085)
c) the issue raised by Frank about the MvdL1 restriction in case of SMVD?

[fbossen]

The VTM does the following (which I would say is correct, except for a typo in a variable name):

hor = (hor + mvClipPeriod) & (mvClipPeriod - 1);
hor = (hor >= halMvClipPeriod) ? (hor - mvClipPeriod) : hor;

where mvClipPeriod is 2¹⁸ and halMvClipPeriod is 2¹⁷

IBC needs to be fixed in the same way

[bbross]

Thanks Frank, how about the following fix to prevent modulo for negative numbers as in VTM:

uLX[ 0 ] = ( mvpLX[ 0 ] + mvdLX[ 0 ] + 2¹⁸ ) & (2¹⁸-1) (462)
uLX[ 1 ] = ( mvpLX[ 1 ] + mvdLX[ 1 ] + 2¹⁸ ) & (2¹⁸-1) (464)

uLX[ cpIdx ][ 0 ] = ( mvpCpLX[ cpIdx ][ 0 ] + mvdCpLX[ cpIdx ][ 0 ] + 2¹⁸ ) & (2¹⁸-1) (655)
uLX[ cpIdx ][ 1 ] = ( mvpCpLX[ cpIdx ][ 1 ] + mvdCpLX[ cpIdx ][ 1 ] + 2¹⁸ ) & (2¹⁸-1) (657)

u[ 0 ] = ( bvL[ 0 ] + bvd[ 0 ] + 2¹⁸ ) & ( 2¹⁸ − 1 ) (1083)
u[ 1 ] = ( bvL[ 1 ] + bvd[ 1 ] + 2¹⁸ ) & ( 2¹⁸ − 1 ) (1085)

Apart from that, any opinions the MvdL1 restriction in case of SMVD. Is there an issue?

[bheng]

I might be overlooking something, but it's not clear to me what purpose the + 2¹⁸ would serve in either the VTM code or the text above. It will just get masked-off immediately by the &.

Also, in other places within the text, the result of & with a negative number is understood to be treated as an unsigned value. So, the result from & (2¹⁸-1) would always be positive, and that's not really what you want here.

I think it either needs the additional conditional subtraction used in VTM, or if you prefer it without a conditional statement, something like:

( ( mvpLX[ 0 ] + mvdLX[ 0 ] + 2¹⁷ ) & (2¹⁸-1) ) - 2¹⁷

[bbross]

Don't we have that additional additional conditional subtraction in the draft already in the equations that follow the modified ones from my comment above. My suggested fix in complete context would be e.g.:

uLX[ 0 ] = ( mvpLX[ 0 ] + mvdLX[ 0 ] + 2¹⁸ ) & (2¹⁸-1) (462)
mvLX[ 0 ][ 0 ][ 0 ] = ( uLX[ 0 ] >= 2¹⁷ ) ? ( uLX[ 0 ] − 2¹⁸ ) : uLX[ 0 ] (463)

Apart from question whether "+ 2¹⁸ " is useless, I understand that that would match VTM or am I missing an equation here?

[bheng]

Sorry, I originally understood your suggestion to also include removing the conditional equations (equations 463 and 465). You are correct, it's fine with the full context there. Thanks.

[bbross]

Great, so I can keep the fixes in my current draft. Thanks for confirming!

[fbossen]

Brian is correct about the +2¹⁸ being superfluous in the VTM code and suggested text.

Regarding the issue with MvdL1 and SMVD, one way to address it is to remove the following from the syntax table in 7.3.11.5:

MvdL1[ x0 ][ y0 ][ 0 ] = −MvdL0[ x0 ][ y0 ][ 0 ]
MvdL1[ x0 ][ y0 ][ 1 ] = −MvdL0[ x0 ][ y0 ][ 1 ]

And replace it by the following in 7.4.12.8:

– When sym_mvd_flag[ x0 ][ y0 ] is equal to 1, MvdL1[ x0 ][ y0 ][ compIdx ] is set equal to −lMvd[ compIdx ] for compIdx = 0..1.

That way it is clear that the following restriction applies and when:

When sym_mvd_flag[ x0 ][ y0 ] is equal to 1, the value of MvdL1[ x0 ][ y0 ][ compIdx ] shall be in the range of −2¹⁷ to 2¹⁷ − 1, inclusive.

[bbross]

The suggested clarification for SMVD falls short on the same problem. When moving the MvdL1 derivation for SMVD to mvd_coding semantics, but keeping the condition io sym_mvd_flag in CU syntax, mvd_coding is never called for L1 in case of sym_mvd_flag==1.
Hence I suggest rephrasing the restriction so it applies to MvdL0 for which mvd_coding is called in case of SMVD:

When all of the following conditions are true, the value of -MvdL0[ x0 ][ y0 ][ compIdx ] shall be in the range of −217 to 217 − 1, inclusive:

• sym_mvd_flag[ x0 ][ y0 ] is equal to 1.
• inter_pred_idc[ x0 ][ y0 ] is not PRED_L0.
• ph_mvd_l1_zero_flag is equal to 0.

[bbross]

Actually, the additional conditions are not need since they are always true when sym_mvd_flag is equal to 1.

[bbross]

Proposed fix:

When sym_mvd_flag[ x0 ][ y0 ] is equal to 1, the value of −MvdL0[ x0 ][ y0 ][ compIdx ] shall be in the range of −217 to 217 − 1, inclusive.

[fbossen]

That works. If you want to make it shorter:

When sym_mvd_flag[ x0 ][ y0 ] is equal to 1, the value of MvdL0[ x0 ][ y0 ][ compIdx ] shall not be equal to −2¹⁷.

[bbross]

Nice! If we would not be done with v1, I see at least three bug fix contributions in this ticket ;)
Will use the short version.

[bbross]

Fixed as discussed/suggested in JVET-T0110-v2.