Conversation
…ive_override_flag=1 Agent-Logs-Url: https://github.com/hgaiser/pixelforge/sessions/573adfc5-fa5b-40d6-9c58-fdbc9e3f4856 Co-authored-by: hgaiser <716138+hgaiser@users.noreply.github.com>
Agent-Logs-Url: https://github.com/hgaiser/pixelforge/sessions/573adfc5-fa5b-40d6-9c58-fdbc9e3f4856 Co-authored-by: hgaiser <716138+hgaiser@users.noreply.github.com>
Agent-Logs-Url: https://github.com/hgaiser/pixelforge/sessions/573adfc5-fa5b-40d6-9c58-fdbc9e3f4856 Co-authored-by: hgaiser <716138+hgaiser@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adjusts the H.264 encoder’s slice-header/reference-list signaling to avoid decoder errors when the PPS default reference count exceeds the currently-available DPB references (e.g., immediately after an IDR), and cleans up build output handling.
Changes:
- Forces per-slice signaling of the active reference count and updates reference-list construction to use the actual available L0 references (no padding duplicates).
- Updates internal comments around H.264 ref list signaling behavior.
- Ignores Cargo build artifacts by adding
/target/to.gitignore.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/encoder/h264/encode.rs | Sets num_ref_idx_active_override_flag and adjusts L0 reference list population to avoid relying on PPS defaults/padding. |
| .gitignore | Adds /target/ to prevent committing Rust build artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build StdVideoEncodeH264SliceHeader. | ||
| let slice_header_flags = ash::vk::native::StdVideoEncodeH264SliceHeaderFlags { | ||
| _bitfield_align_1: [], | ||
| _bitfield_1: ash::vk::native::StdVideoEncodeH264SliceHeaderFlags::new_bitfield_1( | ||
| 0, // direct_spatial_mv_pred_flag | ||
| 0, // num_ref_idx_active_override_flag | ||
| // Always override the PPS default so decoders use the per-slice active count, | ||
| // preventing "Missing reference picture" errors when the DPB is not yet full. | ||
| 1, // num_ref_idx_active_override_flag | ||
| 0, // reserved | ||
| ), | ||
| }; |
There was a problem hiding this comment.
num_ref_idx_active_override_flag is set to 1 unconditionally, but in H.264 this syntax element is only present for P/B/SP slices. For IDR/I slices (and any case where picture_info.pRefLists is NULL), forcing this flag risks generating a non-conformant slice header or a driver-side mismatch between the slice header and provided reference list info. Consider setting it conditionally based on slice_type (e.g., only when references are actually signaled) and keeping it 0 for IDR/I slices.
src/encoder/h264/encode.rs
Outdated
| // P-frame: L0 reference list using actual available references. | ||
| // Clamp to the negotiated active count maximum. | ||
| let actual_count = self | ||
| .l0_references | ||
| .len() | ||
| .min(self.active_reference_count as usize); | ||
|
|
||
| for (i, ref_info) in self.l0_references.iter().take(actual_count).enumerate() { | ||
| if i < 32 { | ||
| ref_list0[i] = ref_info.dpb_slot; | ||
| } |
There was a problem hiding this comment.
actual_count is already bounded by active_reference_count, and H.264 L0 reference lists are capped at 32; in this codebase active_reference_count is clamped to ≤32 during init. This makes the if i < 32 guard and the repeated .min(32) a bit misleading/redundant—consider clamping once (e.g., let actual_count = ...min(32)) and then writing without the extra conditional to keep the list length logic easier to reason about.
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/hgaiser/pixelforge/sessions/a897b031-932f-4a08-8f88-71ca0e4fc332 Co-authored-by: hgaiser <716138+hgaiser@users.noreply.github.com>
Applied both review changes in 3e90f05:
|
|
I tested it and did not get any error 👍. Thanks for the fix. Btw, you might want to squash those commits as they contain 60MB of build artifacts. |
Great 👍 Yeah, I noticed.. thanks for the warning though! |
With
max_reference_frames > 1, the PPS advertisesnum_ref_idx_l0_default_active_minus1 = max_refs - 1, but the slice header hadnum_ref_idx_active_override_flag = 0, so decoders used that PPS default for every slice — including P-frames immediately after an IDR where the DPB holds only one picture. Result: "Missing reference picture" warnings and decode failures until the DPB filled up.Changes
num_ref_idx_active_override_flag = 1— always set in the slice header so each slice encodes its own active reference count, overriding the PPS default. Decoders now see the true per-slice count rather than the max-configured value.RefPicList0toactive_reference_countentries. This didn't fix the decoder-side issue and is no longer needed; the list is now populated with only the actually-available references (clamped toactive_reference_count).target/to.gitignore— prevent Rust build artifacts from being committed.