Closed silverwind closed 1 year ago
Also, trying to upload a animated WEBP image currently gives this error, likely because the conversion code can not deal with animated WEBP correctly:
hmm... as far as I can see the squaring process is unrelated to the choice of format.
If you take a look at the code for Prepare:
The squaring happens because of:
Similarly the increase in size occurs because of the call to resize.
Exporting as png appears to occur in two places:
These would both need to change to use webp.
We could probably change the code in generateRandomAvatar to use webp too:
(It does not look like:
would need to change)
Unfortunately the golang x/image/png library does not support animation. There has been a feature request:
https://github.com/golang/go/issues/53364
but no action has been taken.
There are other libraries that use cgo to bind to libwebp and should provide animation support but we would need to change from using image.Decode to some other function and it would need to detect if there is animation present.
Gitea at present will probably just take the first frame of an animated gif. So I think animation is going to take a bit of work.
As far as I can see resize.Resize
(and in some cases cutter.Crop
) will convert the avatar into image.RGBA
which decompresses the image and may make it quite large - taking up memory. This does not seem quite ideal.
So I think what we need to do is to rethink about what the avatar code is supposed to do.
Should we be doing any of this resizing and cropping at all? Should we simply be enforcing our max height and width and storing like that?
I don't think we need to square or crop. Browser rendering can and should take care of letterboxing the image. We can certainly ensure this in our rendering. If other sites that embed gitea avatars assume our avatars are always square, they will need to fix it themselves.
Should we store as webp?
Yes, browser support is good enough that we can store a well-compressed webp.
Should we look at being able to store animations?
Animations are nice to have, but not a requirement for me. Happy to defer this to https://github.com/golang/go/issues/53364.
Should we look at being able to store animations?
this is not needed. animated avatars can be fun on a casual website, but on a serious website like Gitea they would be just annoying.
Should we be doing any of this resizing
I guess resizing could be considered above a certain pixel count. Let's say a lazy user directly uploads a photo taken from a digital camera which is let's say 50 Megapixels. We'd definitely want to downsize that to not destroy rendering performance and bandwidth consumption for other users.
I would say we could use a Megapixel constant above which to downsize the image to something more suitable for web consumption. Maybe make it 1 Megapixel, which is a 1000x1000 image that is suitable for fine rendering on 4dppx devices down to 250px width/height, which I think is close to avatars rendered on user pages.
OK looking at encoding directly to webp we can't use x/image/webp and would need to use something that wraps the libwebp C library or write a PR to x/image/webp to do the encoding.
The first option I came across: https://github.com/kolesa-team/go-webp requires that I install libwebp-dev on my system - this would likely cause a significant issue with compiling on Windows.
https://github.com/chai2010/webp appears to wholesale import the libwebp source - which should work but we'd have to keep on top of ensuring that it's up to date.
https://git.sr.ht/~jackmordaunt/go-libwebp is a Ccgo port of libwebp to go - which could work without cgo - however, the library is marked experimental so is questionable.
https://github.com/chai2010/webp sounds like the best bet to me, but it still needs CGO, right? I thought we could get rid of CGO eventually, so I doubt that adding a new CGO dependency will be worth it.
I guess as a first step, we can go and alter the resizing/cropping mechanism as discussed while still outputting well compressed PNGs, which in turn would solve https://github.com/go-gitea/gitea/issues/8972.
I guess introducing a full featured webp/png converter would bloat Gitea not just a little ....
I guess introducing a full featured webp/png converter would bloat Gitea not just a little
If I recall correctly, gitea already converts all uploaded avatars to PNG, so we do already feature the PNG encoder. Problem is just that the encoding is not doing any compression I think, resulting in unnecessarly huge avatar image sizes.
IMO CGO is somewhat better than CCGO at the moment, CCGO is not that popular and users will be locked in it (there is a stale PR: "Pure Go SQLIite", IIRC it also uses CCGO ....)
Of course CGO is not ideal either, it affects generated executable.
A new brief idea: introduce sidecar/external-plugin mechanism: provide a "GiteaImageConvert" server, if users need it, they could deploy it with Gitea together, then Gitea will use it to convert images. If such mechanism can be made general and stable enough, the CustomAvatarServer/PagesServer and more features could be integrated with Gitea while keeping Gitea itself as simple as possible.
A new brief idea: introduce sidecar/external-plugin mechanism: provide a "GiteaImageConvert" server, if users need it, they could deploy it with Gitea together, then Gitea will use it to convert images. If such mechanism can be made general and stable enough, the CustomAvatarServer/PagesServer and more features could be integrated with Gitea while keeping Gitea itself as simple as possible.
I'd certainly not see this necessary for the avatar encoder. png encode is in the stdlib so will be forever stable.
The problem is webp
......
Yes, webp is another topic, but i don't think it's one to invest too much time into currently as the size benefits over PNG aren't that great if both are compressed well. Sooner or later, a proper pure golang webp encoder will emerge and then we will use it.
Problem is just that the encoding is not doing any compression I think, resulting in unnecessarly huge avatar image sizes.
After reading the code, I think png.Encode
already does default compression
.
png.Encode -> writeIDATs -> e.writeImage(e.bw, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))
The CompressionLevel
defaults to 0, aka DefaultCompression -> zlib.DefaultCompression
But, the affect is not good?
I didn't test this, but if https://github.com/go-gitea/gitea/issues/8972#issuecomment-1542513735 is to be believed, something may be wrong, either the upsizing (that we should remove and only downsize above 1 megapixel) or the compression would be my guess.
we should downsize to 1024px² instead of 1000px² so that the downsized images could be rendered slightly faster.
citation: Make Better Textures, The 'Power Of Two' Rule & Proper Image Dimensions (this is for games but i think it applies to computer images everywhere)
we should downsize to 1024px² instead of 1000px² so that the downsized images could be rendered slightly faster.
Good idea, that makes the downsize threshold 1048576 pixels then.
OK now I have a PR that will make avatars webp we can look at the resize/crop code.
Its helpful first to understand what we already have:
Default values are:
This rejection is important because resize and crop will create RGBA images - so those could be huge byte slices.
So we are currently downsizing/upsizing everything to 290 x 290 pixels.
crop
code portionimgCfg.Width > 290 || imgCfg.Height > 290
maxDim = max(imgCfg.Width, imgCfg.Height)
imgCfg.Width/maxDim*290, imgCfg.Height / maxDim * 290
- Only resize if imgCfg.Width > 290 || imgCfg.Height > 290
- Resize to imgCfg.Width/maxDim290, imgCfg.Height / maxDim 290
I see 290 was chosen because it is the size of the biggest rendered avatar on the user page. But we should actually take into account Avatar.RenderedSizeFactor
for this resize, so that would with the default value of 3 make the size 870 above which to resize and to which resize to.
Other points sound good.
-> Improve avatar uploading / resizing / compressing #24653
JPG/PNG/APNG/WEBP all works (for most cases)
Feature Description
As discussed in https://github.com/go-gitea/gitea/pull/24248#issuecomment-1518131032 and below, we should evaluate converting and optimizing all avatars to WEBP. Currently all input formats (WEBP,PNG,JPG,GIF) end up as PNG during the avatar processing which squares them.
Notably, animation via APNG or WEBP input would be nice to have as well because WEBP can do animation, so even APNG should be convertable. Currently this avatar convertion strips APNG animation, would be nice if it can be preserved.
Related old issue: https://github.com/go-gitea/gitea/issues/18907