Fix RGB<->YCbCr/GRAY color conversion for signed and float pixel types - #6514
Open
jantonguirao wants to merge 5 commits into
Open
jantonguirao wants to merge 5 commits into
jantonguirao wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Signed minimum values remain inconsistent between CPU and GPU when producing floating-point output.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes signed and floating-point RGB/YCbCr/GRAY conversion by applying normalized saturation within shared color-conversion math.
Changes:
- Adds normalized input saturation across color conversions.
- Adds exhaustive type-pair and CPU/GPU agreement tests.
- Adds signed TIFF decoding regressions.
| File | Description |
|---|---|
color_space_conversion_impl.h |
Implements shared saturation logic. |
color_space_conversion_impl_test.cc |
Tests conversion types and edge values. |
convert_gpu_test.cc |
Compares CPU and GPU conversions. |
test_imgcodec.py |
Tests signed TIFF decoding end-to-end. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The color conversion helpers in color_space_conversion_impl.h scale the input by max_value<Output>() / max_value<Input>(), i.e. they already treat integer samples as normalized (UNORM/SNORM) values, but they fed the normalized value straight into the conversion matrix. For a signed input and an unsigned output this means that a negative sample contributes a large negative term to the dot product, producing a color that is not representable in the output and saturates to a wrong value - e.g. the imgcodec CPU decoder produced YCbCr [0, 255, 255] instead of [106, 202, 221] for an int32 RGB pixel [2122218880, -2088533504, 2139061888], while the GPU path (which normalizes to the output type first, then converts the color space) produced the latter. Saturate each input sample to the range of normalized values that ConvertSatNorm<Output> keeps, before the color transform: - signed integers are SNORM (v / max_value), min_value is clamped to -1, - [0, 1] for unsigned outputs, [-1, 1] for signed integer outputs, - floating point outputs are unbounded (float -> float is unchanged). This makes the conversion from any input type equivalent to ConvertSatNorm<Output> followed by the conversion in Output type (which is what the GPU decoder path does, and consistent with decoding to RGB), but done in a single step, without the intermediate rounding. Unsigned inputs are never modified, so results for unsigned inputs (and for float -> float) are bit-identical to before. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
Add a double-precision reference of the UNORM/SNORM convention and check rgb_to_y/cb/cr, rgb_to_ycbcr, ycbcr_to_rgb, gray_to_y, y_to_gray, gray_to_ycbcr, ycbcr_to_gray and rgb_to_gray (ITU-R BT.601 and JPEG) for every Input x Output combination of uint8/int8/uint16/int16/uint32/int32/ float, over all combinations of edge values (min, min+1, -max, -max/3, -1, 0, 1, max/3, max/2+1, max-1, max) and random values over the full range of the type. Also check that converting directly is equivalent to ConvertSatNorm to the output type followed by the conversion in the output type (the GPU decoder path), pin hand-computed values for signed and cross-type conversions (incl. a pixel from a real int32 TIFF), statically check the input saturation helper, and extend the existing round-trip tests with signed types. Fix an integer division in the ITU reference gray_to_y, which made it inexact for types whose max_value is not a multiple of 255 (int16). Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
ConvertGPU normalizes the data to the output type before converting the color space, while ConvertCPU converts directly from the input type. Check that both agree (up to the GPU's intermediate rounding) for signed, unsigned and float inputs and outputs, for all RGB/BGR/YCbCr/GRAY conversions, and pin the int32 RGB pixel for which CPU and GPU decoding to YCbCr/GRAY used to differ. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
Decode an int32 TIFF (the DALI_extra fixture and a synthetic image with all combinations of edge values) to uint8 YCbCr and GRAY on both backends and compare them with each other and with an independent numpy reference of the SNORM convention. The test requires int32 nvImageCodec sample types to be mapped to DALI types (and the DALI_extra fixture) and, for the CPU backend, an nvImageCodec whose libTIFF extension decodes signed samples; it is skipped otherwise. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
…loat output Both Copilot and Greptile independently flagged the same real inconsistency: saturate_norm clamped a signed sample's minimum value to -1 unconditionally, but ConvertGPU/ConvertSatNorm<float> only clamp for an integer Output, leaving it unclamped (e.g. int8_t(-128) -> -128/127) for a floating point Output. Only apply the clamp when Output is an integer type, matching GPU, and update the reference implementation, static_asserts, pinned values and the CPU/GPU equivalence test accordingly (it now also covers float outputs, which were previously excluded specifically because of this mismatch). Also address the remaining review comments: - Bump the copyright year on the substantially-modified test file. - Replace the broad "any RuntimeError containing 'Failed to decode' becomes a skip" pattern in the new Python test with a one-time capability probe on a tiny synthetic image; the real per-image assertions no longer swallow exceptions, so a genuine decoding regression now fails the test instead of silently skipping it. - Add a test for empty and corrupt (non-TIFF) input rejection on both backends. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
jantonguirao
force-pushed
the
fix/color-space-conversion-signed-types
branch
from
September 29, 2026 16:36
75a9012 to
94ad44d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
kernels::color::itu_r_bt_601/jpeg(dali/kernels/imgproc/color_manipulation/color_space_conversion_impl.h)implement the RGB↔YCbCr/GRAY color-space math used by both the CPU and GPU imgcodec decode paths.
The coefficients/biases are calibrated for a channel value in
[0, max_value<T>()], as for atypical unsigned pixel type, but the code had no notion of a signed
Inputwhose raw value can benegative. On the CPU path (
ConvertCPUindali/operators/imgcodec/util/convert.h), this matrixran directly on raw signed samples: a negative sample contributed a large negative term to the dot
product instead of a small positive one, producing wildly wrong, unclamped Y/Cb/Cr values (e.g. a
signed int32 TIFF decoded to
YCbCrgave[0, 255, 255]instead of a sane value oncpu, whilemixed/GPU decoded it correctly).Investigation showed the GPU path (
ConvertGPU/convert_gpu.cu) already avoids this bug: itnormalizes to the output dtype first (scale + saturating clamp, matching the SNORM→UNORM
convention used elsewhere in DALI's own
dali/core/convert.hand matching OpenGL/Vulkan/D3D'ssigned-normalized-texture convention) and only then runs the color conversion on already-clamped,
same-typed data. This PR ports that same clamp into the shared color-math itself
(
detail::saturate_norm<Output>(Input)), so the fix is in one place and both backends'<Output, Input>color functions can be called directly on raw signed (or float) samples with correct,consistent results — mathematically equivalent to "convert to
Outfirst, then color-convert",but in a single step with no intermediate rounding.
This generalizes the narrower CPU-only workaround from #6510 (which pre-scales to
OutviaConvertSatNormbefore calling the color matrix) into the shared math, so it benefits everycaller of
color_space_conversion_impl.h, not just the imgcodec CPU decode path.Additional information:
Affected modules and functionalities:
dali/kernels/imgproc/color_manipulation/color_space_conversion_impl.h— addeddetail::saturate_norm<Output>(Input), applied inrgb_to_y,rgb_to_cb,rgb_to_cr,rgb_to_ycbcr,y_to_gray,gray_to_y,ycbcr_to_rgb,ycbcr_to_gray,gray_to_ycbcr(bothitu_r_bt_601andjpegcoefficient sets), and the freergb_to_gray. Public<Output, Input>signatures are unchanged.
dali/kernels/imgproc/color_manipulation/color_space_conversion_impl_test.cc— extensive newcoverage (see Tests below); also fixes an inexact integer-division bug in the existing
itu_ref::gray_to_ytest-reference helper forint16.dali/operators/imgcodec/util/convert_gpu_test.cc— new CPU-vs-GPU agreement tests across dtypepairs and format conversions.
dali/test/python/decoder/test_imgcodec.py— new end-to-end regression test decoding a signedint32 TIFF to
YCbCr/GRAYoncpuandmixedand checking both against a numpy reference.Key points relevant for the review:
[-1, 1]via division bymax_value<Input>(), with the single extra negative code-2^(n-1)clamping to-1), matchingConvertSatNorm/ConvertNormelsewhere in DALI andmatching how the (already-correct) GPU path behaves — not a signed→unsigned
0.5*(1+snorm)shift, which would make0decode as mid-gray instead of black and beinconsistent with plain RGB/ANY_DATA decode of the same source.
float inputs (no regression for the common
uint8_tcase, which the pre-existingstatic_asserts incolor_space_conversion_impl_test.cccontinue to pin).1 LSB forward / 3 LSB inverse, or a sub-0.2% bias interpretation difference for
differing-bit-depth YCbCr inputs) — CPU is the more numerically correct of the two; test
tolerances account for the difference.
this DALI color-conversion code) and is linear/unclamped there, which is inconsistent with the
RGB/YCbCr convention fixed here — that would need a follow-up on the nvImageCodec side, out of
scope for this PR.
MR !676 (CPU signed-TIFF decode support) to actually exercise the
cpubackend; until both aremerged/released it skips itself rather than failing.
Tests:
Existing tests apply
New tests added
N/A
color_space_conversion_impl_test.cc: a double-precision SNORM/UNORM reference checked acrossall 7×7
Input×Outputtype pairs (uint8_t/int8_t/uint16_t/int16_t/uint32_t/int32_t/float) × {ITU-R BT.601, JPEG} coefficient sets, for every conversion function(
rgb_to_y/cb/cr,rgb_to_ycbcr,ycbcr_to_rgb,gray_to_y,y_to_gray,gray_to_ycbcr,ycbcr_to_gray,rgb_to_gray), at edge values (min,min+1,-max,-max/3,-1,0,1,max/3,max/2+1,max-1,max) plus randomized values over the full range; anequivalence check against "convert-to-
Out-first" (GPU semantics);static_asserts on the newsaturation helper; and pinned hand-computed values (including the originally-failing int32
pixel,
0decoding to black, the SNORM-128clamp, and float clamping).convert_gpu_test.cc:ConvertCPUvsConvertGPUagreement across 16 dtype pairs × 8 colorformat conversions, plus the originally-failing int32 pixel on both backends.
test_imgcodec.py:test_tiff_signed_color_conversion— decodes a signed int32 TIFF (thereal fixture plus a synthetic all-edge-values one) to
YCbCr/GRAYoncpuandmixed,checked against numpy.
Full existing
dali_kernel_testandtest_imgcodec.pysuites run with zero new regressions(the only remaining Python failures are 48 pre-existing
test_image_decoder_fusederrors froman unrelated nvImageCodec-version mismatch in the local dev environment, and the int32/uint32
fixture-dependent tests that require Fix uint32/int32 TIFF decoding (missing dtype mapping) #6510/MR !676 to be present, which correctly skip here).
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A