fix: stop the codecs losing, mis-parsing and over-allocating - #49
Conversation
67ec158 to
b29920c
Compare
b29920c to
5781fe0
Compare
IAmJSD
left a comment
There was a problem hiding this comment.
This review was written by Claude (Fable 5), acting on Astrid's behalf.
This is genuinely good codec work — every commit's claim checks out, the tests are real round-trips through the public read/write API, the descriptor/affinity hardening is strictly safer than the old desync-and-truncate behaviour, the adjustment-layer encoder is a tested true inverse of the parser, and the 16-bit import/export and probe fixes (WebP-in-RIFF, BigTIFF) are correct. One blocker and one major keep it from merging as-is:
Blocker — preserved-block padding mismatch loses the blocks it preserves (crates/codec-psd/src/writer/mod.rs:290): the writer pads each document-level preserved block with b.pad_to(4) — alignment to the absolute file offset — while the reader (reader/layers.rs:130) skips (4 - len % 4) % 4 bytes relative to the payload length. They disagree whenever a block doesn't happen to start 4-aligned, which depends on layer-name lengths, channel data sizes, and the (now-preserved, possibly odd-length) global layer mask — so real files hit it constantly. Reproduced: a doc with two preserved blocks (Patt, lnk2) and a layer named "ab" (whose luni shifts alignment by 2) round-trips to just [Patt] — the reader's tolerant trailing-garbage break silently discards everything after the first block, and Photoshop would misparse the stream the same way. Fix is two lines: emit (4 - len % 4) % 4 pad bytes per block, as the per-layer extras loop already does relatively.
Major — smart-object sources are quantized to 8-bit (crates/codec-psd/src/smart.rs:59): write_smart serializes via pixel(x, y).to_u8(), so a 16-bit image placed as a smart object in a 16-bit document silently loses 8 bits per channel on save/reopen — contradicting both the block's purpose ("keep their source pixels") and this PR's own bit-depth commit, which fixes exactly this loss class in the importers.
Smaller items:
- Preserved blocks always re-emit an
8BIMsignature, but the reader accepts8B64; an8B64block with a key outsidePSB_U64_KEYSis read with a u64 length and re-written with a u32 one — not byte-exact for PSBs, anddata.len() as u32would silently truncate a >4 GiB block. - The PR body says the TIFF ICC profile "is asserted on the bytes" in a test — no such test exists in this diff (the encoders do implement
set_icc_profile, so the code works, but the claim is unbacked). Value::Unknowninpsd-descriptoris now a never-constructed dead variant; remove it.- The old
blit_rgba8doc comment now dangles aboveblit_rgba_f32(core/src/document.rs:1024), stale RGBA8 sentence included. - Commit 5c002f6 bundles the unrelated (correct) colormgmt soft-proof fix with the PSD fill-opacity fix, and the body's "png/jpeg dropped icc profiles entirely" describes #48, already on main.
With the padding fix and the smart-object depth either fixed or explicitly accepted as a known limitation, this is a merge. Happy to re-review quickly.
b7e91af to
f9851da
Compare
|
fixed |
IAmJSD
left a comment
There was a problem hiding this comment.
Re-reviewed by Claude (Fable 5) on Astrid's behalf. All findings addressed: the preserved-block padding is now relative and symmetric with the reader (with an alignment-sweep test across all four residues — nice), smart-object sources round-trip losslessly via the v2 f32 payload with v1 still readable, the 8B64/u64 truncation path is fixed, the TIFF/WebP ICC assertions exist, the dead variant and doc-comment splices are gone, and the soft-proof revert is byte-identical to main (the fix now lives in #50 as advertised). Two non-blocking notes for later: the smart-object decompress bound's "guard against gigabytes" comment is optimistic now that v2 samples are 16 bytes (~6.4 GB transient worst case from a hostile zlib payload), and storing f32 even for 8-bit sources quadruples the block on disk — the version field makes a per-depth width a compatible future change. Merging.
groups eight branches: what the psd and affinity codecs lose, mis-parse or refuse to admit.
data silently dropped on save
Pattpatterns,lnk2linked smart objects,Txt2,FMsk) and per-layer blending ranges ("Blend If"). open a photoshop file with any of them, save, and they were gone — while the readme says every block is preserved byte-for-byte.Layer::smart, which the writer never serialized, so after save-and-reopen a smart object was a plain raster of its last rasterization and every further transform degraded it. now written to a privateScSoblock, the same trick the type tool plays withPsTx.params_json, which no psd reader understands.parsing that desynchronised or over-allocated
depth through the non-psd codecs
(icc profiles for png and jpeg landed separately in #48; what is left here is webp and tiff, plus depth.)
ExportOptions::bit_depthwas consulted only to pick the dither level, never to choose an output depth.what consolidation caught
merging the icc work with the bit-depth work meant routing 16-bit export through
ImageEncoder::write_image(&[u8], ..), which reads samples back as native-endian. the first attempt wrote big-endian and every sample came out byte-swapped — invisible in either branch alone, caught byexport_honours_the_requested_bit_depth.one thing left alone
the audit also says 16-bit psd samples should be normalised over
0..=32768rather than0..65535. the psd reader's own comment argues the opposite, and only a photoshop-authored fixture settles it; guessing halves or doubles the brightness of every 16-bit file. the false claim incrates/colorthat a conversion happens at the codec boundary is fixed; the numbers are untouched and the open question is documented.cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace— 651 passed, 0 failed.how this relates to my other open prs
these seven are independent of each other — each branches off
mainand each is green on its own. they do share files with my five open prs (#36, #38, #42, #44, #46), mostlyworkspace.rs, so whichever lands first will leave the others needing a rebase. happy to rebase in whatever order suits you, or to split any of these further if one is too big to review in a sitting.