fix: magick preview of multiframe files#4024
Conversation
| cmd:arg("-flatten") | ||
| cmd:arg(tostring(job.file.path)):arg("-flatten") | ||
| else | ||
| cmd:arg(tostring(job.file.path) .. "[0]") |
There was a problem hiding this comment.
Thanks for the patch!
Appending [0] after the file path causes an error when image filenames already contain [0], like a.jpg[0] or b.jpg[0][0]:
`magick` exited with error code: 1without manually appending [0], they can be previewed normally with the magick previewer.
Perhaps some kind of escaping is needed for the file path here?
There was a problem hiding this comment.
This turned out to be more difficult than I thought. There a combination of two problems.
- Apparently,
magickdoes not provide a way to escape individual characters in a file path. Moreover, you cannot even pass an input path tomagickin such a way that it does not internally process the characters in it at all. For the output path, there is a setting that allows this (-define filename:literal=true), but looks like its implementation for the input path was not added: Expand-define filename:literal=trueto also encompass inputs ImageMagick/ImageMagick#8035 (comment) - You can only select a frame by adding square brackets to the end of the path. There is no alternative argument for this operation.
So, as I understand it, magick works with filenames as follows:
- If a file exists with exactly the name that was passed as an argument, it uses that file. It doesn't matter whether it contains special characters or not, everything is interpreted as part of the path. Therefore, files like
a.jpg[0]are now handled correctly. - If a file with exactly that name does not exist,
magicktries to parse the options passed in[], but it also parse the[]that were part of the filename, which leads to an error.
In other words, you can avoid using magick's functions that activated by special characters in the filename, and everything will be fine with any filenames. But if you use at least one of these functions (and we need Frame Selection, which is only available by adding [] to the filename), then ALL special characters in the filename start being treated as special. There is no option in between, like escaping some characters and having special use for others.
At first, I thought about checking path for a trailing ] (that was the only source of issues with square brackets). But then I found that magick also have special meaning for :, @, and %. And having these in a filename while trying to use Frame Selection ([0]) trips up magick too, because it now tries to interpret them. It becomes too complicated to deal with, so maybe it's better not to use Frame Selection at all (maybe sometime in the future if magick adds a proper argument for it).
Alternative way to get the first frame
-delete 1--1 argument discards all frames from the second (1) to (-) the last (-1).
The only concern is performance. I'm not good at benchmarking (perhaps someone else should run more tests) but here are my observations:
For a multiframe file:
[0]is the fastest. As I understand it, it does not depend on the number of frames, because it doesn't even load them for processing, only the first one. But we cannot use this.-delete 1--1is significantly slower, because it loads all frames and then deletes the extras.- no arguments is even slower than the previous option (but not for much). That's how it works now, creates one file per frame — not what we need.
-flattentakes about the same time as the previous option.
For a single-frame file:
- no arguments is how it works now, nothing is done with frames.
[0]takes the same time as above.-delete 1--1takes the same time as above.-flattenis slightly slower.
So if we use -delete 1--1, performance for single-frame files should not change. For multi-frame files, performance will not be the maximum that magick could achieve, but still it will be the same/faster than now (and working previews will be generated instead of a bunch of numbered frames).
Rationale of this PR
What changed
If the
magickpreviewer is used without the--flattenargument, it now processes only the first frame of the file. For example, instead of runningmagickwithinput.jpg, it will run it withinput.jpg[0](which discards all frames but first and for files with only single frame it changes nothing).Why do you need this?
Two reasons.
1st reason. If you set the yazi previewer to
magickand try to preview a file with several frames (layered image or animation), it generates preview JPEGs for each frame and names them<hash>-0,<hash>-1, etc. As a result, preview still won't be displayed (because no file named just<hash>was created). It seems that this logic has no use case and simply punishes a user who forgot to add--flattenargument to themagickpreviewer, by creating a bunch of unnecessary files and not showing preview anyway.2nd reason. Moreover sometimes for a useful preview you need the first frame instead of a flattened image.
Example 1.
Animated
.avifwith transparent background.On the left: preview generated after this PR with the previewer
magick --bg=gray.On the right: preview generated with
magick --flatten --bg=gray(you forced to use--flattenbefore this PR, otherwise a valid preview file would not be generated at all as described above).So before this PR you could only get a preview of all frames mingled together.
(Furthermore, the
--bgargument is not respected. This is likely because inmagick.luathe background color is set after flattening, but that's another issue that I don't touch here.)Example 2.
Layered
.psdfile with usage of layer masks.ImageMagick's flatten does not respect layer masks (which should hide parts of a layer). But you don't really even need to flatten
.psdfiles because they already have a preview which is stored as the first frame.Files used for examples: examples.zip
Checklist