Skip to content
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.

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

Add nclx->icc colour management to heifload #3912

Open
mertalev opened this issue Mar 27, 2024 · 24 comments
Open

Add nclx->icc colour management to heifload #3912

mertalev opened this issue Mar 27, 2024 · 24 comments

Comments

@mertalev
Copy link

mertalev commented Mar 27, 2024

Bug report

HDR AVIF input images look very desaturated in the output image. This happens for both srgb and rgb16 pipeline colorspaces and both srgb and p3 target ICC profiles. It also occurs when outputting to any of JPEG, WebP, AVIF or PNG (these are the outputs formats I tested).

To Reproduce
Steps to reproduce the behavior:

  1. Use the raw0016-positive.avif image from this zip
  2. Convert to any of JPEG, WebP or PNG
  3. Note the desaturated colors

Expected behavior

When outputting to JPEG or WebP, the colors should be somewhat representative of the input image (minus the HDR, of course). When outputting to 10-bit AVIF, it should have roughly the same colors including HDR.

Actual behavior

The output image is unnaturally desaturated.

Screenshots

Apple Preview:

raw0016-positive-preview

libvips:

immich-thumbnail

Environment
(please complete the following information)

  • OS: Debian Bookworm
  • Vips: 8.15.2
  • sharp 0.33.3
  • libheif 1.15.1 (also tried a build with 1.17.6 with same results)

Additional context

libheif has a convert_hdr_to_8bit setting that's referenced here, but doesn't seem to be used anywhere.

@mertalev mertalev added the bug label Mar 27, 2024
@jcupitt
Copy link
Member

jcupitt commented Mar 27, 2024

Hi @mertalev,

You two images above ^^ look identical on firefox on linux, but different in firefox on macos, interestingly.

libvips uses ICC profiles for colour management, but your image has no ICC profile. I would guess it is using nclx for encoding HDR, and that is (as far as I know) only supported by heif. I tried in imagemagick6 and 7, and they both gave the same result as libvips.

We could add nclx support, but that would be a huge amount of work. Maybe you could save your image with an ICC profile? It might at least be less work to support.

@jcupitt
Copy link
Member

jcupitt commented Mar 27, 2024

... you're right, we could also add an sdr (for example) option to heifload which enabled convert_hdr_to_8bit. That might be a simple fix.

@mertalev
Copy link
Author

It does apparently use nclx:

Color Representation            : nclx 12 16 6

A flag to enable convert_hdr_to_8bit sounds good! The only thing this doesn't handle is HDR -> HDR, but we can avoid converting in that case and just strip metadata.

@mertalev
Copy link
Author

Oh, but would this still let us use a Display P3 profile in the output image? Or would it have to be sRGB?

@jcupitt
Copy link
Member

jcupitt commented Mar 28, 2024

It'd become a regular SDR image, yes. All the extra HDR bits would be removed.

I think you'd need to switch to an ICC workflow and encode with 10 or 12 bits to get HDR with a P3 profile. Your test image has no profile attached.

Though I'm a bit unclear on the details -- I've never actually tried this. I ought to get an HDR display.

@mertalev
Copy link
Author

For context, this is for a photo backup / library app where users import images that get compressed, resized and stripped to serve more quickly. On our end, the goal is to make the most of what we're given when converting these images.

Looking into this a bit more, NCLX seems to be popular for this format - all the online examples for HDR HEIC/AVIF images I could find use it. I wonder if there's a straightforward way to convert NCLX to ICC? I could find this that uses liblcms2, but it's beyond my knowledge haha

@jcupitt
Copy link
Member

jcupitt commented Mar 28, 2024

Oh, that's a good find! It looks like they have this code:

https://github.com/mnaydenov/FreeImage-Sidecar/blob/master/src/PluginHEIF.cpp#L98-L169

Which is pretty simple and should be easy to add to libvips.

Testing would be a PITA though. I have a mac mini, which has quite good HDR support, but of course it's plugged into a non-HDR display. Perhaps I can persuade libvips.org to buy me a HDR monitor ...

@mertalev
Copy link
Author

If it helps, I'm happy to test a branch with the changes!

@jcupitt jcupitt added enhancement and removed bug labels Mar 28, 2024
@jcupitt jcupitt changed the title HDR AVIF input images are outputted as flat and desaturated Add nclx->icc colour management to heifload Mar 28, 2024
@jcupitt
Copy link
Member

jcupitt commented Mar 28, 2024

Let's make this into an enhancement issue and make a test PR.

@Starbix
Copy link

Starbix commented May 9, 2024

I've started coding a bit and wanted to share my findings.

Setting the correct ICC profile (Display P3; SMPTE ST 2084 PQ) using this in vips_foreign_load_heif_set_header works great and you get a JPG that looks very close to how it should. I used the ICC profile Apple provides.

vips_image_set_blob(out, VIPS_META_ICC_NAME,
			(VipsCallbackFn) vips_area_free_cb, data, length);

Thus generating an ICC profile from the NCLX data seems to be the way to go. colorist does exactly that. Simply using colorist convert in.avif out.jpg results in a veeeery dark image, due to forcing the max luminance to 10'000 nits (only for PQ, not sure why). However force-omitting it (or a lower value around 100 nits) results in very good results.
EDIT: according to this presentation, 203 nits should be used

libheif presents a nice API to read out the relevant information of the nclx triplet. The values of this triplet are defined in H.273.

I can look into implementing this more cleanly than what I have right now.
The code in already looks really good, but it's not handling HDR cases (PQ or HLG curves). For these, colorist has the curves in the source code as binary data. Either we copy this (I'm not quite sure where those curves come from initially) or we use a parametric curve like in the Apple profile. However the Apple profile also includes "Intent-0, device to PCS conversion table" tags which may be required in conjunction with the simple parametric tone response curve and I don't know how to replicate those with LCMS2.

I've included a zip file with some relevant results and ICC profiles.
nclx_shenanigans.zip

EDIT: I've emailed the author of colorist and his response was very helpful to understanding more about the choices colorist made. The curves definitely need to be in the repo. He created them himself from the spec.

@jcupitt
Copy link
Member

jcupitt commented May 10, 2024

Oh very nice @Starbix, I'll try to have a look this weekend!

@Starbix
Copy link

Starbix commented May 12, 2024

Embedding the corresponding ICC works, but if exporting AVIF/HEIC or any other image format capable of NCLX tags we should instead opt for setting the same NCLX tags again. This leads to smaller file sizes and better support for certain viewers. I've observed that Apple Preview handles AVIFs with the generated PQ ICC well, whereas mpv doesn't, since it doesn't detect HDR without the NCLX tags. As far as I understand here's also no good way to signal PG/HLG in ICC profiles and the way colorist handles it is a bit hacky.

While I don't have a great overview over the image pipeline of libvips, I imagine that's a bit harder than the proposed embedding of the equivalent ICC profile.

@jcupitt
Copy link
Member

jcupitt commented May 12, 2024

We can probably do both. Maybe:

  1. heifload reads the nclx tags, makes and attaches an ICC profile, and also attaches the nclx tags.
  2. The operations that transform images with icc profiles will remove the profile once it becomes out of date. We could remove any nclx tags at the same time.
  3. A saver for a format that supports nclx writes with nclx tags if available, otherwise with an icc profile.

I'm a bit surprised that ICC profiles don't have useful HDR support, but I don't know much about it. Maybe we should ask an ICC profile expert for an opinion? Marti Maria (lcms author) is usually very helpful.

@joedrago
Copy link

Hello -- I'm the author of colorist and libavif and I'd like to clear up some confusion here, as I'm clearly responsible for the error. 😄

For these, colorist has the curves in the source code as binary data.

This is bad/wrong; please don't do this. Let me explain.

When I first wrote colorist, any decision I made with regards to PQ and HLG in an ICC profile was simply to try to fit a square peg in a round hole, not to follow any standard, but specifically to abuse it. I needed a means to signal PQ and maxLum in a file that I could then read back, so I chose a specific binary payload (alluded to above) for a curve, and to abuse the informational-only lumi tag for maxLum. Neither of these paths will behave anywhere other than in colorist or its direct derivatives (like the viewer Vantage).

The three ICC profiles in colorist's docs/profiles subdir were generated by a color expert over at Adobe and use iccMAX-only tags (A2B0, B2A0) to competely override the typical primaries/curve type tags in an ICC file, and effectively approximate/mimic a PQ curve with the assumption that your reference white is 100 nits. The payloads are also 18 kilobytes, which should be a non-starter for any embedding into an image format.

The only real standards-honoring, supported, and reasonable way to signal PQ in a modern file right now is by setting the CICP transfer_characteristics value to 16 (ST 2084, aka PQ). CICP is short for the beginning of the name of the h.273 standard "Coding-independent code points", and is implicitly referred to in the discussion above as nclx, which is the tag used to signal the ColourInformationBox in any BMFF-derived file. AVIFs are HEIFs are BMFFs, so by including an nclx box in the right spot, associated with the right items in an AVIF will be 100% honored by modern AVIF viewers like Chrome.

I use CICP instead of nclx when referring to h.273 values stored in an nclx box (colour_primaries, transfer_characteristics, matrix_coefficients) not only because that is where those enums are declared/defined, but also because nclx isn't the only way to signal these as of recent updates(!). The next PNG standard is adding a cICP chunk which stores the exact same values as a BMFF nclx box, which is quite friendly, but only helps you with PNGs. ICC profiles themselves are also proposing to add a cicpType tag, which would also allow for super cheap signaling (in file size).

TL;DR - Do not attempt to signal PQ or HLG by generating or embedding any curves; they are pretty much nonsense hallucinations I made while R&D'ing proper HDR support. Instead lean heavily into CICP signaling, and over time, implement all possible reads/writes for where to find these values (nclx, PNG cICP, ICC cicpType). Video formats have been signaling color profiles with these 3 enums for a while now, and they're right. This is the way!

@Starbix
Copy link

Starbix commented May 13, 2024

Thanks for chiming in and clearing up some stuff! As far as I can see JPG does not support signalling cICP values right? Thus we'd need to tonemap the original HDR image to SDR for JPG. (ignoring gain maps/Ultra HDR)
Probably according to some algorithms mentioned here: whatwg/html#9112 (comment)

@joedrago
Copy link

The only way you'll see CICP show up in a JPEG will be via a cicpType tag inside of an ICC profile in the JPEG, which is something we'll eventually see commonly used, I hope. That said, JPEGs are only 8bpc anyway, so it isn't the greatest choice for any HDR just due to precision. PQ was designed around 12bpc, and then due to the realities of memory constraints (fitting a pixel into 32 bits), stole 6 bits from what is typically alpha and went 10bpc and is "good enough". 8bpc won't look great for such a large luminance range anyway, so yes, you'll want to convert to SDR. You could certainly retain the color primaries though and leave it as Wide Color Gamut (WCG). Having a P3 or BT.2020 JPEG with a more typical/boring transfer function is quite reasonable; you don't have to convert all the way to (say) SRGB just because it is a JPEG, although many will just want SRGB.

I haven't personally used libvips yet -- does it support generic color profile conversion already, or is this a whole new avenue to implement?

@Starbix
Copy link

Starbix commented May 13, 2024

Yep that makes total sense. However Google's jpegli somehow can squeeze 10+bits out of JPEGs. The end-device obviously also needs to support that, so it's not really practical.

@joedrago
Copy link

Heh yeah, the original libjpeg source simply has a define for bits per channel and you can make it values other than 8 and it'll roundtrip data just fine, but it is a compile-time choice that the whole world sets to 8. Any library out there that implements its own JPEG can be quite flexible here, but as you mentioned, you're probably asking for trouble compatibility-wise.

@joedrago
Copy link

I've added a really loud warning to colorist just now which points back at this discussion in case anyone else runs into this.

@jcupitt
Copy link
Member

jcupitt commented May 13, 2024

Modern libjpegs let you select 12 bit encoding at runtime, and I expect you could encode HDR somehow in the ICC profile. But the number of applications that would be able to read it as intended must be near zero haha.

@jcupitt
Copy link
Member

jcupitt commented May 13, 2024

I haven't personally used libvips yet -- does it support generic color profile conversion already, or is this a whole new avenue to implement?

Yes, it uses lcms2 for colour management, so all V4.4 profiles should work.

@mm2 could I ping you on this? What's the state of HDR support in lcms2?

@mm2
Copy link

mm2 commented May 13, 2024

Hi @jcupitt nice to contact you again.
Current version of lcms does support 16 bits and floating point, using that latter it can deal with values over 1.0 for highlights. All features in ICC v4 are supported. A different thing is if ICC is prepared for full HDR, some profiles can do the trick, but they are pretty rare.

@jcupitt
Copy link
Member

jcupitt commented May 14, 2024

Thank you @mm2. We're hoping to make HDR profiles automatically from the nclx tags on formats like AVIF. It sounds like it should just work if we use algebraic LUT specification plus a matrix.

@mm2
Copy link

mm2 commented May 14, 2024

As long as you use matrix-shaper with parametric tone curves (those that are specified by a formula) the profile will be HDR-ready, You could use cmsCreateRGBProfile() to create such kind of profiles

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants