Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The unit-test fixture records an empty active reference list, causing the reordered tail to be overwritten and masking malformed output.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes long-term reference reordering in non-Baseline CABAC decoding by matching the maintained frame index.
Changes:
- Matches and compacts long-term references using
iLongTermFrameIdx. - Adds unit and stream-level regression coverage.
- Registers the new unit test across build systems.
| File | Description |
|---|---|
codec/decoder/core/src/manage_dec_ref.cpp |
Corrects long-term reference matching. |
test/decoder/DecUT_RefListReorder.cpp |
Adds focused reorder coverage. |
test/decoder/targets.mk |
Registers the unit test for Make. |
test/decoder/meson.build |
Registers the unit test for Meson. |
test/build/win32/codec_ut/codec_unittest.vcproj |
Registers the unit test for Windows. |
test/api/decoder_test.cpp |
Adds the regression stream and output hash. |
res/ltr_reorder_high_cabac.264 |
Provides the CABAC regression stream. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4517797 to
52bd2c3
Compare
|
We hit the same bug decoding the H.264 that Windows Remote Desktop sends (the RDP graphics pipeline's AVC444 mode, MS-RDPEGFX), through FreeRDP's OpenH264 backend, and can confirm this PR fixes it. The scenario. In AVC444v2 the server sends each frame as two pictures, a regular 4:2:0 one (luma) and an auxiliary one carrying the remaining chroma samples, interleaved in a single H.264 sequence and decoded by one decoder instance (
As both long-term pictures have When it was introduced. In v2.5.0, by fa6d099 (#3723), which switched Measured. Native builds of each version. Every access unit was decoded as FreeRDP does (
We also checked a stream from another Windows host whose encoder marks the chroma pictures |
|
A follow-up: a correction to my description above, and a 3-picture stream that reproduces the bug. Correction. It isn't only the chroma pictures. The Windows encoders in our captures mark every picture long-term and reorder by
Picture 3 asks for long-term picture 1 (picture 2), and v2.5.0 to v2.7.0 predict it from picture 1. The split I described (main view on index 0, chroma on index 1) only comes after that. So all 8 wrong pictures of stream B are main-view ones, and 3 of the 7 in A. In C and D, all of them are chroma. A 3-picture stream. The first three pictures of one of B's sequences, unmodified (attached: rdp_ltr_reorder_3_pictures.264.zip). It's 93,027 bytes: Main profile, level 3.1, CABAC, 1056x752, two slices per picture,
The SHA-1 covers the whole I420 output, row by row, the way |
**Problem** When a slice carries `ref_pic_list_reordering` commands for long-term references (`reordering_of_pic_nums_idc == 2`), `WelsReorderRefList2()` matched the requested `long_term_pic_num` against each picture's `uiLongTermPicNum` field. That field is unreliable because long-term references are created via MMCO 3 (short-to-long) and MMCO 6 (mark current as long), both of which only carry `long_term_frame_idx` in the bitstream and never a `long_term_pic_num`. As a result `AddLongTermToList()` stores `uiLongTermPicNum` as `0` for every such picture. With multiple long-term references present, they all compare equal (`0`), so the reorder failed to select the requested entry (matching either the wrong one or none at all), leaving the wrong picture at the head of the reference list and corrupting inter prediction. It only surfaces when multiple long-term references and a long-term reorder command occur together. In practice this is hit when decoding AirPlay Mirroring streams from iOS and macOS, whose High-profile CABAC streams use multiple long-term references with reordering, producing visibly corrupted video. Note that `WelsReorderRefList2()` is only used for non-Baseline CABAC streams (`uiProfileIdc != 66 && bEntropyCodingModeFlag`). The problem was introduced disabled (`#if 0`) by 1b8caef, and later enabled by fa6d099, which flipped it to `#if 1` and exposed it. **Solution** Match by `iLongTermFrameIdx` instead of `uiLongTermPicNum`, in both the selection loop and the subsequent compaction loop. `iLongTermFrameIdx` is the identifier that is actually maintained on the picture (set in `AddLongTermToList`) and, per the spec (8.2.4.1), for frame coding `LongTermPicNum == LongTermFrameIdx`, so the value parsed as `long_term_pic_num` matches it directly. This aligns the reorder path with how the rest of the code already identifies long-term pictures, e.g. `WelsDelLongFromList()` used by MMCO 2, which likewise matches against `iLongTermFrameIdx`. **Testing** Added a focused unit test (`DecUT_RefListReorder.cpp`) that reproduces the case via three long-term references sharing `uiLongTermPicNum == 0` with distinct `iLongTermFrameIdx`, plus an example High-profile CABAC regression stream (`res/ltr_reorder_high_cabac.264`) wired into the decoder API test. **Considerations** Keeping the match on `uiLongTermPicNum` but populating it correctly, i.e. deriving `LongTermPicNum` from `LongTermFrameIdx` per slice (8.2.4.1) or setting it at marking time, would be an alternative option. However, `LongTermPicNum` is a per-slice derived quantity, not a persistent property of the picture, so storing it is spec-awkward. For frame coding, it just equals `LongTermFrameIdx`, making the field redundant. It would also touch the marking path (`AddLongTermToList`, `MarkAsLongTerm`) for no functional gain.
52bd2c3 to
fccad3d
Compare

Problem
When a slice carries
ref_pic_list_reorderingcommands for long-term references (reordering_of_pic_nums_idc == 2),WelsReorderRefList2()matched the requestedlong_term_pic_numagainst each picture'suiLongTermPicNumfield.That field is unreliable because long-term references are created via MMCO 3 (short-to-long) and MMCO 6 (mark current as long), both of which only carry
long_term_frame_idxin the bitstream and never along_term_pic_num. As a resultAddLongTermToList()storesuiLongTermPicNumas0for every such picture.With multiple long-term references present, they all compare equal (
0), so the reorder failed to select the requested entry (matching either the wrong one or none at all), leaving the wrong picture at the head of the reference list and corrupting inter prediction.It only surfaces when multiple long-term references and a long-term reorder command occur together. In practice this is hit when decoding AirPlay Mirroring streams from iOS and macOS, whose High-profile CABAC streams use multiple long-term references with reordering, producing visibly corrupted video.
Note that
WelsReorderRefList2()is only used for non-Baseline CABAC streams (uiProfileIdc != 66 && bEntropyCodingModeFlag).The problem was introduced disabled (
#if 0) by 1b8caef, and later enabled by fa6d099, which flipped it to#if 1and exposed it.Solution
Match by
iLongTermFrameIdxinstead ofuiLongTermPicNum, in both the selection loop and the subsequent compaction loop.iLongTermFrameIdxis the identifier that is actually maintained on the picture (set inAddLongTermToList) and, per the spec (8.2.4.1), for frame codingLongTermPicNum == LongTermFrameIdx, so the value parsed aslong_term_pic_nummatches it directly.This aligns the reorder path with how the rest of the code already identifies long-term pictures, e.g.
WelsDelLongFromList()used by MMCO 2, which likewise matches againstiLongTermFrameIdx.Testing
Added a focused unit test (
DecUT_RefListReorder.cpp) that reproduces the case via three long-term references sharinguiLongTermPicNum == 0with distinctiLongTermFrameIdx, plus an example High-profile CABAC regression stream (res/ltr_reorder_high_cabac.264) wired into the decoder API test.Considerations
Keeping the match on
uiLongTermPicNumbut populating it correctly, i.e. derivingLongTermPicNumfromLongTermFrameIdxper slice (8.2.4.1) or setting it at marking time, would be an alternative option. However,LongTermPicNumis a per-slice derived quantity, not a persistent property of the picture, so storing it is spec-awkward. For frame coding, it just equalsLongTermFrameIdx, making the field redundant.It would also touch the marking path (
AddLongTermToList,MarkAsLongTerm) for no functional gain.