Commit 94085e61 for openh264
commit 94085e614baa78b3ad63151dce313ab36f7a6443
Author: tyan0 <takashi.yano@nifty.ne.jp>
Date: Tue Sep 15 17:43:58 2026 +0900
decoder: fix bi-predicted 16x8/8x16 macroblocks in B slices (#4004)
* decoder: fix bi-predicted 16x8/8x16 macroblocks in B slices
In GetInterBPred() the shared pMCRefMem destination is advanced to the
second partition from inside the reference-list loop, and it is never
reset between partitions. For a bi-predicted partition this goes wrong
in two ways:
- The BaseMC() call for LIST_1 writes over the LIST_0 prediction that is
already in pMCRefMem, so the following BiPrediction() (or
BiWeightPrediction()) combines LIST_1 with LIST_1. The partition ends
up holding a plain LIST_1 prediction instead of the average of both
lists.
- For the second partition (i == 1) the "if (i)" offset is applied once
per list rather than once per partition, so the destination is
advanced twice. The LIST_1 motion compensation and the combine step
then write outside the macroblock, into the neighbouring one, and the
partition itself keeps the plain LIST_0 prediction.
Because Intra_16x16 DC and Vertical prediction copy from the left and top
neighbours, a single affected macroblock is enough to smear the error
across the rest of the picture, and reference B pictures propagate it
into later frames.
Derive the destination of each partition from a saved macroblock base,
predict the first list of a partition into sRef and the second into
sTempRef so that the second BaseMC() no longer clobbers the first, and
combine once after the list loop.
The two Cisco_Men_whisper_640x320_*_Bframe_9 vectors in res/ exercise
this path. Before the fix openh264 differs from independent decoders on
4 of their 9 frames; afterwards the output is bit-exact with both the
Microsoft Media Foundation and the Intel Quick Sync decoders, which agree
with each other. Their expected hashes in test/api/decoder_test.cpp are
updated accordingly, as are those of the VID_*_temporal_direct vectors,
whose output also changes.
Of the 54 decodable vectors in res/, 46 are unchanged by this commit.
test/api/thread_decoder_test.cpp keeps its own table for the same vectors
and is deliberately left alone: the multi-threaded decoder is currently
non-deterministic, so no stable expected hash can be produced for it.
Six runs of one binary over res/VID_1280x544_cabac_temporal_direct.264
at a fixed thread count gave six different digests, while single-threaded
decoding is reproducible. That is a pre-existing problem unrelated to
this change and is reported separately.
Fixes #3712
* decoder: avoid copying sMCRefMember to silence -Wmaybe-uninitialized
The previous version saved the macroblock base by copying the whole
sMCRefMember struct. At that point the pSrc* members are still
uninitialized (GetRefPic() fills them in later), so some GCC versions
report "pMCRefMem may be used uninitialized" and the meson --werror
build fails.
Set only the three destination pointers instead, taking the macroblock
base straight from the pPredYCbCr / pTempPredYCbCr parameters. Decoded
output is byte-identical to the previous version.
diff --git a/codec/decoder/core/src/rec_mb.cpp b/codec/decoder/core/src/rec_mb.cpp
index 2fbf7877..8e025c65 100644
--- a/codec/decoder/core/src/rec_mb.cpp
+++ b/codec/decoder/core/src/rec_mb.cpp
@@ -742,41 +742,39 @@ int32_t GetInterBPred (uint8_t* pPredYCbCr[3], uint8_t* pTempPredYCbCr[3], PWels
int32_t iPartIdx = i << 3;
uint32_t listCount = 0;
int32_t lastListIdx = LIST_0;
+ // The partition offset belongs to the partition, not to the reference
+ // list, so both destinations are (re)set once here for each partition.
+ pMCRefMem.pDstY = pPredYCbCr[0] + (i ? (iDstLineLuma << 3) : 0);
+ pMCRefMem.pDstU = pPredYCbCr[1] + (i ? (iDstLineChroma << 2) : 0);
+ pMCRefMem.pDstV = pPredYCbCr[2] + (i ? (iDstLineChroma << 2) : 0);
+ pTempMCRefMem.pDstY = pTempPredYCbCr[0] + (i ? (iDstLineLuma << 3) : 0);
+ pTempMCRefMem.pDstU = pTempPredYCbCr[1] + (i ? (iDstLineChroma << 2) : 0);
+ pTempMCRefMem.pDstV = pTempPredYCbCr[2] + (i ? (iDstLineChroma << 2) : 0);
for (int32_t listIdx = LIST_0; listIdx < LIST_A; ++listIdx) {
if (IS_DIR (iMBType, i, listIdx)) {
+ // The first list of a partition predicts into pMCRefMem and the second
+ // into pTempMCRefMem, so the second BaseMC() no longer overwrites the
+ // prediction made for the first list.
+ sMCRefMember* pTarget = (listCount == 0) ? &pMCRefMem : &pTempMCRefMem;
lastListIdx = listIdx;
iMVs[0] = pCurDqLayer->pDec->pMv[listIdx][iMBXY][iPartIdx][0];
iMVs[1] = pCurDqLayer->pDec->pMv[listIdx][iMBXY][iPartIdx][1];
iRefIndex = pCurDqLayer->pDec->pRefIndex[listIdx][iMBXY][iPartIdx];
- WELS_B_MB_REC_VERIFY (GetRefPic (&pMCRefMem, pCtx, iRefIndex, listIdx));
- if (i) {
- pMCRefMem.pDstY += (iDstLineLuma << 3);
- pMCRefMem.pDstU += (iDstLineChroma << 2);
- pMCRefMem.pDstV += (iDstLineChroma << 2);
- }
- BaseMC (pCtx, &pMCRefMem, listIdx, iRefIndex, iMBOffsetX, iMBOffsetY + iPartIdx, pMCFunc, 16, 8, iMVs);
- if (++listCount == 2) {
- iMVs[0] = pCurDqLayer->pDec->pMv[LIST_1][iMBXY][iPartIdx][0];
- iMVs[1] = pCurDqLayer->pDec->pMv[LIST_1][iMBXY][iPartIdx][1];
- iRefIndex1 = pCurDqLayer->pDec->pRefIndex[LIST_1][iMBXY][iPartIdx];
- WELS_B_MB_REC_VERIFY (GetRefPic (&pTempMCRefMem, pCtx, iRefIndex1, LIST_1));
- if (i) {
- pTempMCRefMem.pDstY += (iDstLineLuma << 3);
- pTempMCRefMem.pDstU += (iDstLineChroma << 2);
- pTempMCRefMem.pDstV += (iDstLineChroma << 2);
- }
- BaseMC (pCtx, &pTempMCRefMem, LIST_1, iRefIndex1, iMBOffsetX, iMBOffsetY + iPartIdx, pMCFunc, 16, 8, iMVs);
- if (pCurDqLayer->bUseWeightedBiPredIdc) {
- iRefIndex0 = pCurDqLayer->pDec->pRefIndex[LIST_0][iMBXY][iPartIdx];
- iRefIndex1 = pCurDqLayer->pDec->pRefIndex[LIST_1][iMBXY][iPartIdx];
- BiWeightPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, iRefIndex0, iRefIndex1, bWeightedBipredIdcIs1, 16, 8);
- } else {
- BiPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, 16, 8);
- }
- }
+ WELS_B_MB_REC_VERIFY (GetRefPic (pTarget, pCtx, iRefIndex, listIdx));
+ BaseMC (pCtx, pTarget, listIdx, iRefIndex, iMBOffsetX, iMBOffsetY + iPartIdx, pMCFunc, 16, 8, iMVs);
+ ++listCount;
}
}
- if (listCount == 1) {
+ if (listCount == 2) {
+ iRefIndex0 = pCurDqLayer->pDec->pRefIndex[LIST_0][iMBXY][iPartIdx];
+ iRefIndex1 = pCurDqLayer->pDec->pRefIndex[LIST_1][iMBXY][iPartIdx];
+ if (pCurDqLayer->bUseWeightedBiPredIdc) {
+ BiWeightPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, iRefIndex0, iRefIndex1,
+ bWeightedBipredIdcIs1, 16, 8);
+ } else {
+ BiPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, 16, 8);
+ }
+ } else if (listCount == 1) {
if (bWeightedBipredIdcIs1) {
iRefIndex = pCurDqLayer->pDec->pRefIndex[lastListIdx][iMBXY][iPartIdx];
WeightPrediction (pCurDqLayer, &pMCRefMem, lastListIdx, iRefIndex, 16, 8);
@@ -787,41 +785,39 @@ int32_t GetInterBPred (uint8_t* pPredYCbCr[3], uint8_t* pTempPredYCbCr[3], PWels
for (int32_t i = 0; i < 2; ++i) {
uint32_t listCount = 0;
int32_t lastListIdx = LIST_0;
+ // The partition offset belongs to the partition, not to the reference
+ // list, so both destinations are (re)set once here for each partition.
+ pMCRefMem.pDstY = pPredYCbCr[0] + (i ? 8 : 0);
+ pMCRefMem.pDstU = pPredYCbCr[1] + (i ? 4 : 0);
+ pMCRefMem.pDstV = pPredYCbCr[2] + (i ? 4 : 0);
+ pTempMCRefMem.pDstY = pTempPredYCbCr[0] + (i ? 8 : 0);
+ pTempMCRefMem.pDstU = pTempPredYCbCr[1] + (i ? 4 : 0);
+ pTempMCRefMem.pDstV = pTempPredYCbCr[2] + (i ? 4 : 0);
for (int32_t listIdx = LIST_0; listIdx < LIST_A; ++listIdx) {
if (IS_DIR (iMBType, i, listIdx)) {
+ // The first list of a partition predicts into pMCRefMem and the second
+ // into pTempMCRefMem, so the second BaseMC() no longer overwrites the
+ // prediction made for the first list.
+ sMCRefMember* pTarget = (listCount == 0) ? &pMCRefMem : &pTempMCRefMem;
lastListIdx = listIdx;
iMVs[0] = pCurDqLayer->pDec->pMv[listIdx][iMBXY][i << 1][0];
iMVs[1] = pCurDqLayer->pDec->pMv[listIdx][iMBXY][i << 1][1];
iRefIndex = pCurDqLayer->pDec->pRefIndex[listIdx][iMBXY][i << 1];
- WELS_B_MB_REC_VERIFY (GetRefPic (&pMCRefMem, pCtx, iRefIndex, listIdx));
- if (i) {
- pMCRefMem.pDstY += 8;
- pMCRefMem.pDstU += 4;
- pMCRefMem.pDstV += 4;
- }
- BaseMC (pCtx, &pMCRefMem, listIdx, iRefIndex, iMBOffsetX + (i ? 8 : 0), iMBOffsetY, pMCFunc, 8, 16, iMVs);
- if (++listCount == 2) {
- iMVs[0] = pCurDqLayer->pDec->pMv[LIST_1][iMBXY][i << 1][0];
- iMVs[1] = pCurDqLayer->pDec->pMv[LIST_1][iMBXY][i << 1][1];
- iRefIndex1 = pCurDqLayer->pDec->pRefIndex[LIST_1][iMBXY][i << 1];
- WELS_B_MB_REC_VERIFY (GetRefPic (&pTempMCRefMem, pCtx, iRefIndex1, LIST_1));
- if (i) {
- pTempMCRefMem.pDstY += 8;
- pTempMCRefMem.pDstU += 4;
- pTempMCRefMem.pDstV += 4;
- }
- BaseMC (pCtx, &pTempMCRefMem, LIST_1, iRefIndex1, iMBOffsetX + (i ? 8 : 0), iMBOffsetY, pMCFunc, 8, 16, iMVs);
- if (pCurDqLayer->bUseWeightedBiPredIdc) {
- iRefIndex0 = pCurDqLayer->pDec->pRefIndex[LIST_0][iMBXY][i << 1];
- iRefIndex1 = pCurDqLayer->pDec->pRefIndex[LIST_1][iMBXY][i << 1];
- BiWeightPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, iRefIndex0, iRefIndex1, bWeightedBipredIdcIs1, 8, 16);
- } else {
- BiPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, 8, 16);
- }
- }
+ WELS_B_MB_REC_VERIFY (GetRefPic (pTarget, pCtx, iRefIndex, listIdx));
+ BaseMC (pCtx, pTarget, listIdx, iRefIndex, iMBOffsetX + (i ? 8 : 0), iMBOffsetY, pMCFunc, 8, 16, iMVs);
+ ++listCount;
}
}
- if (listCount == 1) {
+ if (listCount == 2) {
+ iRefIndex0 = pCurDqLayer->pDec->pRefIndex[LIST_0][iMBXY][i << 1];
+ iRefIndex1 = pCurDqLayer->pDec->pRefIndex[LIST_1][iMBXY][i << 1];
+ if (pCurDqLayer->bUseWeightedBiPredIdc) {
+ BiWeightPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, iRefIndex0, iRefIndex1,
+ bWeightedBipredIdcIs1, 8, 16);
+ } else {
+ BiPrediction (pCurDqLayer, &pMCRefMem, &pTempMCRefMem, 8, 16);
+ }
+ } else if (listCount == 1) {
if (bWeightedBipredIdcIs1) {
iRefIndex = pCurDqLayer->pDec->pRefIndex[lastListIdx][iMBXY][i << 1];
WeightPrediction (pCurDqLayer, &pMCRefMem, lastListIdx, iRefIndex, 8, 16);
diff --git a/test/api/decoder_test.cpp b/test/api/decoder_test.cpp
index ab060a3a..35f90eec 100644
--- a/test/api/decoder_test.cpp
+++ b/test/api/decoder_test.cpp
@@ -130,15 +130,15 @@ static const FileParam kFileParamArray[] = {
{"res/test_scalinglist_jm.264", "992a25b4ec98db4a16d61c097e614eb16afe3478"},
{"res/test_vd_1d.264", "5827d2338b79ff82cd091c707823e466197281d3"},
{"res/test_vd_rc.264", "eea02e97bfec89d0418593a8abaaf55d02eaa1ca"},
- {"res/Cisco_Men_whisper_640x320_CABAC_Bframe_9.264", "931ba1caf075e7b47445c1f4410ade77a46048f6"},
- {"res/Cisco_Men_whisper_640x320_CAVLC_Bframe_9.264", "9819c0345abdd4faedbaf8f8c4dadb7749515e4d"},
+ {"res/Cisco_Men_whisper_640x320_CABAC_Bframe_9.264", "2b349c1bc806b6e0412008747b2463d77b576476"},
+ {"res/Cisco_Men_whisper_640x320_CAVLC_Bframe_9.264", "e5b76ff7e2f44e9b33906f8a4039d0d2bdb1580b"},
{"res/Cisco_Adobe_PDF_sample_a_1024x768_CAVLC_Bframe_9.264", "9d758d9e6f4dead0d7b361f3ddf2ee009d0ea190"},
- {"res/VID_1280x544_cabac_temporal_direct.264", "b7f04399f38a90c866f0b518d1dd93c823d5d91f"},
- {"res/VID_1280x720_cabac_temporal_direct.264", "dabc1d0d44921a5c72ed2d4fde1d602465249c97"},
- {"res/VID_1920x1080_cabac_temporal_direct.264", "6e719adb650cee4ca99a45242685d261257c04cc"},
- {"res/VID_1280x544_cavlc_temporal_direct.264", "33bfa44b4a3c87fe28354cace1d4b99a03d2967d"},
- {"res/VID_1280x720_cavlc_temporal_direct.264", "4face6b5d73a378b6e564a831b49311c230158e4"},
- {"res/VID_1920x1080_cavlc_temporal_direct.264", "b35dc99604ea2a1fda5b84d1b9098cb7565dec8f"},
+ {"res/VID_1280x544_cabac_temporal_direct.264", "02299df3b9d83300d244b36601699859c57fe905"},
+ {"res/VID_1280x720_cabac_temporal_direct.264", "0ef0818cb23445d209b8a7632c13f1c7e820cc27"},
+ {"res/VID_1920x1080_cabac_temporal_direct.264", "ad2b1d1456919693e38a1e3e8cd9c21699688cec"},
+ {"res/VID_1280x544_cavlc_temporal_direct.264", "71a12ff2b548b765a34c11f39eef1faa19b38d59"},
+ {"res/VID_1280x720_cavlc_temporal_direct.264", "f39cecb32ba20ca4f3b3a385db9ef46ba340e41f"},
+ {"res/VID_1920x1080_cavlc_temporal_direct.264", "6aae2d569a1ebbe5ae20e2dfc5e709cc05ab1a21"},
};
INSTANTIATE_TEST_SUITE_P (DecodeFile, DecoderOutputTest,