transform: optionally carry the source JPEG's ICC color profile - #505
Open
datasage wants to merge 1 commit into
Open
transform: optionally carry the source JPEG's ICC color profile#505datasage wants to merge 1 commit into
datasage wants to merge 1 commit into
Conversation
image/jpeg sends every APPn marker but APP0 and APP14 to decoder.ignore, and Encode writes no APP segments at all, so a JPEG's embedded ICC profile does not survive the decode/encode round trip. The output is then rendered as sRGB, which visibly desaturates images authored in a wider gamut: measured against a correct color-managed render at /x80/, a Display P3 source is 3.64% RMSE off and an Adobe RGB source 4.93%. With the profile carried over both drop to ~0.3%, which is JPEG quantization noise. Add a passthroughICC flag, off by default, following the same Proxy -> Options path as scaleUp. It is opt-in because the profile's bytes are added to every response that carries one -- commonly 0.5-4KB, which is a large relative cost on a small thumbnail. The profile is only reattached where the decoded pixels are still in the space it describes. YCbCr is converted to RGB with the fixed JPEG matrix, which is color-space agnostic, so an RGB source's triplets stay in its own primaries; a CMYK or YCCK source is converted by naive inversion, so its profile no longer describes the result. Hence the guard: source and output both JPEG, profile data color space RGB, decoded image not CMYK or Gray, and profile no larger than 64KB. Fixes willnorris#160 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Open
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
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.
Fixes #160.
I fixed this issue downstream in my own project by injecting a patch into my source build. Making this available upstream if there is interest.
The problem
image/jpegsends everyAPPnmarker except APP0 (JFIF) and APP14 (Adobe) todecoder.ignore, andEncodewrites no APP segments at all. So a JPEG's embedded ICC profile cannot surviveDecode→transformImage→Encode: transformed output beginsff d8 ff db— SOI straight into DQT.An untagged JPEG is rendered as sRGB, so anything authored in a wider gamut comes back desaturated. Measured against a correct color-managed render at
/x80/:The residual ~0.3% is JPEG quantization noise. Sample pixel from the Adobe RGB source: correct
148,0,133, currently served as126,0,128, and148,0,132with the flag on.The change
A
passthroughICCflag, off by default, following the sameProxy→OptionspathscaleUpalready uses (Proxy.PassthroughICC→req.Options.PassthroughICC, with a matching option token that is likewise not documented in the options list).envygives itIMAGEPROXY_PASSTHROUGHICCfor free.Off by default because the profile's bytes land on every response that carries one — commonly 0.5–4KB, which is a large relative cost on a small thumbnail (+33% on an
x80, ~+3% atx600). That trade-off felt like the operator's call rather than a default, which is why this is a flag rather than unconditional behavior.Implementation is
icc.go: walk the source's marker segments, reassemble theICC_PROFILEAPP2 chunks, and splice them back in after SOI on the encoded output. Pure stdlib, no new dependencies, no cgo.The guard is the interesting part
Reattaching a profile is only correct when the decoded pixels are still in the space it describes. YCbCr → RGB uses the fixed JPEG matrix, which is color-space agnostic, so an RGB source's triplets stay in its own primaries and the profile still describes them. A CMYK/YCCK source is different — the decoder converts it by naive inversion, so its profile no longer describes the result and copying it over would make things worse, not better.
So passthrough requires all of: source and output both JPEG, profile data color space
RGB, decoded image not*image.CMYK/*image.Gray, profile at least a full 128-byte ICC header and no larger than 64KB.The 64KB ceiling does double duty — it keeps a large LUT profile off a thumbnail, and it bounds reassembly, since a crafted source can declare 255 chunks of 65519 bytes.
extractICCProfilesums the chunk lengths and bails before allocating rather than building ~16MB for the guard to discard. Happy to change that number or make it configurable if you'd rather.This deliberately does not address #213 (CMYK), which needs real color conversion rather than passthrough, or PNG output, which would need an
iCCPchunk.Testing
go test ./...passes. Six tests added covering the helpers and the flag end to end:Transformitself with the flag off and on, asserting the profile is dropped and preserved respectivelyI mutation-tested the new guards rather than trusting coverage — removing the flag check, the size cap, or the header-length check each makes the suite fail rather than pass silently.
Verified end to end through a built container against a local origin as well: Display P3 and Adobe RGB sources keep their profile, and CMYK, grayscale, and profile-less sources come back byte-identical to current output.
Process notes
docs/contributing.mdasks that new functionality be discussed in an issue first. I took #160 as that discussion — it describes this exact defect and has been open a while — but this is a PR against an 8-year-old bug rather than a feature nobody asked for, so if you'd rather hash out the flag name, the default, or the 64KB cap before reviewing code, I'm happy to move it back to the issue.I've left
docs/changelog.mdalone: entries there carry merge commit SHAs and PR numbers, so it reads as something you curate at release time rather than per-PR. Say the word and I'll add an[Unreleased]entry.golangci-lint v2.11.4(the version pinned inlinter.yml, with the repo's.golangci.yml) reports 0 issues, andgo vet ./...andgo test ./...are clean.