Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace supported image extension list with image detection via MagickImageInfo. #818

Merged
merged 7 commits into from
May 14, 2021

Conversation

mooflu
Copy link
Contributor

@mooflu mooflu commented Jan 16, 2021

Fixes #802 #768

@rabelux rabelux requested a review from xupefei January 16, 2021 23:25
@xupefei
Copy link
Member

xupefei commented Jan 17, 2021

My main concern is ImageMagick handles way more file types than images, such as PDF.
Could you adjust the Property of this plugin to -4 (1 higher than TextViewer) so that it will not steal previews of other plugins?

@mooflu
Copy link
Contributor Author

mooflu commented Jan 17, 2021

Good point. With ffmpeg installed it'll also try to handle video formats. VideoViewer has priority -10. I guess prio values would need to be VideoViewer > ImageViewer > TextViewer. E.g. -3, -4, -5 ; comment in VideoViewer says "make it lower than TextViewer" but not why.

@mooflu
Copy link
Contributor Author

mooflu commented Jan 17, 2021

It was changed to lower than TextViewer here: 24c0316

I see .ts as a video file extension, so if video was > text it would try to view typescript files as video. So I can see why video would need to be lower that text if video only goes by file extension. :(

Change VideoViewer priority to -3 and detect audio/video via MediaInfo instead of file extensions.
@mooflu
Copy link
Contributor Author

mooflu commented Jan 17, 2021

Tried to do the same in VideoViewer and detect audio/video file via MediaInfo instead of file extensions. See latest WIP - currently re-creates MediaInfo in both CanHandle and Prepare. Out of time for today...

fb-te added 2 commits January 18, 2021 20:44
Add some notes about MediaInfo Open and Close.
If there was an exception due to MediaInfo it would have already occurred in CanHandle.
@mooflu
Copy link
Contributor Author

mooflu commented Jan 20, 2021

@User198263321
Copy link

User198263321 commented Jan 23, 2021

Does it also support .jfif files?

EDIT: This was added in a test build 4 months ago

@mooflu
Copy link
Contributor Author

mooflu commented Jan 25, 2021

@Michael82548 yes, should work. Both in the nightly build (https://github.com/QL-Win/QuickLook#downloadinstallation) as well as the test build for this merge request above.

@mooflu mooflu mentioned this pull request Mar 6, 2021
@xupefei
Copy link
Member

xupefei commented Apr 8, 2021

I'm quite busy in the past few days. Will try to review this PR by this weekend.

For other image formats, let ImageMagick try to detect by file content.
Upgrade to latest Magick.NET
@rabelux rabelux requested a review from xupefei May 5, 2021 10:05
@xupefei
Copy link
Member

xupefei commented May 14, 2021

Great job, thanks!

@xupefei xupefei merged commit 3ef980b into QL-Win:master May 14, 2021
@xyz7z
Copy link

xyz7z commented May 16, 2021

@mooflu
Thanks for pulling such a request, and it helps me a lot. However, It seems that the new merged commit has some performance problems.
Once you select one JP2 file, the program will take a long time to open it and not all files with JP2 extension are supported. that is the first problem.
Before the program succeeds to open the first JP2 file, select the next file using arrow keys, it seems that the program failed to release the memory of the fist image, even if you close the window of QuickLook completely. that is the second problem.
image

@mooflu mooflu deleted the moar-image-types branch June 13, 2021 02:59
@xupefei
Copy link
Member

xupefei commented Jun 27, 2021

I disabled detecting images using ImageMagick due to:

  • The detection will load the whole file, which is very slow when the file is large,
  • Need a better way to handle misdetection. For example, ImageMagick treat all XML files as SVG.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for .dds images?
5 participants