Skip to content

Add support for OpenEXR image format#3096

Open
brianpopow wants to merge 81 commits intomainfrom
bp/openExr
Open

Add support for OpenEXR image format#3096
brianpopow wants to merge 81 commits intomainfrom
bp/openExr

Conversation

@brianpopow
Copy link
Copy Markdown
Collaborator

@brianpopow brianpopow commented Mar 29, 2026

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This PR adds support for decoding and encoding of images in the OpenEXR format. OpenEXR is a HDR image format, therefore I have added two new pixel formats Rgb96 and Rgba128 which store each color channel as UInt32.

The specification can be found here: https://openexr.com/en/latest/index.html#openexr

A reference implementation can be found here: https://github.com/AcademySoftwareFoundation/openexr.git

# Conflicts:
#	src/ImageSharp/Configuration.cs
# Conflicts:
#	src/ImageSharp/Formats/Bmp/BmpConstants.cs
#	src/ImageSharp/Formats/Bmp/BmpDecoder.cs
#	src/ImageSharp/Formats/Bmp/BmpFormat.cs
# Conflicts:
#	src/ImageSharp/Configuration.cs
#	src/ImageSharp/Formats/Bmp/BmpArrayFileHeader.cs
#	src/ImageSharp/Formats/Bmp/BmpConstants.cs
#	src/ImageSharp/Formats/Bmp/BmpFormat.cs
#	src/ImageSharp/Formats/Bmp/BmpImageFormatDetector.cs
#	src/ImageSharp/Formats/ImageExtensions.Save.cs
#	src/ImageSharp/Formats/Pbm/BufferedReadStreamExtensions.cs
#	tests/ImageSharp.Tests/ConfigurationTests.cs
#	tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs
# Conflicts:
#	src/ImageSharp/Configuration.cs
#	src/ImageSharp/Formats/Bmp/BmpArrayFileHeader.cs
#	src/ImageSharp/Formats/Bmp/BmpConstants.cs
#	src/ImageSharp/Formats/Bmp/BmpDecoder.cs
#	src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs
#	src/ImageSharp/Formats/Bmp/BmpFileHeader.cs
#	src/ImageSharp/Formats/Bmp/BmpFormat.cs
#	src/ImageSharp/Formats/Bmp/BmpImageFormatDetector.cs
#	src/ImageSharp/Formats/Bmp/BmpInfoHeader.cs
#	src/ImageSharp/Formats/ImageDecoderUtilities.cs
#	src/ImageSharp/Formats/ImageExtensions.Save.cs
#	src/ImageSharp/Formats/Pbm/BufferedReadStreamExtensions.cs
#	src/ImageSharp/Image.Decode.cs
#	tests/ImageSharp.Tests/ConfigurationTests.cs
#	tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs
# Conflicts:
#	src/ImageSharp/Formats/Bmp/BmpFormat.cs
#	src/ImageSharp/Formats/ImageDecoderUtilities.cs
#	src/ImageSharp/Formats/ImageExtensions.Save.cs
#	src/ImageSharp/Formats/OpenExr/ExrDecoderCore.cs
# Conflicts:
#	src/ImageSharp/Configuration.cs
#	src/ImageSharp/Formats/ImageDecoderUtilities.cs
#	tests/ImageSharp.Tests/TestUtilities/TestEnvironment.Formats.cs
@JimBobSquarePants
Copy link
Copy Markdown
Member

@brianpopow I'll review this asap.

/// <param name="component">The 32 bit component value.</param>
/// <returns>The <see cref="byte"/> value.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static byte From32BitTo8Bit(uint component) => (byte)(component * Scale32Bit);
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf is maybe not a primary concern at this point but I wonder if there is a cheaper way to correctly calculate this that avoids FP conversion? How does the reference implementation calculate this (if they care about decoding into lower res format)?

Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brianpopow actually, I don't understand this. From32BitTo8Bit(uint.MaxValue) == 1. Are you sure this logic is correct? If it's incorrect, I wonder why tests didn't catch it? Wouldn't (byte)(component >> 16) do the job?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov Yes I think you are right, this does not look right.

I wonder why tests didn't catch it?

There are no pixel conversion tests yet for the new pixel types. I will try to add tests for the new pixlel types Rgb96 and Rgba128 next.

Wouldn't (byte)(component >> 16) do the job?

Yeah that's more like it, but I think it should be (byte)(component >> 24)? Lets say we have for example Int.Max / 2, e.g. 2147483647. Converting this to 8 bit should be 127.

Copy link
Copy Markdown
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of places where the return value from stream.Read(...) is not checked. I may have missed some, it is worth a comprehensive check.

@JimBobSquarePants
Copy link
Copy Markdown
Member

@brianpopow I'll be able to focus on a proper review this week.

}

/// <inheritdoc/>
public override ExrCompression Method => ExrCompression.Zip;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

/// </para>
/// </summary>
[StructLayout(LayoutKind.Sequential)]
public partial struct Rgba128 : IPixel<Rgba128>, IEquatable<Rgba128>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean our processes are all actually lossy?

PixelAlphaRepresentation.Unassociated);

/// <inheritdoc/>
public readonly Rgba32 ToRgba32() => throw new NotImplementedException();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to implement all these.

See https://github.com/ImageMagick/ImageMagick/commit/27a0a9c37f18af9c8d823a3ea076f600843b553c
-->
<PackageReference Update="Magick.NET-Q16-AnyCPU" Version="13.10.0" />
<PackageReference Update="Magick.NET-Q16-AnyCPU" Version="14.11.1" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you got this working!! Fantastic!! 🙌

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to upgrade to the latest version of Magick.NET, because of an issue decoding some exr files, which was fixed here

Unfortunately this also changed how some bitmaps which are RLE compressed are decoded. I had to change some testcases to use CompareToReferenceOutput instead of using the reference decoder: 0c3ca3d

for (int i = 1; i < predicted.Length; i++)
{
int d = (predicted[i] - p + 128 + 256) & 255;
p = predicted[i];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use ref here


if (totalRead == 0)
{
ExrThrowHelper.ThrowInvalidImageContentException("Could not read zip compressed image data!");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just break here? Normally we try to return as much pixel information as we can.


namespace SixLabors.ImageSharp.Formats.Exr.Compression;

internal abstract class ExrBaseDecompressor : ExrBaseCompression
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know these types are internal but I'm finding it very useful currently to document the crap out of everything (AI really helps here.)

@brianpopow
Copy link
Copy Markdown
Collaborator Author

@brianpopow I'll be able to focus on a proper review this week.

@JimBobSquarePants: dont worry keep your time, it is not urgent.

Just as a heads up: I noticed issues with the pixel conversion methods introduced with the new pixel types. I am trying to add tests for them in the next days. Those conversion methods will change, so maybe exclude them from a review until then, because they will change anyway.


PixelComponentInfo? maybeComponentInfo = info.ComponentInfo;
Assert.NotNull(maybeComponentInfo);
PixelComponentInfo componentInfo = maybeComponentInfo.Value;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

formats:exr Any PR or issue related to OpenExr file format

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants