jasper-software / xv

XV Software
Other
26 stars 9 forks source link

#7 Added webp load/save support #8

Closed tanabi closed 1 year ago

tanabi commented 1 year ago

This commit addresses issue #7 and gives xv webp support.

Some 'release notes':

apt-get install libwebp-dev

I think that's it. I properly supported it in CMake and it detects libwebp if you have it installed. I didn't touch the Makefile / Imakefile as I assume those are legacy build files, but let me know if we need to support them and I can probably make it work. Let me know if you have any questions!

Resolves #7

mdadams commented 1 year ago

@tanabi I had a quick look at your changes and they seem reasonable to me. I didn't dig into the details of the main implementation file, but the other files looked reasonable. You said that your code does not save grayscale. I wasn't clear if it sliently does the wrong thing (i.e., by saving color instead of grayscale) or gives an error. I think that an error would be much better than silently doing the wrong thing (i.e., silently and incorrectly saving color instead of grayscale would be bad.) If you don't give an error in this case, I think that you should. (Of course, saving grayscale would be better, but I appreciate that you may not have time/motivation to do this.) I would also suggest that you add a few small image files (in the webp format) for testing purposes to the data/images directory. It is extremely important that the images used not cause copyright issues. So, I would suggest deriving the images from the ones already in the images directory (by cropping/warping and format conversion) since the images in this directory should be clean as for as copyright is concerned. If you have some small images of your own for which you own the copyright, this would be okay as well.

@pghmcfc What do you think about this PR? Would you be okay with it being merged (since you are using this code base in Fedora, I wanted to ask your opinion).

tanabi commented 1 year ago

@mdadams I've sussed out how to make a greyscale in xv and I'll update this PR so it handles that properly in addition to some sample webp's momentarily.

tanabi commented 1 year ago

@mdadams I failed to find any truly free webp's -- or rather, images that I am 100% confident are truly free webp's -- so I flipped around one of the existing images using xv and saved it. I'd love to have different kinds of webp's to test, but I'm not really sure how to make that happen with my available resources.

That said, at least I did test xv with every variant of webp I could find even if I can't include my test files. And greyscale now works properly.

mdadams commented 1 year ago

@tanabi I had hoped that Paul (@pghmcfc) would provide some input on your PR. Since he hasn't and much time has passed and your changes do not look unreasonable to me, I have merged this PR.

pghmcfc commented 1 year ago

Hi all, apologies for my late response - I've been really busy this last couple of months. I gave the 4.1.1 release a try and works fine for me. I'll be updating the RPM Fusion packaging to use the build from this repo soon (RPM Fusion is a third-party RPM package repository for Fedora users that packages things that aren't allowed in Fedora proper, such as shareware like xv).