Improve RGB input handling in color conversion (support normalized an…#2264
Improve RGB input handling in color conversion (support normalized an…#2264THE-Amrit-mahto-05 wants to merge 2 commits intoCCExtractor:masterfrom
Conversation
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit ad4886e...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit ad4886e...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
cfsmp3
left a comment
There was a problem hiding this comment.
Deep Review — Regression Found
The analysis is correct: rgb_to_hsv expected [0.0, 1.0] but all callers pass [0, 255]. That's a real inconsistency. However, the fix introduces a regression.
Regression: white text on black background
We tested with a simple burned-in subtitle video:
ffmpeg -y -f lavfi -i color=c=black:s=1280x720:r=25:d=10 \
-vf "drawtext=fontfile=/usr/share/fonts/truetype/dejavu/DejaVuSans.ttf:\
text='HARDSUBX TEST':fontcolor=white:fontsize=40:x=40:y=h-60" \
-c:v libx264 -pix_fmt yuv420p hardsubx_test.mp4
ccextractor --quiet --hardsubx --ocrlang eng -o output.srt hardsubx_test.mp4| Build | Output |
|---|---|
| Master | HARDSUBX TEST (correct) |
| This PR | WARDSUBX TEST (first character lost) |
Yellow-on-blue text produced identical correct output on both, so the regression is color-combination dependent.
Root cause of the regression
The auto-detection heuristic (if max_val > 1.0, assume 0-255) changes the HSV color classification boundary, which shifts which pixels are detected as text vs background. The "H" is at the edge of the detected text region and gets clipped.
Suggested fix
Don't auto-detect. Both functions should consistently expect [0, 255] since that's what ALL callers pass (both C and Rust callers pass r as f32 where r is a pixel byte value). The fix should be:
// rgb_to_hsv — currently treats input as [0,1], should normalize like rgb_to_lab
let rgb = LinSrgb::new(R / 255.0, G / 255.0, B / 255.0);This matches what rgb_to_lab already did before this PR. Simple, explicit, no auto-detection magic.
Requirements for resubmission
- Fix the regression — use explicit
/255.0normalization, not auto-detection - Test with the sample above and include the exact output in the PR description
- Show before/after — what does master produce vs your fix on at least 2 different color combinations
- No repro, no merge — we need to see the exact commands and exact output, not just "cargo test passes"
The underlying bug is real and worth fixing. Just needs a simpler, non-regressing approach.
fix Standardize RGB input to normalized range in hardsubx imgops
Reason for this PR:
Sanity check:
Problem
The
rgb_to_hsvandrgb_to_labfunctions in src/rust/src/hardsubx/imgops.rs assumed inconsistent RGB input formats across callers.Some inputs were provided in normalized format
[0.0, 1.0], while legacy C-side callers used[0, 255], leading to incorrect color conversion results.Root Cause
The implementation did not consistently handle mixed input formats, causing:
Fix
This PR adds backward-compatible input normalization:
This ensures both legacy C callers and Rust callers work correctly.
Repro Steps
cargo test --features hardsubx_ocr imgops::testBefore fix:
test_rgb_to_labfails due to incorrect scalingAfter fix:
Expected behavior
RGB inputs are treated consistently as normalized values in [0.0, 1.0].
Actual behavior (before fix)
Incorrect color conversion due to missing normalization handling.
Impact