Skip to content

Conversation

@brianpopow
Copy link
Collaborator

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 Tiff images with the CieLab color space.

@brianpopow brianpopow requested a review from a team May 31, 2022 19:14
where TPixel : unmanaged, IPixel<TPixel>
{
// Note: The image from MagickReferenceDecoder does not look right, maybe we are doing something wrong
// converting the pixel data from Magick.NET to our format with CieLab?
Copy link
Member

Choose a reason for hiding this comment

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

What's happening here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The image Tiff\CieLab.tiff looks wrong when using the MagickReferenceDecoder:
output

It should look like this:
expected

ImageMagick itself can decode this image just fine, that's why I thought we maybe doing something wrong in MagickReferenceDecoder.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right.... Yep,

IMagickImage<T> contains ColorSpace enum which we are blindly ignoring when converting. I always assumed they did the same as us and converted color spaces on decode.

It looks like we should be checking that first and converting on the fly.

@dlemstra Am I correct here?

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Just checked the ref vs input. Looks great! 👍 Nice work!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants