-
Notifications
You must be signed in to change notification settings - Fork 621
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
Metadata #1448
base: main
Are you sure you want to change the base?
Metadata #1448
Conversation
The fields of Metagram are now public, since it is not intended that they do keep invariants relative to one another. The documentation also clarifies the relation to the actual processing applied by the decoding. The SharedRecorder now has the same basic set of methods for adding meta data as the original Recorder. Added documentation and some examples.
How should something like png's sRGB chunk show up here? Also why |
Maybe enum ColorSpace {
SRGB,
GammaAndChromaticity(GammaAndChromaticity),
ICC(Vec<u8>),
Unknown
}
struct GammaAndChromaticity {
gamma: f32,
white: XY,
red: XY,
green: XY
} |
Color profile refers to a standardized ICC profile and we do not (yet) want to interpret it. Its internal format is a TIF-like directory structure encoded which takes quite a bit to decode fully. This can also be done afterwards so I didn't want to task the decoder itself with it. Encoding one from the png chunks is easier :) |
Yeah, I'm familiar with ICC profiles (I wrote qcms). I wouldn't recommend having the decoders construct ICC profiles for color spaces that have a simpler representation. If an application ever wants to know or display the name of a color space it's lot easier do that with a named constant vs trying to reconstruct it from an ICC profile. For example, if you want to avoid the cost of color conversion it's a lot easier to compare names than it is ICC profiles. FWIW, here are some examples of other applications representing color spaces by name: |
Good to know, thanks for the input. Out of all the image formats, AVIF's representation seems most reasonable with regards to extensibility (although the enum approach as proposed by you could rule out some invalid representations). It introduces quite a lot of new enums for the representation. I already have a draft for this as well, for the description of texels to be specific, in a private project. Would you say it should be part of this PR, then I'd add it here. |
Yeah, the AVIF approach seems reasonable. JPEG-XL uses something similar: |
We can choose two ways here: Add a new field with he 'decoded' color space, or the approach implemented currently which is to have an enum which presents an alternative.
/// ITU Rec.2020 with 10 bit quantization. | ||
Bt2020_10bit, | ||
/// ITU Rec.2020 with 12 bit quantization. | ||
Bt2020_12bit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Bt2020_10bit use a different transfer function than Bt2020_12bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not mathematically, but the quantization is different. Anyways having both of them is straightup copied from the transfer_characteristics
field of AV1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Color
handling part of this looks quite plausible to me, though I'm not an expert on color spaces so I can't actually say whether it is correct in that regard.
I'm less convinced by the Recorder
aspect. Have you considered alternatives like say an ImageDecoder::read_image_metadata
function that returned the image data and matadata at the same time?
|
||
/// Color information of an image's texels. | ||
#[derive(Clone, Debug, PartialEq)] | ||
pub enum Color { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be more clear as ColorSpace
?
Renamed main struct to MetadataContainer, integrated configuration into the recorder with documentation.
#[allow(missing_copy_implementations)] | ||
pub struct RecorderConfig { | ||
/// Whether the decoder should try to provide color profile data. | ||
pub color_profile: DatumRequested, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason this couldn't be pub skip_color_profile: bool
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it took me a while to make up my own mind. My vague idea was to have a third variant, Required
, that would fail decoding when no information is available. This could allow decoders to fail fast instead of needing to wait for the final check which would complement Limits
nicely. However, there are some pain points:
- We will have to add a check after the decoder in any case if we want to make the an actionable guarantee for the caller. But when those checks trigger we will merely discard already correct data which is rather odd and mostly detrimental.
- If we have any sort of recovery or fallback then we need to relax it to
Requested
in any case with similar concerns as above. - There might be overlapping constraints such as 'ensure a correct, complete and accurate record of all color conversion and quantization steps in the image creation pipeline' which wouldn't fit in the tri-state idea in any case.
- They can always be converted to a flag on the recorded
MetadataContainer
. Consider the above then we could add abool
attribute that signals the completeness of the color information in other fields.
This leaves only code clarity as a potential benefit of an explicit new enum.
Adds a common interface for decoding meta data.
The
Metagram
struct, storing all meta data, is partly public and has aDefault
implementation that should be interpreted as having no meta data. The goal is that the metadata can later again be used by encoders, and in any case there are no invariants between the fields. Also note that EXIF and ICC are treated as opaque blobs for now. We do not yet interpret the color conversion and source information given in them to inform the conversion to thergb
ish types ofDynamicImage
¹.This considers two decoder implementations: Either the data is immediately available or it will be supplied during the decoding process. In the first case the decoders has already decoded all data from a header and stores it directly into a
Recoder
borrowed from the caller while in the second case they want to keep an instance for the following decoding call, realized with aSharedRecorder
that provides thread-safe shared access to a single target.The design also considers recursive use in that a
Recorder
can share the target of aSharedRecorder
. This allows any decoder, holding on to aSharedRecorder
, to create a new argument that can be passed to another decoder'sImageDecoder::metagram
method.The combined encoding-decoding/decoding-encoding process may be lossy in all instances, for the moment it is a best-effort. There is no attempt at adding any impls in this PR.
¹Reasoning: We don't have library integration for this yet. Furthermore, it would affect downstream users to change the output color types too suddenly. Instead, my traint of thought is to establish that in the future both original and output color spaces will be defined in
Metagram
and make those fields as accurate as possible. That preparse decoders for the flexibility necessary to transparently handle a change in the decoding interpretation, but the discussion on this is not part of the PR.