Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jpeg loaded with pink color blur #2174

Closed
4 tasks done
teamnimbus opened this issue Jul 14, 2022 · 7 comments · Fixed by #2177
Closed
4 tasks done

Jpeg loaded with pink color blur #2174

teamnimbus opened this issue Jul 14, 2022 · 7 comments · Fixed by #2177

Comments

@teamnimbus
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have verified that I am running the latest version of ImageSharp
  • I have verified if the problem exist in both DEBUG and RELEASE mode
  • I have searched open and closed issues to ensure it has not already been reported

ImageSharp version

2.1.2

Other ImageSharp packages and versions

2.1.3, 3.0.0-alpha.0.11, 3.0.0-alpha.0.21

Environment (Operating system, version and so on)

Windows 10 21H1 x64

.NET Framework version

.Net 6.0

Description

We stumbled on a Jpeg image that gets loaded with wrong colors on all versions after 2.1.1. Tested with 2.1.2, 2.1.3, 3.0.0-alpha.0.11 and 3.0.0-alpha.0.21.
This is very similar to issues #2133, #2141 and #2152. However we don't think it's a duplicate since it's not fixed in either 2.1.3 (like #2133) or 3.0.0-alpha.0.11 (like #2141 / #2152).

Steps to Reproduce

This NUnit test passes on 2.1.1 and fails on all later versions with "Expected: Rgba32(255, 255, 255, 255) But was: Rgba32(255, 128, 128, 255)".

[Test]
public async Task ImageShouldNotBePink()
{
	var response = await new HttpClient().GetAsync("https://www.mottafashionplace.com/media/catalog/product/1/0/10085001_11.jpg");
	await using var imageStream = await response.Content.ReadAsStreamAsync();

	var image = Image.Load<Rgba32>(imageStream);

	Assert.That(image[0, 0], Is.EqualTo(new Rgba32(255, 255, 255, 255)));
}

Images

We don't own the problematic image so we can't distribute it. You can download it and use it as a starting point to create another image with the same properties.

@brianpopow
Copy link
Collaborator

The image has 3 components with no subsampling, that's why ImageSharp deduces the colorspace to RGB. It seems it should be YCbCr instead, but I am not sure how we can come to this conclusion from the components info's. The component Id's are 0, 1 and 2.

I wish the jpeg spec would define the color space properly. This guessing the colorspace from the component info's really gets out of hand.

@br3aker
Copy link
Contributor

br3aker commented Jul 14, 2022

The image has 3 components with no subsampling, that's why ImageSharp deduces the colorspace to RGB. It seems it should be YCbCr instead, but I am not sure how we can come to this conclusion from the components info's. The component Id's are 0, 1 and 2.

Actually it should be ycbcr regardless of the components' ids: 3 components without APP14 marker is YCbCr. RGB colorspace is defined by App14 marker with flag=0.

AFAIR we had this check did we change it some time ago?

P.S.
First time seeing [0, 1, 2] components' ids. It's usually [1, 2, 3] for ycbcr or [82,71,66] for rgb. Most likely we have some colorspace deduction based on ids.

@brianpopow
Copy link
Collaborator

Actually it should be ycbcr regardless of the components' ids: 3 components without APP14 marker is YCbCr. RGB colorspace is defined by App14 marker with flag=0.

@br3aker: The colorspace is determined as RGB, because it has 3 components and no subsampling, but it also has no APP14 marker. So this seems wrong then. It was introduced with #2124, with commit: 89668a6.

@br3aker
Copy link
Contributor

br3aker commented Jul 14, 2022

So this seems wrong then. It was introduced with #2124, with commit: 89668a6.

I guess we just need to remove final if check here and return ycbcr as a fallback colorspace:

if (this.Components[2].Id == 3 && this.Components[1].Id == 2 && this.Components[0].Id == 1)
{
return JpegColorSpace.YCbCr;
}
JpegThrowHelper.ThrowNotSupportedColorSpace();
}

@br3aker
Copy link
Contributor

br3aker commented Jul 14, 2022

Oh wait, it's actually an app14 code path.

Colorspace deduction is done here:

private JpegColorSpace DeduceJpegColorSpace(byte componentCount)

IMO we should remove any id-based deduction:

if(has App14)
{
    // current logic works fine
}
else
{
    // spec doesn't say anything about id-based color deduction
    // [1, 2, 3] == YCbCr worked before but image from this issue
    // has [0, 1, 2] ids and yet is completely valid according to the spec
    // AFAIR adobe spec about jpeg colorspace defines RGB colorspace
    // only if App14 is present
    return ColorSpace.YCbCr;
}

@JimBobSquarePants
Copy link
Member

Was just reading through the decoder there as the condition looked incorrect. Agreed on the fix.

@br3aker
Copy link
Contributor

br3aker commented Jul 15, 2022

I'll open PR today.

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

Successfully merging a pull request may close this issue.

4 participants