Skip to content

Improve scRGB support #3623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Oct 16, 2023
Merged

Improve scRGB support #3623

merged 7 commits into from
Oct 16, 2023

Conversation

jcupitt
Copy link
Member

@jcupitt jcupitt commented Aug 24, 2023 •
edited
Loading

Handling of scRGB has a couple of issues:

  • When converting scRGB to and from sRGB and RGB16 we are changing the image gamma, so any profiles on the image will break. They need to be removed.

  • Conversion for saveable RGB did not handle scRGB

See #3621

Using the test image from the linked issue (a float TIFF saved by photoshop) with this PR I can now run:

$ vips copy tiff7.tiff x.png

To make:

x

Though this makes a black image:

$ vips icc_transform tiff7.tiff x.png srgb

So I suppose icc_transform does not handle scRGB correctly. I'll mark this as a draft until this extra issue is resolved too.

jcupitt added 5 commits August 24, 2023 12:19
@jcupitt
strip icc profile when gamma changes
0d665ba 
When converting scRGB to and from sRGB, the image gamma changes. This
breaks all ICC profiles causing a range of problems downstream.

This patch removes any profile on sRGB <-> scRGB conversion.

See #3621
@jcupitt
handle scRGB in convert for saveable
3a6d39a
@jcupitt
note changes
f15643a
@jcupitt
also drop profile for scRGB <-> RGB16
dc148f9 
... since RGB16 also has a gamma
@jcupitt
Merge branch 'master' into improve-scrgb-support
a497e4a
@jcupitt jcupitt marked this pull request as draft August 24, 2023 13:14
@jcupitt jcupitt changed the title Improve scRGB support WiP: Improve scRGB support Aug 24, 2023
@kleisauke
Copy link
Member

Issue #2858 and its test case may also be relevant here.

@jcupitt jcupitt marked this pull request as ready for review October 11, 2023 10:19
@jcupitt
Copy link
Member Author

jcupitt commented Oct 11, 2023

The test case in #2858 works with this PR:

$ vips colourspace k2.jpg x.v scrgb
$ vips copy x.v x.jpg

@jcupitt jcupitt mentioned this pull request Oct 11, 2023
Closed
@kleisauke
Copy link
Member

test_jxlsave fails as it expects scRGB images to be passed as-is.

libvips/libvips/foreign/jxlsave.c

Lines 526 to 528 in 74adb39

/* This lets throuigh scRGB too, which we then save as jxl float.
*/
save_class->saveable = VIPS_SAVEABLE_RGBA;

Changing that to VIPS_SAVEABLE_ANY fixes that, but also requires other changes to let jxlsave encode arbitrary extra channels (i.e. more than 4), which is only supported since libjxl v0.7.0.

encoder API: ability to encode arbitrary extra channels:
JxlEncoderInitExtraChannelInfo, JxlEncoderSetExtraChannelInfo,
JxlEncoderSetExtraChannelName and JxlEncoderSetExtraChannelBuffer.

@jcupitt
Copy link
Member Author

jcupitt commented Oct 11, 2023

Ah, the icc_import failure is because we don't support linear RGB profiles, we only have 8 and 16-bit device space. I think that this should probably be a separate issue.

@jcupitt
Copy link
Member Author

jcupitt commented Oct 11, 2023

requires other changes to let jxlsave encode

Ugh, this will take a while. We should get #3712 in at the same time.

Shall we park this for 8.16? It would delay release by quite a while.

@kleisauke
Copy link
Member

I had a very quick attempt with commit kleisauke@d7daf54, and it appears to work; however, jxlload is unable to decode these images due to:
https://github.com/libjxl/libjxl/blob/e37516e7a7494f49613ac2cde1b58dbc06e52e62/lib/jxl/decode.cc#L2302-L2304

To decode these extra channels, you apparently also need to call JxlDecoderExtraChannelBufferSize() / JxlDecoderSetExtraChannelBuffer(). A similar issue occurs when trying to decode the CMYK + Alpha test image from the libjxl conformance repository:
https://github.com/libjxl/conformance/blob/master/testcases/cmyk_layers/input.jxl

@kleisauke
Copy link
Member

kleisauke commented Oct 13, 2023 •
edited
Loading

Shall we park this for 8.16? It would delay release by quite a while.

I think this PR would still be useful to have for 8.15. How about this commit?:
kleisauke@59e8b6c

That would mimic VIPS_SAVEABLE_RGBA in jxlsave and still allow it to operate on scRGB images.

To decode these extra channels, you apparently also need to call JxlDecoderExtraChannelBufferSize() / JxlDecoderSetExtraChannelBuffer().

On a somewhat tangent, it seems that the new streaming encode API in libjxl uses a callback-based approach, rather than a 'push'-based tile/scanline approach (used in libraries like libtiff).
https://libjxl.readthedocs.io/en/latest/api_encoder.html#_CPPv426JxlChunkedFrameInputSource

@jcupitt
Copy link
Member Author

jcupitt commented Oct 14, 2023

OK, let's do that, and I suppose leave the jxl load / save reworking for 8.16?

@kleisauke
Copy link
Member

Indeed, let's defer the jxl load/save rework to the 8.16 cycle.

@kleisauke kleisauke changed the title WiP: Improve scRGB support Improve scRGB support Oct 16, 2023
@kleisauke kleisauke merged commit 1130557 into master Oct 16, 2023
@jcupitt jcupitt deleted the improve-scrgb-support branch October 16, 2023 12:57
@kleisauke kleisauke mentioned this pull request Mar 9, 2024
Draft
@jcupitt jcupitt mentioned this pull request Mar 10, 2024
Merged
@DarthSim DarthSim mentioned this pull request Dec 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleisauke kleisauke kleisauke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcupitt @kleisauke