[hgaiser]

@DatCaptainHorse @porkloin could you give this PR a test on your systems?

This PR should simplify how the encoder is used and eliminates the need to convert to YUV beforehand. It also avoids having a separate API for the RGB input.

[hgaiser]

Rough summary of the new API:

// --- Set-up ---------------------------------------------------------------
let context = VideoContextBuilder::new()
    .require_encode(Codec::H265)
    .build()?;                       // or build_from_existing_encode(...) for your own device

let config = EncodeConfig::h265(1920, 1080)
    .with_bit_depth(EncodeBitDepth::Ten)
    .with_pixel_format(PixelFormat::Yuv420)
    .with_rate_control(RateControl::Cbr { bitrate: 20_000_000 })
        // or Cqp { qp } / Vbr { target, max }
    .with_source(Source::Rgb(RgbSource::new(InputFormat::BGRx, ColorSpec::Srgb)))
        // or Source::Yuv (default)
    .with_color(StreamColor::Hdr10 { range: ColorRange::Limited })
        // or Sdr { range } / Other(ColorDescription) for YUV only
    .with_frame_rate(60, 1)
    .with_gop_size(0);
    // .with_conversion(ConversionPreference::Shader)  // only to force a path

let mut encoder = Encoder::new(context.clone(), config)?;
    // checks rate control, picks Conversion::{None, Encoder, Shader}
encoder.conversion();                // which path it picked

// --- One frame, three kinds of source --------------------------------------

// (a) Write into the encoder's own image: no copy. Not available with Conversion::Shader.
let slot = encoder.input_slot()?;            // waits until the slot is free
render_or_copy_into(slot.image());           // must be VIDEO_ENCODE_SRC layout
let future = encoder.submit(Frame::input(slot).wait(&[rendered]))?;
    // a slot from before the last submit is an error

// (b) Your own vk::Image: copied or converted on the GPU.
let source = SourceImage::new(&context, image, Handoff::Device { layout: GENERAL })?;
    // or Handoff::External for imported memory
let future = encoder.submit(Frame::image(&source).wait(&[rendered]))?;
// Before you write the image again:
//     your GPU submit waits on source.released()     (Vec<TimelinePoint>)
//     or source.wait_released()?                     (CPU)
// Dropping `source` waits too. Drop it before you destroy the image.

// (b') CPU frames
let mut upload = InputImage::new(context.clone(), Codec::H265, w, h, depth, format)?;
upload.upload_yuv420(&yuv)?;                 // waits by itself for the previous read
let future = encoder.submit(Frame::image(upload.source()))?;
//   or: let slot = encoder.input_slot()?; upload.upload_yuv420_to(&slot, &yuv)?;
//       encoder.submit(Frame::input(slot))?;

// (c) [dmabuf] A DMA-BUF that pixelforge imports or allocates
let formats = encoder.input_formats()?;      // DRM format and which modifiers are zero-copy
let ext = encoder.import(&dmabuf)?;          // or encoder.allocate_input(&modifiers)?, then ext.export()?
let mut future = encoder.submit(
    Frame::external(&ext)
        .wait_sync_file(producer_fence)      // [dmabuf] GPU wait on a sync_file
        .signal_sync_file(),                 // [dmabuf] ask for a release fence
)?;
let release_fd = future.take_release_sync_file();   // give back to the producer
// or wait on ext.released()

// --- Packets ---------------------------------------------------------------
let packet = future.await?;     // or pollster::block_on(future)
// packet.data, .pts, .dts, .is_key_frame, .frame_type
// submit() returns at once; it waits on the CPU only when every slot is in flight

// --- During the stream -------------------------------------------------------
encoder.reconfigure(new_source, new_color)?;
    // no-op if nothing changed; shader switches in place;
    // otherwise the encoder is made again; pts continue;
    // a new colour gives an IDR
encoder.request_idr();
encoder.invalidate_reference_frames(first_lost_pts);
let future = encoder.repeat()?;  // last frame again (e.g. IDR while idle); your source can be gone
encoder.flush()?;                // all futures resolved

// --- Errors ------------------------------------------------------------------
match result {
    Err(PixelForgeError::DeviceLost)          => { /* make a new context */ }
    Err(PixelForgeError::NoSuitableDevice(_)) => { /* this device cannot; try another config */ }
    Err(PixelForgeError::InvalidInput(_))     => { /* the request itself is wrong */ }
    ...
}

Old API:

// --- Set-up ---------------------------------------------------------------
let context = VideoContextBuilder::new()
    .require_encode(Codec::H265)
    .build()?;

let config = EncodeConfig::h265(1920, 1080)
    .with_bit_depth(EncodeBitDepth::Ten)
    .with_pixel_format(PixelFormat::Yuv420)
    .with_rate_control(RateControlMode::Cbr)  // Disabled / Cqp / Cbr / Vbr
    .with_target_bitrate(20_000_000)          // ignored in Cqp
    .with_max_bitrate(30_000_000)             // used only in Vbr; may be below the target
    .with_quality_level(28)                   // used only in Cqp; range differs per codec
    .with_b_frames(0)                         // anything above 0 fails in Encoder::new
    .with_color_description(ColorDescription::bt2020_pq())   // raw H.273 codes
    .with_frame_rate(60, 1)
    .with_gop_size(0);

// --- RGB input: the caller picks the path ----------------------------------
let conv_config = ColorConverterConfig::new(
    w, h, InputFormat::BGRx, OutputFormat::P010,  // the caller must match the encoder's format
    ColorSpec::Srgb, ColorSpec::Bt2020Pq, ColorRange::Limited,
);
let (mut encoder, mut converter) = match conv_config.rgb_encode_input(&context, Codec::H265) {
    // Path 1 (RADV only): the encoder converts RGB itself.
    Some(format) => (Encoder::new(context.clone(), config.with_rgb_input(format))?, None),
    // Path 2: the colour converter's shader.
    None => (
        Encoder::new(context.clone(), config)?,
        Some(ColorConverter::new(context.clone(), conv_config)?),
    ),
};
// The stream's colour description and the converter's target are set
// separately; nothing checks that they agree.

// --- One frame -------------------------------------------------------------

// (a) Write into the encoder's own image.
let target = encoder.input_image();      // raw vk::Image; must stay in VIDEO_ENCODE_SRC
render_or_copy_into(target);
let future = encoder.encode(target)?;    // in place only because the handle is equal

// (b) Your own image, YUV source or encoder conversion: copied.
let future = encoder.encode(image)?;     // CPU waits for the copy (fence)
let future = encoder.encode_after(image, vk::ImageLayout::GENERAL, &[rendered])?;
    // the layout UNDEFINED secretly means "first DMA-BUF import, acquire from EXTERNAL"

// (b') RGB with the shader: two calls, in the right order, into the right image.
let converter = converter.as_mut().unwrap();
converter.convert(image, src_layout, encoder.input_image())?;   // CPU waits
let future = encoder.encode(encoder.input_image())?;
//   or async:
let done = converter.convert_async(image, src_layout, encoder.input_image(), &[rendered])?;
let future = encoder.encode_after(encoder.input_image(), vk::ImageLayout::GENERAL, &[done])?;
//   nothing tells you when `image` can be written again,
//   and nothing stops you from converting into a stale input_image()

// (b'') CPU frames
input_image.upload_yuv420(&yuv)?;
let future = encoder.encode(input_image.image())?;
//   or upload_yuv420_to(encoder.input_image(), &yuv) with any raw vk::Image

// (c) DMA-BUF: no API. The caller imports the buffer into a vk::Image with
//     its own Vulkan code (as Moonshine's DmaBufImporter does), passes
//     UNDEFINED the first time, and handles sync_file fences on the CPU.

// --- Packets ---------------------------------------------------------------
let packet = future.await?;

// --- During the stream -------------------------------------------------------
encoder.set_color_description(ColorDescription::bt709())?;
    // changes only the label: the converter keeps its old target, and the
    // encoder's own RGB conversion keeps its old matrix (the PR #46 bug)
if let Some(c) = &mut converter {
    // the caller has to update the converter too, in an order that never
    // passes through an invalid source/target pair (Moonshine's
    // set_converter_colors)
    c.set_target(ColorSpec::Srgb)?;
    c.set_source(ColorSpec::Srgb)?;
    c.set_range(ColorRange::Limited);
}
// A new input format needs a new ColorConverter (Moonshine kept one per fourcc).

// IDR while idle: re-encode "the input image"
let future = encoder.encode(encoder.input_image())?;  // with 2 slots: the frame before the last

encoder.request_idr();
encoder.invalidate_reference_frames(first_lost_pts);
encoder.flush()?;

// --- Errors ------------------------------------------------------------------
// Device loss is hidden in String variants:
//     e.to_string().contains("device has been lost")

[porkloin]

Tested on a 7900 XTX (RADV, Mesa 26.2.6) with the validation layer on, comparing against main.

All GPU tests pass: dmabuf, submit, converter device tests, rgb_encode, adopted, roundtrip, verify_all and encode_format_support.

Validation: every message on this branch also shows up on main, and none looks like a pixelforge bug:

• rgb_encode: VUID-VkImageViewCreateInfo-image-08336 (BGRA view with VIDEO_ENCODE_SRC usage, no VIDEO_ENCODE_INPUT format feature) and VUID-VkQueryPoolCreateInfo-pNext-pNext (unknown VkVideoEncodeProfileRgbConversionInfoVALVE in the chain). The layer (header 363) doesn't know VK_VALVE_video_encode_rgb_conversion, so it doesn't know the extension makes BGRA a valid encode input, and it rejects the profile struct.
• adopted (the Vulkan 1.1 device test): VUID-VkShaderModuleCreateInfo-pCode-08737. The shaders are SPIR-V 1.6 and a 1.1 device only accepts 1.3. RADV takes them anyway and the test passes, but a stricter driver might not.
• verify_all: VUID-VkPhysicalDeviceVideoEncodeQualityLevelInfoKHR-pVideoProfile-08259, from querying quality levels for profiles the device doesn't support (H.264 10-bit and the like), before those cases are skipped.

On main, rgb_encode also hit 08259, but it doesn't this PR.

Unrelated/Pre-existing: rfi fails on all three codecs with "recovery frame 12 fell back to a keyframe (RFI did not engage)". Main fails the same way, and it's the RADV result reported in in #39.

Could you push your Moonshine branch for this? I'd like to run it through the same live test I did for #254: hdrneon cycling SDR/HDR10/scRGB in an HDR session, HEVC and AV1, with and without packet loss.

[DatCaptainHorse]

Only one failure on ANV (Arc A310):

---- converter::device_tests::shader_source_matches_converting_by_hand stdout ----

thread 'converter::device_tests::shader_source_matches_converting_by_hand' (1455732) panicked at src/converter/device_tests.rs:593:9:
AV1: the streams differ
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/std/src/panicking.rs:679:5
   1: core::panicking::panic_fmt
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/core/src/panicking.rs:80:14
   2: pixelforge::converter::device_tests::shader_source_matches_converting_by_hand
             at ./src/converter/device_tests.rs:593:9
   3: pixelforge::converter::device_tests::shader_source_matches_converting_by_hand::{closure#0}
             at ./src/converter/device_tests.rs:580:50
   4: <pixelforge::converter::device_tests::shader_source_matches_converting_by_hand::{closure#0} as core::ops::function::FnOnce<()>>::call_once
             at /home/kristian/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   5: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    converter::device_tests::shader_source_matches_converting_by_hand

test result: FAILED. 142 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.68s

[hgaiser]

Thanks!

Regarding the moonshine branch: hgaiser/moonshine#258

[hgaiser]

@porkloin I think these should be resolved with the latest commits, could you check if they are?

@DatCaptainHorse I don't know exactly what would cause that issue.. does it happen every time?

[DatCaptainHorse]

@hgaiser yep, though I wouldn't consider that a blocker here, the Vulkan Video support on ANV is still experimental, with AV1 being even worse off than H.26X codecs there.

I did notice a new failure come up after latest changes though:

---- converter::device_tests::pipelined_shader_frames_do_not_overwrite_each_other stdout ----

thread 'converter::device_tests::pipelined_shader_frames_do_not_overwrite_each_other' (821343) panicked at src/converter/device_tests.rs:638:9:
AV1: a pipelined frame was overwritten
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/std/src/panicking.rs:679:5
   1: core::panicking::panic_fmt
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/core/src/panicking.rs:80:14
   2: pixelforge::converter::device_tests::pipelined_shader_frames_do_not_overwrite_each_other
             at ./src/converter/device_tests.rs:638:9
   3: pixelforge::converter::device_tests::pipelined_shader_frames_do_not_overwrite_each_other::{closure#0}
             at ./src/converter/device_tests.rs:625:61
   4: <pixelforge::converter::device_tests::pipelined_shader_frames_do_not_overwrite_each_other::{closure#0} as core::ops::function::FnOnce<()>>::call_once
             at /home/kristian/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   5: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- converter::device_tests::shader_source_matches_converting_by_hand stdout ----

thread 'converter::device_tests::shader_source_matches_converting_by_hand' (821395) panicked at src/converter/device_tests.rs:615:9:
AV1: the streams differ
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/std/src/panicking.rs:679:5
   1: core::panicking::panic_fmt
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/core/src/panicking.rs:80:14
   2: pixelforge::converter::device_tests::shader_source_matches_converting_by_hand
             at ./src/converter/device_tests.rs:615:9
   3: pixelforge::converter::device_tests::shader_source_matches_converting_by_hand::{closure#0}
             at ./src/converter/device_tests.rs:602:50
   4: <pixelforge::converter::device_tests::shader_source_matches_converting_by_hand::{closure#0} as core::ops::function::FnOnce<()>>::call_once
             at /home/kristian/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   5: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/58cf8f9c35b355199727627d07ae34a4e2846d67/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    converter::device_tests::pipelined_shader_frames_do_not_overwrite_each_other
    converter::device_tests::shader_source_matches_converting_by_hand

[hgaiser]

I did notice a new failure come up after latest changes though:

Yeah that doesn't surprise me. Those tests create a YUV frame through ColorConverter and pass it to the Encoder and compare it to a RGB frame passed directly to the Encoder (letting the encoder do the conversion).

I suspect on your machine it encodes into different values every time you run the same input through an encoder. I just added a test that specifically checks determinism. Can you run that test? I expect it will also fail on ANV.

cargo test --lib encode_is_deterministic -- --ignored --nocapture

If that indeed fails, I guess we'll ignore the ANV issues for now and chalk it up to a beta AV1 implementation 🤷

[porkloin]

@hgaiser Looks good on my end now.

Tested 2707f5d on a 7900 XTX (RADV) against main, validation layer on.

• Validation: 08737 and 08259 are fixed. The 08336 and pNext messages left in rgb_encode look like the layer not knowing the VALVE RGB extension.
• GPU tests: 25/25 pass, encode_is_deterministic included, so RADV is deterministic.
• Streaming (with #258): no errors in any codec. Host latency is 3–8% lower with the convert stage is gone.

[hgaiser]

Great, then let's merge this in the current state. Thanks for testing.

[DatCaptainHorse]

cargo test --lib encode_is_deterministic -- --ignored --nocapture

If that indeed fails, I guess we'll ignore the ANV issues for now and chalk it up to a beta AV1 implementation 🤷

Here:

running 1 test
test converter::device_tests::encode_is_deterministic ... H264 run 1: frames that differ from run 0: []
H264 run 2: frames that differ from run 0: []
H264 run 3: frames that differ from run 0: []
H264 run 4: frames that differ from run 0: []
H265 run 1: frames that differ from run 0: []
H265 run 2: frames that differ from run 0: []
H265 run 3: frames that differ from run 0: []
H265 run 4: frames that differ from run 0: []
AV1 run 1: frames that differ from run 0: [1, 2, 3, 4, 5, 6, 7]
AV1 run 2: frames that differ from run 0: [1, 2, 3, 4, 5, 6, 7]
AV1 run 3: frames that differ from run 0: [1, 2, 3, 4, 5, 6, 7]
AV1 run 4: frames that differ from run 0: [1, 2, 3, 4, 5, 6, 7]
ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 149 filtered out; finished in 0.93s

Bit late, sorry about that 🙏

[hgaiser]

No worries, I didn't wait for it because either the test works or it doesn't, but I'd have merged anyway :).

Thanks for testing though, that confirms that for AV1 on ANV it is not deterministic at the moment :\