fix: garbled TRMNL sleep screen: unpack sub-8-bit grayscale PNGs when using trmnl.app#1
fix: garbled TRMNL sleep screen: unpack sub-8-bit grayscale PNGs when using trmnl.app#1mcmphd wants to merge 4 commits into
Conversation
The PNG draw/convert paths assumed 8-bit grayscale (one byte per pixel) and read pixels[srcX] / memcpy'd the scanline directly. PNGdec delivers grayscale rows at the source bit depth, packed MSB-first, so 1/2/4-bit grayscale packs 8/4/2 pixels per byte. TRMNL's /api/display serves 2-bit grayscale PNGs (800x480, og_png model), so the 8-bit-only read walked ~4x too far along each scanline, shearing the image into a compressed, repeated strip on the left of the panel. Unpack sub-8-bit grayscale samples using pDraw->iBpp and scale to 0..255 in both PNG consumers: - SleepActivity.cpp pngOverlayDraw (TRMNL sleep screen, overlay sleep) - PngToFramebufferConverter.cpp convertLineToGray (EPUB images) 8-bit and higher paths are unchanged. Indexed sub-8-bit PNGs remain a known separate limitation (not exercised by TRMNL). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer's GuideUnpacks sub-8-bit grayscale PNG scanlines in both the EPUB image conversion pipeline and TRMNL sleep overlay rendering, using PNGdec’s bit-depth metadata to correctly expand 1/2/4-bit grayscale to 8-bit and fixing the observed horizontal shearing/compression for TRMNL-provided images. Sequence diagram for grayscale PNG rendering with sub-8-bit unpackingsequenceDiagram
actor TrmnlServer
participant PNGdec
participant SleepActivity
participant EpubPngToFramebufferConverter
TrmnlServer->>PNGdec: /api/display 1/2/4-bit grayscale PNG
PNGdec-->>SleepActivity: pngOverlayDraw(pDraw)
PNGdec-->>EpubPngToFramebufferConverter: pngDrawCallback(pDraw)
alt [Sleep screen]
SleepActivity->>SleepActivity: pngOverlayDraw(pDraw)
alt [bpp >= 8]
SleepActivity->>SleepActivity: gray = pixels[srcX]
else [bpp < 8]
SleepActivity->>SleepActivity: unpack grayscale from packed pixels
end
else [EPUB image]
EpubPngToFramebufferConverter->>EpubPngToFramebufferConverter: convertLineToGray(pPixels, grayLine, width, pixelType, palette, hasAlpha, bpp)
alt [bpp >= 8]
EpubPngToFramebufferConverter->>EpubPngToFramebufferConverter: memcpy(grayLine, pPixels, width)
else [bpp < 8]
EpubPngToFramebufferConverter->>EpubPngToFramebufferConverter: unpack grayscale from packed pixels
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The sub-8-bit grayscale unpacking logic is duplicated between
convertLineToGrayandpngOverlayDraw; consider extracting a shared helper to avoid divergence in future changes. - For robustness, guard the new
bpphandling against unexpected values (e.g.,bppnot dividing 8 or being 0) and fail fast or fall back to a safe path instead of relying on PNGdec invariants.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The sub-8-bit grayscale unpacking logic is duplicated between `convertLineToGray` and `pngOverlayDraw`; consider extracting a shared helper to avoid divergence in future changes.
- For robustness, guard the new `bpp` handling against unexpected values (e.g., `bpp` not dividing 8 or being 0) and fail fast or fall back to a safe path instead of relying on PNGdec invariants.
## Individual Comments
### Comment 1
<location path="lib/Epub/Epub/converters/PngToFramebufferConverter.cpp" line_range="109-110" />
<code_context>
// For indexed PNGs with tRNS chunk, alpha values are stored at palette[768] onwards.
// Processing the whole line at once improves cache locality and reduces per-pixel overhead.
-void convertLineToGray(uint8_t* pPixels, uint8_t* grayLine, int width, int pixelType, uint8_t* palette, int hasAlpha) {
+void convertLineToGray(uint8_t* pPixels, uint8_t* grayLine, int width, int pixelType, uint8_t* palette, int hasAlpha,
+ int bpp) {
switch (pixelType) {
case PNG_PIXEL_GRAYSCALE:
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify and guard assumptions about `bpp` values, particularly 16-bit grayscale.
The current branching on `bpp >= 8` vs `< 8` doesn’t account for 16-bit grayscale. PNG grayscale supports 1, 2, 4, 8, and 16 bits, and if `bpp == 16` we’ll `memcpy(grayLine, pPixels, width)` but later treat it as 8-bit, effectively discarding half the data and misreading the source buffer. Please either explicitly handle `bpp == 16` (e.g., downscale to 8-bit or reject it) and/or assert that `bpp <= 8` for this path to avoid silent mis-rendering when higher bit depths are used.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…bit case
- Extract the sub-8-bit grayscale unpack logic that was duplicated between
pngOverlayDraw() (SleepActivity.cpp) and convertLineToGray()
(PngToFramebufferConverter.cpp) into a single shared helper,
pngUnpackGraySample() in lib/PngGraySample/PngGraySample.h.
- The helper explicitly handles every valid PNG grayscale bit depth
(1/2/4/8/16) via a closed switch, closing two gaps the previous
bpp >= 8 / bpp < 8 branching had:
- 16-bit grayscale (2 bytes/sample, big-endian per spec) was
previously read as if 1 byte/sample, misinterpreting every high/low
byte pair as two separate samples.
- Any bpp PNGdec didn't actually emit falls through to a safe neutral
gray (128) instead of an unguarded division/shift on attacker- or
encoder-controlled input, rather than relying on PNGdec's invariants
as the previous if/else structure implicitly did.
- convertLineToGray() keeps its bulk memcpy fast path for the common
8-bit case (this function's own comment flags cache locality/throughput
as a real concern for whole-scanline processing); every other depth
now routes through the shared per-sample helper. pngOverlayDraw() was
already single-pixel-at-a-time, so it calls the helper directly with
no fast-path split.
- No behavioral change to the 1/2/4-bit unpack math itself (the fix this
PR exists for) -- only relocated, not altered. Verified via a clean
`pio run -e simulator` build.
Not in scope for this commit: bytesPerPixelFromType() /
requiredPngInternalBufferBytes() elsewhere in PngToFramebufferConverter.cpp
hardcode 8-bit-per-channel byte counts for every pixel type (not just
grayscale), which under-estimates PNGdec's real internal scanline buffer
requirement by ~2x for any genuine 16-bit-depth PNG and could let an
oversized image slip past the pre-decode buffer-overflow guard. This is
pre-existing, broader than grayscale, and outside what this review comment
raised -- flagged separately rather than folded in here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The goal on this and the other two pull requests (#2, #3) was to get the TRMNL sleep screen working on my X4 with the TRMNL server and BYOD license option. I found at minimum there needed to be an adjustment to the PNG reader (this request) and a tweak to the URL decoder (#2, started by doubling the buffer length, then went ahead and made the decoder more robust). I also found that my home Wifi, which is WPA2/3, was timing out about half the time: doubling the refresh timeout in request #3 seems to fix that completely. With all 3 patches, TRMNL is reliably running on my X4 on my Wifi. |
Problem
On the X4, TRMNL sleep screens render as a horizontally compressed,
repeated strip on the left of the panel instead of the full image.
Root cause
TRMNL's
/api/displayserves 1-bit or 2-bit grayscale PNGs (800×480,og_pngmodel). Both PNG consumers assume 8-bit grayscale — one byte per pixel:
SleepActivity.cpppngOverlayDraw()→gray = pixels[srcX];PngToFramebufferConverter.cppconvertLineToGray()→memcpy(grayLine, pPixels, width);PNGdec delivers grayscale scanlines at the source bit depth, packed
MSB-first (2-bit = 4 pixels/byte). Reading one byte per pixel walks ~4×
too far along each scanline, shearing the image — exactly the observed
compressed/repeated strip.
Fix
Unpack sub-8-bit grayscale using
pDraw->iBpp, scaling each sample to0–255. 8-bit and higher paths are unchanged; transparency/tRNS handling
is untouched.
Scope / notes
path had the same latent bug, unexercised because EPUB images are 8-bit).
exercised by TRMNL.
Testing
Verified the served TRMNL image is
PNG 800×480, 2-bit grayscale, 1 sample/pixelviasips. Reproduced the shear onmainand confirmedthe fix in the CrossXT
simulatorenv (macOS/arm64) with a synthetic2-bit test vector (solid regions + diagonal + isolated box) — output
matches the expected reference exactly. Built and flashed
debugon anXTeink X4; TRMNL sleep screen now renders correctly on hardware.
Summary by Sourcery
Handle sub-8-bit grayscale PNGs correctly in both sleep-screen and EPUB rendering paths to prevent horizontally sheared images.
Bug Fixes: