From ef7912cc7cca2c96d26092e377a559e051eafeb4 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sun, 2 Aug 2026 01:37:03 +1000 Subject: [PATCH] fix(encoders): stop AMF being reported broken, harden its ffmpeg args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses #51. The availability probe encoded a 64x64 frame. Hardware encoders have minimum dimensions and AMD AMF rejects that size, so VapourBox marked working AMF encoders as broken — the reporter confirmed the same command succeeds at a larger size. Probe at 512x512 (AMF's floor is 192x128 on some ASICs); still one frame, so the cost is unchanged. Verified the larger frame keeps both VideoToolbox encoders passing. The reported ffmpeg crash under CQP (exit -1073741819 = access violation) can't be reproduced without AMD hardware, so this addresses the two things in our invocation that could plausibly cause it: - AMF is now pinned to nv12. Left to negotiate, a >8-bit source reaches the encoder as p010, and 10-bit HEVC encode only exists on some AMD ASICs — where it doesn't, the AMF runtime faults rather than failing cleanly. h264_amf has no 10-bit mode at all. Custom FFmpeg Arguments still override it, so 10-bit HEVC stays reachable where supported. - -qp_b is set alongside -qp_i/-qp_p. B-frames were left on the encoder's default QP while I and P were pinned, so a GOP could mix the chosen quality with an unrelated one. Separately, presets can no longer cross encoder families. Each family has its own vocabulary (x264 ultrafast…placebo, NVENC p1…p7, QSV veryfast…veryslow, AMF speed|balanced|quality) and a saved preset can pair a codec with the wrong one; ffmpeg then rejects the option and fails the encode. normalized_preset() substitutes the family default. --- CLAUDE.md | 28 +++++++ .../services/hardware_encoder_detector.dart | 28 +++++-- app/test/hardware_encoder_probe_test.dart | 37 +++++++++ worker/src/models/video_job.rs | 52 ++++++++++++ worker/src/pipeline_executor.rs | 82 ++++++++++++++++++- 5 files changed, 216 insertions(+), 11 deletions(-) create mode 100644 app/test/hardware_encoder_probe_test.dart diff --git a/CLAUDE.md b/CLAUDE.md index 8b69393..3e9ef3d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -466,6 +466,34 @@ Presets save complete filter pipeline + encoding settings. Built-in presets (Fas --- +## Hardware Encoders (issue #51) + +Three invariants that are easy to break and hard to notice, because a wrong +value here shows up as "this GPU doesn't work" rather than as an error: + +- **The availability probe must use a large enough frame.** Hardware encoders + have minimum dimensions, and below them they fail — so a too-small probe + reports a *working* encoder as broken. AMD AMF rejects 64x64, which is exactly + what the probe used to send. `HardwareEncoderDetector.probeFrameSize` is now + 512 (AMF's floor is 192x128 on some ASICs); `hardware_encoder_probe_test.dart` + keeps it there. +- **Presets are per-family vocabularies and must not cross over.** x264 takes + `ultrafast…placebo`, NVENC `p1…p7`, QSV `veryfast…veryslow`, AMF + `speed|balanced|quality`. A saved preset or imported job config can pair a + codec with the wrong family's preset; ffmpeg then rejects the option and the + whole encode dies. `VideoCodec::normalized_preset` (Rust) substitutes the + family default — the Rust `available_presets`/`default_preset` must stay in + step with `availablePresets`/`defaultPreset` in `app/lib/models/video_job.dart`. +- **AMF is pinned to `nv12`.** Left to negotiate, a >8-bit source reaches the + encoder as p010; 10-bit HEVC encode exists only on some AMD ASICs, and where + it doesn't the AMF runtime faults (0xC0000005) instead of failing cleanly. + h264_amf has no 10-bit mode at all. Custom FFmpeg Arguments still override it + (a later `-pix_fmt` wins), which is the escape hatch for 10-bit HEVC on + hardware that supports it. + +None of the AMF behaviour can be verified in CI or on macOS — there is no AMD +hardware in the matrix — so changes here rest on reporter confirmation. + ## QTGMC Parameters Reference The most important parameters: diff --git a/app/lib/services/hardware_encoder_detector.dart b/app/lib/services/hardware_encoder_detector.dart index 9a898ca..1cc277e 100644 --- a/app/lib/services/hardware_encoder_detector.dart +++ b/app/lib/services/hardware_encoder_detector.dart @@ -125,14 +125,28 @@ class HardwareEncoderDetector extends ChangeNotifier { } } + /// Frame size for the functional probe. + /// + /// Hardware encoders have minimum dimensions and a tiny frame makes them + /// report a *false* failure: AMD AMF rejects 64x64, which had VapourBox + /// marking working AMF encoders as broken (issue #51). AMF's minimum is + /// 192x128 on some ASICs, and NVENC/QSV have their own floors, so probe at a + /// size comfortably above all of them. One frame, so the cost is unchanged. + static const int probeFrameSize = 512; + + /// The ffmpeg arguments used to functionally probe [codecValue]. Exposed for + /// tests so the frame-size guarantee above can't silently regress. + static List probeArgs(String codecValue) => [ + '-hide_banner', '-loglevel', 'error', + '-f', 'lavfi', + '-i', 'color=c=black:s=${probeFrameSize}x$probeFrameSize:r=5:d=1', + '-frames:v', '1', + '-c:v', codecValue, + '-f', 'null', '-', + ]; + Future _probe(String ffmpegPath, VideoCodec codec) async { - final args = [ - '-hide_banner', '-loglevel', 'error', - '-f', 'lavfi', '-i', 'color=c=black:s=64x64:r=5:d=1', - '-frames:v', '1', - '-c:v', codec.value, - '-f', 'null', '-', - ]; + final args = probeArgs(codec.value); var ok = false; String? errorLog; try { diff --git a/app/test/hardware_encoder_probe_test.dart b/app/test/hardware_encoder_probe_test.dart new file mode 100644 index 0000000..6fc20d7 --- /dev/null +++ b/app/test/hardware_encoder_probe_test.dart @@ -0,0 +1,37 @@ +// Guards the hardware-encoder functional probe against regressing to a frame +// size that hardware encoders reject (issue #51: AMD AMF fails at 64x64, so +// working AMF encoders were reported as broken). +// +// Run with: flutter test test/hardware_encoder_probe_test.dart + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/services/hardware_encoder_detector.dart'; + +void main() { + /// The largest minimum-dimension any bundled hardware encoder imposes. AMF is + /// the strictest at 192x128 on some ASICs; the probe must clear it with room + /// to spare on both axes. + const int strictestEncoderMinimum = 192; + + test('probes at a frame size every hardware encoder accepts', () { + expect( + HardwareEncoderDetector.probeFrameSize, + greaterThan(strictestEncoderMinimum), + reason: 'a frame below the encoder minimum makes a working encoder ' + 'report failure (issue #51)', + ); + // Odd dimensions are their own source of encoder rejections. + expect(HardwareEncoderDetector.probeFrameSize.isEven, isTrue); + }); + + test('probe args encode one frame with the requested codec', () { + final args = HardwareEncoderDetector.probeArgs('hevc_amf'); + + final size = HardwareEncoderDetector.probeFrameSize; + expect(args, containsAllInOrder(['-i', 'color=c=black:s=${size}x$size:r=5:d=1'])); + expect(args, containsAllInOrder(['-c:v', 'hevc_amf'])); + expect(args, containsAllInOrder(['-frames:v', '1'])); + // Discard the output — the probe only cares whether the encoder opens. + expect(args, containsAllInOrder(['-f', 'null', '-'])); + }); +} diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index f2bc9fa..50f0e55 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -456,10 +456,62 @@ impl VideoCodec { pub fn forced_pix_fmt(&self) -> Option<&'static str> { match self { VideoCodec::Huffyuv => Some("yuv422p"), + // AMF takes nv12 natively. Left to negotiate, ffmpeg will hand a + // >8-bit source to the encoder as p010, and 10-bit HEVC encode is + // only supported on some AMD ASICs — where it isn't, the AMF + // runtime faults (0xC0000005) instead of failing cleanly, which is + // one candidate for the crashes in issue #51. h264_amf has no + // 10-bit mode at all. Pinning nv12 makes the conversion explicit + // and identical on every card. Custom FFmpeg Arguments still wins: + // a later -pix_fmt overrides this one. + VideoCodec::H264Amf | VideoCodec::H265Amf => Some("nv12"), _ => None, } } + /// Encoder presets this codec accepts. Mirrors `availablePresets` in + /// `app/lib/models/video_job.dart` — keep the two in step. + pub fn available_presets(&self) -> &'static [&'static str] { + match self.encoder_family() { + EncoderFamily::Software => &[ + "ultrafast", "superfast", "veryfast", "faster", "fast", "medium", + "slow", "slower", "veryslow", "placebo", + ], + EncoderFamily::Nvenc => &["p1", "p2", "p3", "p4", "p5", "p6", "p7"], + EncoderFamily::Qsv => &[ + "veryfast", "faster", "fast", "medium", "slow", "slower", "veryslow", + ], + EncoderFamily::Amf => &["speed", "balanced", "quality"], + // VideoToolbox, ProRes and the lossless codecs take no preset. + _ => &[], + } + } + + /// The preset to fall back on when the configured one isn't accepted. + pub fn default_preset(&self) -> &'static str { + match self.encoder_family() { + EncoderFamily::Nvenc => "p4", + EncoderFamily::Amf => "balanced", + _ => "medium", + } + } + + /// The configured preset if this encoder accepts it, else its default. + /// + /// Preset vocabularies are per-family and don't overlap — x264's "medium" + /// means nothing to AMF, which wants speed/balanced/quality. A saved preset + /// or an imported job config can pair a codec with another family's preset, + /// and passing that through makes ffmpeg reject the option and fail the + /// whole encode. Falling back keeps the job running with a sane setting. + pub fn normalized_preset(&self, configured: &str) -> String { + let allowed = self.available_presets(); + if allowed.is_empty() || allowed.contains(&configured) { + configured.to_string() + } else { + self.default_preset().to_string() + } + } + /// Whether this codec produces H.264 output. pub fn is_h264(&self) -> bool { matches!(self, VideoCodec::H264 | VideoCodec::H264Nvenc | VideoCodec::H264Qsv | diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index 7c60a51..416fae6 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -810,6 +810,9 @@ impl PipelineExecutor { /// Build encoder-family-specific quality and preset arguments. fn build_encoder_quality_args(args: &mut Vec, job: &VideoJob) { let settings = &job.encoding_settings; + // Never pass another family's preset vocabulary to the encoder — ffmpeg + // rejects the option and the encode dies. See `normalized_preset`. + let preset = settings.codec.normalized_preset(&settings.encoder_preset); if let Some(profile) = settings.codec.prores_profile() { args.push("-profile:v".to_string()); args.push(profile.to_string()); @@ -817,7 +820,7 @@ impl PipelineExecutor { match settings.codec.encoder_family() { EncoderFamily::Software => { args.extend(["-crf".to_string(), settings.quality.to_string()]); - args.extend(["-preset".to_string(), settings.encoder_preset.clone()]); + args.extend(["-preset".to_string(), preset.clone()]); } EncoderFamily::Nvenc => { // VBR with constant quality (-cq) provides much better quality @@ -826,7 +829,7 @@ impl PipelineExecutor { args.extend(["-rc".to_string(), "vbr".to_string()]); args.extend(["-cq".to_string(), settings.quality.to_string()]); args.extend(["-b:v".to_string(), "0".to_string()]); - args.extend(["-preset".to_string(), settings.encoder_preset.clone()]); + args.extend(["-preset".to_string(), preset.clone()]); args.extend(["-tune".to_string(), "hq".to_string()]); args.extend(["-multipass".to_string(), "fullres".to_string()]); args.extend(["-rc-lookahead".to_string(), "32".to_string()]); @@ -840,7 +843,7 @@ impl PipelineExecutor { } EncoderFamily::Qsv => { args.extend(["-global_quality".to_string(), settings.quality.to_string()]); - args.extend(["-preset".to_string(), settings.encoder_preset.clone()]); + args.extend(["-preset".to_string(), preset.clone()]); } EncoderFamily::Videotoolbox => { // VideoToolbox's constant-quality mode (-q:v) is only @@ -872,7 +875,12 @@ impl PipelineExecutor { args.extend(["-rc".to_string(), "cqp".to_string()]); args.extend(["-qp_i".to_string(), settings.quality.to_string()]); args.extend(["-qp_p".to_string(), settings.quality.to_string()]); - args.extend(["-quality".to_string(), settings.encoder_preset.clone()]); + // B-frames were left on the encoder's default QP while I and + // P were pinned, so a GOP could mix a chosen quality with an + // unrelated one. AMF ignores the option on ASICs without + // B-frame support, so setting it is safe either way. + args.extend(["-qp_b".to_string(), settings.quality.to_string()]); + args.extend(["-quality".to_string(), preset.clone()]); } EncoderFamily::Lossless | EncoderFamily::ProRes => { // No quality/preset args needed @@ -1707,10 +1715,76 @@ mod tests { assert!(qp_p_idx.is_some(), "AMF should use -qp_p"); assert_eq!(args[qp_p_idx.unwrap() + 1], "20"); + let qp_b_idx = args.iter().position(|a| a == "-qp_b"); + assert!(qp_b_idx.is_some(), "AMF should pin -qp_b too (issue #51)"); + assert_eq!(args[qp_b_idx.unwrap() + 1], "20"); + let quality_idx = args.iter().position(|a| a == "-quality"); assert!(quality_idx.is_some(), "AMF should use -quality"); assert_eq!(args[quality_idx.unwrap() + 1], "balanced"); assert!(!args.contains(&"-crf".to_string()), "AMF should not use -crf"); } + + /// Issue #51: left to negotiate, a >8-bit source reaches AMF as p010 and + /// 10-bit HEVC encode only exists on some AMD ASICs — elsewhere the runtime + /// faults rather than failing cleanly. Pin the format both encoders always + /// support. + #[test] + fn test_ffmpeg_args_amf_forces_nv12() { + for codec in [VideoCodec::H264Amf, VideoCodec::H265Amf] { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = codec; + + let args = build_ffmpeg_args_for_test(&job); + + let idx = args.iter().position(|a| a == "-pix_fmt"); + assert!(idx.is_some(), "{:?} should force a pixel format", codec); + assert_eq!(args[idx.unwrap() + 1], "nv12", "codec {:?}", codec); + } + } + + /// A preset from another encoder family (a saved preset carrying x264's + /// "medium", say) must not reach ffmpeg — it rejects the option and the + /// whole encode fails. Each family falls back to its own default instead. + #[test] + fn test_ffmpeg_args_foreign_preset_falls_back() { + let cases = [ + (VideoCodec::H264Amf, "-quality", "balanced"), + (VideoCodec::H264Nvenc, "-preset", "p4"), + (VideoCodec::H264Qsv, "-preset", "medium"), + ]; + + for (codec, flag, expected) in cases { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = codec; + // "placebo" is x264-only — meaningless to every hardware family. + job.encoding_settings.encoder_preset = "placebo".to_string(); + + let args = build_ffmpeg_args_for_test(&job); + let idx = args.iter().position(|a| a == flag); + assert!(idx.is_some(), "{:?} should pass {}", codec, flag); + assert_eq!(args[idx.unwrap() + 1], expected, "codec {:?}", codec); + } + } + + /// A preset the encoder *does* accept passes through untouched. + #[test] + fn test_ffmpeg_args_valid_preset_preserved() { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = VideoCodec::H265Amf; + job.encoding_settings.encoder_preset = "speed".to_string(); + + let args = build_ffmpeg_args_for_test(&job); + let idx = args.iter().position(|a| a == "-quality").unwrap(); + assert_eq!(args[idx + 1], "speed"); + + // Software x264 keeps its own vocabulary. + let mut sw = create_test_job("output.mp4"); + sw.encoding_settings.codec = VideoCodec::H264; + sw.encoding_settings.encoder_preset = "veryslow".to_string(); + let sw_args = build_ffmpeg_args_for_test(&sw); + let sw_idx = sw_args.iter().position(|a| a == "-preset").unwrap(); + assert_eq!(sw_args[sw_idx + 1], "veryslow"); + } }