search

shutter-project / shutter

Screenshot tool for Linux
https://shutter-project.org/
GNU General Public License v3.0
538 stars 35 forks source link

Added AVIF support #691

Open Photon89 opened 3 months ago

Photon89 commented 3 months ago

Added AVIF support as requested in #690.

DarthGandalf commented 3 months ago

Does this need any new dependency?

Looking at the code, why do we need so much custom code if the only thing different between different formats is the name of the setting where the quality is stored?

Photon89 commented 3 months ago

Indeed, it requires libavif as optional dependency for AVIF support.

I just did a copy&paste from the recently merged PR where I implemented the WEBP support, but actually you are right, I will try to simplify the code!

Photon89 commented 3 months ago

I'm out of ideas how to further simplify the code. It's still a bit ugly in some places, but I think, the biggest chunks should be taken care of now.

Photon89 commented 3 months ago

Please test and review, ready to merge from my end!

Photon89 commented 3 months ago

One remark: After getting rid of the int_png, int_jpg etc we don't have control over the order of the supported file types in the file type drop down list and also when choosing the default file type (for an empty settings file).

On my machine, the order is png, bmp, jpg, avif, webp, that is, when the settings file is empty, png is taken as default if available (second priority is bmp and so on). I'm not sure if this order is the same on all machines, it is the order of the list which is returned by https://docs.gtk.org/gdk-pixbuf/type_func.Pixbuf.get_formats.html. Saving and loading the settings also relies on the fact that this order doesn't change, because the number that is saved in the settings file is referring to this order.

Before the refactoring the order for default file type priority was png, jpg, bmp, webp, avif (fixed like that in https://github.com/shutter-project/shutter/blob/bce8db87fc8b8e61baf6fffab64d74c3e7d3c092/bin/shutter#L1196). The numbers 0 and 1 have been fixed to jpg and png respectively, but the other file types relied on the order in the Pixbuf.get_formats list as well, as far as I understand.

We get a problem if a lib supporting a new format is installed and then the number in the settings file doesn't suit any more. Say, without libavif, we have png, bmp, jpg, webp, so webp has number 3 in the settings file. But after installing libavif, we have png, bmp, jpg, avif, webp and thus number 3 in the settings file corresponds to avif while webp shifts to number 4.

I'm not sure if this is acceptable or we should try to assign a number to each supported file format globally (which will probably make the code a bit uglier again, but more stable).