Skip to content

exr: Support reading and writing Luma<f32> images - #3066

Open
upuddu wants to merge 2 commits into
image-rs:mainfrom
upuddu:exr-luma-f32
Open

exr: Support reading and writing Luma<f32> images#3066
upuddu wants to merge 2 commits into
image-rs:mainfrom
upuddu:exr-luma-f32

Conversation

@upuddu

@upuddu upuddu commented Jul 7, 2026

Copy link
Copy Markdown

Closes #3016.

The OpenEXR codec previously only handled RGB/RGBA float images, so saving a Luma<f32> image (e.g. ImageBuffer::from_fn(w, h, |x, y| Luma([...]))) failed with an unsupported-color error.

This adds read and write support for single-channel luma. The encoder writes a single Y channel, as recommended by the OpenEXR specification. The decoder looks for a Y channel and decodes it as Luma<f32>; when a file contains both RGB and Y channels it still prefers RGB/RGBA, so existing behavior is unchanged.

Luma + alpha is intentionally out of scope, as noted in the issue.

Tested with a roundtrip (roundtrip_luma): generate a Luma<f32> image, encode it to an in-memory EXR, decode it back, and compare — mirroring the existing roundtrip_rgb/roundtrip_rgba tests.

@RunDevelopment

RunDevelopment commented Jul 7, 2026

Copy link
Copy Markdown
Member

Given that you made 9 PRs to 8 repos in the last hour, I'll assume that all of this is AI-generated, correct?

If so, then I'm not sure whether we should accept this. "Good first issue" issues aren't supposed to be fodder for AI agents but starting points for humans.

@197g What's your take on this?

@197g

197g commented Jul 7, 2026

Copy link
Copy Markdown
Member

Sure, I'd prefer them to be non-AI but it's not dogmatic either in that I suppose some newer programmers just do that nowadays so that is the shape of their first contribution. I'd say the problem here is the LLM'ism of the code itself. The code needs a bit of structural thought but instead gets pure repetition.

Firstly, LumaA is maybe out-of-scope but that's different from more or less ignoring its existence. That is, if it's not supported to decode that plane and tack it on then there should be an error to avoid behavior change in the future when it gets added. (Note the exclusion is partly an artifact of missing ColorType::LumaA32F which is resolved, it's not a requirement not to do it).

Secondly, I have a parse-don't-validate concern. While parsing the header there's a boolean flag that decides Rgba vs. Luma decoding based on available channels that was tacked on to the existing is_alpha_present flag. I'd rather we have a proper newtype enum representation of the three possible decision instead, so that the one we made here that can be later used instead of re-computing the same in a potential different way and having an error. Planning for non-standard color of alternate names, it'd be interesting to have a type where we can retain information on the channel index when it is parsed.

@upuddu

upuddu commented Jul 7, 2026

Copy link
Copy Markdown
Author

Fair question, and yes I used AI assistance on these. I do review, test, and stand behind what I send, but I hear the concern, especially on good-first-issues. If AI-assisted PRs aren't welcome here, I completely understand - say the word and I'll close it.

On the substance, @197g:

  • LumaA: agreed it shouldn't be silently dropped. I'll return an explicit unsupported-color error for a Y+A file, so adding LumaA later (now that ColorType::LumaA32F exists) is an additive change.
  • parse-don't-validate: I'll replace the boolean-on-is_alpha_present with a small enum that captures the RGB/RGBA/Luma decision once at header-parse time (keeping the channel info), and have the reader consume that instead of re-deriving it.

Update incoming - happy to iterate.

Replace the loose `is_luma`/`alpha_present_in_file` booleans with a
private `ExrColorLayout` enum (Rgb/Rgba/Luma) that captures the color
decision made while parsing the selected header exactly once. The enum
retains the parsed channel indices; `prepare_image` and `read_image`
consume this decision instead of re-inspecting channels, and decoding
resolves those indices to the channel names it reads so the header is
never re-interpreted in a different way.

Detect luma-plus-alpha (`Y` + `A`) images during parsing and return an
unsupported-color error (`La32F`) instead of silently dropping the alpha
plane, so that adding `LumaA<f32>` decoding later is an additive change
rather than a behavior change.
@upuddu

upuddu commented Aug 3, 2026

Copy link
Copy Markdown
Author

@197g Sorry for the long silence - I pushed these changes in e93a978 the same day and then never actually said so here.

Both of your points are addressed:

LumaA is no longer silently dropped. A header carrying Y + A now returns ImageError::Unsupported with UnsupportedErrorKind::Color(ExtendedColorType::La32F), instead of decoding as plain luma and discarding the alpha plane. That way, adding real LumaA<f32> support later is purely additive — no file that decodes today would change meaning.

Parse, don't validate. is_luma and alpha_present_in_file are both gone, replaced by a single ExrColorLayout enum covering exactly the three decisions — Rgb, Rgba, Luma — computed once in with_alpha_preference. The rest of the decoder consumes that decision rather than re-inspecting the channel list, so prepare_image and read_image can't interpret the same header in two different ways. Each variant also retains the resolved channel indices (r/g/b/a/y), per your note about non-standard names and alternate layouts, so future work can build on the parsed positions instead of rescanning.

Happy to rework it if the shape isn't what you had in mind.

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.

Exr: Support reading and writing Luma<f32> images

3 participants