Skip to content

JPEG decoder will generate wrong pixel colors sometime #155

Description

@sgjsakura

Description

The JPEG Decoder will generate pixel colors with slight changes compared with the original color.

Steps to Reproduce

Using the following simple code to reproduce the problem, here the test image "test1.jpg" can be downloaded via http://galnote.com/StaticFiles/attachments/1703/djqanebq.jpg

var inputFile = @"image\test1.jpg";

using (var image = new Image(inputFile))
using (var accessor = image.Lock())
{
	var c = accessor[1000, 555];
}

The above code will get the color of the pixel at (1000, 555) location, and the value of c will be RGB(249,243,222). However the actual color in this point should be RGB(250, 243, 224). This problem will further image processings such as saving it using SaveAsBmp method. However Photoshop and Micorosft Paint app can correctly decode the pixels.

System Configuration

  • ImageSharp version: 1.0.0-alpha4-00048
  • Other ImageSharp packages and versions: None
  • Environment (Operating system, version and so on): Windows 10 Build 1607 with Jan 2017 Update
  • .NET Framework version: .NET Core App 1.1 (the version of Micorosft.NETCore.App is 1.1.1)
  • Additional information: None

Activity

  1. changed the title [-]JPEG decoder will generate run pixel colors sometime[/-] [+]JPEG decoder will generate wrong pixel colors sometime[/+] on Mar 26, 2017
  2. tocsoft commented on Mar 26, 2017

    @tocsoft
    Member

    I guess will be this is due to the fact that this image has an ICC profile associated with it.

    Currently we don't have ICC profile support, but we are currently actively working on getting it in.

  3. blackcity commented on Mar 26, 2017

    @blackcity

    @tocsoft We had a similar issue in our testing. I don't think it is an ICC issue, because ICC is non-destructive, so the underlying pixel is the same with or without ICC profile, only the monitor display color changes. I didn't test the image, but I bet if we open it in Photoshop and remove the ICC profile we will always get the same color values for this pixel.

  4. JimBobSquarePants commented on Mar 26, 2017

    @JimBobSquarePants
    Member

    I'd like to do a proper test against the supplied image comparing multiple decoders to see what the result is. There may be more variance than we think.

  5. antonfirsov commented on Mar 26, 2017

    @antonfirsov
    Member

    Thanks for the detailed feedback!
    There are several rounding conversions in the Jpeg Decoder code resulting in cumulated roundoff errors bigger than necessary.
    I'm planning to do a floating-point-all-the-way refactor for Jpeg decoder; @JimBobSquarePants will implement a Vector4-based pixel type. Let's see the results after these changes have been implemented!

    However I'm afraid we can't provide 100% numerical correctness. because ImageSharp codebase depends heavily on floating point SIMD operations.

  6. JimBobSquarePants commented on Mar 27, 2017

    @JimBobSquarePants
    Member

    @sgjsakura @antonfirsov There's something more to this.

    I've been doing some experimenting in LinqPad with the following methods: An equivalent of our shifting method and a known, accurate formula.

    Given an input YCbCr with the following values.

    byte Y = 243;
    byte Cb = 117;
    byte Cr = 133;
    

    We get the following results:

    1. Shifting rgb (250, 243, 224)
    2. Accurate rgb(250, 244, 223)

    As you can see, our method is slightly inaccurate but nowhere near the level of inaccuracy that the later pixel value is. This means we must be somehow introducing error elsewhere.

    public void ToRGBShift(byte y, byte cb, byte cr)
    {
    	int ccb = cb - 128;
    	int ccr = cr - 128;
    
    	// Speed up the algorithm by removing floating point calculation
    	// Scale by 65536, add .5F and truncate value. We use bit shifting to divide the result
    	int r0 = (int)((1.402F * 65536) + .5F) * ccr; // (1.402F * 65536) + .5F
    	int g0 = (int)((0.344136F * 65536) + .5F) * ccb; // (0.344136F * 65536) + .5F
    	int g1 = (int)((0.714136F * 65536) + .5F) * ccr; // (0.714136F  * 65536) + .5F
    	int b0 = (int)((1.772F * 65536) + .5F) * ccb; // (1.772F * 65536) + .5F
    
    	byte r = (byte)(y + (r0 >> 16)).Clamp(0, 255);
    	byte g = (byte)(y - (g0 >> 16) - (g1 >> 16)).Clamp(0, 255);
    	byte b = (byte)(y + (b0 >> 16)).Clamp(0, 255);
    
    	Color.FromArgb(255, r, g, b).Dump();
    }
    
    
    public void ToRGB(byte y, byte bcb, byte bcr)
    {
    	double cb = bcb - 128;
    	double cr = bcr - 128;
    
    	byte r = (byte)(Math.Round(y + (1.402 * cr), MidpointRounding.AwayFromZero).Clamp(0, 255));
    	byte g = (byte)(Math.Round(y - (0.344136 * cb) - (0.714136 * cr), MidpointRounding.AwayFromZero).Clamp(0, 255));
    	byte b = (byte)(Math.Round(y + (1.772 * cb), MidpointRounding.AwayFromZero)).Clamp(0, 255);
    
    	Color.FromArgb(255, r, g, b).Dump();
    
    }
  7. sgjsakura commented on Mar 27, 2017

    @sgjsakura
    Author

    @antonfirsov So I would like to know how Photoshop and MS Paint do to decode the pixels, I don't think they both rely on the same system library, (Photoshop should has its own decoder and encoder implementations), and we should also be able to get the same result if they can do it since they are using the same CPU and its instuctions set are identical.

  8. JimBobSquarePants commented on Mar 27, 2017

    @JimBobSquarePants
    Member

    So I would like to know how Photoshop and MS Paint do to decode the pixels

    Wouldn't we all! 😄

    The maths are pretty simple all in all, we just need to find a better middle ground between speed and accuracy than we have just now.

  9. sgjsakura commented on Mar 27, 2017

    @sgjsakura
    Author

    @JimBobSquarePants thank you for your reply, And for my understanding the direct implementation way is simple but slow, and you are making some accelations with errors inroduced? Yep the differences for pixels is slight, however for image processing targeted applications, these slight differences may also have a huge effect. I'm not saying we should copy the code of Photoshop since I believe coding for sciencific computation is a piece of cake for all your guys, however the method how Photoshop balances between effeiciency and accuracy may be more useful for ImageSharp :-)

  10. antonfirsov commented on Mar 27, 2017

    @antonfirsov
    Member

    I'm quite sure the Jpeg refactor I'm planning will bring benefits in both performance and accuracy.
    It's a lot of work though, and my list is pretty long. :(

  11. JimBobSquarePants commented on Mar 27, 2017

    @JimBobSquarePants
    Member

    @antonfirsov Don't let this stress you out my friend. We have plenty of time and I'm sure more people will join in to help out. 😄

  12. antonfirsov commented on Mar 27, 2017

    @antonfirsov
    Member

    Yeah, I'm planning to open a "JpegDecoder Improvements" issue, summarizing all the Jpeg Decoder related TODO-s, so anyone who wants to join the project could pick it ;)

  13. JimBobSquarePants commented on Apr 27, 2017

    @JimBobSquarePants
    Member

    Update:

    I've just pushed code that makes our color conversion code the same as libjpeg and ran some tests decoding the image and saving it as a bmp.

    I'm still getting differences and the color conversion code is now not a contributing factor. I tested this by running both our fast and a high-precision version of the conversion code against the image.

    I'm wondering whether it has something to do with our float/byte conversion when doing our DCT transform?

    @antonfirsov I think I might be onto something with the float/byte conversion. Upping this to 127.5F get's us closer.

  14. antonfirsov commented on Apr 27, 2017

    @antonfirsov
    Member

    Yeah, maybe I could rethink the rounding logic here one day with a clean afternoon brain :)

  15. antonfirsov commented on Sep 16, 2017

    @antonfirsov
    Member

    @sgjsakura our Jpeg accuracy has been improved a lot with beta-1 (now available on NuGet). Do you find the results good enough now?

  16. JimBobSquarePants commented on Nov 23, 2017

    @JimBobSquarePants
    Member

    I'm gonna close this. Our testing shows a massive improvement

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions