Add frame option to ImageMagick Thumb Driver

[tobimori]

This PR …

Adds a frame option to the ImageMagick Thumb/Darkroom Driver, that allows one to select a single frame from an animated or multi-layered image. This should be useful when e.g. generating inline thumbs that shouldn't be animated, with e.g. kirby-blurry-placeholder - see also.

Ready?

• [ ] Unit tests for fixed bug/feature
• [x] In-code documentation (wherever needed)
• [x] Tests and checks all pass

For review team

• [ ] Add changes to release notes draft in Notion
• [ ] Add to website docs release checklist (if needed)

[tobimori]

Unsure about how detailed the unit test should be, but selecting the first frame should also work on single-layer/single-frame images - so we could extend the current testProcess test with the frame option, or add a new one that specifically transforms a GIF.

[distantnative]

@tobimori would you be able to add unit tests for this? honest answer, no grudges if not. only then someone from us would need to pick it up at some point.

[tobimori]

I'm moving atm and don't have capacities for that, so I'd appreciate your help :)

[lukasbestle]

@tobimori Sorry that we kept this dormant so long. Are you still interested in this feature? Then we can try to finish it together.

[tobimori]

I would be, and think it's a general issue that needs to be tackled at some point (especially for use cases like the placeholder plugin), but it's not a priority for me, as the client project I needed this for is done, and I disabled placeholders for all (or they stayed huge, dunno, wasn't a big deal in the end)

If you need any help in finishing this, I'll gladly help, although it's christmas season, and I'm on holidays from the end of next week until new years.

[distantnative]

I have to admit I gave unit tests a try once but failed and abandoned it quickly 😅🙈

[lukasbestle]

Thanks for your quick reply. I think before we can tackle unit tests, we need to answer this open question:

What happens if a subsequent frame is selected for a static image? I.e. if I tried to get the third frame of a JPG? Would be great to have a fallback for that unless ImageMagick already handles this case internally.

[tobimori]

Thanks for your quick reply. I think before we can tackle unit tests, we need to answer this open question:

What happens if a subsequent frame is selected for a static image? I.e. if I tried to get the third frame of a JPG? Would be great to have a fallback for that unless ImageMagick already handles this case internally.

Still returns the first frame: -> both same output

[lukasbestle]

@tobimori I've rebased the PR against our develop branch and made minor updates to the code. I think the change itself is good, we "just" need a good unit test for it that actually tests the behavior. Could you prepare a test case? We can help to integrate it.

[tobimori]

Just stumbled upon this while working on a test case. It only happened with a file that has a limited number of frames, not e.g. a .png file that only has one.

The result of this command is two images with the frame attached as suffix.

It's the same as when converting a .gif to a single-level format like .png anyway, this shouldn't be a problem, right?

Edit: Throws error if out of bounds.

[tobimori]

I've added a unit test. It checks a sample GIF with a reference file generated by the same snippet.

[tobimori]

Summarizing my previous comments:

Currrent behavior when transforming a .gif with multiple frames to a .png without selecting a frame: Multiple files generated with frame index as suffix.

When transforming a .gif with multiple frames to a .png with an not existing frame: Generates the same files as above, but shows that the command execution failed.

When transforming a .gif with multiple frames to a .png with an existing frame: A .png with the same file name as the .gif is generated as expected with the selected frame.

When transforming a .jpeg/.png without multiple frames to an image with an not existing frame selected: The first frame will always be output.

[lukasbestle]

Thanks for adding the tests and for checking the edge cases. I think it would be good to have those covered as well, in the order of importance:

• Is it possible to stop ImageMagick from writing the frames in the "non-existing frame is selected" case? It's good that the error is generated, but an error case shouldn't generate files.
• I think it would be good to have special handling for the case when an animated image is converted to a non-animated image (like GIF to PNG) but no frame is passed. This should either trigger an error or use the first frame (whichever is easiest to implement; the main problem probably being that it's hard to detect whether an image is animated). AFAICT, the current behavior leads to broken thumbs anyway because of the automatically inserted suffix that won't be detected by Kirby. So this change would fix a bug and make the behavior consistent.
  • Possible solution from the top of my head: The default for the frame prop could be 0 when the target format does not have frame support. This would implement the "use the first frame" idea but still with compatibility for non-animated source images. However it's again difficult to detect whether the target format has animation support. We could hard-code ['jpg', 'png'] (with APNG, WebP and AVIF not listed as they support frames), but maybe there's a better option.

Could you please add unit tests for the edge cases as well?

[tobimori]

I've looked through the ImageMagick docs and haven't found anything that stops IM from creating files in an error case. :/

[tobimori]

I've implemented a new method that checks the frame count using the ImageMagick identify CLI. This is unfortunately the only method I found that works, besides using the Imagick extension. This requires a new config option, identifyBin, which works similar to the current bin option when using the ImageMagick driver.

Each image generation now checks using such method whether a frame is in bounds or not, and automatically fallbacks if not converting to a supported format (Currently avif, webp, gif listed as supported).

Unit tests are added covering the following cases:

• Exception gets thrown when a frame out of bounds is requested
• Output image by fallback is the same as when requesting frame 0 manually
• Frame count method works as expected, returning 2 for a sample GIF and 1 for a PNG.

I think this is a fair trade-off, given this fixes a significant bug that happens when trying to transform a GIF to any other unsupported format, if included in a major release of course. It'd also open the possibility to add APNG support in the future, as such must be specified with the APNG: prefix manually, which we could probably detect using identify.

[tobimori]

Any updates?

[lukasbestle]

Thanks for your research and sorry for the delay. We haven't yet found the time to look into this again. Especially because the edge cases make it all a lot more complex. To the point where I feel like the current implementation could be a downgrade for 99% of users who don't need to access frames.

[tobimori]

To the point where I feel like the current implementation could be a downgrade for 99% of users who don't need to access frames.

The way I look at this PR now is not an enhancement or feature, but rather a critical bug fix for everyone working with GIFs as those currently do not get generated/converted properly. If there is an alternative driver, such as the ImageMagick one, which most important difference to GD is to provide support for animated files, it should support them properly IMHO. (AVIF support is added to SimpleImage as we speak as it's in GDlib by now, so it'll probably be in the next Kirby release too)

If you are completely sure that this is not interesting to you, I'd also be up for moving this in a plugin. Although I heavily think this should be in core.

[tobimori]

I moved this into a plugin: https://github.com/tobimori/kirby-magick-extended

[lukasbestle]

I'm not sure yet. As I said, we haven't found the time to look at this again in detail due to other projects.

[tobimori]

Revisiting this after a few months, I think:

• The default behavior should be limiting to frame 0, when the target format does not support frames (e.g. not avif, webp, gif, etc.), otherwise do not pass frame options.
• This would solely fix the bugs described when e.g. converting a GIF to a JPEG and the current unexpected behavior where all frames will be converted separately as JPEG with suffixes.
• The frame counting behavior and its related options: a. should stay in a plugin, and not become part of Kirby core b. should always fall back to the first frame, if identifyBin is unavailable c. should be togglable by either having identifyBin undefined by default, or having a separate option to toggle the behavior
• If b or c, this would also allow for APNG support in core.

@distantnative

[distantnative]

@tobimori Yep, agreed on the first two. The third, I'd go for (c) - having it undefined by default already adds the option for users to define it and activate those functionalities. Could also be a plugin, but it doesn't seem like much of a burden to have that in the core as well then.

[tobimori]

Perfect. I'll get back to this PR in the next few weeks.

[github-actions[bot]]

This PR has been marked as stale because it requires further changes but has not seen activity in the past months. This is for us to prioritize PRs that can be reviewed and merged. It will be closed if no further activity occurs within the next 15 days. If you still have interest in this PR, please help us finalizing it. Please let us know in case you are stuck on the required changes.

[tobimori]

Still planned, will wait for v4.

[distantnative]

@tobimori if you want, we'd be up to integrate this during the alpha phase already. That way we can also test better if anything breaks.

[tobimori]

I don't have the time right now, sorry.

[github-actions[bot]]

This PR has been marked as stale because it requires further changes but has not seen activity in the past months. This is for us to prioritize PRs that can be reviewed and merged. It will be closed if no further activity occurs within the next 15 days. If you still have interest in this PR, please help us finalizing it. Please let us know in case you are stuck on the required changes.

[github-actions[bot]]

This PR has been marked as stale because it requires further changes but has not seen activity in the past months. This is for us to prioritize PRs that can be reviewed and merged. It will be closed if no further activity occurs within the next 15 days. If you still have interest in this PR, please help us finalizing it. Please let us know in case you are stuck on the required changes.

[github-actions[bot]]

This PR has been marked as stale because it requires further changes but has not seen activity in the past months. This is for us to prioritize PRs that can be reviewed and merged. It will be closed if no further activity occurs within the next 15 days. If you still have interest in this PR, please help us finalizing it. Please let us know in case you are stuck on the required changes.