hdr: correct HDR10 (PQ/BT.2020) -> SDR tone-map primitive + test#21
Conversation
First piece of #16. drmtap_convert_format's AR30/XR30 path is a naive >>2 bit-shift: fine for SDR 10-bit, but for HDR10 it just keeps the top 8 of the 10 PQ-encoded bits with no PQ decode / gamut / tone-map, so an HDR scanout captures washed out (a 1000-nit highlight came out ~53% gray). Add drmtap_tonemap_hdr10(): PQ (SMPTE ST 2084) EOTF to linear light, BT.2020 -> BT.709 gamut, a highlight-preserving tone-map to the SDR range (diffuse white = 203 nits per BT.2408, highlights rolled off toward white), then the sRGB OETF back to 8-bit. PQ and sRGB are table-driven (built once via pthread_once); only the 3x3 gamut matrix and the tone curve run per pixel. test_hdr.c validates the curve without HDR hardware: it PQ-encodes known luminances and checks the 8-bit result is monotonic, dark stays dark, SDR white maps bright (250, not crushed), and highlights reach white. This is the conversion primitive only. Deciding when to apply it (HDR vs plain SDR 10-bit, from the connector Colorspace / HDR_OUTPUT_METADATA, since the pixel format alone does not say) and wiring it into the capture pipeline is the next step on #16; the EGL readback path and the XR48/AR48/P010 formats also remain. Part of #16
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a public HDR10 tone-mapping API, implements PQ/BT.2020 to SDR conversion in the C library and Rust sys crate copy, links ChangesHDR10 tone mapping
Sequence Diagram(s)sequenceDiagram
participant Test as "tests/test_hdr.c"
participant ToneMap as "drmtap_tonemap_hdr10()"
participant Once as "pthread_once"
participant Init as "hdr_lut_init()"
participant Pixel as "tonemap_ar30_pixel"
Test->>ToneMap: drmtap_tonemap_hdr10(src, dst, ...)
ToneMap->>Once: initialize LUTs once
Once->>Init: fill PQ and sRGB tables
ToneMap->>Pixel: tonemap each AR30/XR30 pixel
ToneMap-->>Test: 0 or error code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/rust/libdrmtap-sys/csrc/pixel_convert.c`:
- Around line 424-441: The row-stride checks in drmtap_tonemap_hdr10 are missing
in this Rust sys copy, so callers can pass src_stride or dst_stride smaller than
width * 4 and trigger out-of-bounds access when the per-row loop indexes 32-bit
pixels. Add the same validation used elsewhere in pixel_convert.c before
pthread_once/hdr_lut_init and the row loop, rejecting inputs where either stride
is too small for the requested width, and keep the existing drmtap_tonemap_hdr10
argument checks and format gate intact.
In `@src/pixel_convert.c`:
- Around line 406-413: Rename the new channel temporaries in the pixel
conversion path to snake_case to match C style guidelines. Update the BT.2020
linear-light inputs in the conversion logic around the pixel unpacking and
matrix transform so the variables currently named Rl, Gl, and Bl use descriptive
snake_case names such as r_nits, g_nits, and b_nits, and keep the subsequent
r/g/b calculation using the renamed locals.
- Around line 424-441: The drmtap_tonemap_hdr10 routine currently only validates
pointers, dimensions, and src_format, but it still assumes each row is wide
enough for width 32-bit pixels. Add stride validation in drmtap_tonemap_hdr10
before the loop so src_stride and dst_stride must be at least width *
sizeof(uint32_t), and return -EINVAL when either row stride is too small to hold
a full row; use the existing drmtap_tonemap_hdr10 symbol and the row access
through s[x]/d[x] as the place to enforce this check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfa4b31c-4158-4ede-b1c7-f5376782b5ec
📒 Files selected for processing (7)
bindings/rust/libdrmtap-sys/build.rsbindings/rust/libdrmtap-sys/csrc/drmtap.hbindings/rust/libdrmtap-sys/csrc/pixel_convert.cinclude/drmtap.hmeson.buildsrc/pixel_convert.ctests/test_hdr.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Keep lines to a 100-character soft limit and 120-character hard limit.
Use//for single-line comments and/* */for multi-line comments.
Always checkmallocreturns and free resources in cleanup paths.
Files:
tests/test_hdr.cinclude/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.hsrc/pixel_convert.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.c
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: Write tests using simpleassert-based validation without an external test framework.
UseDRM_DEVICEfrom the environment in integration tests instead of hardcoding/dev/dri/card0.
Files:
tests/test_hdr.c
**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
**/*.h: Use 4-space indentation, never tabs, and 1TBS brace style in C header files.
Name C functions and variables in snake_case, with public API functions prefixeddrmtap_.
Name macros and constants in UPPER_SNAKE_CASE, prefixedDRMTAP_when project-specific.
Usesnake_case_tfor internal types anddrmtap_*for public types.
Use#ifndef DRMTAP_MODULE_H/#define DRMTAP_MODULE_Hstyle header guards.
Public API functions ininclude/drmtap.hmust use Doxygen comments describing purpose, parameters, return values, and error codes.
Files:
include/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.h
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Use 4-space indentation, never tabs, and 1TBS brace style in C source files.
Name C functions and variables in snake_case, with public API functions prefixeddrmtap_.
Name macros and constants in UPPER_SNAKE_CASE, prefixedDRMTAP_when project-specific.
Usesnake_case_tfor internal types anddrmtap_*for public types.
Return negative errno values for errors and never abort.
Every.cfile must start with the standard libdrmtap copyright/license header block, followed by a Doxygen@filecomment.
Organize C source files as includes first, then private types/constants, then static functions, then public functions in header-declaration order.
Use simple//comments for internal static functions instead of full Doxygen blocks.
Files:
src/pixel_convert.c
🔇 Additional comments (7)
include/drmtap.h (1)
320-345: LGTM!bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
320-345: LGTM!meson.build (1)
62-64: LGTM!Also applies to: 78-78, 211-221
bindings/rust/libdrmtap-sys/build.rs (1)
62-63: LGTM!src/pixel_convert.c (1)
28-29: LGTM!Also applies to: 329-405, 414-421, 436-450
bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
28-29: LGTM!Also applies to: 329-423, 436-450
tests/test_hdr.c (1)
1-101: LGTM!
| int drmtap_tonemap_hdr10(const void *src, void *dst, | ||
| uint32_t width, uint32_t height, | ||
| uint32_t src_stride, uint32_t dst_stride, | ||
| uint32_t src_format) { | ||
| if (!src || !dst || width == 0 || height == 0) { | ||
| return -EINVAL; | ||
| } | ||
| /* XR30 = XRGB2101010 ('XR30'), AR30 = ARGB2101010 ('AR30'). */ | ||
| if (src_format != 0x30335258u && src_format != 0x30335241u) { | ||
| return -ENOTSUP; | ||
| } | ||
|
|
||
| pthread_once(&g_hdr_once, hdr_lut_init); | ||
|
|
||
| for (uint32_t y = 0; y < height; y++) { | ||
| const uint32_t *s = (const uint32_t *) | ||
| ((const uint8_t *)src + (size_t)y * src_stride); | ||
| uint32_t *d = (uint32_t *)((uint8_t *)dst + (size_t)y * dst_stride); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mirror the row-stride validation in the Rust sys copy.
This copy also indexes width 32-bit pixels per row without ensuring src_stride and dst_stride are at least width * 4, so invalid caller strides can overread or overwrite memory.
Proposed validation
if (src_format != 0x30335258u && src_format != 0x30335241u) {
return -ENOTSUP;
}
+ size_t row_bytes = (size_t)width * sizeof(uint32_t);
+ if (row_bytes > UINT32_MAX ||
+ src_stride < row_bytes ||
+ dst_stride < row_bytes) {
+ return -EINVAL;
+ }
+
pthread_once(&g_hdr_once, hdr_lut_init);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int drmtap_tonemap_hdr10(const void *src, void *dst, | |
| uint32_t width, uint32_t height, | |
| uint32_t src_stride, uint32_t dst_stride, | |
| uint32_t src_format) { | |
| if (!src || !dst || width == 0 || height == 0) { | |
| return -EINVAL; | |
| } | |
| /* XR30 = XRGB2101010 ('XR30'), AR30 = ARGB2101010 ('AR30'). */ | |
| if (src_format != 0x30335258u && src_format != 0x30335241u) { | |
| return -ENOTSUP; | |
| } | |
| pthread_once(&g_hdr_once, hdr_lut_init); | |
| for (uint32_t y = 0; y < height; y++) { | |
| const uint32_t *s = (const uint32_t *) | |
| ((const uint8_t *)src + (size_t)y * src_stride); | |
| uint32_t *d = (uint32_t *)((uint8_t *)dst + (size_t)y * dst_stride); | |
| int drmtap_tonemap_hdr10(const void *src, void *dst, | |
| uint32_t width, uint32_t height, | |
| uint32_t src_stride, uint32_t dst_stride, | |
| uint32_t src_format) { | |
| if (!src || !dst || width == 0 || height == 0) { | |
| return -EINVAL; | |
| } | |
| /* XR30 = XRGB2101010 ('XR30'), AR30 = ARGB2101010 ('AR30'). */ | |
| if (src_format != 0x30335258u && src_format != 0x30335241u) { | |
| return -ENOTSUP; | |
| } | |
| size_t row_bytes = (size_t)width * sizeof(uint32_t); | |
| if (row_bytes > UINT32_MAX || | |
| src_stride < row_bytes || | |
| dst_stride < row_bytes) { | |
| return -EINVAL; | |
| } | |
| pthread_once(&g_hdr_once, hdr_lut_init); | |
| for (uint32_t y = 0; y < height; y++) { | |
| const uint32_t *s = (const uint32_t *) | |
| ((const uint8_t *)src + (size_t)y * src_stride); | |
| uint32_t *d = (uint32_t *)((uint8_t *)dst + (size_t)y * dst_stride); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/rust/libdrmtap-sys/csrc/pixel_convert.c` around lines 424 - 441, The
row-stride checks in drmtap_tonemap_hdr10 are missing in this Rust sys copy, so
callers can pass src_stride or dst_stride smaller than width * 4 and trigger
out-of-bounds access when the per-row loop indexes 32-bit pixels. Add the same
validation used elsewhere in pixel_convert.c before pthread_once/hdr_lut_init
and the row loop, rejecting inputs where either stride is too small for the
requested width, and keep the existing drmtap_tonemap_hdr10 argument checks and
format gate intact.
| double Rl = g_pq_lut[(pixel >> 20) & 0x3FF]; /* BT.2020 linear nits */ | ||
| double Gl = g_pq_lut[(pixel >> 10) & 0x3FF]; | ||
| double Bl = g_pq_lut[(pixel) & 0x3FF]; | ||
|
|
||
| /* BT.2020 -> BT.709 in linear light. */ | ||
| double r = 1.660491 * Rl - 0.587641 * Gl - 0.072850 * Bl; | ||
| double g = -0.124550 * Rl + 1.132900 * Gl - 0.008349 * Bl; | ||
| double b = -0.018151 * Rl - 0.100579 * Gl + 1.118730 * Bl; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the channel temporaries to snake_case.
Rl, Gl, and Bl are new C variables in src/**/*.c; use names like r_nits, g_nits, and b_nits.
Proposed rename
- double Rl = g_pq_lut[(pixel >> 20) & 0x3FF]; /* BT.2020 linear nits */
- double Gl = g_pq_lut[(pixel >> 10) & 0x3FF];
- double Bl = g_pq_lut[(pixel) & 0x3FF];
+ double r_nits = g_pq_lut[(pixel >> 20) & 0x3FF]; /* BT.2020 linear nits */
+ double g_nits = g_pq_lut[(pixel >> 10) & 0x3FF];
+ double b_nits = g_pq_lut[(pixel) & 0x3FF];
/* BT.2020 -> BT.709 in linear light. */
- double r = 1.660491 * Rl - 0.587641 * Gl - 0.072850 * Bl;
- double g = -0.124550 * Rl + 1.132900 * Gl - 0.008349 * Bl;
- double b = -0.018151 * Rl - 0.100579 * Gl + 1.118730 * Bl;
+ double r = 1.660491 * r_nits - 0.587641 * g_nits - 0.072850 * b_nits;
+ double g = -0.124550 * r_nits + 1.132900 * g_nits - 0.008349 * b_nits;
+ double b = -0.018151 * r_nits - 0.100579 * g_nits + 1.118730 * b_nits;As per coding guidelines, src/**/*.c: “Name C functions and variables in snake_case.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double Rl = g_pq_lut[(pixel >> 20) & 0x3FF]; /* BT.2020 linear nits */ | |
| double Gl = g_pq_lut[(pixel >> 10) & 0x3FF]; | |
| double Bl = g_pq_lut[(pixel) & 0x3FF]; | |
| /* BT.2020 -> BT.709 in linear light. */ | |
| double r = 1.660491 * Rl - 0.587641 * Gl - 0.072850 * Bl; | |
| double g = -0.124550 * Rl + 1.132900 * Gl - 0.008349 * Bl; | |
| double b = -0.018151 * Rl - 0.100579 * Gl + 1.118730 * Bl; | |
| double r_nits = g_pq_lut[(pixel >> 20) & 0x3FF]; /* BT.2020 linear nits */ | |
| double g_nits = g_pq_lut[(pixel >> 10) & 0x3FF]; | |
| double b_nits = g_pq_lut[(pixel) & 0x3FF]; | |
| /* BT.2020 -> BT.709 in linear light. */ | |
| double r = 1.660491 * r_nits - 0.587641 * g_nits - 0.072850 * b_nits; | |
| double g = -0.124550 * r_nits + 1.132900 * g_nits - 0.008349 * b_nits; | |
| double b = -0.018151 * r_nits - 0.100579 * g_nits + 1.118730 * b_nits; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pixel_convert.c` around lines 406 - 413, Rename the new channel
temporaries in the pixel conversion path to snake_case to match C style
guidelines. Update the BT.2020 linear-light inputs in the conversion logic
around the pixel unpacking and matrix transform so the variables currently named
Rl, Gl, and Bl use descriptive snake_case names such as r_nits, g_nits, and
b_nits, and keep the subsequent r/g/b calculation using the renamed locals.
Source: Coding guidelines
| int drmtap_tonemap_hdr10(const void *src, void *dst, | ||
| uint32_t width, uint32_t height, | ||
| uint32_t src_stride, uint32_t dst_stride, | ||
| uint32_t src_format) { | ||
| if (!src || !dst || width == 0 || height == 0) { | ||
| return -EINVAL; | ||
| } | ||
| /* XR30 = XRGB2101010 ('XR30'), AR30 = ARGB2101010 ('AR30'). */ | ||
| if (src_format != 0x30335258u && src_format != 0x30335241u) { | ||
| return -ENOTSUP; | ||
| } | ||
|
|
||
| pthread_once(&g_hdr_once, hdr_lut_init); | ||
|
|
||
| for (uint32_t y = 0; y < height; y++) { | ||
| const uint32_t *s = (const uint32_t *) | ||
| ((const uint8_t *)src + (size_t)y * src_stride); | ||
| uint32_t *d = (uint32_t *)((uint8_t *)dst + (size_t)y * dst_stride); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject strides that cannot hold a full row.
Line 444 reads s[x] and Line 445 writes d[x] for width 32-bit pixels, but src_stride/dst_stride can be smaller than width * 4, causing row overreads or overwrites instead of returning -EINVAL.
Proposed validation
if (src_format != 0x30335258u && src_format != 0x30335241u) {
return -ENOTSUP;
}
+ size_t row_bytes = (size_t)width * sizeof(uint32_t);
+ if (row_bytes > UINT32_MAX ||
+ src_stride < row_bytes ||
+ dst_stride < row_bytes) {
+ return -EINVAL;
+ }
+
pthread_once(&g_hdr_once, hdr_lut_init);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int drmtap_tonemap_hdr10(const void *src, void *dst, | |
| uint32_t width, uint32_t height, | |
| uint32_t src_stride, uint32_t dst_stride, | |
| uint32_t src_format) { | |
| if (!src || !dst || width == 0 || height == 0) { | |
| return -EINVAL; | |
| } | |
| /* XR30 = XRGB2101010 ('XR30'), AR30 = ARGB2101010 ('AR30'). */ | |
| if (src_format != 0x30335258u && src_format != 0x30335241u) { | |
| return -ENOTSUP; | |
| } | |
| pthread_once(&g_hdr_once, hdr_lut_init); | |
| for (uint32_t y = 0; y < height; y++) { | |
| const uint32_t *s = (const uint32_t *) | |
| ((const uint8_t *)src + (size_t)y * src_stride); | |
| uint32_t *d = (uint32_t *)((uint8_t *)dst + (size_t)y * dst_stride); | |
| int drmtap_tonemap_hdr10(const void *src, void *dst, | |
| uint32_t width, uint32_t height, | |
| uint32_t src_stride, uint32_t dst_stride, | |
| uint32_t src_format) { | |
| if (!src || !dst || width == 0 || height == 0) { | |
| return -EINVAL; | |
| } | |
| /* XR30 = XRGB2101010 ('XR30'), AR30 = ARGB2101010 ('AR30'). */ | |
| if (src_format != 0x30335258u && src_format != 0x30335241u) { | |
| return -ENOTSUP; | |
| } | |
| size_t row_bytes = (size_t)width * sizeof(uint32_t); | |
| if (row_bytes > UINT32_MAX || src_stride < row_bytes || dst_stride < row_bytes) { | |
| return -EINVAL; | |
| } | |
| pthread_once(&g_hdr_once, hdr_lut_init); | |
| for (uint32_t y = 0; y < height; y++) { | |
| const uint32_t *s = (const uint32_t *) | |
| ((const uint8_t *)src + (size_t)y * src_stride); | |
| uint32_t *d = (uint32_t *)((uint8_t *)dst + (size_t)y * dst_stride); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pixel_convert.c` around lines 424 - 441, The drmtap_tonemap_hdr10 routine
currently only validates pointers, dimensions, and src_format, but it still
assumes each row is wide enough for width 32-bit pixels. Add stride validation
in drmtap_tonemap_hdr10 before the loop so src_stride and dst_stride must be at
least width * sizeof(uint32_t), and return -EINVAL when either row stride is too
small to hold a full row; use the existing drmtap_tonemap_hdr10 symbol and the
row access through s[x]/d[x] as the place to enforce this check.
Review feedback. drmtap_tonemap_hdr10() now rejects src/dst strides that cannot hold a full width*4 row (-EINVAL), matching the guards the other conversions use, so a bogus caller stride can't over-read/over-write. Added a test case for it. Renamed the per-pixel linear channel temporaries to snake_case (r_lin/g_lin/b_lin).
|
Addressed all three:
meson test 6/6, asan clean, cppcheck clean. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/rust/libdrmtap-sys/csrc/pixel_convert.c`:
- Around line 437-439: The row size calculation in pixel_convert.c has the same
wrapped-`row_bytes` risk as the pre-multiply path in src/pixel_convert.c, so
keep the guards in sync by validating `width` before computing `row_bytes` in
the conversion logic around the stride checks. Update the relevant
pre-multiply/row-bytes guard near the existing `row_bytes`, `src_stride`, and
`dst_stride` validation so the overflow protection happens before the
multiplication and matches the behavior of the source implementation.
In `@src/pixel_convert.c`:
- Around line 437-439: The stride validation in the pixel conversion path is
checking for overflow only after computing row_bytes, which can wrap first on
narrower targets. Update the width handling in the row size calculation near the
row_bytes check so the overflow is validated before multiplying, and then use
that safely computed value in the existing src_stride and dst_stride checks
inside the pixel conversion logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c169c82d-d326-41da-a703-167a5380f550
📒 Files selected for processing (3)
bindings/rust/libdrmtap-sys/csrc/pixel_convert.csrc/pixel_convert.ctests/test_hdr.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Use 4-space indentation, never tabs, and 1TBS brace style in C source files.
Name C functions and variables in snake_case, with public API functions prefixeddrmtap_.
Name macros and constants in UPPER_SNAKE_CASE, prefixedDRMTAP_when project-specific.
Usesnake_case_tfor internal types anddrmtap_*for public types.
Return negative errno values for errors and never abort.
Every.cfile must start with the standard libdrmtap copyright/license header block, followed by a Doxygen@filecomment.
Organize C source files as includes first, then private types/constants, then static functions, then public functions in header-declaration order.
Use simple//comments for internal static functions instead of full Doxygen blocks.
Files:
src/pixel_convert.c
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Keep lines to a 100-character soft limit and 120-character hard limit.
Use//for single-line comments and/* */for multi-line comments.
Always checkmallocreturns and free resources in cleanup paths.
Files:
src/pixel_convert.ctests/test_hdr.cbindings/rust/libdrmtap-sys/csrc/pixel_convert.c
tests/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.c: Write tests using simpleassert-based validation without an external test framework.
UseDRM_DEVICEfrom the environment in integration tests instead of hardcoding/dev/dri/card0.
Files:
tests/test_hdr.c
🔇 Additional comments (3)
src/pixel_convert.c (1)
406-413: LGTM!bindings/rust/libdrmtap-sys/csrc/pixel_convert.c (1)
406-413: LGTM!tests/test_hdr.c (1)
92-96: LGTM!
| size_t row_bytes = (size_t)width * 4u; | ||
| if (row_bytes > UINT32_MAX || src_stride < row_bytes || | ||
| dst_stride < row_bytes) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mirror the pre-multiply overflow guard here too.
This Rust sys copy has the same wrapped-row_bytes risk on 32-bit size_t; keep it in lockstep with src/pixel_convert.c by checking width before multiplying.
Proposed fix
- size_t row_bytes = (size_t)width * 4u;
- if (row_bytes > UINT32_MAX || src_stride < row_bytes ||
- dst_stride < row_bytes) {
+ if (width > UINT32_MAX / sizeof(uint32_t)) {
+ return -EINVAL;
+ }
+ size_t row_bytes = (size_t)width * sizeof(uint32_t);
+ if (src_stride < row_bytes || dst_stride < row_bytes) {
return -EINVAL;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t row_bytes = (size_t)width * 4u; | |
| if (row_bytes > UINT32_MAX || src_stride < row_bytes || | |
| dst_stride < row_bytes) { | |
| if (width > UINT32_MAX / sizeof(uint32_t)) { | |
| return -EINVAL; | |
| } | |
| size_t row_bytes = (size_t)width * sizeof(uint32_t); | |
| if (src_stride < row_bytes || dst_stride < row_bytes) { | |
| return -EINVAL; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/rust/libdrmtap-sys/csrc/pixel_convert.c` around lines 437 - 439, The
row size calculation in pixel_convert.c has the same wrapped-`row_bytes` risk as
the pre-multiply path in src/pixel_convert.c, so keep the guards in sync by
validating `width` before computing `row_bytes` in the conversion logic around
the stride checks. Update the relevant pre-multiply/row-bytes guard near the
existing `row_bytes`, `src_stride`, and `dst_stride` validation so the overflow
protection happens before the multiplication and matches the behavior of the
source implementation.
| size_t row_bytes = (size_t)width * 4u; | ||
| if (row_bytes > UINT32_MAX || src_stride < row_bytes || | ||
| dst_stride < row_bytes) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Check width overflow before multiplying.
row_bytes > UINT32_MAX only catches overflow after the multiply if size_t is wider than uint32_t; on 32-bit targets the (size_t)width * 4u expression can wrap first and let an invalid stride through.
Proposed fix
- size_t row_bytes = (size_t)width * 4u;
- if (row_bytes > UINT32_MAX || src_stride < row_bytes ||
- dst_stride < row_bytes) {
+ if (width > UINT32_MAX / sizeof(uint32_t)) {
+ return -EINVAL;
+ }
+ size_t row_bytes = (size_t)width * sizeof(uint32_t);
+ if (src_stride < row_bytes || dst_stride < row_bytes) {
return -EINVAL;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t row_bytes = (size_t)width * 4u; | |
| if (row_bytes > UINT32_MAX || src_stride < row_bytes || | |
| dst_stride < row_bytes) { | |
| if (width > UINT32_MAX / sizeof(uint32_t)) { | |
| return -EINVAL; | |
| } | |
| size_t row_bytes = (size_t)width * sizeof(uint32_t); | |
| if (src_stride < row_bytes || dst_stride < row_bytes) { | |
| return -EINVAL; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pixel_convert.c` around lines 437 - 439, The stride validation in the
pixel conversion path is checking for overflow only after computing row_bytes,
which can wrap first on narrower targets. Update the width handling in the row
size calculation near the row_bytes check so the overflow is validated before
multiplying, and then use that safely computed value in the existing src_stride
and dst_stride checks inside the pixel conversion logic.
|
Note: the stride guard re-flagged here is already in place (src/pixel_convert.c lines 435-439, and the csrc mirror) — it's the exact check from the previous round, with a test case. The bot just re-anchored the thread. Merging. |
1 participant
First piece of #16 (does not close it). Adds the correct HDR10->SDR conversion primitive and a hardware-free test for it.
Problem
drmtap_convert_format()'s AR30/XR30 path is a naive>>2bit-shift. That is correct for SDR 10-bit, but for HDR10 it just keeps the top 8 of the 10 PQ-encoded bits — no PQ decode, no BT.2020 gamut, no tone-map — so an HDR scanout captures washed out (a 1000-nit highlight came out ~53% gray).This change
New public
drmtap_tonemap_hdr10():PQ and sRGB are table-driven (built once via
pthread_once); only the 3x3 matrix and the tone curve run per pixel. libm is wired into meson and the cratebuild.rs.Test (no HDR hardware)
test_hdr.cPQ-encodes known luminances and checks the 8-bit curve:Monotonic, dark stays dark, SDR white maps bright (250) instead of crushed, highlights reach white.
Scope / what's left on #16
This is the conversion primitive only. Still open on #16:
Colorspace/HDR_OUTPUT_METADATA— the pixel format alone doesn't say (XR30/AR30 is used for SDR 10-bit too), so applying this unconditionally would wrongly tone-map SDR. Needs helper-side metadata reading + new wire fields.gpu_auto_process, and make the highlight roll-off peak-aware using the metadata peak luminance.glReadPixels) for tiled HDR scanouts.Validation
release+werror, debug+asan+ubsan, buildtype=plain+werror, meson test 6/6 (incl. new hdr), cppcheck clean, cargo build/test clean (sys + wrapper, clean rebuild confirms the libm link).
Part of #16