Avoid requesting an Avatar image bigger than what the template asks for #17422

strk · 2021-10-24T17:16:20Z

Hopefully fixes #16287

It makes NO sense to request an avatar of 1160x1160 when it is rendered MUCH MUCH smaller.
See https://try.gitea.io/strk and right-click -> open image in new tab, then check the URL, is formed with ?size=1160 now. I think this PR makes it ask for a size=256 instead (default 128, sizeFactor 2)

Hopefully fixes go-gitea#16287

wxiaoguang · 2021-10-24T17:19:48Z

Very curious, how is the s=1160 generated? Is it related to this PR or something else?

The DefaultAvatarPixelSize and AvatarRenderedSizeFactor are just moved from old code, I have no idea why these two values were chosen ...

strk · 2021-10-24T17:22:15Z

@wxiaoguang as reported in #16287 I think it came from a request of 290 as the default size and a factor of 4.

Now I see that the 290 value is STILL in modules/avatar/avatar.go (AvatarSize) and is DIFFERENT from the DefaultAvatarPixelSize that I just changed from 28 to 128 in models/avatars/avatar.go -- I'm not sure what uses which of the two values, and if one should just be discarded/dropped

wxiaoguang · 2021-10-24T17:24:15Z

OMG, I think we should refactor modules/avatar/avatar.go together. There must be some wrong usages of these values.

strk · 2021-10-24T17:25:24Z

Indeed, I don't even understand why would we want to use a AvatarRenderedSizeFactor

All I know is that we do NOT want a 1160x1160 image fetched from avatar services by the browsers of the poor Gitea users

strk · 2021-10-24T17:25:47Z

I'm seeing that AvatarSize is used for the generated avatars

wxiaoguang · 2021-10-24T17:34:26Z

We need more time to think about the avatar size problem.

And don't worry, the URL https://try.gitea.io/avatar/fe2a9e759730ee64c44bf8901bf4ccc3?size=1160 and https://avatars.kbt.io/avatar/fe2a9e759730ee64c44bf8901bf4ccc3?d=identicon&s=1160 won't bring you a image with size=1160, it just returns a normal image.

wxiaoguang · 2021-10-24T17:38:10Z

The template requests to get a image with size 290 .... 😭

wxiaoguang · 2021-10-24T17:40:06Z

Indeed, I don't even understand why would we want to use a AvatarRenderedSizeFactor

The reason for AvatarRenderedSizeFactor, I guess it makes the displayed image clearer on a 2K or 4K Hi-DPI monitor.

strk · 2021-10-24T17:40:33Z

We need more time to think about the avatar size problem.

And don't worry, the URL https://try.gitea.io/avatar/fe2a9e759730ee64c44bf8901bf4ccc3?size=1160 and https://avatars.kbt.io/avatar/fe2a9e759730ee64c44bf8901bf4ccc3?d=identicon&s=1160 won't bring you a image with size=1160, it just returns a normal image.

It depends on the service. avatars.kbt.io is a service I control, so I know it won't return a bigger image, but other services (it's a federated service) could return a bigger one. What happens on my service is that if someone asks for an avatar bigger than the biggest image I have of my avatar, it returns that silly drawing instead of a picture of myself, that's why I noticed it :)

strk · 2021-10-24T17:41:44Z

Indeed, I don't even understand why would we want to use a AvatarRenderedSizeFactor

The reason for AvatarRenderedSizeFactor, I guess it makes the displayed image clearer on a 2K or 4K Hi-DPI monitor.

Yes, that's written in the comment, but I'm not using a Hi-DPI monitor so I'd think I should NOT get the SizeFactor applied. It should ideally be decided by JavaScript if one needs the scale applied or not, shouldn't it ?

wxiaoguang · 2021-10-24T17:44:52Z

It should ideally be decided by JavaScript if one needs the scale applied or not, shouldn't it ?

Theoretically, yes. But actually, many links are generated by backend code, which do not have knowledge about the frontend information (eg: what display device you are using).

So I think the avatar size problem is not an easy problem (and not a serious problem yet), we need think more about it and make a global plan.

strk · 2021-10-24T17:46:58Z

I'd say it's more important to get a 290x290 avatar (as the old times) and see it "pixelated" on 4K monitors than download a huge image (or wrong image, in my case) on a normal monitor.

strk · 2021-10-24T17:47:42Z

Do you have a screenshot of how those pages look on a 4K monitor, btw ?

wxiaoguang · 2021-10-24T17:56:49Z

I'd say it's more important to get a 290x290 avatar (as the old times) and see it "pixelated" on 4K monitors than download a huge image (or wrong image, in my case) on a normal monitor.

Agree, but we only need to change AvatarRenderedSizeFactor to 2 or 1 if we want to reduce the size.

Do you have a screenshot of how those pages look on a 4K monitor, btw ?

No. Actually, for myself, I do not care about the image quality of avatars ... but the code was here, I just happened to have done a refactoring.

About the AvatarRenderedSizeFactor and the avatar size, maybe we can ask @silverwind .

wxiaoguang · 2021-10-24T18:01:00Z

Oh, by the way, maybe we can introduce another const: MaxAvatarSize , then we can limit the requested size not larger than MaxAvatarSize. It's unreasonable to request a much bigger avatar image indeed.

strk · 2021-10-24T18:01:32Z

For the general plan, how about using a cookie to store wheter the user is using a 4K monitor and only then apply the ScaleFactor ?

wxiaoguang · 2021-10-25T01:59:26Z

For the general plan, how about using a cookie to store wheter the user is using a 4K monitor and only then apply the ScaleFactor ?

That's too complex, it sounds like we want to kill a chicken with a dragon sword.

lunny · 2021-10-25T02:21:48Z

The max size could be set since only serval places to reference the avatar. The biggest one is in user's profile page which is 260x260, with the border/padding/margin, it's 290x290. So I think we should change it to 260 but not 290. And 4x260=1040 is the size for Hi-DPI monitor.

We could detect the monitor at frontend, we can check window.devicePixelRatio to know the factor and decide the size to send to backend.

wxiaoguang · 2021-10-25T03:11:09Z

I just tested with GitHub, it's not that complex, I don't think we should use window.devicePixelRatio for this simple purpose.

For GitHub, it doesn't care about devicePixelRatio (it can be simulated in Chrome DevTools)

Small avatars <img> are all responded with 2x size. For example, the up right corner avatar is 20x20, the responded image is 40x40.

For large avatars, ~~I think they would use the same 2x logic.~~ they seem to use the original size.

We can just follow the main stream designs, instead of re-inventing the wheels.

wxiaoguang · 2021-10-25T03:41:22Z

models/avatars/avatar.go

@@ -19,10 +19,10 @@ import (
 )

 // DefaultAvatarPixelSize is the default size in pixels of a rendered avatar
-const DefaultAvatarPixelSize = 28
+const DefaultAvatarPixelSize = 128


This const shouldn't be changed. It is used for small avatars.

wxiaoguang · 2021-10-25T03:41:36Z

models/avatars/avatar.go


 // AvatarRenderedSizeFactor is the factor by which the default size is increased for finer rendering
-const AvatarRenderedSizeFactor = 4
+const AvatarRenderedSizeFactor = 2


This one seems fine. L-G-T-M.

But I think it just has been changed in #15941 from 2 to 4. @silverwind

strk · 2021-10-26T07:43:11Z

Unfortunately a x2 factor is still not good for me:
https://avatars.kbt.io/avatar/fe2a9e759730ee64c44bf8901bf4ccc3?s=580

And it still doesnt' make sense to multiply by 2 if the HTML tag keeps the original size anyway (check the generated HTML).
I'd drop that factor completely

wxiaoguang · 2021-10-26T08:28:37Z

As mentioned by lunny, the AvatarRenderedSizeFactor was changed by silverwind from 2 to 4 before, I think we should make a general agreement for the chosen value. Otherwise there will be more time spent on discussions.

I prefer to follow the GitHub way instead of re-inventing wheels.

For small avatar, factor = 2
For large avatar (in the profile page), do not touch the size, use the original size.

silverwind · 2021-10-26T10:04:06Z

I disagree on reducing AvatarRenderedSizeFactor. It should match or exceed the highest devicePixelRatio we support and certain mobile phones have this value at 4. My desktop browser for example runs on 2.22 currently, so even downgrading to 2 would be a quality loss for me.

What instead should be done is have the backend output dynamic image sizes based on what the browser requests by URL, e.g. <img src="/avatars/id?size=<size>" width="32" height="32" where size is width*devicePixelRatio. Unfortunately, devicePixelRatio can not be interpolated into HTML and there is no HTTP header for it, so it'd probably have to be a hardcoded value. GitHub currently hardcodes it at 2.

silverwind · 2021-10-26T10:31:11Z

https://github.com/disintegration/imaging might be useful for resizing, but it certainly can not be done on-the-fly on every request for performance reasons. One solution may be to maintain a disk-based cache of the commonly used sizes and regenerate it when a user changes their avatar.

silverwind · 2021-10-27T17:23:59Z

So we can just use the original image in for large avatar rendering (in user profile page), just as what GitHub does, and let browser do the scale.

I think GitHub either resizes all avatars to 460px on save or stores them in higher resolution and just defaults to HTTP endpoint to 460px when no size argument is given. Generally, I think we don't need not special-case the avatar on the user page. It's just another avatar image that is bigger than others, but not special in any other way.

Resizing (to factor*280) and/or optimizing (pngcrush or zopflipng) avatar on save might be a good future change to prevent overly large avatars from eating bandwidth.

Save a bit of bandwidth by only requesting 3-times the rendered avatar size. Factor 4 is only really beneficial on a handful of mobile phones and I don't think they are the primary device we design for. Fixes: go-gitea#17422 Fixes: go-gitea#16287

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2021-12-15T17:48:51Z

@strk strk#1 makes this configurable.

codecov-commenter · 2021-12-15T18:08:29Z

Codecov Report

Merging #17422 (5ae0ac9) into main (f58e687) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #17422      +/-   ##
==========================================
+ Coverage   45.28%   45.30%   +0.01%     
==========================================
  Files         820      820              
  Lines       90932    90932              
==========================================
+ Hits        41179    41195      +16     
+ Misses      43194    43184      -10     
+ Partials     6559     6553       -6

Impacted Files	Coverage Δ
models/avatars/avatar.go	`31.76% <ø> (ø)`
modules/queue/workerpool.go	`49.23% <0.00%> (-1.91%)`	⬇️
modules/queue/queue_channel.go	`95.00% <0.00%> (-1.67%)`	⬇️
modules/queue/queue_bytefifo.go	`59.28% <0.00%> (-0.60%)`	⬇️
services/pull/pull.go	`41.78% <0.00%> (-0.41%)`	⬇️
models/issue_comment.go	`53.04% <0.00%> (+0.56%)`	⬆️
models/repo_list.go	`84.01% <0.00%> (+0.81%)`	⬆️
models/notification.go	`65.73% <0.00%> (+0.86%)`	⬆️
modules/notification/ui/ui.go	`62.31% <0.00%> (+1.44%)`	⬆️
modules/process/manager.go	`82.94% <0.00%> (+1.55%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f58e687...5ae0ac9. Read the comment docs.

Make avatar size factor a configurable option

strk · 2021-12-15T21:25:20Z

Thanks @zeripath - I've merged your PR

zeripath

I'm going to approve this as it's now configurable.

I don't mind which setting is used but 4 is almost certainly too high and 1 may be too low.

However, in the interests of getting this merged I will approve.

lafriks · 2021-12-15T21:56:33Z

Can we make default more close to current behavior, for example 3?

silverwind · 2021-12-15T22:18:33Z

Yes, default should be 3 for reasons outlined in #17951.

zeripath · 2021-12-15T23:05:22Z

I put a pr against @silverwind's to add configuration - I'd be happy to approve that or if you want to make the suggestions against this pr to make the default 3 I think that would be acceptable to.

silverwind · 2021-12-15T23:45:45Z

#17951 is ready again.

Save a bit of bandwidth by only requesting 3-times the rendered avatar size. Factor 4 is only really beneficial on a handful of mobile phones and I don't think they are the primary device we design for. Configurability contributed by zeripath. Fixes: go-gitea#17422 Fixes: go-gitea#16287

silverwind

Blocking merge as the consensus seems to be factor 3 is a good default. #17951 replaces this PR.

wxiaoguang · 2021-12-16T00:56:25Z

Oh, I see the different, then we use #17951 😊

Save a bit of bandwidth by only requesting 3-times the rendered avatar size. Factor 4 is only really beneficial on a handful of mobile phones and I don't think they are the primary device we design for. Configurability contributed by zeripath. Fixes: #17422 Fixes: #16287 Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

strk · 2021-12-16T16:40:56Z

Thanks guys !

…17951) Save a bit of bandwidth by only requesting 3-times the rendered avatar size. Factor 4 is only really beneficial on a handful of mobile phones and I don't think they are the primary device we design for. Configurability contributed by zeripath. Fixes: go-gitea#17422 Fixes: go-gitea#16287 Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Bring up default avatar size to 128 and sizeFactor down to 2

0dca9c0

Hopefully fixes go-gitea#16287

strk requested a review from wxiaoguang October 24, 2021 17:16

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 24, 2021

wxiaoguang reviewed Oct 25, 2021

View reviewed changes

Default avatar size back to 28 and AvatarSizeFactor to 1

2e8cb1e

strk changed the title ~~Bring up default avatar size to 128 and sizeFactor down to 2~~ Avoid requesting an Avatar image bigger than what the template asks for Oct 26, 2021

silverwind mentioned this pull request Dec 10, 2021

Make AvatarRenderedSizeFactor configurable and set it to 3 #17951

Merged

zeripath added 3 commits December 15, 2021 17:30

fix-tests

77f50b6

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge remote-tracking branch 'origin/main' into smaller-avatar

5ae0ac9

Make avatar size factor a configurable option

4af65be

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge pull request #1 from zeripath/smaller-avatar

88a95d2

Make avatar size factor a configurable option

zeripath approved these changes Dec 15, 2021

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 15, 2021

wxiaoguang approved these changes Dec 16, 2021

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 16, 2021

silverwind requested changes Dec 16, 2021

View reviewed changes

wxiaoguang added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Dec 16, 2021

lunny closed this in #17951 Dec 16, 2021

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Avoid requesting an Avatar image bigger than what the template asks for #17422

Avoid requesting an Avatar image bigger than what the template asks for #17422

Conversation

strk commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021 • edited

strk commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

strk commented Oct 24, 2021

strk commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

strk commented Oct 24, 2021

strk commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

strk commented Oct 24, 2021

strk commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

wxiaoguang commented Oct 24, 2021

strk commented Oct 24, 2021

wxiaoguang commented Oct 25, 2021

lunny commented Oct 25, 2021 • edited

wxiaoguang commented Oct 25, 2021 • edited

wxiaoguang Oct 25, 2021

Choose a reason for hiding this comment

wxiaoguang Oct 25, 2021

Choose a reason for hiding this comment

lunny Oct 25, 2021

Choose a reason for hiding this comment

strk commented Oct 26, 2021

wxiaoguang commented Oct 26, 2021 • edited

silverwind commented Oct 26, 2021 • edited

silverwind commented Oct 26, 2021

silverwind commented Oct 27, 2021

zeripath commented Dec 15, 2021

codecov-commenter commented Dec 15, 2021

Codecov Report

strk commented Dec 15, 2021

zeripath left a comment

Choose a reason for hiding this comment

lafriks commented Dec 15, 2021

silverwind commented Dec 15, 2021

zeripath commented Dec 15, 2021

silverwind commented Dec 15, 2021

silverwind left a comment

Choose a reason for hiding this comment

wxiaoguang commented Dec 16, 2021

strk commented Dec 16, 2021

wxiaoguang commented Oct 24, 2021 •

edited

lunny commented Oct 25, 2021 •

edited

wxiaoguang commented Oct 25, 2021 •

edited

wxiaoguang commented Oct 26, 2021 •

edited

silverwind commented Oct 26, 2021 •

edited