We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 2788 - color space fix for ImageIO backend (OS X, iOS)
Summary: color space fix for ImageIO backend (OS X, iOS)
Status: RESOLVED FIXED
Alias: None
Product: SDL_image
Classification: Unclassified
Component: misc (show other bugs)
Version: 2.0.0
Hardware: x86_64 Mac OS X 10.8
: P2 critical
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-20 00:10 UTC by Jeffrey Carpenter
Modified: 2014-12-06 12:35 UTC (History)
0 users

See Also:


Attachments
proposed colorspace fix for ImageIO backend (1.02 KB, patch)
2014-11-20 00:10 UTC, Jeffrey Carpenter
Details | Diff
reference test case data (diag_main.txt before patch) (490 bytes, text/plain)
2014-11-20 00:11 UTC, Jeffrey Carpenter
Details
comparison test case data (diag_main.txt after patch) (490 bytes, text/plain)
2014-11-20 00:12 UTC, Jeffrey Carpenter
Details
RGB8888 (24-bit, BMP) test case image (243.12 KB, image/bmp)
2014-11-20 00:12 UTC, Jeffrey Carpenter
Details
ARGB8888 (32-bit, PNG) test case image (4.37 KB, image/png)
2014-11-20 00:13 UTC, Jeffrey Carpenter
Details
test case (for use with test.bmp, test.png to produce diag_main.txt output) (5.77 KB, text/x-csrc)
2014-11-20 00:14 UTC, Jeffrey Carpenter
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeffrey Carpenter 2014-11-20 00:10:37 UTC
Created attachment 1935 [details]
proposed colorspace fix for ImageIO backend

I believe I may have found the origin of a serious bug in the ImageIO implementation of SDL_image, where it inaccurately returns pixel color values that are off by one -- i.e.: R(110), G(143), B(189) instead of R(110), G(144), B(190). I've been using a custom built SDL_image with libpng & friends for the backend to work around this issue.

Cheers,
Jeffrey Carpenter <i8degrees@gmail.com>
Comment 1 Jeffrey Carpenter 2014-11-20 00:11:53 UTC
Created attachment 1936 [details]
reference test case data (diag_main.txt before patch)
Comment 2 Jeffrey Carpenter 2014-11-20 00:12:23 UTC
Created attachment 1937 [details]
comparison test case data (diag_main.txt after patch)
Comment 3 Jeffrey Carpenter 2014-11-20 00:12:45 UTC
Created attachment 1938 [details]
RGB8888 (24-bit, BMP) test case image
Comment 4 Jeffrey Carpenter 2014-11-20 00:13:01 UTC
Created attachment 1939 [details]
ARGB8888 (32-bit, PNG) test case image
Comment 5 Jeffrey Carpenter 2014-11-20 00:14:09 UTC
Created attachment 1940 [details]
test case (for use with test.bmp, test.png to produce diag_main.txt output)
Comment 6 Jeffrey Carpenter 2014-11-20 00:18:52 UTC
Sorry .. I probably should have zipped all that before hand -_- Anyhow ...

Test case resources (should be able to be replaced with your own as long as the pixel format and file encoding matches):
  test.bmp RGB8888 (24-bit, BMP)
  test.png ARGB8888 (32-bit, PNG)

This patch reverts [revision 292](https://hg.libsdl.org/SDL_image/log?rev=292) back to the original author's implementation (Eric Wing). The comments made in this revision seem to contradict what is intended; see CGColorSpaceCreateCalibratedRGB [1] and CGColorSpaceCreateDeviceRGB [2] for Apple's API notes. Also note that the revision comments by "Albert" use a test case that is for use with SDL1 -- how this may change things is unknown to me.

1. https://developer.apple.com/library/ios/documentation/GraphicsImaging/Reference/CGColorSpace/index.html#//apple_ref/c/func/CGColorSpaceCreateCalibratedRGB
2. https://developer.apple.com/library/ios/documentation/GraphicsImaging/Reference/CGColorSpace/index.html#//apple_ref/c/func/CGColorSpaceCreateDeviceRGB

  Additional notes
* Tested on Mac OS X 10.10 "Yosemite".
* Has not been tested against iOS, SDL1 or images that have been processed with pre-multiplied alpha.

Build test case:
  clang++ sdl2_image-colorspace-fix.cpp -o sdl2_image-colorspace-fix -framework SDL2 -framework SDL2_image

Run test case to generate my existing data (my baseline / reference):
(output in diag_main.txt, using test.bmp and test.png)

bitmap(0, 0) = ff6e8fbd
png(0, 0) = ff6e8fbd
sdlbmp(0, 0) = 6e90be

bitmap(31, 0) = ff6e8fbd
png(31, 0) = ff6e8fbd
sdlbmp(31, 0) = 6e90be

<snip remaining debug info>

NOTE: ff6e8fbd = R(110) G(143) B(189)
The actual pixel value is 110, 144, 190. SDL_LoadBMP gets it right in both
cases, whereas IMG_Load (ImageIO backend) gets it wrong on both counts.

Apply revert-rev-292.patch:
  hg import revert-rev-292.patch

Rebuild test case, ensuring that we compile and **run** with the proper runtime
search paths for our patched SDL2_image library (I built mine using Xcode project files):

```sh
# man dyld (1)
DYLD_FRAMEWORK_PATH=<my_local_prefix_lib_path> ./sdl2_image-colorspace-fix
```

Comparison output of reference data with patched library:
(output in diag_main.txt, using test.bmp and test.png)

bitmap(0, 0) = ff6e90be
png(0, 0) = ff6e90be
sdlbmp(0, 0) = 6e90be

bitmap(31, 0) = ff6e90be
png(31, 0) = ff6e90be
sdlbmp(31, 0) = 6e90be

<snip remaining debug info>

NOTE: ff6e90be = R(110) G(144) B(190)
The actual pixel values match using both SDL_LoadBMP and IMG_Load. This is the
expected behavior.

  References
1. [Introduction of ImageIO backend](https://forums.libsdl.org/viewtopic.php?p=14777)
2. [Pixel bug in Mac OS X](https://forums.libsdl.org/viewtopic.php?p=30317)
3. [Perhaps again? Pixel bug in Mac OS X](https://forums.libsdl.org/viewtopic.php?t=10013&sid=34e8ef7cf74074039495037c1a077025)
Comment 7 Sam Lantinga 2014-11-29 20:09:25 UTC
Fixed, thanks!
https://hg.libsdl.org/SDL_image/rev/55801d955e3a

I'm not sure what the problem was with the original device RGB colorspace. I tested this with the default calibration and a tweaked monitor calibration and albert's original test case worked fine here.

Thanks!
Comment 8 Jeffrey Carpenter 2014-11-30 00:20:50 UTC
Yay! You the man! :D

Your comments make me curious ... I'll see if I can get different results by playing around with my display's calibration -- I've got three of them to test on. I'll report back if I find anything. 

When was it that you tested? I'm guessing during round about time of Albert's patch?
Comment 9 Sam Lantinga 2014-11-30 06:29:29 UTC
Albert's original sample test is still live, if you want to grab it. I played with calibration and it didn't affect the test here.
Comment 10 Jeffrey Carpenter 2014-12-06 12:35:11 UTC
Using Albert's sdlimagetest.cpp [1] and the official distributed binaries of SDL_image (AKA Albert's patch), I get the following output:

$ DYLD_FRAMEWORK_PATH=/Users/jeff/Projects/scratch/sdl/sdl2/sdl2_image-colorspace-fix/libs/before ./sdlimagetest
PixelFormat:
  BitsPerPixel: 32,  BytesPerPixel: 4
  R/G/B/A mask: ff0000/ff00/ff/ff000000
  R/G/B/A loss: 0/0/0/0
  Colorkey: 0,  Alpha: 255
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF
 FF0500FF

FF0500FF is RGBA(255, 5, 0, 255). Note that this differs from the expected "bad" output of SDL_image, which expects FF1514F4.

Using Albert's sdlimagetest.cpp [1] with my proposed patch built in SDL_image, I get the following output:

$ DYLD_FRAMEWORK_PATH=/Users/jeff/Projects/scratch/sdl/sdl2/sdl2_image-colorspace-fix/libs/after ./sdlimagetest
PixelFormat:
  BitsPerPixel: 32,  BytesPerPixel: 4
  R/G/B/A mask: ff0000/ff00/ff/ff000000
  R/G/B/A loss: 0/0/0/0
  Colorkey: 0,  Alpha: 255
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF
 FF0000FF

FF0000FF is RGBA(255, 0, 0, 255). Note that this is what Albert's test expected, too, with his patch.

Choosing different color calibrated color profiles at random made no difference whatsoever for me in any of these tests.

I think Albert's test case was built with SDL1 in mind, whereas my tests are with SDL2. My tests also use a newer version of SDL_image than at the revision where Albert's changes were introduced. I don't know how this might change any of the results.

In summary, I don't know either! The conflicting test results leave me ever more... confused, with more questions than answers. Such is often the case :-)

In any case, the fact that my proposed patch doesn't **appear** to introduce problems for either one of us is hopefully a good sign of things. I haven't yet found anybody else to help test this with, but it's only a matter of time! In conclusion, until more information comes to light, I feel OK about my proposed patch.

Thanks,
Jeffrey Carpenter <i8degrees@gmail.com>

1. https://hg.libsdl.org/SDL_image/rev/5953114c0d27