fix(export): match ProRes swscale output to the declared full colour range - #2034
fix(export): match ProRes swscale output to the declared full colour range#2034DPS0340 wants to merge 4 commits into
Conversation
The ProRes encoder advertises Range::JPEG (full range) in the stream metadata, but the RGBA -> YUVA444P10LE conversion used swscale's default, which emits limited range (16-235). Players then expand the already limited-range samples as if they were full range, crushing contrast and shifting colour. Tell swscale to emit full range so the pixels match what the stream declares. ProRes 4444 is a full-range format, so the declaration is correct and the conversion was the side that was wrong.
|
@tembo please review |
| let mut inv_table: *const i32 = std::ptr::null(); | ||
| let mut table: *const i32 = std::ptr::null(); | ||
| let mut src_range: i32 = 0; | ||
| let mut dst_range: i32 = 0; | ||
| let mut brightness: i32 = 0; | ||
| let mut contrast: i32 = 0; | ||
| let mut saturation: i32 = 0; | ||
|
|
||
| if ffmpeg::ffi::sws_getColorspaceDetails( | ||
| context.as_mut_ptr(), | ||
| &mut inv_table as *mut _ as *mut *mut i32, | ||
| &mut src_range, | ||
| &mut table as *mut _ as *mut *mut i32, | ||
| &mut dst_range, | ||
| &mut brightness, | ||
| &mut contrast, | ||
| &mut saturation, | ||
| ) >= 0 |
There was a problem hiding this comment.
Nice catch on swscale's default range mismatch. One small thing: you can avoid the const/mut pointer casts here by matching the FFI signature directly.
| let mut inv_table: *const i32 = std::ptr::null(); | |
| let mut table: *const i32 = std::ptr::null(); | |
| let mut src_range: i32 = 0; | |
| let mut dst_range: i32 = 0; | |
| let mut brightness: i32 = 0; | |
| let mut contrast: i32 = 0; | |
| let mut saturation: i32 = 0; | |
| if ffmpeg::ffi::sws_getColorspaceDetails( | |
| context.as_mut_ptr(), | |
| &mut inv_table as *mut _ as *mut *mut i32, | |
| &mut src_range, | |
| &mut table as *mut _ as *mut *mut i32, | |
| &mut dst_range, | |
| &mut brightness, | |
| &mut contrast, | |
| &mut saturation, | |
| ) >= 0 | |
| let mut inv_table: *mut i32 = std::ptr::null_mut(); | |
| let mut table: *mut i32 = std::ptr::null_mut(); | |
| let mut src_range: i32 = 0; | |
| let mut dst_range: i32 = 0; | |
| let mut brightness: i32 = 0; | |
| let mut contrast: i32 = 0; | |
| let mut saturation: i32 = 0; | |
| if ffmpeg::ffi::sws_getColorspaceDetails( | |
| context.as_mut_ptr(), | |
| &mut inv_table, | |
| &mut src_range, | |
| &mut table, | |
| &mut dst_range, | |
| &mut brightness, | |
| &mut contrast, | |
| &mut saturation, | |
| ) >= 0 |
Addresses review feedback: sws_getColorspaceDetails takes *mut *mut i32, so declaring the locals as *mut i32 removes the const-to-mut casts.
|
Adopted — pushed. You're right, I'd defensively written them as |
|
@tembo please re-review |
| let coefficients = ffmpeg::ffi::sws_getCoefficients(ffmpeg::ffi::SWS_CS_ITU709); | ||
|
|
||
| ffmpeg::ffi::sws_setColorspaceDetails( | ||
| context.as_mut_ptr(), | ||
| coefficients, | ||
| 1, | ||
| coefficients, | ||
| 1, | ||
| brightness, | ||
| contrast, | ||
| saturation, | ||
| ); |
There was a problem hiding this comment.
Might be worth guarding the FFI calls here: check sws_getCoefficients for null and sws_setColorspaceDetails for an error return. If either ever fails, a warn log will make the fallback behavior obvious.
| let coefficients = ffmpeg::ffi::sws_getCoefficients(ffmpeg::ffi::SWS_CS_ITU709); | |
| ffmpeg::ffi::sws_setColorspaceDetails( | |
| context.as_mut_ptr(), | |
| coefficients, | |
| 1, | |
| coefficients, | |
| 1, | |
| brightness, | |
| contrast, | |
| saturation, | |
| ); | |
| let coefficients = ffmpeg::ffi::sws_getCoefficients(ffmpeg::ffi::SWS_CS_ITU709); | |
| if coefficients.is_null() { | |
| tracing::warn!("sws_getCoefficients returned null for ITU709"); | |
| } else { | |
| let ret = ffmpeg::ffi::sws_setColorspaceDetails( | |
| context.as_mut_ptr(), | |
| coefficients, | |
| 1, | |
| coefficients, | |
| 1, | |
| brightness, | |
| contrast, | |
| saturation, | |
| ); | |
| if ret < 0 { | |
| tracing::warn!("sws_setColorspaceDetails failed: {ret}"); | |
| } | |
| } |
There was a problem hiding this comment.
Adopted — pushed b217944a7.
Took the guards, and split them so each failure mode names itself instead of sharing one message:
sws_getColorspaceDetails < 0— previously this silently skipped the whole block. Now it warns.sws_getCoefficientsnull — warns and leaves swscale untouched.sws_setColorspaceDetails < 0— warns with the return code.
One deliberate difference from the suggestion: the getColorspaceDetails failure path now logs too. It was already an early-exit in my original code, so that branch was the one silent failure that could produce exactly the bug this PR fixes — limited-range pixels in a stream that declares Range::JPEG — with nothing in the logs to point at it. Both < 0 messages say that explicitly so the symptom is searchable.
Kept them at warn! rather than propagating an error: the fallback is the current shipping behaviour, so failing the export outright would be a regression for anyone whose ffmpeg build returns an error here.
cargo check -p cap-enc-ffmpeg, cargo clippy -p cap-enc-ffmpeg, cargo fmt -p cap-enc-ffmpeg -- --check all clean.
|
@tembo please re-review |
| saturation, | ||
| ); | ||
|
|
||
| if ret < 0 { |
There was a problem hiding this comment.
One thought on the failure path: if sws_getColorspaceDetails/sws_setColorspaceDetails fails, we’ll still declare Range::JPEG later, which reintroduces the mismatch you’re fixing (just with a warning). Might be worth tracking a declare_full_range flag here and falling back to Range::MPEG when swscale can’t be configured, so the file stays internally consistent.
There was a problem hiding this comment.
Right, and this is the better version of my own fix — adopted in bb913fd88.
My warnings described the mismatch instead of preventing it: the log said "output will stay limited-range while the stream declares full range", which is just an accurate description of the original bug with better observability. Tracking the flag makes the file internally consistent either way.
let mut declare_full_range = true;
// ... any of the three failure paths sets it to false
encoder.set_color_range(if declare_full_range { color::Range::JPEG } else { color::Range::MPEG });So the two possible outcomes are now: swscale configured full-range + Range::JPEG declared (the fix), or swscale left at its limited-range default + Range::MPEG declared (correct, just not the wider range). Neither declares something the pixels don't match.
One thing I checked before wiring the flag, since it decides whether the fallback is reachable at all: declare_full_range is only ever cleared inside the converter branch, so I wanted to confirm the None branch isn't a silent third case. It isn't reachable in practice — ProResEncoder::input_format() is RawVideoFormat::Rgba and output_format is YUVA444P10LE, so input_config.pixel_format != output_format always holds and a converter is always built. The flag stays true on a path that can't occur, and if that input format ever changes there'd be no swscale conversion to disagree with.
cargo check, cargo clippy, cargo fmt --check all clean for cap-enc-ffmpeg.
|
@tembo please re-review |
|
Closing for queue size — trimming my nine open PRs down to five so the ones closing real user reports are not buried behind them. This is the most expensive of my PRs to review and the least urgent, which is a bad combination when you have a backlog. It uses The finding itself I still believe is correct and worth recording, so it is not lost if this stays closed: encoder.set_color_range(color::Range::JPEG); // we declare full range 0-255but the RGBA → No user has reported this, which is why it goes rather than one of the four that close user-filed issues. Happy to reopen against current |
Problem
The ProRes encoder declares full range in the stream metadata:
…but the RGBA →
YUVA444P10LEconversion feeding it is a plainscaling::Context::get(...)with no colorspace details set, and swscale defaults to limited range output (16-235).So the pixels written are limited-range while the container says full-range. A player reads
Range::JPEG, assumes 16-235 was never applied, and expands the samples again — blacks lift, whites clip, and the whole image shifts. It's the classic double-range-conversion look.Evidence
Reproduced the swscale default outside of Cap, so this doesn't depend on reading our pipeline correctly. A 2×2 image of pure white and pure black, converted with ffmpeg:
Pure white lands on 235 and pure black on 16 under the default — i.e. limited range — while the encoder is telling the container those same samples are full range. Confirms the two sides disagree.
Why ProRes specifically
h264.rsandhevc.rsboth declareRange::MPEGand also use swscale's limited-range default, so those two are internally consistent — I checked before touching anything, and deliberately left them alone.prores.rsis the only encoder declaringRange::JPEG, which makes it the only one where the declaration and the conversion disagree. ProRes 4444 is a full-range format, so the declaration is right and the conversion was the side that was wrong — hence fixing the scaler rather than downgrading the metadata toMPEG.Fix
Set the swscale colorspace details so source and destination are both full range, using the BT.709 coefficients that match the declared
Space::BT709:Existing brightness/contrast/saturation are read back via
sws_getColorspaceDetailsand preserved rather than reset to zero, and the whole thing is guarded on that call succeeding, so a failure leaves the previous behaviour intact instead of silently mangling the conversion.Scope
This affects MOV/ProRes export (
crates/export/src/mov.rs), which builds frames asRawVideoFormat::Rgbaand hands them straight to this encoder. MP4/H.264 export is untouched.Relevant to #1914, which reports colour shift on exported video — though that report is about an external capture card and also describes stutter and compression artifacts, so I'd expect this to be one contributing factor rather than the whole story. I didn't want to claim it closes that issue without a capture card to verify against.
Verification
cargo check -p cap-enc-ffmpeg— clean.cargo build -p cap-enc-ffmpeg— clean.cargo clippy -p cap-enc-ffmpeg— no warnings on this file.cargo fmtapplied.Diff: 1 file.