Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

improved handling of animated images #357

Open
wants to merge 3 commits into
base: Alpha-v9.4
Choose a base branch
from

Conversation

BPplays
Copy link

@BPplays BPplays commented Aug 21, 2024

I've changed the handling of animated images to include support for more types of animated images.
PR features:

  • includes transcoding from an animated image type to webp or gif using pillow.
  • only counts an image as animated if it has more then 1 frame
  • checks what image types are support by qt and by pillow, as well as includes a list of known good types what pillow can encode with animation, and sorts available image types by quality

this PR also includes the changes from #344 so feel free to close that one if you prefer this one, though sadly that plugin doesn't seem to support decoding animated jxls

this also closes #333

@BPplays
Copy link
Author

BPplays commented Aug 22, 2024

i added some code to test converting into animated webp before showing the image but it's sometimes causing crashing and i have no idea why, when it works for a small amount of time it seems to work.

@BPplays
Copy link
Author

BPplays commented Aug 22, 2024

i think i might have fixed the crashing
it hasn't crashed once after testing a lot while developing it more, so im gonna call it fixed

@BPplays BPplays marked this pull request as ready for review August 22, 2024 21:38
@CyanVoxel CyanVoxel added Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience Status: Review Needed A review of this is needed labels Aug 22, 2024
@CyanVoxel CyanVoxel added this to the Alpha 9.5 milestone Aug 23, 2024
@CyanVoxel
Copy link
Member

Wanted to chime in here with the state of this PR and other relevant developments around the project.

#344 was at a sticking point regarding licenses, but is probably going to move forward with pillow-jpegxl-plugin assuming it functions correctly (I need to get my hands on some .jxl files).

In the meantime, I've unknowingly implemented some of changes here in #409 - mostly the animated image media type category and some code for loading GIFs into memory rather than streaming them from disk.

I would also suggest rebasing this to Alpha-v9.4, however there's likely due to be more than a few merge conflicts... Some of this is due to the changes mentioned above, and others are due to some git mishaps that happened in a recent update of Alpha-v9.4. But with the rest of the Alpha v9.4 milestone features out of the way, this should have a clear path on that branch now (outside of #344 presumably getting merged).

@CyanVoxel CyanVoxel added the Priority: Medium An issue that shouldn't be be saved for last label Sep 1, 2024
@BPplays BPplays changed the base branch from thumbnails to Alpha-v9.4 September 1, 2024 22:02
@BPplays
Copy link
Author

BPplays commented Sep 1, 2024

@CyanVoxel i manually merged the work from the older version of this branch on to Alpha-v9.4
edit: here is a jxl

@eivl
Copy link
Collaborator

eivl commented Sep 6, 2024

Are no tests affected by this PR after the gif changes? And is there a way to add missing tests?
I don't see anything that makes me think this would not work; I would also like a short description of how to test this live on my machine.

@BPplays
Copy link
Author

BPplays commented Sep 8, 2024

Are no tests affected by this PR after the gif changes?

@eivl are there any tests for animated image handling?
i couldn't find any after a quick look in ./tagstudio/tests in the Alpha-v9.4 branch.

And is there a way to add missing tests?

probably

@CyanVoxel CyanVoxel removed the Status: Review Needed A review of this is needed label Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium An issue that shouldn't be be saved for last Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants