IMG.c: Loader now keeps trying other formats, if other supported format(s) failed to load an image

[Retro52]

Hi

This PR makes IMG.c keep trying to load an image if another format fails.

The reason I even had this problem comes from trying to load this WebP image on iOS (I could not attach it to PR as GitHub does not support this format)

For some reason, this image was recognized as a TIF format image by the UTTypeConformsTo. I tried replacing it with the UTTypeEqual, but got the same result. The image load was failing because the TIF format check was performed before checking against WEBP.

While the problem I had is likely some rare edge case, I believe it would still be a good thing to try to handle such rare hiccups.

[slouken]

This means that if we fail to load an image the error will always be "Unsupported image format" and we lose any informative error set by the loader.

[slouken]

The TIFF image format is well defined, I'm not sure why this RIFF/WEBP image would be detected as TIFF.

[Retro52]

The TIFF image format is well defined, I'm not sure why this RIFF/WEBP image would be detected as TIFF.

I suspect that the image is, maybe, too small, but that's not the bug that SDL could fix.

[slouken]

We could also move TIFF to the bottom of the list.

Did you see this comment?

This means that if we fail to load an image the error will always be "Unsupported image format" and we lose any informative error set by the loader.

[Retro52]

We could also move TIFF to the bottom of the list.

Did you see this comment?

This means that if we fail to load an image the error will always be "Unsupported image format" and we lose any informative error set by the loader.

Yep, just thinking if there is a nice way to handle that :)

[Retro52]

Moving TIFF to the bottom would probably fix my issue; However, I'm a little bit worried that it may cause others.

What's your opinion on that? Moving TIFF to the bottom is the easiest way, but I could spend some more time & fix the SDL_GetError() issue in my PR.

[slouken]

Fixing the GetError() issue is probably the best route.

[Retro52]

GetError() issue is now fixed by tracking how many load attempts we've made.
Btw, I tried to follow the coding standard of the file, but if there is something you'd like to change, just let me know

[slouken]

You actually need to save off the error in the case where an image load was attempted, as the error might be overridden by a subsequent is() call.

[Retro52]

Did not account for that, sry
Fixed (?)

[slouken]

Looks good. We need to do the same in IMG_LoadAnimationTyped_IO()

[sezero]

I'm not 100% comfortable with this patch

[Retro52]

I can see why, it may be too much to fix, possibly, one small edge case

We are going to keep the original version of it anyway, cause without it, I'd have to go and manually find all the broken images & convert them to other formats, like PNG.

On the other hand, I hate to modify original source code, and since Apple is (almost) definitely not going to do anything about it, I thought that maybe an official fix is possible

[Retro52]

Hi again.
When I initially addressed this issue, I was in a rush, so I didn't have time to thoroughly investigate it. As a result, my initial solution was simply the first approach that came to mind - not ideal.

Recently, I had some free time and decided to spend a few hours looking deeper into the problem to find a more robust solution.

I narrowed the issue down to the Internal_isType function. For some reason, my WebP image was detected as a "public.tiff". Initially, I suspected that this issue might relate to the comment within the function itself, which mentions that the container type could differ from the image type. However, after some experimentation, it became clear that there isn’t an easy way to retrieve the actual image type directly from an index. I couldn't get CGImageSourceCopyPropertiesAtIndex to provide such value, and it seems to be the only proper getter function for images within a container. The only alternative seems to be creating an image via CGImageSourceCreateImageAtIndex, and check for the result - stupidly inefficient, but fixes the problem (kind of :) ).

It seemed very weird that no one else had reported this problem, so I tested another idea: maybe the issue was caused by the hint container provided before creating the image source:

CFDictionaryRef hint_dictionary = CreateHintDictionary(uti_string_to_test);
CGImageSourceRef image_source = CreateCGImageSourceFromIOStream(rw_ops, hint_dictionary);

My idea was that (for whatever reason) the provided hint might confuse the CGImageSourceCreateWithDataProvider function, resulting in an incorrectly configured image source that reports an incorrect image type.
In this case, provifing nil instead of the hint options would solve the problem.
To my surprise, doing just that eliminated all false-positives, and an application ran without any issues.

Quick summary: the root issue appears to be on the Core Graphics side, where providing hint options can lead to incorrect image type detection. For SDL_Image to handle this edge case, it might be best not to provide any hints to CreateCGImageSourceFromIOStream from Internal_isType, letting Core Graphics determine the image format on its own.

If you think this fix is appropriate, please let me know. I'll update this PR or open a new one.

[slouken]

Yes, this sounds like a better approach. Thanks for looking deeper into it!

[slouken]

We probably need to remove the hint dictionary from both places so the detection and loading match.

[Retro52]

Well, I'm not sure, but providing this hint, when loading, does no harm, and may serve as an additional optimization for CGImageSourceCreateWithDataProvider - I'd assume this way the function would first check if the hinted format is correct, and skip other checks.

But if you'd prefer consistency over this I'll, of course, indulge and make the change)

[slouken]

In the original bug, is the hinted format incorrect?

[Retro52]

In the original bug, is the hinted format incorrect?

Yep, but now is() returns truly correct format (I hope so, at least), so the hinted format is also the true underlying format of the image.

[slouken]

In the original bug, is the hinted format incorrect?

Yep, but now is() returns truly correct format (I hope so, at least), so the hinted format is also the true underlying format of the image.

Ah, that makes sense, thanks!