Add source color primaries metadata to image assets - #25472
Add source color primaries metadata to image assets#25472stuartparmenter wants to merge 4 commits into
Conversation
38755bd to
f761e25
Compare
b39f6e3 to
2d37554
Compare
2d37554 to
671f841
Compare
The LinearRec2020 section describes a type this branch no longer adds, the primaries module and HDR-safe clamps shipped upstream with their own release content, and the image source-primaries section is superseded by the note that PR bevyengine#25472 carries. What remains is the opt-in Rec.2020 working color space, so the note is renamed to match.
Adopt the PR branch's refined implementation: PNG cICP support, the reworked KTX2 data-format-descriptor read, Image::from_buffer and ktx2_buffer_to_image taking a source_primaries override, from_chromaticities taking RgbPrimaries, and the PR's release content. Branch-only HDR work in the same files stays: the SMAA HDR pipeline key, the float screenshot conversion, and the tonemapping LUT caller just gains the new argument.
JMS55
left a comment
There was a problem hiding this comment.
Technically sound, but docs need some work.
Also, I think we should talk to the image-rs people about trying to get this upstreamed, seems like code that would be better off living with them long term.
| /// Returns `None` when the chunk is absent, the header cannot be parsed, or the | ||
| /// primaries are not supported. Unsupported primaries warn once, and so does a PQ or | ||
| /// HLG transfer function, since the data is loaded as if it were sRGB-encoded. | ||
| pub(crate) fn png_source_primaries(bytes: &[u8]) -> Option<SourceColorPrimaries> { |
There was a problem hiding this comment.
Really feels like we should try to upstream this stuff into the image crate...
There was a problem hiding this comment.
I agree, I can file an upstream issue and look at it, but don't want to block
| reflect(Default, Debug, Clone, PartialEq, Hash) | ||
| )] | ||
| #[cfg_attr(not(feature = "bevy_reflect"), derive(TypePath))] | ||
| pub enum SourceColorPrimaries { |
There was a problem hiding this comment.
Can you write some documentation explaining in simple terms what color primaries are, and then link to the wikipedia page or another resource explaining it in more detail? We shouldn't assume people looking at this type have any idea what a color primary or white point is is :)
There was a problem hiding this comment.
would it be OK to link to the recently added
bevy/crates/bevy_color/src/primaries.rs
Line 98 in 17e28cd
There was a problem hiding this comment.
Oh hmm. Yeah it would be, but why can't we use that directly?
There was a problem hiding this comment.
The renderer needs a discrete label it can hash/compare/match on, for example in pipeline keys, and the loader settings need stable names in .meta files, which is what SourceColorPrimaries is -- RgbPrimaries is a struct of chromaticity floats focused on colormetric data. Not sure it makes sense to force one in to the other
| /// set's value. Files write primaries with three or four decimal places, so `2e-3` | ||
| /// absorbs that rounding. The supported sets all differ by at least `0.09` in some | ||
| /// coordinate, so a file can never match two sets. | ||
| const CHROMATICITY_MATCH_TOLERANCE: f32 = 2e-3; |
There was a problem hiding this comment.
I rewrote the comment to explain it more or less in these terms:
Basically its a helper for EXR and Radiance HDR files -- other files have basically enums.
A Radiance HDR file can carry a header line like this:
PRIMARIES= 0.640 0.330 0.300 0.600 0.150 0.060 0.3127 0.3290
Those eight numbers pin down which exact red, green, blue, and white the file's RGB values are mixing between -- two numbers per color, locating it on a standard color chart. EXR files can carry the same eight numbers in a chromaticities attribute. Each of the three sets we support (BT.709, BT.2020, Display P3) is defined by its own well known eight numbers, so identifying the file's set is just asking: whose numbers does this line match?
The catch is that these are decimals written by whatever program made the file, and programs round differently. The standard white point is 0.3127, but plenty of writers emit 0.313. Same intent, but a straight == would fail. Allowing each number to be off by up to 0.002 absorbs the rounding.
And 0.002 can't pick the wrong set, because the three sets aren't remotely close to each other: any two of them differ by at least 0.09 in one of the eight numbers, about 45 times the tolerance. A file's numbers can only ever be near one of them.
|
Question: How do you plan to utilize the image color primaries information? In shaders, it is impossible to attach primaries information to every texture for performance reasons. |
I have planned, but haven't actually built, tested, and profiled the answer to your question yet -- was wanting to avoid making the already large HDR branch even bigger. Today the full branch carries the the metadata through to GpuImage, but then nothing reads it. Under the (upcoming) wide working space, every color texture is assumed to be Rec.709-authored, and the composed color converts once at the end of composition. The future plan (that I will build out once more of the PRs have made progress and the dust has settled) is roughly:
I can open a issue and add it to my HDR tracking issue if you want to discuss approaches there on how to best do it, would love any input! |
Objective
An image asset does not record the gamut its colors came from. Once the working color space widens, every existing texture shifts saturation, and nothing can tell which textures were already wide-gamut.
Solution
Image::source_primariesfield andSourceColorPrimaries, a closed set ofBt709,Bt2020andDisplayP3.colorPrimaries, the PNGcICPchunk, RadiancePRIMARIES=header lines, and the OpenEXRchromaticitiesattribute.Bt709. The glTF 2.0 spec mandates it, and KHR_texture_basisu requires the same from KTX2 files.source_primariesloader setting to override it per asset, in code or in a.metafile. Order is setting, then file metadata, thenBt709. The setting also flows throughImage::from_buffer, so an override skips the file read and its warnings in every format.is_srgb. Each warning names the fix, either a loader setting or re-encoding the file.Image { .. }literals in the repo.This is metadata only. Nothing reads the field yet, so decoding and rendering are unchanged. The future working color space PR (from the HDR series) carries the value into
GpuImage, and a follow-up will use it to convert wide-gamut textures instead of assuming every texture is Rec. 709.Design notes
SourceColorPrimariesis a closed set of named gamuts, not a second copy ofbevy_color::RgbPrimaries. The renderer needs a discrete label it can hash, compare, and match on, for example in pipeline keys, and the loader settings need stable names in.metafiles.RgbPrimariescannot provide either, since it is a struct of chromaticity floats. It stays the colorimetric data you convert through, andto_rgb_primariesbridges the two.There is no
Unspecifiedvariant. A file that declares nothing is treated as sRGB. That is what most tools assume, and it matches howis_srgbalready works onImage.The metadata reads never repeat the pixel decode. The EXR read parses only the header, and the PNG read stops at the first image-data chunk and skips ICC profiles, so the extra cost stays proportional to the header, not the file.
Testing
bevy_imageandbevy_gltf, including new ones for PNGcICPparsing and the loader-setting override.cargo check -p bevywith png, ktx2, hdr, exr, gltf and serialize, and themipmap_generatorworkspace member compiles.cargo check -p bevy_imagewith the ctt and basis-universal saver features, combined and universal-only.cargo clippyandcargo fmt --check.Migration
Imagegains a field and has a manualDefault, so exhaustiveImage { .. }literals need it.Image::from_bufferandktx2_buffer_to_imagegain a trailingsource_primaries: Option<SourceColorPrimaries>parameter. It only sets the stamped metadata, and withNonethe file's own metadata wins. See_release-content/migration-guides/image_source_color_primaries.md.This PR was built by me with the assistance of Claude Code w/ Fable 5