Opened 12 years ago Closed 12 years ago #887 closed defect (fixed)bug in merge mode motion vector derivation
Description
In TDecEntropy::decodePUWise(), TComMvField cMvFieldNeighbours[] is not reinitialized when the CU contains more than one PU with merge_flag equal to 1. This causes incorrectly derived merging candidates: diff --git a/source/Lib/TLibDecoder/TDecEntropy.cpp b/source/Lib/TLibDecoder/TDecEntropy.cpp index 07dd3ee..c9ec01a 100644 --- a/source/Lib/TLibDecoder/TDecEntropy.cpp +++ b/source/Lib/TLibDecoder/TDecEntropy.cpp @@ -155,7 +155,6 @@ Void TDecEntropy::decodePUWise( TComDataCU* pcCU, UInt uiAbsPartIdx, UInt uiDept UInt uiNumPU = ( ePartSize == SIZE_2Nx2N ? 1 : ( ePartSize == SIZE_NxN ? 4 : 2 ) ); UInt uiPUOffset = ( g_auiPUOffset[UInt( ePartSize )] << ( ( pcCU->getSlice()->getSPS()->getMaxCUDepth() - uiDepth ) << 1 ) ) >> 4; - TComMvField cMvFieldNeighbours[MRG_MAX_NUM_CANDS << 1]; // double length for mv of both lists UChar uhInterDirNeighbours[MRG_MAX_NUM_CANDS]; for ( UInt ui = 0; ui < pcCU->getSlice()->getMaxNumMergeCand(); ui++ ) @@ -172,6 +171,7 @@ Void TDecEntropy::decodePUWise( TComDataCU* pcCU, UInt uiAbsPartIdx, UInt uiDept decodeMergeFlag( pcCU, uiSubPartIdx, uiDepth, uiPartIdx ); if ( pcCU->getMergeFlag( uiSubPartIdx ) ) { + TComMvField cMvFieldNeighbours[MRG_MAX_NUM_CANDS << 1]; // double length for mv of both lists decodeMergeIndex( pcCU, uiPartIdx, uiSubPartIdx, ePartSize, uhInterDirNeighbours, cMvFieldNeighbours, uiDepth ); UInt uiMergeIndex = pcCU->getMergeIndex(uiSubPartIdx); if ( pcCU->getSlice()->getPPS()->getLog2ParallelMergeLevelMinus2() && ePartSize != SIZE_2Nx2N && pcSubCU->getWidth( 0 ) <= 8 ) Change History (6)comment:1 Changed 12 years ago by DefaultCC Plugin
comment:2 in reply to: ↑ description Changed 12 years ago by huiyongcomment:3 Changed 12 years ago by stefane
There are code paths for which some entries of cMvFieldNeighbours[] are not initialized but used later on. One way to fix this is by initializing all entries of cMvFieldNeighbours[] with m_iRefIdx = -1. I agree that my original patch might be wrong. Instead it would probably have to be done like this: diff --git a/source/Lib/TLibDecoder/TDecEntropy.cpp b/source/Lib/TLibDecoder/TDecEntropy.cpp index 07dd3ee..87e2423 100644 --- a/source/Lib/TLibDecoder/TDecEntropy.cpp +++ b/source/Lib/TLibDecoder/TDecEntropy.cpp @@ -186,6 +186,8 @@ Void TDecEntropy::decodePUWise( TComDataCU* pcCU, UInt uiAbsPartIdx, UInt uiDept } else { + for( UInt ui = 0; ui < (MRG_MAX_NUM_CANDS << 1); ++ui ) + cMvFieldNeighbours[ui].setMvField( TComMv( 0, 0 ), -1 ); uiMergeIndex = pcCU->getMergeIndex(uiSubPartIdx); pcSubCU->getInterMergeCandidates( uiSubPartIdx-uiAbsPartIdx, uiPartIdx, cMvFieldNeighbours, uhInterDirNeighbours, numValidMergeCand, uiMergeIndex ); }
A perhaps better solution is to make sure no code paths exist that don't initialize cMvFieldNeighbours[]. I have identified two: diff --git a/source/Lib/TLibCommon/TComDataCU.cpp b/source/Lib/TLibCommon/TComDataCU.cpp index ea24c8a..c081f13 100644 --- a/source/Lib/TLibCommon/TComDataCU.cpp +++ b/source/Lib/TLibCommon/TComDataCU.cpp @@ -2779,11 +2779,13 @@ Void TComDataCU::getInterMergeCandidates( UInt uiAbsPartIdx, UInt uiPUIdx, TComM } else { + pcMvFieldNeighbours[ ( uiArrayAddr << 1 ) + 1 ].setMvField( TComMv( 0, 0 ), -1 ); puhInterDirNeighbours[uiArrayAddr] = 1; } } else { + pcMvFieldNeighbours[ ( uiArrayAddr << 1 ) + 1 ].setMvField( TComMv( 0, 0 ), -1 ); puhInterDirNeighbours[uiArrayAddr] = 1; } if ( mrgCandIdx == iCount )
But I don't know if those are the only ones.
Best regards,
Stefan. comment:4 Changed 12 years ago by mcoban
cMvFielNeighbours[] should be initialized before inter merge candidates are derived. Encoder accesses unitialized ref_idx in xGetColMVP() function leading to encoder/decoder mismatches.
The following fix should address the problem. Index: Lib/TLibCommon/TComDataCU.cpp =================================================================== --- Lib/TLibCommon/TComDataCU.cpp (revision 3164) +++ Lib/TLibCommon/TComDataCU.cpp (working copy) @@ -2528,6 +2528,8 @@ for( UInt ui = 0; ui < getSlice()->getMaxNumMergeCand(); ++ui ) { abCandIsInter[ui] = false; + pcMvFieldNeighbours[ ( ui << 1 ) ].setRefIdx(NOT_VALID); + pcMvFieldNeighbours[ ( ui << 1 ) + 1 ].setRefIdx(NOT_VALID); } numValidMergeCand = getSlice()->getMaxNumMergeCand(); // compute the location of the current PU comment:5 Changed 12 years ago by stefane
This is to confirm that
a) the bug is still present in HM-10.0rc1 comment:6 Changed 12 years ago by ksuehring
the patch has been applied in r3350 Note: See
TracTickets for help on using
tickets. | This list contains all users that will be notified about changes made to this ticket. These roles will be notified: Reporter, Owner, Subscriber, Participant
|
Replying to stefane:
As far as I understand, the initialization of cMvFieldNeighbours[] in the code spot is not more than just a usual initialization, hence should cause no harm even when you initialize it with different values. Thus, I don’t think it’s necessary to reinitialize cMvFieldNeighbours[] for each PU. cMvFieldNeighbours[] will be overwritten for each PU anyway.
On the other hand, if you reinitialize cMvFieldNeighbours[] for each PU, then I believe it will cause an incorrect merging candidate for the second PU for some cases (i.e.., in the cases when the condition “(pcCUgetSlice()getPPS()getLog2ParallelMergeLevelMinus2() && ePartSize != SIZE_2Nx2N && pcSubCUgetWidth(0) <= 8)” in the code is satisfied.)
Best regards,
Hui Yong