Opened 13 years ago Closed 13 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 13 years ago by DefaultCC Plugin
comment:2 in reply to: ↑ description Changed 13 years ago by huiyongcomment:3 Changed 13 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 13 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 13 years ago by stefane
This is to confirm that
a) the bug is still present in HM-10.0rc1 comment:6 Changed 13 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