Skip to content

Conversation

@br3aker
Copy link
Contributor

@br3aker br3aker commented Jul 16, 2022

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

Fixes #2174

@brianpopow
Copy link
Collaborator

There were 3 failing tiff tests, but I believe it was an issue with the Tiff JpegDecompressor. Hopefully this 1959bf6 fixes it now.

@br3aker
Copy link
Contributor Author

br3aker commented Jul 16, 2022

There were 3 failing tiff tests, but I believe it was an issue with the Tiff JpegDecompressor.

I believe it's not not that simple :D

TIFF with PhotometricInterpretation=RGB can have YCbCr signature at jpeg decoding level which would lead to invalid results.
Always using Rgb24 converter would lead to YCbCr encoded jpeg having RGB color conversion. I guess I have an idea: we can just pass PhotometricInterpretation to new TiffSpectralConverter and resolve color converter using that value.

}

/// <summary>
/// Returns the correct colorspace based on the image component count and the jpeg frame component id's.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment is no longer valid, maybe change it like this:

Suggested change
/// Returns the correct colorspace based on the image component count and the jpeg frame component id's.
/// Returns the correct colorspace based on the image component count and adobe APP14 marker's color transform flag.

private JpegColorSpace DeduceJpegColorSpace(byte componentCount)
internal static JpegColorSpace DeduceJpegColorSpace(byte componentCount, ref AdobeMarker adobeMarker)
{
if (componentCount == 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have found a jpeg image with an app14 marker which is gray, should that not be possible according to the spec?
gray-sample

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adobe spec doesn't say anything about grayscale images but I think we can include it as decoding it doesn't violate the spec :)

@br3aker
Copy link
Contributor Author

br3aker commented Jul 16, 2022

I've implemented tests for the specific method which resolves encoded jpeg color space. Not only it's easier to maintain, it is also much faster and there's no need to find a ton of images with different setups.

This is ready to be merged after the review.

[InlineData(3, JpegConstants.Adobe.ColorTransformYCbCr, JpegColorSpace.YCbCr)]
[InlineData(4, JpegConstants.Adobe.ColorTransformUnknown, JpegColorSpace.Cmyk)]
[InlineData(4, JpegConstants.Adobe.ColorTransformYcck, JpegColorSpace.Ycck)]
internal void DeduceJpegColorSpaceAdobeMarker_ShouldReturnValidColorSpace(byte componentCount, byte adobeFlag, JpegColorSpace expectedColorSpace)
Copy link
Member

Choose a reason for hiding this comment

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

This is all very nice 👍

@JimBobSquarePants JimBobSquarePants added this to the 3.0.0 milestone Jul 18, 2022
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.

This all looks great! Thanks @br3aker

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.

Jpeg loaded with pink color blur

3 participants