TagStudio icon indicating copy to clipboard operation
TagStudio copied to clipboard

improved handling of animated images

Open BPplays opened this issue 1 year ago • 6 comments

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 avatar Aug 21 '24 22:08 BPplays

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 avatar Aug 22 '24 13:08 BPplays

~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 avatar Aug 22 '24 17:08 BPplays

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 avatar Sep 01 '24 20:09 CyanVoxel

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

BPplays avatar Sep 01 '24 22:09 BPplays

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.

eivl avatar Sep 06 '24 18:09 eivl

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

BPplays avatar Sep 08 '24 03:09 BPplays

@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.

BPplays avatar Oct 21 '24 17:10 BPplays

@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.

BPplays avatar Oct 26 '24 14:10 BPplays

The overall functionality present here has been achieved in the program at this point, with the significant hole of JXL support (#333) being resolved with #549. As for the part in the animated image handling, there doesn't seem to be any specific reason not to just load an image as a GIF or WebP, as brought up previously. There is a lot of logic and burden to do all this switching and checks, that might even need to be changed and adjusted as underlying API and support changes. There also seems to be the issues with large files and threading, as such I'm afraid the implementation here cannot be accepted.

xarvex avatar Mar 12 '25 05:03 xarvex