refactor Image::convert() - #1324
Conversation
Co-authored-by: Victor Sohier <42362224+VictorSohier@users.noreply.github.com>
WalkthroughAdds generic slice-backed I/O, block-based image format readers and writers, and routes optimized ChangesImage I/O Conversion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ImageConvert
participant IOImageReader
participant IOImageWriter
participant SourceSlice
participant DestinationSlice
ImageConvert->>SourceSlice: create source reader
ImageConvert->>DestinationSlice: create destination writer
ImageConvert->>IOImageReader: decode source 4x4 block
IOImageReader-->>ImageConvert: return ColorRGBAF32x16
ImageConvert->>IOImageWriter: encode target 4x4 block
IOImageWriter->>DestinationSlice: write encoded bytes
ImageConvert->>IOImageWriter: flush mipmap output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 20
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (2)
core/io/io.h-64-71 (1)
64-71: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
destroydereferencesvtblwithout checking it, so destroying a never-successfully-made handle crashes.Both helpers assume
vtbl != nullptr. Handles are conventionally zero-initialized (IO::Reader reader = {};incore/io/image.cppLine 540) andmakeleavesvtbluntouched when allocation fails — andcore/io/image.cppLine 549 discardsmake's result and unconditionally callsdestroyat Line 597. Guard the pointer:🛡️ Proposed fix
static void destroy(Reader* reader) noexcept { - if (reader->vtbl->destroy) + assert(reader); + if (reader->vtbl && reader->vtbl->destroy) { reader->vtbl->destroy(reader->state); } *reader = {}; }Same for
Writer::destroy. The equivalent helpers incore/io/image_rw.h(Lines 67-74 and 119-126) have the identical hole.Also applies to: 118-125
🤖 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 `@core/io/io.h` around lines 64 - 71, Guard vtbl before dereferencing it in both Reader::destroy and Writer::destroy, while still resetting the handle with *reader = {} or *writer = {}. Apply the same null-check to the equivalent destroy helpers in image_rw.h so zero-initialized or failed handles can be safely destroyed.core/io/io.h-37-42 (1)
37-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace
ssize_twithptrdiff_tfor seek offsets.
ssize_tis POSIX and not guaranteed by the headers included here, so MSVC builds can fail unless an additional compatibility definition is added. Useptrdiff_tinSeekProc,Reader::seek,Writer::seek, andsliceSeekso the API has the same signed offset type everywhere.🛡️ Proposed fix
using SeekProc = Error (*)( void *state, - ssize_t offset, + ptrdiff_t offset, size_t *where, Whence whence ) noexcept;The same substitution is needed in
Reader::seek,Writer::seek, andsliceSeek.🤖 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 `@core/io/io.h` around lines 37 - 42, Replace ssize_t with ptrdiff_t throughout the seek API, including SeekProc and the Reader::seek, Writer::seek, and sliceSeek declarations or definitions. Keep the offset signed and consistent across all related interfaces.
🧹 Nitpick comments (8)
core/io/io.cpp (1)
132-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the slice callbacks internal linkage and make the vtables
const.
sliceRead,sliceWrite,sliceSeek,sliceClose,sliceSize,sliceFlush, andsliceDestroyall have external linkage with very generic names in the global namespace — an ODR/symbol-collision risk in a codebase this size. The two vtables are never mutated either, andWriter::vtblis alreadyconst VTbl *.♻️ Proposed refactor
-static Reader::VTbl SLICE_READER_VTABLE = { +static const Reader::VTbl SLICE_READER_VTABLE = { .read = (ReadProc)sliceRead,Mark each
slice*functionstatic(or wrap the whole set in an anonymous namespace).🤖 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 `@core/io/io.cpp` around lines 132 - 146, Give sliceRead, sliceWrite, sliceSeek, sliceClose, sliceSize, sliceFlush, and sliceDestroy internal linkage by marking them static or placing them in an anonymous namespace. Declare SLICE_READER_VTABLE and SLICE_WRITER_VTABLE as const, preserving their existing callback assignments.core/templates/Slice.h (1)
70-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
subslice_texpands to an unqualifiedsubslicecall.Unlike
sliceAt, this macro omits theSlice::qualification, so it only compiles insideSlice's own member scope (no ADL applies to static members). Qualify it for consistency, and consider parenthesizing thesliceargument insliceAt/subslice_tassliceCountalready does.♻️ Proposed refactor
-#define subslice_t(dst, src, t, begin, count) subslice( \ +#define subslice_t(dst, src, t, begin, count) Slice::subslice( \ (dst), \ (src), \ sizeof(t) * (begin), \ sizeof(t) * (count) \ )🤖 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 `@core/templates/Slice.h` around lines 70 - 77, Update the subslice_t macro to call Slice::subslice explicitly, matching sliceAt and ensuring it works outside Slice member scope. Also parenthesize macro arguments consistently in sliceAt and subslice_t, as already done by sliceCount.core/io/image_rw.h (2)
14-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNamespace and alias names collide with existing ones.
namespace IO::Imageshadows the globalclass Imagefor every lookup insideIO, which is why::Image::Formathas to be spelled out everywhere inimage_rw.cpp. LikewiseIO::Image::ReadProc/WriteProcreuse the names ofIO::ReadProc/IO::WriteProcwith different signatures, so a TU with bothusing namespace IO;andusing namespace IO::Image;becomes ambiguous — and a wrong-but-compiling cast is easy to write. ConsiderIO::ImageIO(orIO::Img) andBlockReadProc/BlockWriteProc.🤖 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 `@core/io/image_rw.h` around lines 14 - 26, Rename the IO::Image namespace to a non-conflicting name such as IO::ImageIO, and rename its ReadProc and WriteProc aliases to BlockReadProc and BlockWriteProc. Update all references, including image_rw.cpp and any public declarations, while preserving the existing callback signatures and FlushProc behavior.
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reader::vtblis non-const whileWriter::vtblisconst.Make
Reader::vtblaconst VTbl *to matchWriter(andIO::Reader); the vtables inimage_rw.cppare never mutated.Also applies to: 84-86
🤖 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 `@core/io/image_rw.h` around lines 34 - 36, Update the Reader struct’s vtbl member to use const VTbl * instead of a mutable VTbl pointer, matching Writer::vtbl and IO::Reader::vtbl. Preserve the existing vtable assignments and access behavior in the image I/O implementation.core/io/io.h (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead declarations and an unused heavy include.
Nothing in this header uses
core/io/file_access.h; it is only there for the commented-outRef<FileAccess>overloads. Either implement those overloads or drop both the comments and the include to keep the dependency surface (and compile times) down.Also applies to: 63-63
🤖 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 `@core/io/io.h` around lines 3 - 4, Remove the unused core/io/file_access.h include and the commented-out Ref<FileAccess> overload declarations from io.h; retain the active Slice-related declarations and avoid implementing the obsolete overloads.core/templates/Slice.cpp (1)
23-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnaligned widened loads/stores through reinterpreted pointers are UB and trap on strict-alignment targets.
*((uint64_t*)&u8dst[i]) = *((const uint64_t*)&u8src[i])at an arbitrary byte offset violates both alignment requirements and strict aliasing. It happens to work on x86-64 but is a real hazard on ARM/RISC-V builds and will be flagged by UBSan.memcpyof a fixed size compiles to the same instruction on every mainstream compiler and is well-defined:memcpy(&u8dst[i], &u8src[i], sizeof(uint64_t));More broadly, since
Slice::copyalready rejectssrc.length > dst.lengthand the overlap heuristic distinguishes the two directions,memmove/memcpywould cover this entire function with far less risk than the hand-unrolled switch ladders.🤖 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 `@core/templates/Slice.cpp` around lines 23 - 45, Replace the unaligned, reinterpreted-pointer loads and stores in Slice::copy’s switch branches with well-defined byte-copy operations, using fixed-size memcpy for each widened transfer and preserving the existing overlap-direction behavior. Prefer simplifying the entire copy path to memmove or appropriately selected memcpy now that length validation and overlap handling already cover the operation; remove the unsafe pointer casts while preserving all copy-size cases.core/io/image.cpp (1)
544-544: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider zero-initializing
blockas defense-in-depth.
ColorRGBAF32x16 block;is left uninitialized; combined with the missing-alpha-init bug inrgba8ReadScalar/rgbafReadScalar<T>(flagged inimage_rw.cpp), stack garbage can propagate into the converted alpha channel. Zero-initializing here (ColorRGBAF32x16 block = {};) is a cheap mitigation regardless of the root-cause fix.🤖 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 `@core/io/image.cpp` at line 544, Zero-initialize the local ColorRGBAF32x16 block in the surrounding image conversion code by using value initialization, ensuring any unassigned channels start at zero while preserving the existing processing flow.core/io/image_rw.cpp (1)
696-946: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSSE4.2/AVX/AVX2 vtables are byte-for-byte identical to the scalar table.
No vectorized implementations exist yet — all four CPU-feature tiers point at the same
*Scalarfunctions, so the__builtin_cpu_supportsdispatch inReader::make/Writer::makecurrently has no effect besides ~4x table duplication. Fine as scaffolding for future SIMD work, but consider a// TODOor aliasing the tables until real variants exist.🤖 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 `@core/io/image_rw.cpp` around lines 696 - 946, The SSE42, AVX, and AVX2 vtables duplicate the scalar implementations without providing CPU-specific behavior. Update Reader::make and Writer::make dispatch support by aliasing these feature tiers to the scalar tables or otherwise removing the redundant table definitions, while preserving the existing scalar fallback and adding a concise TODO if the tables remain as future SIMD scaffolding.
🤖 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 `@core/io/image_rw.cpp`:
- Around line 390-425: Update the destination offset calculation in
rgba8WriteScalar so the block, row, and column pixel position is computed first,
then multiplied by COMPONENT_COUNT[format] before adding c. Preserve the
existing bounds check, color indexing, block advancement, and write behavior.
- Around line 427-478: Update rgbafReadScalar to apply the same block/row offset
correction used by rgba8ReadScalar, ensuring component indexing scales offsets
by COMPONENT_COUNT[format] consistently. Initialize colors->a for formats with
one to three components so RF/RGF/RGBF conversions produce a defined alpha
value, while preserving the existing four-component alpha data.
- Around line 480-516: Restructure rgbafWriteScalar so the block’s horizontal
offset is applied consistently when checking bounds and computing destinations,
matching the corrected read-side block/row indexing. Preserve the existing
component and 4×4 pixel traversal, state->currentBlock advancement, and writer
flush behavior.
- Around line 518-570: The rgba4444ReadScalar function uses a stale block value
after refilling data and computes the row/block offset without accounting for
RGBA4444’s 2-byte-per-pixel layout. Refresh block after a successful
reader.read, and scale both block<<2 and width*i by
CONSTANT_FACTORS[FORMAT_RGBA4444] while preserving the existing nibble/component
offset calculation.
- Around line 572-610: Update defaultFlush to multiply each row offset by
CONSTANT_FACTORS[state->format] in both the destination seek calculation and the
source Slice::subslice offset. Preserve the existing currentBlock length
calculation and error handling while ensuring row strides use byte offsets for
every image format.
- Around line 85-142: Move the local block-index capture in l8ReadScalar until
after the refill logic so it reflects the reset state->currentBlock value before
pixel addressing. Apply the same ordering fix to la8ReadScalar, rgba8ReadScalar,
rgbafReadScalar<T>, and rgba4444ReadScalar, preserving their existing refill and
indexing behavior.
- Around line 144-179: Update the luminance calculations in both l8WriteScalar
and la8WriteScalar to apply the Rec.709 weights to the normalized float color
components before converting to the 8-bit output value; remove the
pre-multiplication uint64_t casts that truncate values below 1. Preserve the
existing rounding and output-buffer behavior.
- Around line 181-292: Fix la8ReadScalar and la8WriteScalar by recomputing block
after any read-side refill/reset so the current block index is not stale. Scale
the complete LA8 pixel position by two when indexing data, including block and
row offsets, while keeping the alpha byte at the following offset. In
la8WriteScalar, promote each color channel to the wider integer type before
multiplying by the luminance coefficients to prevent truncation.
- Around line 336-388: Fix pixel addressing in rgba8ReadScalar and the
corresponding scalar read/write helpers rgba8WriteScalar, rgbafReadScalar,
rgbafWriteScalar, and rgba4444ReadScalar by scaling the complete pixel position
(block and row offsets, plus j) by COMPONENT_COUNT[format] before adding the
component index. Also initialize colors->a to the opaque value for formats with
fewer than four components, including R8, RG8, and RGB8, while preserving source
alpha for four-component formats.
- Around line 57-83: Initialize currentBlock explicitly in
constantFactorStateCtor after successful allocation, using the state expected by
the scalar read/write paths; do not leave it dependent on malloc contents.
Ensure the initialization supports the required first-call behavior, including
forcing readers to perform their initial load rather than skipping it, and
preserve the existing dimension, format, and buffer setup.
In `@core/io/image.cpp`:
- Around line 556-604: Update both `make()` failure branches in the
mip-processing loop to clean up all handles already created for the current mip
and return immediately instead of breaking to `_copy_internals_from(new_img)`.
Ensure the `IO::Reader`, `IO::Writer`, and image reader/writer handles are
destroyed appropriately, including `imReader` in the second failure path, so
failed mip creation cannot commit the partially initialized image.
- Around line 549-585: Capture the return value of IO::Image::Writer::make in
the conversion setup before the loop, and handle non-Okay results like the
existing reader and writer initialization failures. Destroy any already
initialized imReader, reader, and writer handles before returning the error,
preventing IO::Image::Writer::write from using an invalid writer.
In `@core/io/io.cpp`:
- Around line 148-184: Initialize state->closed to false in both Reader::make
and Writer::make immediately after allocating the SliceState, alongside buffer
and offset initialization. Add the direct <cstdlib> include in this translation
unit for the malloc/free usage.
- Around line 84-96: Update the seek offset validation in the visible seek
implementation to reject negative computed positions explicitly while allowing a
position equal to state->buffer.length; retain InvalidOffset for positions
beyond the buffer. Use a signed intermediate for newOffsets[whence] + offset
before converting to size_t, and reuse the validated position when assigning
state->offset and *where.
- Around line 51-63: Update the partial-write logic in the surrounding I/O
method so the source slice passed to Slice::copy has its length truncated to
count, while preserving the destination buffer and offset advancement semantics.
Ensure Slice::copy receives compatible slice lengths, handle its return value
rather than discarding it, and only report count through written after the
requested bytes were actually copied.
In `@core/templates/Slice.cpp`:
- Around line 150-217: Fix Slice::set so every byte in dst is written exactly
once without overrunning the slice. Replace the current Duff’s-device logic with
a bounds-safe memset-equivalent implementation, or otherwise use a single cursor
update strategy; call get with its declared index/size argument order and ensure
each widened store fits within dst.length.
- Around line 6-20: Update Slice::copy so both copy loops use the bounded length
n rather than dst.length when indexing or decrementing, preventing reads beyond
src when dst is larger. Preserve the existing rejection for src.length greater
than dst.length and leave the unused destination tail unchanged for short
sources, including the sliceRead(buffer, tmp) path.
In `@core/templates/Slice.h`:
- Around line 63-68: Fix the success-condition handling in Slice::set: align the
subslice(&tmp, dst, index, src.length) check with subslice’s actual return
polarity so successful creation proceeds to Slice::copy and failure returns
false. If subslice is changed to return true on success, preserve this body;
otherwise invert the condition in Slice::set.
- Around line 32-44: Update Slice::get to validate that the entire element range
fits within self.length, not only that size * index is below it. Compute the
offset with overflow-safe checks before forming the pointer, and return nullptr
when multiplication or offset-plus-size would overflow or exceed the slice
bounds.
- Around line 46-61: Update Slice::subslice to validate ranges without unsigned
underflow by rejecting count values larger than src.length and ensuring begin
plus count stays within the source; return true on successful subslice creation
and false on invalid ranges. Adjust all callers in Slice.h, io.cpp, and
image_rw.cpp to use the corrected success-means-true contract, preserving
existing failure handling.
---
Major comments:
In `@core/io/io.h`:
- Around line 64-71: Guard vtbl before dereferencing it in both Reader::destroy
and Writer::destroy, while still resetting the handle with *reader = {} or
*writer = {}. Apply the same null-check to the equivalent destroy helpers in
image_rw.h so zero-initialized or failed handles can be safely destroyed.
- Around line 37-42: Replace ssize_t with ptrdiff_t throughout the seek API,
including SeekProc and the Reader::seek, Writer::seek, and sliceSeek
declarations or definitions. Keep the offset signed and consistent across all
related interfaces.
---
Nitpick comments:
In `@core/io/image_rw.cpp`:
- Around line 696-946: The SSE42, AVX, and AVX2 vtables duplicate the scalar
implementations without providing CPU-specific behavior. Update Reader::make and
Writer::make dispatch support by aliasing these feature tiers to the scalar
tables or otherwise removing the redundant table definitions, while preserving
the existing scalar fallback and adding a concise TODO if the tables remain as
future SIMD scaffolding.
In `@core/io/image_rw.h`:
- Around line 14-26: Rename the IO::Image namespace to a non-conflicting name
such as IO::ImageIO, and rename its ReadProc and WriteProc aliases to
BlockReadProc and BlockWriteProc. Update all references, including image_rw.cpp
and any public declarations, while preserving the existing callback signatures
and FlushProc behavior.
- Around line 34-36: Update the Reader struct’s vtbl member to use const VTbl *
instead of a mutable VTbl pointer, matching Writer::vtbl and IO::Reader::vtbl.
Preserve the existing vtable assignments and access behavior in the image I/O
implementation.
In `@core/io/image.cpp`:
- Line 544: Zero-initialize the local ColorRGBAF32x16 block in the surrounding
image conversion code by using value initialization, ensuring any unassigned
channels start at zero while preserving the existing processing flow.
In `@core/io/io.cpp`:
- Around line 132-146: Give sliceRead, sliceWrite, sliceSeek, sliceClose,
sliceSize, sliceFlush, and sliceDestroy internal linkage by marking them static
or placing them in an anonymous namespace. Declare SLICE_READER_VTABLE and
SLICE_WRITER_VTABLE as const, preserving their existing callback assignments.
In `@core/io/io.h`:
- Around line 3-4: Remove the unused core/io/file_access.h include and the
commented-out Ref<FileAccess> overload declarations from io.h; retain the active
Slice-related declarations and avoid implementing the obsolete overloads.
In `@core/templates/Slice.cpp`:
- Around line 23-45: Replace the unaligned, reinterpreted-pointer loads and
stores in Slice::copy’s switch branches with well-defined byte-copy operations,
using fixed-size memcpy for each widened transfer and preserving the existing
overlap-direction behavior. Prefer simplifying the entire copy path to memmove
or appropriately selected memcpy now that length validation and overlap handling
already cover the operation; remove the unsafe pointer casts while preserving
all copy-size cases.
In `@core/templates/Slice.h`:
- Around line 70-77: Update the subslice_t macro to call Slice::subslice
explicitly, matching sliceAt and ensuring it works outside Slice member scope.
Also parenthesize macro arguments consistently in sliceAt and subslice_t, as
already done by sliceCount.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 16ba104d-8175-4fb3-98b6-55ed8a64e5e5
📒 Files selected for processing (8)
core/io/image.cppcore/io/image.hcore/io/image_rw.cppcore/io/image_rw.hcore/io/io.cppcore/io/io.hcore/templates/Slice.cppcore/templates/Slice.h
| static IO::Error constantFactorStateCtor( | ||
| UncompressedImageState **state, | ||
| uint32_t width, | ||
| uint32_t height, | ||
| ::Image::Format format | ||
| ) noexcept | ||
| { | ||
| IO::Error ret = IO::Error::Okay; | ||
| // ensure we have enough 4x4 blocks to cover the width of the image | ||
| *state = (UncompressedImageState*)malloc( | ||
| sizeof(UncompressedImageState) + | ||
| ((((width << 2) * CONSTANT_FACTORS[format]) + ((size_t)0xFF)) & ~((size_t)0xFF)) | ||
| ); | ||
| if (*state) | ||
| { | ||
| (*state)->data.data = (*state)->blocks4x4; | ||
| (*state)->data.length = (width << 2) * CONSTANT_FACTORS[format]; | ||
| (*state)->res[0] = width; | ||
| (*state)->res[1] = height; | ||
| (*state)->format = format; | ||
| } | ||
| else | ||
| { | ||
| ret = IO::Error::OutOfMemory; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
currentBlock left uninitialized after malloc — undefined behavior on first use.
malloc does not zero memory, and this constructor never sets (*state)->currentBlock. Every *ReadScalar/*WriteScalar function branches on state->currentBlock on its very first call (if (state->currentBlock >= ((width+3)>>2))), so the first read/write of every mip decides whether to load/flush based on garbage stack/heap contents — this is undefined behavior and can either skip the required initial load (reading garbage pixel data) or trigger a premature flush on the writer side.
🐛 Minimal fix: explicitly initialize the field
if (*state)
{
(*state)->data.data = (*state)->blocks4x4;
(*state)->data.length = (width << 2) * CONSTANT_FACTORS[format];
(*state)->res[0] = width;
(*state)->res[1] = height;
(*state)->format = format;
+ (*state)->currentBlock = 0;
}Note: readers additionally need to force an initial load on the very first call (currently, starting at 0 skips the first reader.read() entirely since 0 < threshold — see the read-side comment below for why a 0-only fix isn't sufficient there).
📝 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.
| static IO::Error constantFactorStateCtor( | |
| UncompressedImageState **state, | |
| uint32_t width, | |
| uint32_t height, | |
| ::Image::Format format | |
| ) noexcept | |
| { | |
| IO::Error ret = IO::Error::Okay; | |
| // ensure we have enough 4x4 blocks to cover the width of the image | |
| *state = (UncompressedImageState*)malloc( | |
| sizeof(UncompressedImageState) + | |
| ((((width << 2) * CONSTANT_FACTORS[format]) + ((size_t)0xFF)) & ~((size_t)0xFF)) | |
| ); | |
| if (*state) | |
| { | |
| (*state)->data.data = (*state)->blocks4x4; | |
| (*state)->data.length = (width << 2) * CONSTANT_FACTORS[format]; | |
| (*state)->res[0] = width; | |
| (*state)->res[1] = height; | |
| (*state)->format = format; | |
| } | |
| else | |
| { | |
| ret = IO::Error::OutOfMemory; | |
| } | |
| return ret; | |
| } | |
| static IO::Error constantFactorStateCtor( | |
| UncompressedImageState **state, | |
| uint32_t width, | |
| uint32_t height, | |
| ::Image::Format format | |
| ) noexcept | |
| { | |
| IO::Error ret = IO::Error::Okay; | |
| // ensure we have enough 4x4 blocks to cover the width of the image | |
| *state = (UncompressedImageState*)malloc( | |
| sizeof(UncompressedImageState) + | |
| ((((width << 2) * CONSTANT_FACTORS[format]) + ((size_t)0xFF)) & ~((size_t)0xFF)) | |
| ); | |
| if (*state) | |
| { | |
| (*state)->data.data = (*state)->blocks4x4; | |
| (*state)->data.length = (width << 2) * CONSTANT_FACTORS[format]; | |
| (*state)->res[0] = width; | |
| (*state)->res[1] = height; | |
| (*state)->format = format; | |
| (*state)->currentBlock = 0; | |
| } | |
| else | |
| { | |
| ret = IO::Error::OutOfMemory; | |
| } | |
| return ret; | |
| } |
🤖 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 `@core/io/image_rw.cpp` around lines 57 - 83, Initialize currentBlock
explicitly in constantFactorStateCtor after successful allocation, using the
state expected by the scalar read/write paths; do not leave it dependent on
malloc contents. Ensure the initialization supports the required first-call
behavior, including forcing readers to perform their initial load rather than
skipping it, and preserve the existing dimension, format, and buffer setup.
| static IO::Error l8ReadScalar( | ||
| IO::Reader reader, | ||
| UncompressedImageState *state, | ||
| ColorRGBAF32x16 *colors | ||
| ) noexcept | ||
| { | ||
| IO::Error ret = IO::Error::Okay; | ||
| Slice data = state->data; | ||
| size_t read; | ||
| uint32_t width = state->res[0]; | ||
| size_t block = state->currentBlock; | ||
| if (state->currentBlock >= ((width + 3) >> 2)) | ||
| { | ||
| if (!data.data) | ||
| { | ||
| return IO::Error::OutOfMemory; | ||
| } | ||
| ret = reader.read(reader, &read, data); | ||
| if (ret == IO::Error::Okay) | ||
| { | ||
| state->currentBlock = 0; | ||
| } | ||
| } | ||
| if (ret == IO::Error::Okay) | ||
| { | ||
| for (size_t c = 0; c < 3; c += 1) | ||
| { | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| for (size_t j = 0; j < 4; j += 1) | ||
| { | ||
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | ||
| { | ||
| colors->c[c][(i << 2) + j] = (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + j) | ||
| )); | ||
| } | ||
| else | ||
| { | ||
| colors->c[c][(i << 2) + j] = 0; | ||
| } | ||
| colors->c[c][(i << 2) + j] /= 255; | ||
| } | ||
| } | ||
| } | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| colors->a[(i << 2) + 0] = 1; | ||
| colors->a[(i << 2) + 1] = 1; | ||
| colors->a[(i << 2) + 2] = 1; | ||
| colors->a[(i << 2) + 3] = 1; | ||
| } | ||
| state->currentBlock += 1; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Stale block index used after buffer refill — corrupts the first tile of every 4-row band.
block is captured from state->currentBlock (line 95) before the refill check resets it to 0 (line 105). After a refill, addressing below still uses the pre-reset (too-large) block value, so the boundary check (((block<<2)+(width*i))+j) < width*(i+1) fails for the newly loaded band's first tile, and pixels are zeroed instead of read from the freshly-loaded buffer.
🐛 Proposed fix: read `block` after the potential reset
IO::Error ret = IO::Error::Okay;
Slice data = state->data;
size_t read;
uint32_t width = state->res[0];
- size_t block = state->currentBlock;
if (state->currentBlock >= ((width + 3) >> 2))
{
if (!data.data)
{
return IO::Error::OutOfMemory;
}
ret = reader.read(reader, &read, data);
if (ret == IO::Error::Okay)
{
state->currentBlock = 0;
}
}
+ size_t block = state->currentBlock;
if (ret == IO::Error::Okay)
{This exact pattern (capture before refill) repeats in la8ReadScalar, rgba8ReadScalar, rgbafReadScalar<T>, and rgba4444ReadScalar below.
📝 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.
| static IO::Error l8ReadScalar( | |
| IO::Reader reader, | |
| UncompressedImageState *state, | |
| ColorRGBAF32x16 *colors | |
| ) noexcept | |
| { | |
| IO::Error ret = IO::Error::Okay; | |
| Slice data = state->data; | |
| size_t read; | |
| uint32_t width = state->res[0]; | |
| size_t block = state->currentBlock; | |
| if (state->currentBlock >= ((width + 3) >> 2)) | |
| { | |
| if (!data.data) | |
| { | |
| return IO::Error::OutOfMemory; | |
| } | |
| ret = reader.read(reader, &read, data); | |
| if (ret == IO::Error::Okay) | |
| { | |
| state->currentBlock = 0; | |
| } | |
| } | |
| if (ret == IO::Error::Okay) | |
| { | |
| for (size_t c = 0; c < 3; c += 1) | |
| { | |
| for (size_t i = 0; i < 4; i += 1) | |
| { | |
| for (size_t j = 0; j < 4; j += 1) | |
| { | |
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | |
| { | |
| colors->c[c][(i << 2) + j] = (*sliceAt( | |
| data, | |
| uint8_t, | |
| (((block << 2) + (width * i)) + j) | |
| )); | |
| } | |
| else | |
| { | |
| colors->c[c][(i << 2) + j] = 0; | |
| } | |
| colors->c[c][(i << 2) + j] /= 255; | |
| } | |
| } | |
| } | |
| for (size_t i = 0; i < 4; i += 1) | |
| { | |
| colors->a[(i << 2) + 0] = 1; | |
| colors->a[(i << 2) + 1] = 1; | |
| colors->a[(i << 2) + 2] = 1; | |
| colors->a[(i << 2) + 3] = 1; | |
| } | |
| state->currentBlock += 1; | |
| } | |
| return ret; | |
| } | |
| static IO::Error l8ReadScalar( | |
| IO::Reader reader, | |
| UncompressedImageState *state, | |
| ColorRGBAF32x16 *colors | |
| ) noexcept | |
| { | |
| IO::Error ret = IO::Error::Okay; | |
| Slice data = state->data; | |
| size_t read; | |
| uint32_t width = state->res[0]; | |
| if (state->currentBlock >= ((width + 3) >> 2)) | |
| { | |
| if (!data.data) | |
| { | |
| return IO::Error::OutOfMemory; | |
| } | |
| ret = reader.read(reader, &read, data); | |
| if (ret == IO::Error::Okay) | |
| { | |
| state->currentBlock = 0; | |
| } | |
| } | |
| size_t block = state->currentBlock; | |
| if (ret == IO::Error::Okay) | |
| { | |
| for (size_t c = 0; c < 3; c += 1) | |
| { | |
| for (size_t i = 0; i < 4; i += 1) | |
| { | |
| for (size_t j = 0; j < 4; j += 1) | |
| { | |
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | |
| { | |
| colors->c[c][(i << 2) + j] = (*sliceAt( | |
| data, | |
| uint8_t, | |
| (((block << 2) + (width * i)) + j) | |
| )); | |
| } | |
| else | |
| { | |
| colors->c[c][(i << 2) + j] = 0; | |
| } | |
| colors->c[c][(i << 2) + j] /= 255; | |
| } | |
| } | |
| } | |
| for (size_t i = 0; i < 4; i += 1) | |
| { | |
| colors->a[(i << 2) + 0] = 1; | |
| colors->a[(i << 2) + 1] = 1; | |
| colors->a[(i << 2) + 2] = 1; | |
| colors->a[(i << 2) + 3] = 1; | |
| } | |
| state->currentBlock += 1; | |
| } | |
| return ret; | |
| } |
🤖 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 `@core/io/image_rw.cpp` around lines 85 - 142, Move the local block-index
capture in l8ReadScalar until after the refill logic so it reflects the reset
state->currentBlock value before pixel addressing. Apply the same ordering fix
to la8ReadScalar, rgba8ReadScalar, rgbafReadScalar<T>, and rgba4444ReadScalar,
preserving their existing refill and indexing behavior.
| static IO::Error l8WriteScalar( | ||
| IO::Writer writer, | ||
| UncompressedImageState *state, | ||
| ColorRGBAF32x16 *colors | ||
| ) noexcept | ||
| { | ||
| IO::Error ret = IO::Error::Okay; | ||
| Slice data = state->data; | ||
| uint32_t width = state->res[0]; | ||
| size_t block = state->currentBlock; | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| for (size_t j = 0; j < 4; j += 1) | ||
| { | ||
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | ||
| { | ||
| (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + j) | ||
| )) = (13938U * ((uint64_t)colors->r[(i << 2) + j]) + | ||
| 46869U * ((uint64_t)colors->g[(i << 2) + j]) + | ||
| 4729U * ((uint64_t)colors->b[(i << 2) + j]) + | ||
| 32768U | ||
| ) >> 16U; | ||
| } | ||
| } | ||
| } | ||
| state->currentBlock += 1; | ||
| if (state->currentBlock >= ((width + 3) >> 2)) | ||
| { | ||
| ret = writer.write(writer, nullptr, state->data); | ||
| state->currentBlock = 0; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Luminance formula casts normalized float to uint64_t before multiplying — truncates to (almost) zero.
colors->r/g/b are normalized to [0,1] by the corresponding read paths (e.g. l8ReadScalar divides by 255). Here, (uint64_t)colors->r[...] truncates any value < 1.0 straight to 0 before the Rec.709 weights are applied, so 13938*0 + 46869*0 + 4729*0 + 32768) >> 16 == 0 for virtually every pixel that isn't pure white. This produces a near-black output for L8 conversions.
🐛 Proposed fix: do the weighting in float, then convert
- (*sliceAt(
- data,
- uint8_t,
- (((block << 2) + (width * i)) + j)
- )) = (13938U * ((uint64_t)colors->r[(i << 2) + j]) +
- 46869U * ((uint64_t)colors->g[(i << 2) + j]) +
- 4729U * ((uint64_t)colors->b[(i << 2) + j]) +
- 32768U
- ) >> 16U;
+ (*sliceAt(
+ data,
+ uint8_t,
+ (((block << 2) + (width * i)) + j)
+ )) = (uint8_t)(
+ (0.212671f * colors->r[(i << 2) + j] +
+ 0.715160f * colors->g[(i << 2) + j] +
+ 0.072169f * colors->b[(i << 2) + j]) * 255.0f + 0.5f
+ );The identical bug recurs in la8WriteScalar below.
📝 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.
| static IO::Error l8WriteScalar( | |
| IO::Writer writer, | |
| UncompressedImageState *state, | |
| ColorRGBAF32x16 *colors | |
| ) noexcept | |
| { | |
| IO::Error ret = IO::Error::Okay; | |
| Slice data = state->data; | |
| uint32_t width = state->res[0]; | |
| size_t block = state->currentBlock; | |
| for (size_t i = 0; i < 4; i += 1) | |
| { | |
| for (size_t j = 0; j < 4; j += 1) | |
| { | |
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | |
| { | |
| (*sliceAt( | |
| data, | |
| uint8_t, | |
| (((block << 2) + (width * i)) + j) | |
| )) = (13938U * ((uint64_t)colors->r[(i << 2) + j]) + | |
| 46869U * ((uint64_t)colors->g[(i << 2) + j]) + | |
| 4729U * ((uint64_t)colors->b[(i << 2) + j]) + | |
| 32768U | |
| ) >> 16U; | |
| } | |
| } | |
| } | |
| state->currentBlock += 1; | |
| if (state->currentBlock >= ((width + 3) >> 2)) | |
| { | |
| ret = writer.write(writer, nullptr, state->data); | |
| state->currentBlock = 0; | |
| } | |
| return ret; | |
| } | |
| static IO::Error l8WriteScalar( | |
| IO::Writer writer, | |
| UncompressedImageState *state, | |
| ColorRGBAF32x16 *colors | |
| ) noexcept | |
| { | |
| IO::Error ret = IO::Error::Okay; | |
| Slice data = state->data; | |
| uint32_t width = state->res[0]; | |
| size_t block = state->currentBlock; | |
| for (size_t i = 0; i < 4; i += 1) | |
| { | |
| for (size_t j = 0; j < 4; j += 1) | |
| { | |
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | |
| { | |
| (*sliceAt( | |
| data, | |
| uint8_t, | |
| (((block << 2) + (width * i)) + j) | |
| )) = (uint8_t)( | |
| (0.212671f * colors->r[(i << 2) + j] + | |
| 0.715160f * colors->g[(i << 2) + j] + | |
| 0.072169f * colors->b[(i << 2) + j]) * 255.0f + 0.5f | |
| ); | |
| } | |
| } | |
| } | |
| state->currentBlock += 1; | |
| if (state->currentBlock >= ((width + 3) >> 2)) | |
| { | |
| ret = writer.write(writer, nullptr, state->data); | |
| state->currentBlock = 0; | |
| } | |
| return ret; | |
| } |
🤖 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 `@core/io/image_rw.cpp` around lines 144 - 179, Update the luminance
calculations in both l8WriteScalar and la8WriteScalar to apply the Rec.709
weights to the normalized float color components before converting to the 8-bit
output value; remove the pre-multiplication uint64_t casts that truncate values
below 1. Preserve the existing rounding and output-buffer behavior.
| static IO::Error la8ReadScalar( | ||
| IO::Reader reader, | ||
| UncompressedImageState *state, | ||
| ColorRGBAF32x16 *colors | ||
| ) noexcept | ||
| { | ||
| IO::Error ret = IO::Error::Okay; | ||
| Slice data = state->data; | ||
| size_t read; | ||
| uint32_t width = state->res[0]; | ||
| size_t block = state->currentBlock; | ||
| if (state->currentBlock >= ((width + 3) >> 2)) | ||
| { | ||
| if (!data.data) | ||
| { | ||
| return IO::Error::OutOfMemory; | ||
| } | ||
| ret = reader.read(reader, &read, data); | ||
| if (ret == IO::Error::Okay) | ||
| { | ||
| state->currentBlock = 0; | ||
| } | ||
| } | ||
| if (ret == IO::Error::Okay) | ||
| { | ||
| for (size_t c = 0; c < 3; c += 1) | ||
| { | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| for (size_t j = 0; j < 4; j += 1) | ||
| { | ||
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | ||
| { | ||
| colors->c[c][(i << 2) + j] = (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + (j << 1)) | ||
| )); | ||
| } | ||
| else | ||
| { | ||
| colors->c[c][(i << 2) + j] = 0; | ||
| } | ||
| colors->c[c][(i << 2) + j] /= 255; | ||
| } | ||
| } | ||
| } | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| for (size_t j = 0; j < 4; j += 1) | ||
| { | ||
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | ||
| { | ||
| colors->a[(i << 2) + j] = (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + ((j << 1) + 1)) | ||
| )); | ||
| } | ||
| else | ||
| { | ||
| colors->a[(i << 2) + j] = 0; | ||
| } | ||
| colors->a[(i << 2) + j] /= 255; | ||
| } | ||
| } | ||
| state->currentBlock += 1; | ||
| } | ||
| return ret; | ||
| } | ||
|
|
||
| static IO::Error la8WriteScalar( | ||
| IO::Writer writer, | ||
| UncompressedImageState *state, | ||
| ColorRGBAF32x16 *colors | ||
| ) noexcept | ||
| { | ||
| IO::Error ret = IO::Error::Okay; | ||
| Slice data = state->data; | ||
| uint32_t width = state->res[0]; | ||
| size_t block = state->currentBlock; | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| for (size_t j = 0; j < 4; j += 1) | ||
| { | ||
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | ||
| { | ||
| (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + (j << 1)) | ||
| )) = (13938U * ((uint64_t)colors->r[(i << 2) + j]) + | ||
| 46869U * ((uint64_t)colors->g[(i << 2) + j]) + | ||
| 4729U * ((uint64_t)colors->b[(i << 2) + j]) + | ||
| 32768U | ||
| ) >> 16U; | ||
| (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + (j << 1) + 1) | ||
| )) = (uint8_t)(colors->a[(i << 2) + j] * 255); | ||
| } | ||
| } | ||
| } | ||
| state->currentBlock += 1; | ||
| if (state->currentBlock >= ((width + 3) >> 2)) | ||
| { | ||
| ret = writer.write(writer, nullptr, state->data); | ||
| state->currentBlock = 0; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Same stale-block bug (read) + same truncation-before-multiply bug (write) + missing byte-scaling on the row/block offset.
- Read (181-250):
blockis captured before the refill/reset, same issue asl8ReadScalarabove. - Write luminance (272-276): same
(uint64_t)colors->r[...]truncation bug asl8WriteScalar. - Both read and write index the slice with
(((block<<2)+(width*i))+(j<<1))/+(j<<1)+1. LA8 is 2 bytes/pixel, so the block/row terms (block<<2,width*i) must also be scaled ×2 — only the intra-blockjterm is scaled here. E.g. width=8, rowi=1,block=0,j=0: correct L byte offset is16, this formula yields8.
🐛 Proposed fix for the offset (read side shown; write side is analogous)
- colors->c[c][(i << 2) + j] = (*sliceAt(
- data,
- uint8_t,
- (((block << 2) + (width * i)) + (j << 1))
- ));
+ colors->c[c][(i << 2) + j] = (*sliceAt(
+ data,
+ uint8_t,
+ ((((block << 2) + (width * i)) + j) << 1)
+ ));🤖 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 `@core/io/image_rw.cpp` around lines 181 - 292, Fix la8ReadScalar and
la8WriteScalar by recomputing block after any read-side refill/reset so the
current block index is not stale. Scale the complete LA8 pixel position by two
when indexing data, including block and row offsets, while keeping the alpha
byte at the following offset. In la8WriteScalar, promote each color channel to
the wider integer type before multiplying by the luminance coefficients to
prevent truncation.
| static IO::Error rgba8ReadScalar( | ||
| IO::Reader reader, | ||
| UncompressedImageState *state, | ||
| ColorRGBAF32x16 *colors | ||
| ) noexcept | ||
| { | ||
| IO::Error ret = IO::Error::Okay; | ||
| Slice data = state->data; | ||
| size_t read; | ||
| size_t block = state->currentBlock; | ||
| ::Image::Format format = state->format; | ||
| uint32_t width = state->res[0]; | ||
| if (state->currentBlock >= ((width + 3) >> 2)) | ||
| { | ||
| if (!data.data) | ||
| { | ||
| return IO::Error::OutOfMemory; | ||
| } | ||
| ret = reader.read(reader, &read, data); | ||
| if (ret == IO::Error::Okay) | ||
| { | ||
| state->currentBlock = 0; | ||
| } | ||
| } | ||
| if (ret == IO::Error::Okay) | ||
| { | ||
| for (size_t c = 0; c < COMPONENT_COUNT[format]; c += 1) | ||
| { | ||
| for (size_t i = 0; i < 4; i += 1) | ||
| { | ||
| for (size_t j = 0; j < 4; j += 1) | ||
| { | ||
| if ((((block << 2) + (width * i)) + j) < (width * (i + 1))) | ||
| { | ||
| colors->c[c][(i << 2) + j] = (*sliceAt( | ||
| data, | ||
| uint8_t, | ||
| (((block << 2) + (width * i)) + | ||
| (j * COMPONENT_COUNT[format]) + c) | ||
| )); | ||
| } | ||
| else | ||
| { | ||
| colors->c[c][(i << 2) + j] = 0; | ||
| } | ||
| colors->c[c][(i << 2) + j] /= 255; | ||
| } | ||
| } | ||
| } | ||
| state->currentBlock += 1; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Block/row offset missing ×COMPONENT_COUNT[format] scaling, and alpha never initialized for alpha-less formats.
- Offset bug: the slice index
(((block<<2)+(width*i)) + (j*COMPONENT_COUNT[format]) + c)only scales the intra-blockjterm byCOMPONENT_COUNT[format];block<<2andwidth*iare left unscaled. For RGBA8 (COMPONENT_COUNT=4) atwidth=8, i=1, block=0, j=0, c=0, the correct byte offset is32, but this formula yields8. Every row past the first (i>0) and every block past the first (block>0) reads/writes from the wrong location — this is the core addressing bug for every non-L8, non-float-with-1-component format in this file (also affectsrgba8WriteScalar,rgbafReadScalar<T>/rgbafWriteScalar<T>, andrgba4444ReadScalar). - Alpha bug: this function loops only
c < COMPONENT_COUNT[format], so forFORMAT_R8/RG8/RGB8(COMPONENT_COUNT1-3)colors->ais never written. The caller (image.cpp) passes an uninitializedColorRGBAF32x16 block;, so converting e.g. R8→RGBA8 (a combination allowed by_are_formats_compatible) propagates garbage alpha.
🐛 Proposed fixes
if ((((block << 2) + (width * i)) + j) < (width * (i + 1)))
{
colors->c[c][(i << 2) + j] = (*sliceAt(
data,
uint8_t,
- (((block << 2) + (width * i)) +
- (j * COMPONENT_COUNT[format]) + c)
+ ((((block << 2) + (width * i)) + j)
+ * COMPONENT_COUNT[format]) + c
));
}+ for (size_t i = 0; i < 4; i += 1)
+ {
+ colors->a[(i << 2) + 0] = 1;
+ colors->a[(i << 2) + 1] = 1;
+ colors->a[(i << 2) + 2] = 1;
+ colors->a[(i << 2) + 3] = 1;
+ }
state->currentBlock += 1;(guard the alpha fill with if (COMPONENT_COUNT[format] < 4) if desired)
🤖 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 `@core/io/image_rw.cpp` around lines 336 - 388, Fix pixel addressing in
rgba8ReadScalar and the corresponding scalar read/write helpers
rgba8WriteScalar, rgbafReadScalar, rgbafWriteScalar, and rgba4444ReadScalar by
scaling the complete pixel position (block and row offsets, plus j) by
COMPONENT_COUNT[format] before adding the component index. Also initialize
colors->a to the opaque value for formats with fewer than four components,
including R8, RG8, and RGB8, while preserving source alpha for four-component
formats.
| bool Slice::copy(Slice dst, Slice src) noexcept | ||
| { | ||
| uintptr_t s = (uintptr_t)src.data; | ||
| uintptr_t d = (uintptr_t)dst.data; | ||
| size_t n = MIN(src.length, dst.length); | ||
| size_t i; | ||
| bool srcTailDoesntOverlapDstHead = s <= (d - n); | ||
| const uint8_t *u8src = (const uint8_t*)src.data; | ||
| uint8_t *u8dst = (uint8_t*)dst.data; | ||
| if (src.length > dst.length) { return false; } | ||
| if (srcTailDoesntOverlapDstHead) | ||
| { | ||
| i = dst.length; | ||
| while (i > 0) | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Slice::copy reads past the end of src whenever dst.length > src.length.
n is computed as MIN(src.length, dst.length) but is only used for the overlap heuristic; both loops are driven by dst.length and index u8src[i] over that whole range. Since the guard only rejects src.length > dst.length, the common shorter-source case walks off the end of src.
This is reachable from core/io/io.cpp: sliceRead (Line 30) calls Slice::copy(buffer, tmp) where tmp.length is the truncated count at EOF while buffer.length is the caller's full request, so a short read over-reads the backing slice and also clobbers the tail of buffer with garbage rather than leaving it untouched.
🛡️ Proposed fix: drive both loops by `n`
- if (srcTailDoesntOverlapDstHead)
+ if (n == 0) { return true; }
+ if (srcTailDoesntOverlapDstHead)
{
- i = dst.length;
+ i = n;and in the forward branch:
- for (i = 0; i < dst.length;)
+ for (i = 0; i < n;)
{
- switch (dst.length - i)
+ switch (n - i)📝 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.
| bool Slice::copy(Slice dst, Slice src) noexcept | |
| { | |
| uintptr_t s = (uintptr_t)src.data; | |
| uintptr_t d = (uintptr_t)dst.data; | |
| size_t n = MIN(src.length, dst.length); | |
| size_t i; | |
| bool srcTailDoesntOverlapDstHead = s <= (d - n); | |
| const uint8_t *u8src = (const uint8_t*)src.data; | |
| uint8_t *u8dst = (uint8_t*)dst.data; | |
| if (src.length > dst.length) { return false; } | |
| if (srcTailDoesntOverlapDstHead) | |
| { | |
| i = dst.length; | |
| while (i > 0) | |
| { | |
| bool Slice::copy(Slice dst, Slice src) noexcept | |
| { | |
| uintptr_t s = (uintptr_t)src.data; | |
| uintptr_t d = (uintptr_t)dst.data; | |
| size_t n = MIN(src.length, dst.length); | |
| size_t i; | |
| bool srcTailDoesntOverlapDstHead = s <= (d - n); | |
| const uint8_t *u8src = (const uint8_t*)src.data; | |
| uint8_t *u8dst = (uint8_t*)dst.data; | |
| if (src.length > dst.length) { return false; } | |
| if (n == 0) { return true; } | |
| if (srcTailDoesntOverlapDstHead) | |
| { | |
| i = n; | |
| while (i > 0) | |
| { |
🤖 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 `@core/templates/Slice.cpp` around lines 6 - 20, Update Slice::copy so both
copy loops use the bounded length n rather than dst.length when indexing or
decrementing, preventing reads beyond src when dst is larger. Preserve the
existing rejection for src.length greater than dst.length and leave the unused
destination tail unchanged for short sources, including the sliceRead(buffer,
tmp) path.
| void Slice::set(Slice dst, uint8_t n) noexcept | ||
| { | ||
| uint16_t n16 = 0x0101 * n; | ||
| uint32_t n32 = 0x01010101 * n; | ||
| uint64_t n64 = 0x0101010101010101 * n; | ||
| for (size_t i = 0; i < dst.length; i += 1) | ||
| { | ||
| switch (dst.length - i) | ||
| { | ||
| case 4: | ||
| *((uint32_t*)get(dst, 1, i)) = n32; | ||
| i += 4; | ||
| break; | ||
| case 7: | ||
| case 3: | ||
| *((uint8_t*)get(dst, 1, i)) = n; | ||
| i += 1; | ||
| [[fallthrough]]; | ||
| case 6: | ||
| case 2: | ||
| *((uint16_t*)get(dst, 1, i)) = n16; | ||
| i += 2; | ||
| break; | ||
| case 5: | ||
| case 1: | ||
| *((uint8_t*)get(dst, 1, i)) = n; | ||
| i += 1; | ||
| break; | ||
| default: | ||
| *((uint64_t*)get(dst, 1, i)) = n64; | ||
| i += 8; | ||
| [[fallthrough]]; | ||
| case 24: | ||
| case 25: | ||
| case 26: | ||
| case 27: | ||
| case 28: | ||
| case 29: | ||
| case 30: | ||
| case 31: | ||
| *((uint64_t*)get(dst, 1, i)) = n64; | ||
| i += 8; | ||
| [[fallthrough]]; | ||
| case 16: | ||
| case 17: | ||
| case 18: | ||
| case 19: | ||
| case 20: | ||
| case 21: | ||
| case 22: | ||
| case 23: | ||
| *((uint64_t*)get(dst, 1, i)) = n64; | ||
| i += 8; | ||
| [[fallthrough]]; | ||
| case 8: | ||
| case 9: | ||
| case 10: | ||
| case 11: | ||
| case 12: | ||
| case 13: | ||
| case 14: | ||
| case 15: | ||
| *((uint64_t*)get(dst, 1, i)) = n64; | ||
| i += 8; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Slice::set double-advances the cursor and passes get's arguments in the wrong order, leaving unwritten gaps and writing out of bounds.
Three compounding problems:
- The
forheader'si += 1runs in addition to the in-body advances, so one byte is skipped per iteration.dst.length == 5: byte 0 written, loop bumpsito 2, then bytes 2–4 written — byte 1 is never set.dst.length == 9leaves byte 8 unset. get(dst, 1, i)passesindex = 1, size = i; the declared order isget(self, index, size). The address happens to land ondata + ionly becausesize * index == i, and it breaks outright oncegetvalidates the element size.- Nothing verifies that a widened store fits:
case 8writes auint64_tat offsetiwhen only 8 bytes remain fromi, which is fine, but thedefaultpath writes fouruint64_ts while only checking the start offset, so a short trailing region overruns the slice.
🛡️ Proposed fix: single-source-of-truth cursor, no double increment
void Slice::set(Slice dst, uint8_t n) noexcept
{
- uint16_t n16 = 0x0101 * n;
- uint32_t n32 = 0x01010101 * n;
- uint64_t n64 = 0x0101010101010101 * n;
- for (size_t i = 0; i < dst.length; i += 1)
- {
- switch (dst.length - i)
- {
- ...
- }
- }
+ uint64_t n64 = 0x0101010101010101ULL * n;
+ uint8_t *p = (uint8_t *)dst.data;
+ size_t i = 0;
+ for (; (i + sizeof(uint64_t)) <= dst.length; i += sizeof(uint64_t))
+ {
+ memcpy(&p[i], &n64, sizeof(uint64_t));
+ }
+ for (; i < dst.length; i += 1)
+ {
+ p[i] = n;
+ }
}memset would be the idiomatic choice here; the hand-rolled duff's-device variants in this file are unlikely to beat the compiler's builtin and are hard to keep correct.
📝 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.
| void Slice::set(Slice dst, uint8_t n) noexcept | |
| { | |
| uint16_t n16 = 0x0101 * n; | |
| uint32_t n32 = 0x01010101 * n; | |
| uint64_t n64 = 0x0101010101010101 * n; | |
| for (size_t i = 0; i < dst.length; i += 1) | |
| { | |
| switch (dst.length - i) | |
| { | |
| case 4: | |
| *((uint32_t*)get(dst, 1, i)) = n32; | |
| i += 4; | |
| break; | |
| case 7: | |
| case 3: | |
| *((uint8_t*)get(dst, 1, i)) = n; | |
| i += 1; | |
| [[fallthrough]]; | |
| case 6: | |
| case 2: | |
| *((uint16_t*)get(dst, 1, i)) = n16; | |
| i += 2; | |
| break; | |
| case 5: | |
| case 1: | |
| *((uint8_t*)get(dst, 1, i)) = n; | |
| i += 1; | |
| break; | |
| default: | |
| *((uint64_t*)get(dst, 1, i)) = n64; | |
| i += 8; | |
| [[fallthrough]]; | |
| case 24: | |
| case 25: | |
| case 26: | |
| case 27: | |
| case 28: | |
| case 29: | |
| case 30: | |
| case 31: | |
| *((uint64_t*)get(dst, 1, i)) = n64; | |
| i += 8; | |
| [[fallthrough]]; | |
| case 16: | |
| case 17: | |
| case 18: | |
| case 19: | |
| case 20: | |
| case 21: | |
| case 22: | |
| case 23: | |
| *((uint64_t*)get(dst, 1, i)) = n64; | |
| i += 8; | |
| [[fallthrough]]; | |
| case 8: | |
| case 9: | |
| case 10: | |
| case 11: | |
| case 12: | |
| case 13: | |
| case 14: | |
| case 15: | |
| *((uint64_t*)get(dst, 1, i)) = n64; | |
| i += 8; | |
| break; | |
| } | |
| } | |
| } | |
| void Slice::set(Slice dst, uint8_t n) noexcept | |
| { | |
| uint64_t n64 = 0x0101010101010101ULL * n; | |
| uint8_t *p = (uint8_t *)dst.data; | |
| size_t i = 0; | |
| for (; (i + sizeof(uint64_t)) <= dst.length; i += sizeof(uint64_t)) | |
| { | |
| memcpy(&p[i], &n64, sizeof(uint64_t)); | |
| } | |
| for (; i < dst.length; i += 1) | |
| { | |
| p[i] = n; | |
| } | |
| } |
🤖 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 `@core/templates/Slice.cpp` around lines 150 - 217, Fix Slice::set so every
byte in dst is written exactly once without overrunning the slice. Replace the
current Duff’s-device logic with a bounds-safe memset-equivalent implementation,
or otherwise use a single cursor update strategy; call get with its declared
index/size argument order and ensure each widened store fits within dst.length.
| constexpr inline void *Slice::get( | ||
| Slice self, | ||
| size_t index, | ||
| size_t size | ||
| ) noexcept | ||
| { | ||
| void *ret = nullptr; | ||
| if ((size * index) < self.length) | ||
| { | ||
| ret = ((uint8_t *)self.data) + (size * index); | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Slice::get bounds check ignores the element size, permitting out-of-bounds access.
Only the start offset is validated (size * index < self.length), not the end of the element. For length == 6, sliceAt(s, uint32_t, 1) yields offset 4 < 6 and returns a valid-looking pointer, but dereferencing it touches bytes 4–7 — 2 bytes past the slice. Every *sliceAt(...) in core/io/image_rw.cpp dereferences this result directly, so a partial trailing element (common for the last 4x4 block of a row) becomes a heap OOB read/write. size * index can also overflow size_t.
🛡️ Proposed fix: validate the full element range
constexpr inline void *Slice::get(
Slice self,
size_t index,
size_t size
) noexcept
{
void *ret = nullptr;
- if ((size * index) < self.length)
+ if (size != 0 && index <= ((SIZE_MAX / size) - 1) &&
+ ((size * index) + size) <= self.length)
{
ret = ((uint8_t *)self.data) + (size * index);
}
return ret;
}📝 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.
| constexpr inline void *Slice::get( | |
| Slice self, | |
| size_t index, | |
| size_t size | |
| ) noexcept | |
| { | |
| void *ret = nullptr; | |
| if ((size * index) < self.length) | |
| { | |
| ret = ((uint8_t *)self.data) + (size * index); | |
| } | |
| return ret; | |
| } | |
| constexpr inline void *Slice::get( | |
| Slice self, | |
| size_t index, | |
| size_t size | |
| ) noexcept | |
| { | |
| void *ret = nullptr; | |
| if (size != 0 && index <= ((SIZE_MAX / size) - 1) && | |
| ((size * index) + size) <= self.length) | |
| { | |
| ret = ((uint8_t *)self.data) + (size * index); | |
| } | |
| return ret; | |
| } |
🤖 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 `@core/templates/Slice.h` around lines 32 - 44, Update Slice::get to validate
that the entire element range fits within self.length, not only that size *
index is below it. Compute the offset with overflow-safe checks before forming
the pointer, and return nullptr when multiplication or offset-plus-size would
overflow or exceed the slice bounds.
| constexpr inline bool Slice::subslice( | ||
| Slice *dst, | ||
| Slice src, | ||
| size_t begin, | ||
| size_t count | ||
| ) noexcept | ||
| { | ||
| bool ret = (src.length - count) < begin; | ||
| assert(dst); | ||
| if (!ret) | ||
| { | ||
| dst->data = &(((uint8_t*)src.data)[begin]); | ||
| dst->length = count; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
subslice accepts oversized ranges due to unsigned underflow, and its return polarity is inverted relative to its name.
src.length - count wraps when count > src.length, producing a huge value that fails the < begin test, so the range is accepted and dst->length = count exposes memory past src. Example: src.length = 4, begin = 0, count = 8 → underflow → treated as valid.
Separately, ret == true means failure here, which is the opposite of the usual convention and is already mis-consumed at Line 66.
🛡️ Proposed fix: underflow-safe check with success-means-true semantics
constexpr inline bool Slice::subslice(
Slice *dst,
Slice src,
size_t begin,
size_t count
) noexcept
{
- bool ret = (src.length - count) < begin;
assert(dst);
- if (!ret)
+ bool ok = (count <= src.length) && (begin <= (src.length - count));
+ if (ok)
{
dst->data = &(((uint8_t*)src.data)[begin]);
dst->length = count;
}
- return ret;
+ return ok;
}Flipping the polarity requires updating the callers at core/templates/Slice.h Line 66, core/io/io.cpp Lines 29 and 57, and core/io/image_rw.cpp Line 597.
🤖 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 `@core/templates/Slice.h` around lines 46 - 61, Update Slice::subslice to
validate ranges without unsigned underflow by rejecting count values larger than
src.length and ensuring begin plus count stays within the source; return true on
successful subslice creation and false on invalid ranges. Adjust all callers in
Slice.h, io.cpp, and image_rw.cpp to use the corrected success-means-true
contract, preserving existing failure handling.
| inline bool Slice::set(Slice dst, Slice src, size_t index) noexcept | ||
| { | ||
| Slice tmp = {}; | ||
| if (!subslice(&tmp, dst, index, src.length)) { return false; } | ||
| return Slice::copy(tmp, src); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Inverted condition makes Slice::set always fail.
subslice returns false on success, so this bails out precisely when the subslice was created successfully, and falls through to Slice::copy with the zero-initialized tmp when it failed. Either way the function returns false and never copies. If you adopt the success-means-true polarity suggested for subslice, this body becomes correct as written; otherwise invert the test here.
🤖 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 `@core/templates/Slice.h` around lines 63 - 68, Fix the success-condition
handling in Slice::set: align the subslice(&tmp, dst, index, src.length) check
with subslice’s actual return polarity so successful creation proceeds to
Slice::copy and failure returns false. If subslice is changed to return true on
success, preserve this body; otherwise invert the condition in Slice::set.
|
New one in my name at #1325 |
2 participants
This is 100% written by @VictorSohier - He handed me a .diff to apply it for him.
Summary by CodeRabbit
New Features
Reliability