libsdl-org / SDL_image

Image decoding for many popular formats for Simple Directmedia Layer.
zlib License
550 stars 180 forks source link

Automated test failing on macOS #248

Closed smcv closed 2 years ago

smcv commented 2 years ago

When @madebr's branch added coverage on macOS, the automated test that I recently added is failing.

smcv commented 2 years ago

@madebr, if you don't have a Mac where you can run this locally, you could set up the "Upload artifacts" workflow step to run on macOS as well as Linux. You'll get a zip file with the comparison images. Ideally the zip file should have a different name for each test run to avoid race conditions, which would probably mean adding a unique artifacts_name key to matrix.platform or something.

smcv commented 2 years ago

AVIF loading is not quite the same as the reference:

2022-05-20 16:35:24.987 testimage[7044:21439] ERROR: 05/20/22 16:35:24: First detected occurrence at position 0,0 with a squared RGB-difference of 77.

I thought the decoder was deterministic; but if it isn't, then the thing to do is to look at the results, check that they're visually indistinguishable, and if they are, increase the Format.tolerance to some reasonable value like I did for JPEG.

smcv commented 2 years ago

Loading .cur also doesn't match the reference:

2022-05-20 16:35:25.011 testimage[7044:21439] ERROR: 05/20/22 16:35:25: Comparison of pixels with allowable error of 0 failed 947 times.
2022-05-20 16:35:25.011 testimage[7044:21439] ERROR: 05/20/22 16:35:25: First detected occurrence at position 0,0 with a squared RGB-difference of 798.

That looks like a bigger difference - perhaps it's getting decoded incorrectly?

I suspect that decoding .cur is not a major use-case, so changing the CUR test-case to be #if 0 or #ifndef __darwin__ would be a reasonable workaround to get the rest of the tests running.

smcv commented 2 years ago

Actually, a lot of these seem to be getting mis-decoded. Are we perhaps using a special Mac-specific loader implementation?

smcv commented 2 years ago
2022-05-20 16:35:25.145 testimage[7044:21439] ERROR: 05/20/22 16:35:25: Assert 'TIF loading should be enabled': Failed

This one is easy: if the Mac build is not meant to have TIFF support, then we want to only export SDL_IMAGE_TEST_REQUIRE_LOAD_TIF=1 on Linux.

slouken commented 2 years ago

Actually, a lot of these seem to be getting mis-decoded. Are we perhaps using a special Mac-specific loader implementation?

Yes, many image formats on Apple platforms are loaded with ImageIO, but ICO and CUR files should only use that if BMP_USES_IMAGEIO is defined, which I don't think it is, by default.

smcv commented 2 years ago

This should help to determine whether the macOS build is broken, or just needs a bigger tolerance: https://github.com/smcv/SDL_image/suites/6596157923/artifacts/247601499

Corresponding log so you can see what options were used: https://github.com/smcv/SDL_image/runs/6529455984

smcv commented 2 years ago

The AVIF tests (0001 to 0004) look basically the same, so I think they just want more tolerance. Reference followed by 4 test results:

CompareSurfaces0001_Reference bmp CompareSurfaces0001_TestOutput bmp CompareSurfaces0002_TestOutput bmp CompareSurfaces0003_TestOutput bmp CompareSurfaces0004_TestOutput bmp

smcv commented 2 years ago

0009 (loading sample.cur with IMG_Load(), I think) looks like a genuine bug somewhere:

CompareSurfaces0009_TestOutput bmp

Perhaps macOS ImageIO is misidentifying it?

smcv commented 2 years ago

0013 to 0016 (GIF) I can't see an obvious difference, but the log says the top left pixel is different:

CompareSurfaces0013_Reference bmp CompareSurfaces0013_TestOutput bmp

Note that this is the only test-case that uses 8-bit palette images, so maybe the conversion from 8-bit palette to RGBA is going differently on macOS?

smcv commented 2 years ago

0017 (loading sample.ico with IMG_Load()) looks like the same failure more as for .cur:

CompareSurfaces0017_Reference bmp CompareSurfaces0017_TestOutput bmp

smcv commented 2 years ago

0025 (saving and re-loading a JPEG) looks like a genuine bug somewhere:

CompareSurfaces0025_Reference bmp CompareSurfaces0025_TestOutput bmp

smcv commented 2 years ago

0026 looks the same as 0025.

smcv commented 2 years ago

0027 to 0030 (loading .jxl) just need a bit more tolerance. Reference and 4 outputs:

CompareSurfaces0027_Reference bmp CompareSurfaces0027_TestOutput bmp CompareSurfaces0028_TestOutput bmp CompareSurfaces0029_TestOutput bmp CompareSurfaces0030_TestOutput bmp

sezero commented 2 years ago

0025 (saving and re-loading a JPEG) looks like a genuine bug somewhere:

CompareSurfaces0025_Reference bmp CompareSurfaces0025_TestOutput bmp

If saving is done via the tiny_jpg option, https://github.com/libsdl-org/SDL_image/commit/f69cdef846c39ca485672fcae8a78fdd1a3ce3dd was a big patch by me, check with it just in case I screwed up something in there.

smcv commented 2 years ago

0013 to 0016 (GIF) I can't see an obvious difference, but the log says the top left pixel is different

I'm also seeing a difference reported in a Debian autobuilder environment, although annoyingly, not in my development environment where I can inspect the resulting images.

I think testing 8-bit paletted images might have been too ambitious, and I should probably back that part out so that we can rely on the test coverage of the bits that do work.

smcv commented 2 years ago

0009 (loading sample.cur with IMG_Load(), I think) looks like a genuine bug somewhere:

CompareSurfaces0009_TestOutput bmp

257

0025 (saving and re-loading a JPEG) looks like a genuine bug somewhere:

CompareSurfaces0025_Reference bmp CompareSurfaces0025_TestOutput bmp

256

For now, I'm going to work around those two so we can bring test coverage back online.

The AVIF tests (0001 to 0004) look basically the same, so I think they just want more tolerance

I'll add tolerance.

0013 to 0016 (GIF) I can't see an obvious difference, but the log says the top left pixel is different

My code to convert to RGBA for comparison was broken. I'll fix this soon.

0027 to 0030 (loading .jxl) just need a bit more tolerance

I'll add tolerance.

smcv commented 2 years ago

I'm hoping #258 will fix all the parts of this that are not #256 or #257, and work around those two.

smcv commented 2 years ago

258 fixes this. It no longer contains workarounds for #256 or #257, which are fixed already.