Skip to content

fix: convert ICC colors to sRGB before stripping metadata - #278

Closed
yeagoo wants to merge 2 commits into
VERT-sh:mainfrom
yeagoo:fix/image-color-profiles
Closed

fix: convert ICC colors to sRGB before stripping metadata#278
yeagoo wants to merge 2 commits into
VERT-sh:mainfrom
yeagoo:fix/image-color-profiles

Conversation

@yeagoo

@yeagoo yeagoo commented Sep 7, 2026

Copy link
Copy Markdown

With metadata retention disabled, the existing strip() removes the source ICC profile but leaves its encoded channel values unchanged. A Display P3 image then renders as if those values were sRGB, changing its appearance.

Transform profiled pixels into standard sRGB while the source ICC is available, then strip metadata. Leave the existing profile and pixels intact when metadata is retained. Convert other untagged non-sRGB/non-gray spaces through the engine's color-space conversion while leaving untagged gray data alone.

The destination profile is a bundled 480-byte CC0 sRGB profile; conversion needs no network fetch. Source commit, license and SHA-256 hashes for both the profile and the Display P3 test fixture are included beside the asset.

Validation

Display P3 fixtures are round-tripped through PNG, WebP, JXL and TIFF with both metadata settings. Tests verify actual transformed pixels and source-profile removal or preservation. This addresses appearance changes caused by removing ICC; it does not implement HDR tone mapping or preserve out-of-sRGB gamut in an untagged file.

  • bun run test: 13 passed, including five shared behavior tests.
  • The defect-focused tests fail against the unchanged upstream conversion function (4 failing tests before this fix).
  • bun run build: passed with a local .env based on .env.example.
  • ESLint and Prettier: changed files pass.
  • bun run check: the same 7 pre-existing missing Node type errors as upstream; no additional errors. Full-repository lint has existing failures outside this change; changed files pass.

Patch structure

This branch is based on upstream cc7b5a54d5e9c797b377db47b9bdfbb561707783 and includes shared test harness commit c69e961, which moves the existing magickConvert function unchanged into a utility imported by the worker. Tests invoke that exact production function with the installed WASM. No new package dependency is introduced.

Each repair branch can be reviewed independently. After the shared harness lands, the remaining branches will need rebasing onto upstream main to resolve overlapping edits in magick-convert.ts.

Shares the behavior-preserving test harness with #275.

@JovannMC

Copy link
Copy Markdown
Member

note: this is a boilerplate response for numerous open PRs

hiya!

firstly sorry for the late response to your PR (if it has been open for a while). we are closing this, along with other people's PRs, as we are preparing for the very overdue update drop. once merged, it will most likely break most existing PRs and will need to be redone. track the PR #282, and once merged you may wish to open another PR with the new base.

do also note this repository's and our main contributing guidelines which was made recently here, when submitting your new PR: repo & main

@JovannMC JovannMC closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants