chore: Remove miniz and modularise inflation logic#1073
Conversation
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
📝 WalkthroughWalkthroughReplaces in-project miniz/tinfl/uzlib usage with a new InflateReader abstraction; adds InflateReader module; migrates font, PNG, and ZIP decompression to InflateReader streaming or one-shot APIs; removes the public miniz header and adjusts PlatformIO build flags. Changes
Sequence Diagram(s)sequenceDiagram
participant File as File/Stream
participant Reader as InflateReader
participant Decoder as Decoder (PNG/Font/ZIP)
participant Out as Output Buffer
File->>+Reader: provide compressed bytes (setSource or read callback)
Reader->>+Decoder: deliver decompressed chunk (read / readAtMost)
Decoder->>Out: write/defilter/assemble data
Decoder-->>-Reader: request more decompressed data
Reader-->>-File: invoke read callback when more input needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/EpdFont/FontDecompressor.cpplib/EpdFont/FontDecompressor.hlib/InflateReader/InflateReader.cpplib/InflateReader/InflateReader.hlib/PngToBmpConverter/PngToBmpConverter.cpplib/ZipFile/ZipFile.cpplib/miniz/miniz.clib/miniz/miniz.hplatformio.ini
💤 Files with no reviewable changes (2)
- lib/miniz/miniz.h
- platformio.ini
🧰 Additional context used
🧬 Code graph analysis (1)
lib/InflateReader/InflateReader.h (1)
lib/InflateReader/InflateReader.cpp (16)
InflateReader(14-14)init(16-28)init(16-16)deinit(30-36)deinit(30-30)setSource(38-41)setSource(38-38)setReadCallback(43-43)setReadCallback(43-43)cb(43-43)skipZlibHeader(45-48)skipZlibHeader(45-45)read(50-62)read(50-50)readAtMost(64-74)readAtMost(64-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🔇 Additional comments (16)
lib/EpdFont/FontDecompressor.h (2)
3-3: LGTM: new InflateReader dependency.
Keeps the header aligned with the new decompression abstraction.
31-31: LGTM: InflateReader state replaces uzlib internals cleanly.lib/ZipFile/ZipFile.cpp (2)
9-35: Nice encapsulation of streaming inflate state.
The context + callback separation makes the I/O path easier to follow.
420-473: LGTM: ZIP method branching updated for InflateReader.
One-shot inflate keeps raw-deflate handling straightforward.lib/EpdFont/FontDecompressor.cpp (1)
82-85: LGTM: one-shot InflateReader path is straightforward.lib/InflateReader/InflateReader.cpp (2)
7-12: LGTM: 32KB window + layout guard.
The static_assert helps keep the callback cast safe.
38-74: LGTM: source/callback wiring and read APIs look solid.lib/PngToBmpConverter/PngToBmpConverter.cpp (8)
23-52: LGTM: inline BMP writers + Paeth predictor.
54-175: LGTM: PNG/BMP helpers are clear and localized.
177-205: LGTM: decode context updated for InflateReader streaming.
209-267: LGTM: IDAT scanning + read callback look correct.
273-277: LGTM: scanline decode now streams via InflateReader.
502-510: LGTM: context initialization aligns with the new pipeline.
559-568: LGTM: reader init + zlib header skip for PNG IDAT.
593-594: LGTM: fixed‑point scale factor update.lib/InflateReader/InflateReader.h (1)
43-89: LGTM: API covers both one‑shot and streaming use cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/InflateReader/InflateReader.cpp`:
- Around line 16-36: The init method currently overwrites ringBuffer without
freeing previous allocation; before allocating in InflateReader::init when
streaming is true, check if ringBuffer is non-null and free it (or call
InflateReader::deinit behavior) to avoid leaking memory, then proceed to
allocate and zero INFLATE_DICT_SIZE; if allocation fails ensure ringBuffer
remains nullptr and return false; finally call uzlib_uncompress_init(&decomp,
ringBuffer, ringBuffer ? INFLATE_DICT_SIZE : 0) as before.
In `@lib/InflateReader/InflateReader.h`:
- Around line 7-9: Implement the missing inflateBuffer function in
InflateReader.cpp: add a definition matching bool inflateBuffer(const uint8_t*
src, size_t srcLen, uint8_t* dst, size_t dstLen) that performs a one-shot
raw-deflate decompression using zlib (inflateInit2 with -MAX_WBITS), sets
z_stream.next_in/avail_in to src/srcLen and next_out/avail_out to dst/dstLen,
loops calling inflate until Z_STREAM_END or an error, then calls inflateEnd and
returns true only when exactly dstLen bytes were produced (avail_out == 0 and
inflate finished successfully); on any zlib error or short/over production,
clean up with inflateEnd and return false. Include necessary zlib headers and
ensure the function signature matches the declaration in InflateReader.h.
In `@lib/ZipFile/ZipFile.cpp`:
- Around line 577-601: The loop currently sets success when InflateStatus::Done
without validating totals; modify the streaming path around
ctx.reader.readAtMost/outputBuffer to accumulate a running total (e.g.,
totalProduced) of all produced bytes, and after receiving InflateStatus::Done
verify totalProduced == fileStat.uncompressedSize (error and abort if less or if
totalProduced exceeds fileStat.uncompressedSize during accumulation); only set
success = true and log inflatedDataSize when the sizes match, and log an error
and break on mismatch/overflow so truncated or overflown output is rejected.
daveallie
force-pushed
the
chore/cleanup-miniz
branch
from
3a7dfd2 to
ea0c0f1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/InflateReader/InflateReader.cpp (1)
6-8: Consider makingINFLATE_DICT_SIZEa compile-time constant.The value is fixed at 32768 and never changes. Using
constexprwould be more idiomatic and ensures the value is known at compile time.♻️ Suggested change
namespace { -size_t INFLATE_DICT_SIZE = 32768; +constexpr size_t INFLATE_DICT_SIZE = 32768; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/InflateReader/InflateReader.cpp` around lines 6 - 8, Change the runtime modifiable variable INFLATE_DICT_SIZE into a compile-time constant by replacing its definition in the anonymous namespace with a constexpr (e.g., constexpr size_t INFLATE_DICT_SIZE = 32768;) so the value is known at compile time and cannot be modified; update any usages of INFLATE_DICT_SIZE in InflateReader.cpp as needed (no API changes required).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/EpdFont/FontDecompressor.cpplib/EpdFont/FontDecompressor.hlib/InflateReader/InflateReader.cpplib/InflateReader/InflateReader.hlib/PngToBmpConverter/PngToBmpConverter.cpplib/ZipFile/ZipFile.cpplib/miniz/miniz.clib/miniz/miniz.hplatformio.ini
💤 Files with no reviewable changes (2)
- lib/miniz/miniz.h
- platformio.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/EpdFont/FontDecompressor.h
🧰 Additional context used
🧬 Code graph analysis (2)
lib/InflateReader/InflateReader.cpp (2)
lib/InflateReader/InflateReader.h (1)
InflateReader(39-85)lib/uzlib/src/tinflate.c (3)
uzlib_uncompress_init(552-562)uzlib_get_byte(193-217)uzlib_uncompress(565-626)
lib/InflateReader/InflateReader.h (1)
lib/InflateReader/InflateReader.cpp (16)
InflateReader(14-14)init(16-27)init(16-16)deinit(29-35)deinit(29-29)setSource(37-40)setSource(37-37)setReadCallback(42-42)setReadCallback(42-42)cb(42-42)skipZlibHeader(44-47)skipZlibHeader(44-44)read(49-61)read(49-49)readAtMost(63-73)readAtMost(63-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🔇 Additional comments (16)
lib/ZipFile/ZipFile.cpp (4)
9-35: LGTM!The
ZipInflateCtxstruct correctly placesInflateReaderas the first member, enabling the uzlib callback cast pattern. ThezipReadCallbackimplementation properly handles EOF conditions and buffer management, matching the documented pattern inInflateReader.h.
457-463: LGTM!The one-shot decompression path is cleanly implemented with proper RAII scoping. The
InflateReaderdestructor runs beforedeflatedDatais freed, ensuring correct cleanup order.
577-621: LGTM!The streaming decompression correctly tracks
totalProducedand validates both overflow during accumulation and exact size match at completion. The cleanup paths properly handle all allocated resources.
17-19: LGTM!Namespace-scoped constants improve readability and maintainability over magic numbers for compression method checks.
lib/EpdFont/FontDecompressor.cpp (1)
82-88: LGTM!The migration to
InflateReaderis clean. Sinceinit()internally callsdeinit()first, repeated decompression calls are safe. Error handling correctly freesoutBufon failure.lib/InflateReader/InflateReader.h (2)
1-46: LGTM!The
InflateStatusscoped enum and class structure are well-designed. The copy semantics are correctly deleted, and the documentation clearly explains the streaming callback pattern for uzlib integration.
47-85: LGTM!The method declarations provide a clear and well-documented API for both one-shot and streaming decompression modes. The
raw()accessor enables advanced use cases while the primary API remains simple.lib/InflateReader/InflateReader.cpp (3)
10-14: LGTM!The
static_assertensures the callback casting pattern remains valid, and the destructor properly delegates todeinit()for cleanup.
16-35: LGTM!The
init()/deinit()pair correctly handles resource management. Callingdeinit()at the start ofinit()prevents ring buffer leaks on re-initialization, addressing the previously identified issue.
49-73: LGTM!The
read()andreadAtMost()implementations correctly handle the different modes. One-shot mode setsdest_startfor back-reference resolution, while streaming mode relies on the ring buffer. The status mapping inreadAtMost()is correct.lib/PngToBmpConverter/PngToBmpConverter.cpp (6)
23-56: LGTM!The BMP writing helpers and
paethPredictorare correctly implemented. The helper functions follow little-endian BMP format requirements.
178-205: LGTM!The
PngDecodeContextcorrectly placesInflateReaderas the first member, enabling the callback casting pattern. The structure properly encapsulates all state needed for streaming PNG decompression.
233-267: LGTM!The
pngIdatReadCallbackcorrectly handles multi-chunk IDAT streams, properly skipping CRC bytes between chunks and finding subsequent IDAT chunks when needed.
270-320: LGTM!The
decodeScanlinefunction cleanly integrates withInflateReader::read()for decompression and correctly implements all PNG filter types.
559-568: LGTM!The streaming
InflateReaderinitialization is correct. PNG IDAT data uses zlib compression, so callingskipZlibHeader()before decompression is necessary.
593-594: LGTM!The fixed-point scaling arithmetic is safe given the image size constraints (
MAX_IMAGE_WIDTH = 2048,MAX_IMAGE_HEIGHT = 3072). The shift operationwidth << 16won't overflowuint32_twithin these limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/InflateReader/InflateReader.cpp`:
- Around line 6-8: Change the runtime modifiable variable INFLATE_DICT_SIZE into
a compile-time constant by replacing its definition in the anonymous namespace
with a constexpr (e.g., constexpr size_t INFLATE_DICT_SIZE = 32768;) so the
value is known at compile time and cannot be modified; update any usages of
INFLATE_DICT_SIZE in InflateReader.cpp as needed (no API changes required).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/InflateReader/InflateReader.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/InflateReader/InflateReader.cpp`:
- Around line 63-69: readAtMost currently doesn't set decomp.dest_start in
non-streaming (one-shot) mode, which causes back-references to use an
uninitialized dest_start when ringBuffer is null; update
InflateReader::readAtMost to mirror read() by checking if ringBuffer is null and
then set decomp.dest_start = dest (or return an error/InflateStatus indicating
streaming-only) before calling uzlib_uncompress(&decomp), ensuring produced is
only computed after decomp.dest_start is initialized and avoiding corrupted
output/crash when init(false) was used.
d6001e9 to
6c1e32c
Compare
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
…#1073) ## Summary * Remove miniz and move completely to uzlib * Move uzlib interfacing to InflateReader to better modularise inflation code --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor
1 participant
Summary
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? Yes, Claude helped with the extraction and refactor