Opened 9 years ago

Closed 8 years ago

#887 closed defect (fixed)

bug in merge mode motion vector derivation

Reported by: stefane Owned by:
Priority: minor Milestone: HM-10.0
Component: HM Version: HM-9.0
Keywords: Cc: fbossen, ksuehring, davidf, jct-vc@…

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 9 years ago by DefaultCC Plugin

  • Cc fbossen ksuehring davidf jct-vc@… added

comment:2 in reply to: ↑ description Changed 9 years ago by huiyong

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 “(pcCUgetSlice()getPPS()getLog2ParallelMergeLevelMinus2() && ePartSize != SIZE_2Nx2N && pcSubCUgetWidth(0) <= 8)” in the code is satisfied.)

Best regards,
Hui Yong

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 )

comment:3 Changed 9 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 8 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
Last edited 8 years ago by ksuehring (previous) (diff)

comment:5 Changed 8 years ago by stefane

This is to confirm that

a) the bug is still present in HM-10.0rc1
b) applying the patch suggested above by mcoban resolves the observed mismatches

comment:6 Changed 8 years ago by ksuehring

  • Milestone changed from HM-9.1 to HM-10.0
  • Resolution set to fixed
  • Status changed from new to closed

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

  • David Flynn(Subscriber)
  • Frank Bossen(Subscriber)
  • Hui Yong Kim(Participant)
  • jct-vc@…(Subscriber)
  • karl.sharman@…(Always)
  • Karsten Suehring(Subscriber, Participant, Always)
  • Muhammed Coban(Participant)