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 42 commits into
base: main
Choose a base branch
from

Conversation

BPplays
Copy link
Contributor

@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

Closes #333

@BPplays
Copy link
Contributor 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
Contributor 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
Contributor 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
Contributor 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
@CyanVoxel CyanVoxel changed the base branch from Alpha-v9.4 to main October 11, 2024 22:33
CyanVoxel and others added 2 commits October 17, 2024 15:49
Co-Authored-By: BPplays <58504799+bpplays@users.noreply.github.com>
Co-Authored-By: BPplays <58504799+bpplays@users.noreply.github.com>
@BPplays
Copy link
Contributor Author

BPplays commented Oct 21, 2024

@CyanVoxel i think the PR is good enough for you to have another look if you want.
i also moved setting what preview media type to display to it's own function.

Comment on lines +721 to +723
if hasattr(image, "n_frames"):
if image.n_frames > 1:
self.set_anim_img(filepath, ext, file_bytes, image)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an approval comment: I like the check to see if an image has multiple frames - for some reason Qt displays QMovies a bit strangely compared to static images, so this check is nice for improving the quality of non-animated images that share extensions with animated counterparts (i.e. gif).

Comment on lines 166 to 172
ani_img_priority_order = ["jxl", "apng", "png", "webp", "gif"]

self.preview_anim_img_pil_map = {"apng": "png"}

self.preview_anim_img_pil_map_args = {"gif": {"disposal": 2}}

self.preview_anim_img_pil_known_good = {"webp", "gif"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a lot of this code that I opted to leave out when porting the functionality over due to it being difficult to decipher the purpose of and to trace back what was occurring inside the animated image creation block.

If it's necessary for important additional functionality, I would appreciate some documentation on what exactly all of these maps are being used for and why they need to be used over just loading in the image as a gif/webp.

Copy link
Contributor Author

@BPplays BPplays Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyanVoxel this is for choosing image format priority based on what types are supported for any given version of pillow and qt

  • preview_anim_img_fmts: this is a list of qmovie supported formats, it also gets sorted base on:

  • ani_img_priority_order: is an order of image extensions that are sorted first in preview_anim_img_fmts

  • preview_anim_img_pil_map: this maps what extension to use in pillow, pillow encodes animated pngs with the format 'png' instead of 'apng' so if apng comes up anywhere pillow should use png

  • preview_anim_img_pil_map_args: this is special args pillow should use for specific encoding formats, here gif has a trail on transparency if 'disposal' is not 2, but webp doesn't need it.

  • preview_anim_img_pil_known_good: it's a list of image types pillow can encode with animation, i couldn't find a way to test this in the program, it also probably needs expanding i made a commit, preview_anim_img_pil_known_good is also now preview_anim_img_pil_anim_supported

the logic to choose image formats happens in get_anim_ext

@BPplays
Copy link
Contributor Author

BPplays commented Oct 26, 2024

@CyanVoxel it seems like it blocks the gui thread while it's saving an image causing quite a bit of lag for large-ish images,
this should be fixed by saving the image, waiting for it to be done then loading it without blocking the gui but im not sure how to do that.

i've made a basic and kinda janky threaded implementation with in memory caching but, it still has some issues for example: it doesn't update to the animated image until you go back for transcoded images.
also multiprocessing might work better for this.

CyanVoxel added a commit that referenced this pull request Nov 7, 2024
…nd partially port #357)  (#549)

* feat: add JXL image thumbnail support

Co-Authored-By: BPplays <58504799+bpplays@users.noreply.github.com>

* feat: add animated previews for webp and apng

Co-Authored-By: BPplays <58504799+bpplays@users.noreply.github.com>

---------

Co-authored-by: BPplays <58504799+bpplays@users.noreply.github.com>
@CyanVoxel CyanVoxel added the Status: Changes Requested Changes are quested to this label Dec 10, 2024
@CyanVoxel CyanVoxel removed this from the Alpha v9.5 (Post-SQL) milestone Dec 21, 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 Status: Changes Requested Changes are quested to this 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.

[Feature Request]: jpeg xl support
4 participants