This reverts commit 81cf1c16db.
The B frame work was never verified beyond its own four tests: every attempt
to run the whole encode family since has aborted on a device loss. The
bisect that seemed to clear it was worthless, because only the first run
after a reboot was on a healthy GPU and every later measurement was taken on
a ring that had already hung.
Going back to the last commit with a completed run behind it: 120 of 120
executed, 92 passing, none failing. B pictures can come back once they can
be verified one change at a time, on a machine with no other GPU client to
lose.
The encoder refused B pictures outright. Three things were missing.
The slice header writer marked every picture as a reference, nal_ref_idc
hardcoded to 1. A B picture nothing will reference has to say so, otherwise
a decoder keeps it in the DPB and its frame_num accounting drifts until the
DPB overflows; the client's is_reference flag decides now, an IDR aside.
The same writer emitted no B slice syntax at all, so direct_spatial_mv_pred_flag
and the L1 reference list modification flag were absent and the bitstream went
out of sync from that bit onwards. Spatial direct mode is chosen, which
derives motion from neighbours rather than the co-located picture, so no
direct MV buffer has to be read back.
Finally the L1 reference list was never built. It comes from RefPicList1 the
same way L0 comes from RefPicList0, and feeds MFX_AVC_REF_IDX_STATE list 1
and the L1 count in MFX_AVC_IMG_STATE.
Prediction itself is unchanged: a B macroblock is coded as B_L0_16x16, so it
predicts from L0 alone and costs about what a P macroblock costs. What a B
picture buys here is that it need not enter the DPB, not a cheaper prediction.
Bi-prediction would need the macroblock type raised to B_Bi_16x16 and motion
vectors for both lists.
dEQP-VK.video.encode.h264.i_p_b_13_* goes from NotSupported to 4/4 Pass.
NOT FULLY VERIFIED: the run over the whole encode family aborted partway on an
unrelated device loss, with two QualityWarnings in intra_refresh that were not
there before, so this needs re-running on a healthy GPU before being trusted.
Co-Authored-By: Claude Opus 5 <[email protected]>
The encoder ignored VkVideoReferenceSlotInfoKHR entirely: it took
pReferenceSlots[0] as the one reference, wrote its address into frame store 0
and hardcoded the L0 list to that store. Two DPB slots were all it could
honestly advertise, which is fewer than a GOP of any length needs.
Record each slot the operation names under its own index, since a slot maps
onto the frame store of the same index and MFX_PIPE_BUF_ADDR_STATE holds 16
of those, and build the L0 entries from StdVideoEncodeH264ReferenceListsInfo,
whose RefPicList0 already carries slot indices. Clients that pass reference
slots without a list keep the old behaviour.
maxDpbSlots therefore goes to 16. maxActiveReferencePictures stays at 1:
every macroblock is still coded against reference index 0, so a picture may
be kept in the DPB but only the one it names first is predicted from.
dEQP-VK.video.encode.h264.intra_refresh_* goes from 12 Pass and 32 stopped at
the maxDpbSlots >= GOP frame count precondition to 36 Pass, the remaining 8
being per-picture-partition mode, which is not offered. The family as a whole
is 76 of 120 with no failures. ffmpeg's h264_vulkan encoder no longer trips
its slot_index assertion and produces a full GOP.
Co-Authored-By: Claude Opus 5 <[email protected]>
Each macroblock's intra/inter decision is already made on its own when the
PAK objects are emitted, so refreshing a region is a matter of coding the
macroblocks it covers the way an I slice codes them. Regions divide the
picture evenly along rows, columns or raster order, which leaves the block,
block-row and block-column modes supported and gives empty regions for free
when the cycle is longer than the split axis.
Two things needed care. PAK keeps the I slice numbering for MbType5Bits
inside a P slice, where H.264 itself offsets intra mb_types by 5; handing it
the offset value hangs the MFC. And under constrained intra prediction a
neighbour only counts as available if it is in the refresh region too,
because prediction may not cross into an inter macroblock.
Nothing constrains motion away from not-yet-refreshed regions, which is
sound only because there is no motion estimation: every vector is zero, so a
macroblock reads its own co-located block and never leaves its region.
Motion estimation will have to honour VkVideoReferenceIntraRefreshInfoKHR's
dirty region count.
Per-picture-partition mode is not offered. It wants the refreshed slice
coded as an I slice, header included, rather than as intra macroblocks
inside a P slice.
dEQP-VK.video.encode.h264.intra_refresh_* goes from 44 NotSupported to 12
Pass; all three modes pass in the variants that can run. The other 32 stop
at a CTS precondition, maxDpbSlots >= GOP frame count, which the driver's
two slots do not meet. Raising that needs real DPB slot tracking and is
what gates B frames and ffmpeg's encoder too.
Co-Authored-By: Claude Opus 5 <[email protected]>
MFX_QM_STATE and MFX_FQM_STATE hold separate register sets for the intra
and the inter scaling lists, but only the intra ones were programmed. The
hardware therefore quantized every inter residual with whatever those
registers happened to hold, so a coded P macroblock came back corrupted
while I frames were correct. That is why the P path shipped as skip-only.
With both sets programmed, P macroblocks can carry a real residual: encode
them as coded BP_L0_16x16 with a zero motion vector instead of forcing
P_Skip. A P frame now tracks changing content rather than repeating its
reference.
Measured on a 176x144 clip at QP 26, P frame luma PSNR against the true
source: 12.81 dB before, 43.87 dB after (the I frame scores 44.53 dB), at
2062 bytes versus the I frame's 4474. With an unchanged source the P frame
costs 67 bytes, down from 4862.
dEQP-VK.video.encode.h264.* goes from 12 to 14 passes with no new failures.
There is still no motion estimation, so a moving block costs a full residual
rather than a motion vector.
Co-Authored-By: Claude Opus 5 <[email protected]>
Squashed work that had been sitting in the working tree. Grouped by area:
Extensions
VK_EXT_host_image_copy, with a MEMCPY_BIT path that reads and writes the
image's own tiled layout, VkSubresourceHostMemcpySizeEXT filled in, and
VkHostImageCopyDevicePerformanceQueryEXT answered. Depth/stencil and
non-power-of-two bpb stay NotSupported.
VK_EXT_pipeline_robustness, VK_KHR_shader_float_controls2,
VK_KHR_shader_subgroup_rotate, VK_EXT_vertex_input_dynamic_state,
VK_EXT_conditional_rendering (an MI_PREDICATE path for gen7, which has no
GPRs), custom_border_color, border_color_swizzle, map_memory2,
load_store_op_none, depth_clamp_zero_one,
dynamic_rendering_unused_attachments, index_type_uint8,
line_rasterization, vertex_attribute_divisor, mutable_descriptor_type.
H.264 encode
MFC command emission and a bitstream writer, driving intra-only encode
through the video queue. Video profile reporting was also corrected from
270 combinations down to the 11 that actually exist.
Correctness
gfx7 routes typed surface writes through the render cache, so
VK_ACCESS_SHADER_WRITE barriers have to flush it -- without that, nothing a
fragment shader stored was visible.
nir_opt_algebraic's fadd->flrp fusion rules dropped signed zero; '~' only
excludes nir_fp_exact, not preserve_signed_zero, so reflect() with a -0
input came out wrong. Guarded with (nsz).
Stop advertising STORAGE_READ_WITHOUT_FORMAT: shaderStorageImageReadWithout-
Format is false here, and since Vulkan 1.3 the per-format bit is what grants
permission, so reporting it while rejecting the pipeline is a contradiction.
gfx7's typed data port ignores SURFTYPE_NULL -- the null surface still has
one addressable texel, which read and wrote graphics address zero. Bounds
check typed image access against the image param when nullDescriptor is
enabled, the same trick lower_image_atomic_instr already used on IVB.
Formats that require a sampler Y'CbCr conversion no longer report
BLIT_SRC/BLIT_DST/STORAGE_IMAGE. Video decode does not need them.
Check the surface state allocations in emit_binding_table() and in the push
descriptor path instead of writing through a NULL map in release builds.
Co-Authored-By: Claude Opus 5 <[email protected]>
elwrcl