Jpeg compressed tiff: jpeg decoder should handle the conversion from YCbCr to RGB

[brianpopow]

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 provides a fix for #2123: If the image uses Jpeg Compression, the jpeg decoder should handle the conversion from YCbCr to RGB and not the Tiff Decoder, See discussion in #2123

[JimBobSquarePants]

@brianpopow You're not gonna like this but since it's such a critical bug we'll have to backport this PR to the release/2.1.x branch.

[JimBobSquarePants]

@brianpopow This looks to be how libjpeg-turbo figures it out.

https://github.com/libjpeg-turbo/libjpeg-turbo/blob/1f55ae7b0fa3acc348a630171617d0e56d922b68/jdapimin.c#L127

[brianpopow]

I think it is working now as it should. I had to change how the jpeg decoder deduces the colorspace. That's why this should be reviewed carefully.

It is close to how libjpeg does it, but not exactly the same. libjpeg checks for a JFIF marker first and then concludes it should be YCbCr. We first check for adobe with a color transform and if the component Id's are ASCII R, G, B. The reason for that is, that there is a test image Tiff\rgb_jpegcompressed_nojpegtable.tiff, which has a JFIF marker, but the component Id's indicate it's in fact RGB.

@br3aker if you have time to review this part, your feedback would be very much appreciated.

[br3aker]

Ups, completely missed this, will review tomorrow!

[br3aker]

Strictly speaking, oracle docs are not accurate according to jpeg specs. We can have 2 different 'versions' for the jpeg image - jfif and adobe jfif (exif of whatever it's called). So we can have 2 different paths:

1. Have jfif marker
2. Have app14 adobe marker

But we can't have both at the same time according to the specs. I'd suggest to write it like this:

if (componentCount == 1)
{
    return grayscale;
}

if (componentCount == 3)
{
    // we prioritize adobe marker over jfif marker
    // if somebody really encoded this image with redundant adobe marker
    // then it's most likely an adobe jfif image IMO
    if (this.adobeMarker is not null)
    {
        if(this.adobeMarker.Transform == ycbcr) return ycbcr;
        else if(this.adobeMarker.Transform == unknown) return rgb;

        // throw if color transform flag has invalid value?
    }

    if (this.jfifMarker is not null)
    {
        return ycbcr;
    }

    // fallback to the id color deduction
    return this.Components[2].Id == 66 && this.Components[1].Id == 71 && this.Components[0].Id == 82 ? rgb : ycbcr;
}

if (componentCount == 4)
{
    // jfif images don't not support 4 component images
    // so we only check adobe
    if (this.adobeMarker is not null)
    {
        if(this.adobeMarker.Transform == ycck) return ycck;
        else if(this.adobeMarker.Transform == unknown) return cmyk;

        // throw if color transform flag has invalid value?
    }

    // fallback to cmyk as neither of cmyk nor ycck have 'special' component ids
    return cmyk;
}

JpegThrowHelper.ThrowNotSupportedComponentCount(componentCount);
return default;

P.S.
I personally don't like component id color deduction as ids are used to determine components in scans, they have nothing to do with encoding color. AFAIR 'RGB' ASCII ids were proposed by Adobe before jpeg extension via APP14 marker was published.

[brianpopow]

@br3aker thanks for your feedback, I have changed the color deduction accordingly.

For Tiff\rgb_jpegcompressed_nojpegtable.tiff: I am pretty sure this was wrongly encoded (by myself probably). The jpeg data was not following the spec. This is changed now.

[JimBobSquarePants]

If @br3aker is happy with this then I am 😄

@brianpopow I'm gonna create a PR using these files against the release/2.1.x branch and once it's built will merge both.

[brianpopow]

If @br3aker is happy with this then I am 😄

@brianpopow I'm gonna create a PR using these files against the release/2.1.x branch and once it's built will merge both.

Thanks. I was a bit unsure how to do it, never made a release.

[antonfirsov]

I was a bit unsure how to do it, never made a release.

After merging this PR, we should rebase all the commits in this PR on release/2.1.x and do a separate "backport" PR. We may also consider squashing them into a single commit, if that makes things simpler.

@JimBobSquarePants I'm not sure what happens after merging the backport PR, is it enough to just tag the merge commit with 2.1.2 or are there any manual steps you have to perform? How does MinVer figure out that our next patch=2 in the package version?

[antonfirsov]

@JimBobSquarePants is faster doing than me talking 😆

[JimBobSquarePants]

Haha 😄 Yeah, See #2126

It should be enough to PR against the release branch then create a tagged release via the UI here ensuring we are pointing at the correct branch to create the release from.