Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: /public/avatar and /:sharing-id/recipients/:index/avatar #3679

Open
Peltoche opened this issue Jan 18, 2023 · 3 comments
Open

Discussion: /public/avatar and /:sharing-id/recipients/:index/avatar #3679

Peltoche opened this issue Jan 18, 2023 · 3 comments

Comments

@Peltoche
Copy link
Contributor

Hi!

I would like to propose some improvement for the initial avatar system. This issue is created in order to discuss around the subject.

Context

The endpoints /public/avatar and /:sharing-id/recipients/:index/avatar are both used to retrieve a user avatar. Those avatar pic are png files with a round of color and 1 or 2 letters in it. They are looking a lot like the ones generated by this service.

Those endpoints are called to illustrate the users avatar inside the "Share" column in drive and inside the "Share" pop-in used to manage the sharing rules.

In order to generate those png files the convert binary from ImageMagick is called and then cached inside a cache service in order to improve the performances.

Issue

IMHO there is several issues with this process that can be improved:

  1. Using the convert binary force a external dependency to ImageMagick and this is a really big library.
  2. The convert command need a write/read into the filesystem and so it require an access to the filesystem.
  3. The call to an external binary and the access to the FS cost a lot in CPU and time. This is why a cache is required.
  4. The PNG format use rasterization which can be relatively heavy compared to another encodings

Proposed improvements

Do not use those endpoints

You already have a component in you ui librairy that does generate icons without any call to the server. This could be the easiest way to remove an http call, reduce the load on server side and reduce the latency on client side.

Use ajstarks/svgo in order to render an svg

After some tests I have successfully replaced the png generation with convert by the use of a full Go natif library generating an svg. Those change seems to resolve all the issues listed above:

  1. There is no need to use the convert binary anymore so no more dependencies.
  2. There is no more need to access the filesysteme.
  3. The performances are greatly improved, to the point that I guess the cache would not be required anymore
  4. By using SVG format I have successfully reduced the icon size from 3.3KB for the PNG file to 303B for the SVG.

Bonus point: With the SVG format you are using vectorization so your icon would have a perfect resolution whatever the size.

If you are interested I can propose you a PR with a "svgo" implementation for the initial generation.

@nono
Copy link
Member

nono commented Jan 19, 2023

Hi,

Issue

  1. Using the convert binary force a external dependency to ImageMagick and this is a really big library.
  2. The convert command need a write/read into the filesystem and so it require an access to the filesystem.

Yes, but we would still need ImageMagick without this feature. ImageMagick is used to generate thumbnails for uploaded images from the users, and it is quite good for being able to do that with weird formats.

Do not use those endpoints

We make the choice to add an endpoint and make the call to it from webapps, even if we have a component in cozy-ui for that. The reason is that the initials are just a placeholder. Later, we will want to allow users to choose their own avatar. But it is some works to pick a file, crop it, etc. and it has not been prioritized for the moment. Having the endpoint will make things smoother when we will implement that.

Use ajstarks/svgo

Looks good to me.

@Crash-- just to be sure, you don't see any issue by using SVG for the initial avatars?

@Crash--
Copy link
Contributor

Crash-- commented Jan 20, 2023

@Crash-- just to be sure, you don't see any issue by using SVG for the initial avatars?

I just checked on cozy-sharing, the lib just take the URL the stack returns and send it to Avatar (https://github.com/cozy/cozy-libs/blob/2896910f28d12e829f46b429ab25c1b84841a674/packages/cozy-sharing/src/components/Recipient.jsx#L144-L153) and Avatar is only doing an <img src={link} (https://github.com/cozy/cozy-ui/blob/master/react/Avatar/index.jsx#L68) so I think we're good on that.

Also, it seems that we're only adding background-color on the parent element, so I think we're good too on this path.

I don't see any issue by using SVG for initial avatars! Au contraire

@Peltoche
Copy link
Contributor Author

Yes, but we would still need ImageMagick without this feature. ImageMagick is used to generate thumbnails for uploaded images from the users, and it is quite good for being able to do that with weird formats.

I have hope to find a go lib for that but this is an another subject for later.

We make the choice to add an endpoint and make the call to it from webapps, even if we have a component in cozy-ui for that. The reason is that the initials are just a placeholder. Later, we will want to allow users to choose their own avatar. But it is some works to pick a file, crop it, etc. and it has not been prioritized for the moment. Having the endpoint will make things smoother when we will implement that.

This will be so cool!

I have done some tests in local and it seems to work but if you want I can put this change behind a feature flag in order to allow you to test internally. I only need an example of feature flag setup. What do you think of it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants