Album Art Resize to filesize - #3560
Conversation
|
Wow; this is super cool! Awesome work on this so far. The Twitter feed with album art as attached images looks very snazzy. For the image resizing bit, feel free to extend the ArtResizer API to permit a "resize to X bytes" operation. One option would be to use the proposed shortcut for the IM backend but resort to the necessary guess-and-check loop for PIL. |
|
Thanks for the input @sampsyo! I've implemented an addition to the ArtResizer API as you suggested, adding an extra function argument to resize to X bytes ( In order to test it's working I've written tests in |
|
Hey @davidswarbrick, I just want to clarify my explanation on the linting I gave on our discussion. In particular,
Currently, it looks like you have unintentionally made a decent amount of formatting changes to other parts of code that don't deal with your plugin. I see two potential solutions to this:
As for the docstring errors, those won't go away with #3694 😄 |
|
Aha thanks @jtpavlock ! You're right about the reasonably large impact formatting changes have had, I was hoping they weren't too extensive but on review it's more than I realised. I'll address the docstring errors first then see where my efforts will be best spent on the transition to black. |
|
I've fixed the formatting changes & addressed the windows errors, should be ready for review now. |
12f2926 to
4edd8fe
Compare
davidswarbrick
changed the title
|
After considering the state of the Tweet plugin and how niche the use-case is, I've decided to remove it from the subject of this PR, and transfer its status to that of a "third party plugin". However, I believe the additions to the album art resizer remain useful, in that the original PR introduced the functionality to resize an album art image to a particular filesize, which could be useful for optimising libraries with lots of large artwork. |
wisp3rwind
left a comment
wisp3rwind
left a comment
There was a problem hiding this comment.
After considering the state of the Tweet plugin and how niche the use-case is, I've decided to remove it from the subject of this PR, and transfer its status to that of a "third party plugin". However, I believe the additions to the album art resizer remain useful, in that the original PR introduced the functionality to resize an album art image to a particular filesize, which could be useful for optimising libraries with lots of large artwork.
Absolutely, that does sound useful! I left a few comments about slight improvements to the logic and compatibility.
In order to test it's working I've written tests in test/test_tweet.py to call the PIL and ImageMagick functions directly, however this has required me to include Pillow as a new dependency for testing which seems to imply we may have avoided testing the different image conversion methods previously?
I don't think there is a need to add Pillow to the requirements. The way this is handled for other dependencies (or platform-specific features) is to explicitly skip tests which are missing dependencies.
50006fe to
080ec6a
Compare
- In line with comments on PR, adjusted the ArtResizer API to add functionality to "resize to X bytes" through `max_filesize` arg - Added config option of "cautious" to request user input before uploading any information to Twitter - Improved docstring of plugin, in preparation for more complete documentation. - Wrapped lines to 79 characters to
- plugins/tweet.rst to explain usage & installation - Adjustment to changelog.rst to include max_filesize change to ArtResizer and addition of new plugin.
- Checks new resizing functions do reduce the filesize of images - Checks template filling functioning correctly - Some whitespace conflicts between black & flake8 discussed here: https://discourse.beets.io/t/fixing-flake8-errors-for-plugin-contribution/1454/2
As the Tweet plugin is a particularly niche use-case of Beets it makes most sense for it to remain a third-party plugin, however the ArtResizer changes (to a specific filesize) may be useful in other plugins/the future of Beets development.
- Add syspath escaping for OS cross compatibility - Return smaller PIL image even if max filesize not reached. - Test resize logic against known smaller filesize (//2) - Pass integer (not float) quality argument to PIL - Remove Pillow from dependencies - Implement "max_filesize" fetchart option, including logic to resize and rescale if maxwidth is also set.
080ec6a to
d6bac5d
Compare
|
Hey @wisp3rwind / others, sorry for the delay on this but it should be ready for a final review or merging now. |
wisp3rwind
left a comment
wisp3rwind
left a comment
There was a problem hiding this comment.
I left a few inline comments where I think tests might be improved and the plugin might be simplified.
When merging I would want to squash this PR to remove the remainder of the Twitter plugin. Is this fine with you, or what you rather reorganize these commits yourself?
Co-authored-by: Benedikt <wisp3rwind@posteo.eu>
Tests now check that a target filesize is reached with a higher initial quality (a difficult check to pass). With a starting quality of 95% PIL takes 4 iterations to succeed in lowering the example cover image to 90% its original size. To cover all bases, the PIL loop has been changed to 5 iterations in the worst case, and the documentation altered to reflect the 50% loss in quality this implies. This seems reasonable as users concerned about performance would most likely be persuaded to install ImageMagick, or remove the maximum filesize constraint. The previous 30% figure was arbitrary.
|
Thanks so much for the review @wisp3rwind . Please feel free to squash this PR, it has a particularly messy commit history, and the legacy twitter plugin has been moved to a separate personal branch. |
Sorry for dragging this along so long @davidswarbrick, I'll try to give this a final review soon! |
Squashed from the PR, relevant commit messages follow below: Added file size option to artresizer - In line with comments on PR, adjusted the ArtResizer API to add functionality to "resize to X bytes" through `max_filesize` arg - Adjustment to changelog.rst to include max_filesize change to ArtResizer and addition of new plugin. Added explicit tests for PIL & Imagemagick Methods - Checks new resizing functions do reduce the filesize of images Expose max_filesize logic to fetchart plugin - Add syspath escaping for OS cross compatibility - Return smaller PIL image even if max filesize not reached. - Test resize logic against known smaller filesize (//2) - Pass integer (not float) quality argument to PIL - Remove Pillow from dependencies - Implement "max_filesize" fetchart option, including logic to resize and rescale if maxwidth is also set. Added tests & documentation for fetchart additions. Tests now check that a target filesize is reached with a higher initial quality (a difficult check to pass). With a starting quality of 95% PIL takes 4 iterations to succeed in lowering the example cover image to 90% its original size. To cover all bases, the PIL loop has been changed to 5 iterations in the worst case, and the documentation altered to reflect the 50% loss in quality this implies. This seems reasonable as users concerned about performance would most likely be persuaded to install ImageMagick, or remove the maximum filesize constraint. The previous 30% figure was arbitrary.
possible slight enhancements on top of #3560
Album Art Resize to filesize
Sorry for the empty promise here! I went ahead and squashed and merged the PR. Thanks for the contribution 🎉 |
4 participants
The subject of this PR was previously a Twitter plugin (details of which are included below for reference), however I now think that plugin was too specific for my personal use-case to be included in the wider Beets project, especially due to possible longer-term maintenance & support that could be required by the volunteer Beets community! The one useful & generalised development of the PR was to adjust the ArtResizer API to be able to target a filesize rather than just a quality, thus the contents and title of this PR have been adjusted to reflect this. I hope that it now seems like a useful addition to the project, and one that can be squashed & merged in the future! Thanks for reading as always.