Opened 4 years ago

Closed 4 years ago

#1369 closed defect (fixed)

Motion vector wrapping with AMVR

Reported by: bheng Owned by:
Priority: minor Milestone:
Component: spec Version: VVC D10 vG
Keywords: Cc: ksuehring, bbross, XiangLi, fbossen, jvet@…

Description

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

"uLX[ 0 ] = ( mvpLX[ 0 ] + mvdLX[ 0 ] + 218 ) % 218
mvLX[ 0 ][ 0 ][ 0 ] = ( uLX[ 0 ] >= 217 ) ? ( uLX[ 0 ] − 218 ) : uLX[ 0 ]"

The logic used here assumes (mvpLX[ 0 ] + mvdLX[ 0 ]) is always less than or equal to -218, 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 −217 to 217 − 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 -223. 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 "+ 218" to "+ 224" would be one way to solve the problem.

Change history (15)

comment:1 Changed 4 years ago by 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 −217 to 217 − 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.

comment:2 Changed 4 years ago by bbross

Thanks for bringing that up. Two things:

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

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

comment:3 Changed 4 years ago by 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 218 and halMvClipPeriod is 217

IBC needs to be fixed in the same way

comment:4 Changed 4 years ago by bbross

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

uLX[ 0 ] = ( mvpLX[ 0 ] + mvdLX[ 0 ] + 218 ) & (218-1) (462)
uLX[ 1 ] = ( mvpLX[ 1 ] + mvdLX[ 1 ] + 218 ) & (218-1) (464)

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

u[ 0 ] = ( bvL[ 0 ] + bvd[ 0 ] + 218 ) & ( 218 − 1 ) (1083)
u[ 1 ] = ( bvL[ 1 ] + bvd[ 1 ] + 218 ) & ( 218 − 1 ) (1085)

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

comment:5 Changed 4 years ago by bheng

I might be overlooking something, but it's not clear to me what purpose the + 218 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 & (218-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 ] + 217 ) & (218-1) ) - 217

comment:6 Changed 4 years ago by 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 ] + 218 ) & (218-1) (462)
mvLX[ 0 ][ 0 ][ 0 ] = ( uLX[ 0 ] >= 217 ) ? ( uLX[ 0 ] − 218 ) : uLX[ 0 ] (463)

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

comment:7 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by bbross

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

comment:9 Changed 4 years ago by fbossen

Brian is correct about the +218 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 −217 to 217 − 1, inclusive.

comment:10 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by bbross

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

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by 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 −217.

comment:14 Changed 4 years ago by 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.

comment:15 Changed 4 years ago by bbross

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.