feat(byte-codec): encode indexed-colour PNGs when the palette is small enough - #1014
Merged
Conversation
Mearman
marked this pull request as ready for review
September 5, 2026 14:12
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Mearman
force-pushed
the
feat/byte-codec-indexed-png-encode
branch
5 times, most recently
from
September 6, 2026 06:08
77dc7cc to
48c73f5
Compare
Mearman
force-pushed
the
feat/byte-codec-indexed-png-encode
branch
2 times, most recently
from
September 6, 2026 08:10
f127031 to
f34a58e
Compare
…l enough encodePng covered only four of PNG's five colour types -- 0/2/4/6 (gray, truecolor, gray+alpha, truecolor+alpha) -- and never emitted colour type 3 (indexed/palette), even though decodePng already reads it via a PLTE lookup. A channels === 3 source image is now scanned for a lossless indexed-colour representation: every distinct (r, g, b, a) combination becomes one palette entry, addressed by a one-byte-per-pixel index into a PLTE chunk plus a tRNS chunk when the source carried an alpha plane. The scan bails to the existing truecolour path the moment a 257th distinct colour would be needed, since colour type 3 cannot address more than 256 entries. Indexed colour is substantially smaller than truecolour for the flat-colour images typical of diagrams and screenshots that end up embedded in documents, and decodePng(encodePng(image)) round-trips exactly either way.
The module table still described encodePng as producing only truecolour output and decodePng as handling "raw pixels" generically, with no mention of the colour-type-3 (indexed/palette) path either side now covers.
…ncodePng images-read.ts re-encoded a decoded Image XObject's raw samples through this package's own internal src/image/png-encode.ts rather than the byte-codec copy the public barrel already re-exports as this package's encodePng. The two copies started life identical, but nothing kept them in sync, so this package's own re-encoded output could silently diverge from what a caller using its own advertised encodePng would produce for the same raster data -- exactly the risk that shows up the moment one copy gains a capability the other lacks. Importing RawImage/encodePng/readJpegInfo from byte-codec instead removes the second copy from this path entirely.
…chunk Trailing fully-opaque entries were trimmed from the palette's tRNS chunk down to zero when every palette entry was opaque, producing a zero-length tRNS chunk that libpng and other strict PNG consumers reject outright. The trim now stops at one entry, which validly states that single alpha value and leaves every other palette entry to default to 255, exactly matching the fully-opaque case. Strengthens the existing regression test to assert the chunk's length is greater than zero rather than merely present, since presence alone was already satisfied by the invalid zero-length chunk. Also adds an independent structural validator that re-walks the chunk stream and checks each chunk's own CRC-32 plus the PNG spec's tRNS length constraints, deliberately without routing through this repo's own decodePng -- the existing round-trip tests only ever validated encodePng's output against this repo's own reader, which tolerated the invalid chunk.
…of byte-codec's pdf-codec's src/image/png-encode.ts was a truecolour-only copy of the same encoder byte-codec now exports as encodePng, so the two could diverge silently: this package's own writes never gained indexed-colour support, and any future encoder fix would need applying twice. Every caller in this package now imports encodePng from byte-codec directly; pdf-codec's own png-decode.ts is unaffected, since write.ts genuinely decodes an embedded PNG through it and it already supports every colour type byte-codec's encoder can produce. pdf-codec's own png-encode.test.ts is removed rather than kept, since its coverage exercised byte-codec's real encodePng function even before this change and had no equivalent there; that coverage now lives in byte-codec's own suite. This package's own "./*" wildcard export previously published the pdf-codec/image/png-encode subpath, since src/image/png-encode.ts existed; deleting that file removes it. A one-line re-export module at the same path would keep the subpath resolving, but this repository's own barrel-policy lint rule (one barrel per package, at src/index.ts) forbids a re-export living anywhere else, so the subpath genuinely goes away rather than being kept alive through a lint-violating workaround. BREAKING CHANGE: the pdf-codec/image/png-encode subpath no longer exists. Import encodePng (and PngEncodeOptions) from byte-codec directly, or from pdf-codec's own root export, which already re-exports the same function.
…against Node's own zlib Moves the generic (non-indexed) round-trip and IHDR/signature coverage that used to live only in pdf-codec's own png-encode.test.ts into byte-codec's suite, since that coverage exercised byte-codec's real encodePng function even before pdf-codec's own copy was removed and had no equivalent here. Adds a matching pair of Node zlib.inflateSync cross-checks -- one for the plain greyscale path, one for the indexed path -- verifying the compressed IDAT stream against an external decoder rather than only this repo's own inflate.
… removed subpath pdf-codec no longer exports an image/png-encode subpath, since it now re-exports byte-codec's own encodePng from its root instead of keeping a duplicate encoder of its own.
…ribution assertSpecCompliantPng's own comment, and one of its assertions, quoted 'Zero length tRNS chunk' as libpng's own diagnostic string, but that exact string does not appear anywhere in libpng's real source (pngrutil.c) -- libpng's real check does reject a zero-length tRNS chunk, just not via that quoted message. Drops the fabricated quotation and describes the behaviour instead of inventing a string for it. The same comment also called both the tRNS length bounds (>=1 entry, <=palette-entries) 'the two length constraints the PNG spec places on it', but the PNG spec (W3C PNG 3rd edition) only states the upper bound -- a tRNS chunk must never carry more entries than the palette. The lower bound is specifically strict decoders' (libpng included) own additional constraint, not something the spec itself requires. Rewords the comment and the assertion below it to attribute each bound to its real source rather than crediting both to the spec.
The named repro for the earlier zero-length-tRNS fix was an opaque palette, but detectPalette could still emit an invalid PLTE chunk for a genuinely zero-dimension image (width or height 0): the pixel loop never runs, so the palette ends up empty, and a zero-length PLTE is not a valid PNG chunk either -- the format requires at least one entry. detectPalette now returns undefined whenever pixelCount is 0, so a zero-dimension image always falls back to the truecolour path, which carries no PLTE at all and has no equivalent emptiness constraint. Strengthens assertSpecCompliantPng to check the PLTE chunk's own length bounds (1-256 entries) alongside the existing tRNS checks, and adds a regression test covering zero width, zero height, and both, confirming none of them takes the indexed path and all still decode cleanly.
…ification that doesn't exist assertSpecCompliantPng's own comment claimed the empty-tRNS-rejection claim was "verified against real decoders below", but nothing in this file does that: the only external decoder exercised anywhere in the suite is Node's own zlib.inflateSync, which decompresses a DEFLATE stream and has no knowledge of PNG chunk structure at all, let alone tRNS validity. Cites libpng's own pngrutil.c (png_handle_tRNS) instead, the actual primary source for the claim.
… invalid truecolour fallback The earlier fix moved detectPalette's zero-dimension handling to a fallback onto the truecolour path, but that path is exactly as invalid for a zero-dimension image as the indexed one: the PNG spec (section 11.2.2, IHDR) states zero is an invalid value for width and height, regardless of colour type, and real decoders such as Pillow and macOS's sips reject a zero-dimension PNG outright. There is no compliant PNG this function could produce for such an image, so encodePng now throws immediately instead of quietly producing one spec violation in place of another. detectPalette's own zero-pixel-count guard is removed, since encodePng now rejects a zero-dimension image before detectPalette is ever called with one. assertSpecCompliantPng previously never checked IHDR's own width and height, so it incorrectly certified the earlier truecolour fallback as spec-compliant; it now asserts both are positive, and the regression test for zero width/height/both now asserts encodePng throws rather than asserting on the PNG it used to (invalidly) produce.
…on-positive ones encodePng's zero-dimension guard (width <= 0 || height <= 0) was a negative test that NaN and any fractional value silently bypass: NaN <= 0 evaluates to false in JavaScript, and a fractional width or height also passes the check unchanged, then gets truncated on write by IHDR's 4-byte unsigned-integer encoding into a dimension the caller never specified. Either case reaches PNG encoding and can still produce the exact invalid output (an empty PLTE chunk, a mismatched IHDR/pixel-layout) this guard exists to prevent, reachable from ordinary calling code that computes a dimension via a parse, a subtraction, or any other arithmetic that can yield NaN, with no TypeScript-level warning since NaN and fractional values are both plain numbers. The guard is now a positive validity test: both width and height must be finite positive integers (Number.isInteger(n) && n > 0), which rejects NaN, Infinity, negative, zero, and fractional values in one check rather than trying to enumerate their negations.
Three comments cited PNG spec section 11.2.2 for IHDR, but this file already anchors on the W3C PNG Third Edition (per an earlier fix to this same test file), and in that edition IHDR is section 11.2.1 -- 11.2.2 is PLTE. The Third Edition dropped the "11.2.1 General" subsection the 2003 Second Edition placed before IHDR, shifting every critical-chunk section number down by one.
…dimension regression test Extends the existing zero-width/zero-height/both-zero cases with NaN width, NaN height, fractional width, and fractional height, so the regression test actually exercises the values the previous <= 0 guard silently let through before it was tightened to a positive-integer check.
…haviour The module table's own entry for image/png-encode described the palette-detection behaviour but not the new throw for a non-positive or non-integer width/height -- a real new failure mode on a published package's public API that only the source docstring mentioned.
readImageXObject's own Width/Height guard only checked width <= 0, the same shape encodePng's own guard used before it was tightened to a positive-integer check. A fractional value, or a lexer NaN token from a bare '+', '-', or '.' byte where a number was expected, sails past `<= 0` and now hits encodePng's own throw, which nothing in pdf-codec's read path catches -- so one malformed image dict aborts the whole document parse instead of degrading to the existing image/undecodable diagnostic and skipping just that image.
encodePng's positive-integer guard rejected zero, negative, fractional, and NaN dimensions but had no upper bound, so a width or height of 2^31 or larger passed straight through into detectPalette's/writeTruecolorPng's pixel-sized allocations and loops and hung rather than erroring. The PNG spec defines IHDR's width/height fields as a 'PNG four-byte unsigned integer', a datatype the spec itself limits to 0 to 2^31-1, so anything at or above 2^31 has no valid IHDR encoding in the first place.
encodePng's dimension guard rejected each of width and height above 2^31-1 independently, but a pair that individually satisfies that ceiling can still multiply into a pixel count large enough to hang detectPalette's per-pixel colour-map scan and writeTruecolorPng's interleave loop for tens of seconds or longer (e.g. 46341 x 46341, or 65536 x 65536). Adds a PNG_MAX_PIXELS ceiling on width * height, checked alongside the existing per-dimension one, and exports both constants so a caller assembling a RawImage from untrusted input can mirror the same bounds before ever reaching this encoder.
…s smaller encodePng took the indexed-colour path unconditionally whenever a channels=3 image reduced to 256 or fewer distinct colours, on the assumption that indexed colour is always smaller. For small images the PLTE (and, with alpha, tRNS) chunk's own fixed overhead outweighs its one-byte-per-pixel IDAT savings -- measured directly, indexed loses to truecolour up to roughly 64x64 pixels, so small icons and spacer images were coming out larger than they needed to. Both encodings are now produced and the genuinely smaller one returned, since the actual output size is data-dependent and not reliably predictable from pixel or colour count alone.
readImageXObject's /Width //Height guard checked for undefined, a non-integer, and a non-positive value, but not the upper bound encodePng now enforces on each dimension or on their product. An image dict whose declared dimensions exceed either ceiling sailed past this guard and straight into encodePng's own throw, which nothing here catches, aborting the whole document parse over one malformed image dict instead of degrading to the existing image/undecodable diagnostic. Imports PNG_MAX_DIMENSION and PNG_MAX_PIXELS from byte-codec rather than restating either bound.
… timeout Choosing between the indexed and truecolour encodings now means running both to completion and comparing their actual byte lengths, so a test proving indexed colour wins at a full 256-entry palette needs enough pixels for its one-byte-per-pixel IDAT savings to overcome the palette's own larger fixed overhead. That volume of deflate work comfortably clears vitest's 5s default timeout on a plain run but not under v8 coverage instrumentation, where the same hot loops run several times slower -- matching document.test.ts's own precedent of giving a genuinely heavier test an explicit longer timeout rather than shrinking it below what the behaviour under test actually needs.
… encodePng readJpeg2000Image took image.width/image.height straight from the decoded JPEG 2000 codestream's SIZ marker and passed them to encodePng with no check at all, one branch earlier than readImageXObject's own /Width and /Height guard for the generic decode path. A crafted or malformed SIZ marker declaring a pixel count encodePng itself refuses aborted the whole document parse with an uncaught throw instead of degrading to this function's usual "skip this image" diagnostic -- exactly the failure mode the sibling guard exists to prevent for the dictionary-driven path. Mirrors that same PNG_MAX_DIMENSION/PNG_MAX_PIXELS bound right after the codestream decodes, before the per-pixel colour conversion and encodePng call that follow.
…claims The comment claimed the ceiling sat "well above the largest sensors on the consumer/prosumer market so no genuine photograph or scan is rejected" and that it kept every per-pixel loop bounded to "a small fraction of a second". Neither holds up: a currently-sold prosumer medium-format sensor (and an ordinary high-DPI full-page scan) already exceeds 100 million pixels, and encoding an image right at the ceiling measures in the tens of seconds end to end -- dominated by filterScanlines' own adaptive per-row search across all five PNG filter types and the deflate pass that follows, not by the two named per-pixel loops (detectPalette's colour-map scan and writeTruecolorPng's interleave step), which are fast in isolation. Restates what the bound actually does: keep an otherwise-unbounded encode's worst case finite, not fast, and accept that a large enough legitimate image needs downsampling before this encoder rather than claiming none exists.
…gible images Neither the function's own docstring nor the README's module table mentioned that a channels === 3 image eligible for indexed colour is fully filtered and deflated twice -- once as indexed, once as truecolour -- to compare their output sizes, roughly doubling encode time for the flat-colour images this path targets.
…g2000 allocates it decodeJpeg2000 allocated a width*height Int32Array per component before readJpeg2000Image's own PNG_MAX_DIMENSION/PNG_MAX_PIXELS guard ever ran, since that guard only inspected decodeJpeg2000's return value. A producer-declared SIZ marker large enough to exceed those bounds still paid for the full per-component allocation (and, under coverage instrumentation, ran slowly enough to threaten vitest's default test timeout) before being rejected. decodeJpeg2000 now accepts optional maxWidth/maxHeight/maxPixels bounds and checks them immediately after parsing SIZ's declared width and height, before allocating any sample planes. readJpeg2000Image passes PNG_MAX_DIMENSION and PNG_MAX_PIXELS through as these bounds instead of checking the decoded image's own dimensions afterwards, so an oversized canvas is refused at the point its size becomes known rather than after the wasted decode work.
…matching writeTruecolorPng detectPalette read image.data and image.alpha with a non-null assertion, assuming both were always exactly width*height(*channels) long. When either plane was shorter than that -- reachable through pdf-codec's own tolerant-recovery paths, such as a short-streamed /SMask or a truncated inflate -- the missing samples read back as undefined, turning the palette key's arithmetic into NaN for every affected pixel. Since Map treats NaN as equal to itself, a second such pixel reused the first one's palette entry outright, silently replacing its real RGB or alpha with an unrelated pixel's. writeTruecolorPng never had this problem: writing an out-of-range undefined sample into its Uint8Array output already coerces to 0 there. detectPalette now applies that same default explicitly, so a malformed image decodes identically regardless of which of the two candidate encodings encodePng happens to pick.
…ed PNG path Neither existing test constructed a RawImage whose data or alpha length disagreed with width*height(*channels) -- the shape pdf-codec's own tolerant-recovery paths can produce for real, and the one input class where the indexed and truecolour candidate encodings previously disagreed on a malformed image's actual content. Each test forces the indexed path to win (few distinct colours, enough repetition to clear the palette-overhead crossover), then truncates one plane by two trailing pixels' worth of samples -- two, not one, because the two rows alternate colour and the collision this guards against only shows up once a second, differently-valued out-of-range pixel would otherwise reuse the first one's palette entry.
Mearman
force-pushed
the
feat/byte-codec-indexed-png-encode
branch
from
September 6, 2026 11:21
f34a58e to
b83ef8a
Compare
Contributor
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
encodePng covered 4 of PNG's 5 colour types and never emitted colour type 3 (indexed/palette), even though decodePng already reads it via a PLTE lookup. A
channels === 3source image is now scanned for a lossless indexed-colour representation whenever it reduces to 256 or fewer distinct colours; both that encoding and the plain truecolour one are produced and whichever comes out smaller is returned, since the indexed path's own PLTE (+ tRNS) chunk overhead can outweigh its per-pixel savings for small images.decodePng(encodePng(image))round-trips exactly either way.pdf-codec's
images-read.tsre-encoded extracted images through its own internal, independently-copiedsrc/image/png-encode.tsrather than the byte-codec copy its own public barrel already re-exports asencodePng. The two copies started identical but nothing kept them in sync, so this package's own re-encoded output could silently diverge from what byte-codec's advertisedencodePngproduces for the same raster data. Switched that one call site to importRawImage/encodePng/readJpegInfofrombyte-codecdirectly, removing the second copy from this path.encodePngnow rejects a width or height outside the PNG spec's own IHDR range (zero, negative, fractional, NaN, or at/above 2^31), and separately bounds width * height, since a dimension pair that individually satisfies the per-dimension ceiling can still multiply into a pixel count large enough to make one encode take tens of seconds.images-read.tsmirrors both ofencodePng's own dimension and pixel-count ceilings against a PDF image dictionary's own /Width and /Height for its generic decode path, and separately against a JPXDecode codestream's own SIZ marker before that path's ownencodePngcall, so a malformed producer's out-of-range values degrade to the existing diagnostics instead of throwing out ofencodePngand aborting the whole document parse.byte-codec's own
image/png-encode.tsis where all of this actually lives -- the indexed-colour detection, the dimension/pixel-count guards, and the two-way encode-and-compare -- and its README's module table is updated to match;RawImageitself is unchanged (no new field) -- the palette is detected transparently from pixel content rather than supplied by the caller.Fixes #981
Test plan
pnpm exec turbo run _build _lint _typecheck _test _test:workers _test:smoke --filter=byte-codec --filter=pdf-codec --filter=documents.js-- all greenpng-encode.test.tscovers: small-palette round-trip, per-pixel transparency via tRNS, a defined-but-fully-opaque alpha plane, the 256-colour boundary, the 257-colour truecolour fallback, grayscale images never becoming indexed, a high-colour-count image with alpha falling back to truecolour+alpha, and thenonefilter option on the indexed pathit.eachregression table covers zero, negative, NaN, fractional, Infinity, and 2^31-ceiling width/height values, plus a pixel-count ceiling that rejects a width/height pair individually within range whose product isn'timages-read.test.tsmirrors the same width/height and pixel-count checks against a PDF image dictionary, asserting a diagnostic andundefinedrather than a thrown error, and separately against a JPXDecode codestream whose SIZ marker declares an oversized width/heightdocuments.jsbuild+test also verified green (a downstream consumer of byte-codec'sencodePngin its own test-support fixtures)