Replace imghdr #650

jcjgraf · 2023-07-02T20:51:07Z

replaces #646

Since imghdr is getting deprecated from the standard library (see #579), this PR replaces that module by a own (and thereby, legally completely decoupled independent (unlike in #646)), implementation of the magic byte file type detection.

Some advantages of this approach, over using the package:

Detect (all) file types which are actually supported by QT natively or extended (by using qtiamgeformats)
- Well, some are not detectable...
Detect none which are not supported
Have a central place for all magic byte checks
- Cleanup of utils/files and plugin/imageformats
Well structured magic byte checks allows for better maintainability
plugin/imageformats has a specific and well defined purpose
- I.e. enable image type support that require the (manual) installation of some other QT module

And something that I like particularly well:
The runtime validation of check functions, (used to be done for all checks that were registered using utils/files/add_image_formats), is now done for all image types, except the ones natively supported by QT. This way, clients get a notification when they try to open an image of a type that:

is supported only by qtimageformats, without having that module installed
is supported by a 3rd party QT module and was activated via plugins/imageformats, without having that module installed
This should prevent confusions as in Unable to open WebP image format #619, and previous similar issues.

Steps

Add check function for all types natively or extended supported
- WBMP is completely undetectable
- TGA version 1 is undetectable, while version 2 is
- XBM has no signature, but can be matched based on its content
- See docstring in utils/imagheader for status supported types
Add registration function
- Allow for runtime check validation
Add type detection function
Cleanup and adaption of codebase to new utils/imageheader
Cleanup of plugins/imageformats
Tests

I have already added a basic tests that verifies the detection of all filetypes which QPixmap is capable of writing (/creating) (as you suggested in the other PR). Unfortunately, it is capable of creating less types than what it is able to read. I have to think about ways to create and test the other types.

So, code-wise this PR is done, and if time allows, you may have a look at it. I will spend some more time for testing.

In what way does it make sense that copyright in the header of the new file is required to start at 2017? Should it not start in the year the file was first added to the project (i.e. 2023 in this case)?

fixes #579
replaces #646

Also copy own check functions from `utils.files` over.

This is equivalent to the functionality originally provided by `add_image_format`. This implementation has the advantage that it allows uf us to add this validation for ALL types not natively supported (including types supported by qtimageformates).

This also includes removing jp2, as this check is included in `utils.imageheaer`.

Added wrong function to `_registry`, resulting in dynamic check not actually be done.

buzzingwires · 2023-07-04T03:58:05Z

A few comments on this implementation of image detection:

Have you considered using an existing library like python-magic to detect images? It is permissively licensed and libmagic has very extensive format support. libmagic should already be very commonly available on Linux systems, though I don't know how easily it will be to support that on Windows.
For MNG files, isn't the magic number 8A 4D 4E 47 0D 0A 1A 0A, very similar to the PNG magic number, but with 4D 4E 47 == MNG instead?
I think WBMP also lack explicit magic numbers, and I'm not sure what .cur files even are. Mouse cursor images for Windows?
XBM and TGA indeed lack explicit magic numbers.
For XBM files, at least the way imghdr does it is to just see if the file starts with '#define'. A better way to do it might be how libmagic does it, which is roughly equivalent to the Regex #define [a-zA-Z0-9]+_width [0-9]+\n#define [a-zA-Z0-9]+_height [0-9]+. Either way, XBM starts out in a pretty distinct way.
TGA files are much less trivial, but some newer files end with TRUEVISION-XFILE.\0 Otherwise, Targa identification seems to work based on the first 18 bytes of the file, and there are many rules to determine what generally 'makes sense' for a Targa file. In my opinion though, TGA is probably the most likely to be encountered of the more obscure formats mentioned, since it still has relevance to video games. I think this image identification module should be very solid with the already-supported formats, the low-hanging fruit of MNG and XBM, and nontrivial but relevant TGA support.

karlch · 2023-07-04T07:10:26Z

@jcjgraf thanks a lot for the effort that went into this, this is really nice, I hope to take a closer look during this week!

Concerning the Copyright header, you are probably right, but I wouldn't bother in this PR. It may make sense to simplify this overall and remove the years and just keep a small notice at the top of each file. Discussion in a new issue?

@buzzingwires thanks a bunch for your comments and details! 😊

We actually did start a discussion on this in #579 . The reason for pursuing this path was initially, that just copying imghdr seemed absolutely straightforward and did the trick for years for us. To keep this PR on the implementation, I will add another comment in this issue directly to continue the discussion.

karlch

This is absolutely amazing work, thanks again! I only left a few nitpicks, so besides the points you mentioned and including the magic bytes described by @buzzingwires, this is basically ready to go.

It is really cool to see how much cleaner this makes the handling of filetypes, together with the Qt support, and the error / warning messages. And the documentation is amazing.

tests/unit/utils/test_imageheader.py

vimiv/api/__init__.py

vimiv/utils/imageheader.py

Initially, I thought (hoped) that this is not needed. But it seems that for certain types it is required. That is e.g. the case when the first 32 bytes are not sufficient to detect type.

Version 1 is undetectable

Seeking relative to the end of file seems to not work for some files, resulting in an exception. This was fixed.

Assuming a certain seek is not a good idea. Also, apparently, the prefix of `_width` and `_height` is optional (QImageWriter does not produce one).

It would be even cleaner to omit the else bloc and set `check = check_verified` in the then block, and register `check` instead. But python has some recursion issue with that.

It does not like my informative capitalization ;(

jcjgraf · 2023-07-12T18:56:55Z

@buzzingwires Thanks for your really helpful comment! And sorry for the late reply...

I know that some of the formats are probably used by literally nobody (anymore). But since these formats (including .cur, and yes, I had also no clue that they are😅) are supported by QT, and therefore do not require any additional effort except registering a check function, I would like to support them.

For MNG files, isn't the magic number 8A 4D 4E 47 0D 0A 1A 0A

Your correct, thanks! I have added a check for it.

For XBM files, at least the way imghdr does it is to just see if the file starts with '#define'. A better way to do it might be how libmagic does it, which is roughly equivalent to the Regex #define [a-zA-Z0-9]+_width [0-9]+\n#define [a-zA-Z0-9]+_height [0-9]+. Either way, XBM starts out in a pretty distinct way.

XBM is indeed a funky file definition; it is a valid C source file🙈 I have also added a check for it.

TGA files are much less trivial, but some newer files end with TRUEVISION-XFILE.

Indeed. There are two version of TGA. In version 2 they have added a signature to the footer of the file. I have added support for that.

Otherwise, Targa identification seems to work based on the first 18 bytes of the file, and there are many rules to determine what generally 'makes sense' for a Targa file.

I am not really sure if it is works the effort to check for dynamic set bytes. This seems rather complex and is in the end not more that a heuristic (well, that is also the case for some other types, that have a rather "badly" chosen signature 😂), that may lead to false positives.
I think I will not add support for them here, but may do so if there is a specific request.

From python 3.11 on try induces zero-overhead when no exception is raised in the try. This is much more efficient than using suppress. The try is placed inside for loop to ensure that a check that raises does not affect other checks.

Could also be useful in other unit tests in the future.

This should have been changed as port of karlch#650. This plugin is no longer needed for jp2 support, but jp2 is natively supported, given that the qt-imageformats module is installed.

jcjgraf added 7 commits July 1, 2023 13:11

Implement own image detection based on magic bytes

c17aa33

Also copy own check functions from `utils.files` over.

Fix tests affected by removal of imghdr

c56eb86

Remove reference to imghdr from docs

a82664c

Add basic test for all types writable by QPixmap

3f8bfb7

Update imageformats plugin for new imageheader

2a5bbb9

This also includes removing jp2, as this check is included in `utils.imageheaer`.

Simplify assertion thanks to new structure test registry

b3eb932

jcjgraf mentioned this pull request Jul 2, 2023

WIP: Internalize deprecated imghdr packet #646

Closed

7 tasks

Add test for dynamic verify and fix bug

d0a8ba7

Added wrong function to `_registry`, resulting in dynamic check not actually be done.

karlch mentioned this pull request Jul 4, 2023

Lazy Loading Overhaul #651

Draft

karlch reviewed Jul 4, 2023

View reviewed changes

tests/unit/utils/test_imageheader.py Outdated Show resolved Hide resolved

vimiv/api/__init__.py Show resolved Hide resolved

vimiv/utils/imageheader.py Outdated Show resolved Hide resolved

vimiv/utils/imageheader.py Outdated Show resolved Hide resolved

jcjgraf added 5 commits July 4, 2023 19:37

Add check for MNG

deae790

Add passing of opened file to check function

bcb18f3

Initially, I thought (hoped) that this is not needed. But it seems that for certain types it is required. That is e.g. the case when the first 32 bytes are not sufficient to detect type.

Fix copyright header

3c0c8bc

Add check for XBM

c34aea0

Add check for CUR

b82c978

jcjgraf force-pushed the core/imageheader branch from d0a8ba7 to b3eb932 Compare July 5, 2023 14:27

jcjgraf added 6 commits July 6, 2023 10:31

Add check for TGA version 2

2521a90

Version 1 is undetectable

Add note about WBMP being undetectable

ec37dd4

Fix tga check raising for some image types

7cbfb47

Seeking relative to the end of file seems to not work for some files, resulting in an exception. This was fixed.

Fix xbm check assuming certain seek and fix regex

b25cf4b

Assuming a certain seek is not a good idea. Also, apparently, the prefix of `_width` and `_height` is optional (QImageWriter does not produce one).

Add tests for imageheader module

eaf0d1d

Improve structure by moving verify check out wrapped function

63f8c95

It would be even cleaner to omit the else bloc and set `check = check_verified` in the then block, and register `check` instead. But python has some recursion issue with that.

jcjgraf force-pushed the core/imageheader branch from 7af4fbe to 63f8c95 Compare July 6, 2023 21:24

jcjgraf added 2 commits July 7, 2023 00:00

Improve comment about why prioritize external format check

60317dd

Fix lint

564a4c6

It does not like my informative capitalization ;(

karlch mentioned this pull request Jul 12, 2023

Support for PyQt6 (and PySide6) #575

Merged

4 tasks

Improve performance by using try instead of suppress

528b0e9

From python 3.11 on try induces zero-overhead when no exception is raised in the try. This is much more efficient than using suppress. The try is placed inside for loop to ensure that a check that raises does not affect other checks.

jcjgraf requested a review from karlch July 13, 2023 06:16

karlch added 2 commits July 13, 2023 18:16

Tests: remove duplication of tmpfile fixture

8c59515

Could also be useful in other unit tests in the future.

Simple wording and typo changes

ff57aab

karlch approved these changes Jul 13, 2023

View reviewed changes

karlch merged commit 39f7be2 into karlch:master Jul 13, 2023
14 checks passed

karlch mentioned this pull request Jul 13, 2023

Unable to open WebP image format #619

Closed

jcjgraf mentioned this pull request Jul 18, 2023

Fix all XML files detected as SVG #669

Merged

This was referenced Jul 31, 2023

Fix wrong statement that imageformats supports jp2 #674

Merged

support avif image #673

Closed

"Internal" Metadata Support Plugin #676

Draft

jcjgraf mentioned this pull request Aug 15, 2023

Improve JPEG Header Detection #685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace imghdr #650

Replace imghdr #650

jcjgraf commented Jul 2, 2023 •

edited by karlch

Loading

buzzingwires commented Jul 4, 2023 •

edited

Loading

karlch commented Jul 4, 2023

karlch left a comment

jcjgraf commented Jul 12, 2023

Replace imghdr #650

Replace imghdr #650

Conversation

jcjgraf commented Jul 2, 2023 • edited by karlch Loading

Steps

buzzingwires commented Jul 4, 2023 • edited Loading

karlch commented Jul 4, 2023

karlch left a comment

Choose a reason for hiding this comment

jcjgraf commented Jul 12, 2023

jcjgraf commented Jul 2, 2023 •

edited by karlch

Loading

buzzingwires commented Jul 4, 2023 •

edited

Loading