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

only use image loaders which could potentially read file #17368

Merged
merged 9 commits into from
Oct 2, 2024

Conversation

ralfbrown
Copy link
Collaborator

@ralfbrown ralfbrown commented Aug 25, 2024

Rather than blindly applying image loaders until one succeeds, use magic numbers at the start of the image file to determine which loader(s) to use, and apply only those. Also detect a variety of magic numbers for common non-image formats so that we don't even attempt to load them and can give an appropriately specific error to the user.

This PR will need thorough testing because it changes such a core operation. It will also have merge conflicts with #17367, which I will fix if both are accepted.

@ralfbrown ralfbrown added wip pull request in making, tests and feedback needed scope: UI user interface and interactions scope: image processing correcting pixels scope: DAM managing files, collections, archiving, metadata, etc. labels Aug 25, 2024
@jenshannoschwalm
Copy link
Collaborator

Fully agreed on this generally.

@HansBull
Copy link
Contributor

I know this would render the code a little more complicated and is probably out of scope, but one could test if a jpeg file also has the 0xFFD9 end marker to determine a possible premature EOF.

@ralfbrown
Copy link
Collaborator Author

@HansBull I would expect that libJPEG reports that as corruption, but haven't actually checked into it.

@ralfbrown
Copy link
Collaborator Author

Rebased and merge conflicts fixed.

@ralfbrown
Copy link
Collaborator Author

If someone can tell my why CI keeps throwing an error (with empty log) on the "Check if it runs" step, I'll address that on the weekend.

@ralfbrown ralfbrown removed the wip pull request in making, tests and feedback needed label Sep 10, 2024
@ralfbrown ralfbrown changed the title RFC: make image loading smarter, use only loaders which could potentially read file testing needed: only use image loaders which could potentially read file Sep 10, 2024
src/imageio/imageio.c Show resolved Hide resolved
src/imageio/imageio.c Show resolved Hide resolved
src/imageio/imageio.c Show resolved Hide resolved
@ralfbrown
Copy link
Collaborator Author

I was getting nowhere fast trying to trace the data flow to where temperature.c was spitting out its color matrix error message when an image file fails to load and/or aborting the pixelpipe before execution gets there, so I decided to simply suppress the message if the load failed....

@ralfbrown
Copy link
Collaborator Author

@HansBull libJPEG doesn't report corruption if the codestream for the image data proper is truncated, it just fills the missing pixels with gray. For JPEGs, showing as much of the image as is available is probably better behavior than refusing to show it at all. I haven't played around with actual file corruption other than truncation.

@TurboGit TurboGit added this to the 5.0 milestone Oct 1, 2024
@TurboGit TurboGit added the priority: medium core features are degraded in a way that is still mostly usable, software stutters label Oct 1, 2024
@TurboGit
Copy link
Member

TurboGit commented Oct 1, 2024

@ralfbrown : Just to be sure, reading the code it seems to me that if the magic number is not found (not recognized/supported) we fallback to loading as before (that is trying the different loader, right?).

@ralfbrown
Copy link
Collaborator Author

Correct. If a magic number is recognized, use the specific loader, otherwise use the old behavior of trying a bunch until one succeeds or we run out of loaders. This provides a minor speedup and more specific error reports for recognized file types.

@TurboGit
Copy link
Member

TurboGit commented Oct 1, 2024

This fixes QOI indeed but breaks RGBE HDR format that I could open before testing this PR.

Sample here, just decompress:

sample_1920×1280.zip

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bring back RGBE HDR support. Thanks!

@ralfbrown
Copy link
Collaborator Author

Different signature string - I guess they want to move away from the original manufacturer/software name ("#?RADIANCE" is documented, your file has "#?RGBE").

@ralfbrown ralfbrown dismissed TurboGit’s stale review October 1, 2024 21:37

added alternate signature string

@TurboGit
Copy link
Member

TurboGit commented Oct 2, 2024

Will test this soon, question why this non recognized signature has failed to be loaded by the fallback mechanism?

@ralfbrown
Copy link
Collaborator Author

Because I didn't have the fallback run through all of the loaders on the assumption that some are specific enough to have been fully covered by the signatures....

@TurboGit
Copy link
Member

TurboGit commented Oct 2, 2024

Because I didn't have the fallback run through all of the loaders on the assumption that some are specific enough to have been fully covered by the signatures....

Good to see the fallback for all loaders because as experienced here a new signature could avoid dt to load a picture and for users this means waiting months to get the fix.

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and testing ok on my side, let's move forward. Thanks!

@TurboGit TurboGit merged commit 62cf66a into darktable-org:master Oct 2, 2024
6 checks passed
@TurboGit TurboGit changed the title testing needed: only use image loaders which could potentially read file only use image loaders which could potentially read file Oct 2, 2024
@ralfbrown ralfbrown deleted the imageio_magic branch October 2, 2024 12:40
TurboGit added a commit that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: DAM managing files, collections, archiving, metadata, etc. scope: image processing correcting pixels scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants