okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
331 stars 44 forks source link

#2411 - Compress large images attached in the chat #2428

Open SebinSong opened 3 days ago

SebinSong commented 3 days ago

closes #2411



[NOTE]

cypress[bot] commented 3 days ago

group-income    Run #3419

Run Properties:  status check passed Passed #3419  •  git commit a264fa407c ℹ️: Merge db4564d79b83fd5d1926a472758128d226ba5db7 into 3b142e94445de8ab49b435e8d4bd...
Project group-income
Branch Review sebin/task/#2411-compress-images-before-uploading
Run status status check passed Passed #3419
Run duration 09m 18s
Commit git commit a264fa407c ℹ️: Merge db4564d79b83fd5d1926a472758128d226ba5db7 into 3b142e94445de8ab49b435e8d4bd...
Committer Sebin Song
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 111
View all changes introduced in this branch ↗︎
taoeffect commented 3 days ago

So if the browser supports webp format, the outcome will always be webp and otherwise jpeg.

Question: what browsers don't support webp?

And if there are browsers that we want to support that don't support webp, wouldn't that mean that we shouldn't use it at all (since some users wouldn't be able to view the images)?

corrideat commented 2 days ago

I've seen this new PR and I have a few general comments.

First off, and this is a subjective point, WebP is a Google-controlled format and I wouldn't use it without fallback for this very reason, favouring formats that are real standards like JPEG, JPEG XL, AVIF or HEIC (although HEIC is patent-encumbered and therefore might not be a good choice) (*). Furthermore, JPEG is more widely supported than WebP. If there's a potential for users to want to save the images, I'd stick to JPEG because anecdotally users aren't familiar with WebP and may not know what to do with them or may not be as widely supported. For example, on Windows JPEG opens by default on Paint and WebP opens by default on Edge.

Secondly, although WebP sometimes (often even) has superior compression than JPEG, I suspect most of the impressive gains here aren't because of WebP but rather because of re-encoding the image. Although I haven't tried this, I strongly suspect that doing the same thing but with JPEG will yield similar results (albeit the JPEG files will be on average slightly larger, slightly meaning about 30% larger, more or less).

Thirdly, I would avoid doing these operations without providing a way to 'opt out' because re-encoding images is a lossy process that may be unwanted. WhatsApp, for example, compresses images to sizes that display well in mobile, but provides a way to opt out by sending the image as a 'file'.

Fourthly, responding to Greg:

Question: what browsers don't support webp?

All major browsers (Chrome, Edge, Firefox, Safari) and their derivatives support WebP by default, although this can be configured.

And if there are browsers that we want to support that don't support webp, wouldn't that mean that we shouldn't use it at all (since some users wouldn't be able to view the images)?

This is a good point. I think the testing for WebP support is misplaced because, while it may be necessary, it can't test the viewing side.

Based on this, I'd suggest the following changes:

  1. Dropping WebP (admittedly, this point has subjective reasons)
  2. Dropping the resizing factor as well in favour of a max size that displays well in the range of devices GI is expected to run. See https://gs.statcounter.com/screen-resolution-stats/ for common screen sizes.
  3. Provide some way to opt out of post processing images when it'd be undesirable, since this reduces quality and destroys metadata.

(*) Of these, JPEG and AVIF are the only realistic choices (other than WebP) in terms of widespread support. AVIF should compress as well as WebP on average, but efficient AVIF encoding is extremely slow from my experience. Moreover, AVIF has the same 'format weirdness' that WebP has, as do all of the newer formats, so in reality it's JPEG or WebP.

corrideat commented 2 days ago

And a last comment:

The 'long time to load' issue can be addressed by resizing images to different sizes, so that devices with small screens (which are also more likely to have a slower connection) load images faster. However, this would have the effect of requiring more server storage than a one-size-fits-all solution.

SebinSong commented 2 days ago

@taoeffect

Question: what browsers don't support webp?

That idea was based on this can-I-use result: https://caniuse.com/webp

I saw 96.81% there (Also, I myself has not been used this image format commonly in my life either) and so I went ahead to search to see if the world has a javascript logic to check the browser support for it. and then I ran into this article written by Google: https://developers.google.com/speed/webp/faq#in_your_own_javascript

The same article has Which web browsers natively support WebP? section there too and it appears the support level is quite good though. So I can remove the logic to check the support if you think it's not necessary.

SebinSong commented 2 days ago

@corrideat

First off, and this is a subjective point, WebP is a Google-controlled format and I wouldn't use it without fallback.

This is a subjective opinion. So not taking it unless Greg wants to drop it.

  1. Dropping the resizing factor

Like I explained in a comment above, reducing the quality argument to canvas.toBlob and image dimension at the same time have made the image outcome less blurry, when I tested (via this codepen I created. Actually countless times with various different image formats.) Also, this is an approach this article suggests too.

Although I haven't tried this, I strongly suspect that doing the same thing but with JPEG will yield similar results.

Try using canvas.toBlob(imageEl, type, quality) with specifying type to either image/jpeg or image/webp and then compare the results. you will see that passing image/webp achieves similar level of file-size compression with higher value of quality argument.

  1. Provide some way to opt out of post processing images when it'd be undesirable.

I think this is something out of scope of the issue #2411 . You can discuss with Greg after this PR about adding this feature to the app and create an issue and work on it yourself.

corrideat commented 2 days ago

Like I explained in a comment above, reducing the quality argument to canvas.toBlob and image dimension at the same time have made the image outcome less blurry,

I didn't say not to drop the image size. What I said, however, is resizing the image to a fixed size. The reason for this is that most of the image size savings come from reducing the image resolution regardless of image format, it is more consistent and the image will appear complete. For example, if I upload a 40000px by 30000px image, unless I really intend the image to be displayed in its entirety (see point 3 about opting out), likely it'll display well at 2000px by 1500px in any reasonable screen that people use. This is what chat apps already do.

will see that passing image/webp achieves similar level of file-size compression with higher value of quality argument.

Yes, that is expected. WebP is supposed to compress better (I have some counterexamples where it doesn't), which ultimately depends on a lot of factors, including the specific encoder used. There's a Google study on this in fact, which found exactly this: https://developers.google.com/speed/webp/docs/webp_study.

SebinSong commented 2 days ago

@corrideat Thanks for clarification. Now I understand what you meant. Then, I feel we would def need to make sure the image's physical size doesn't get too small and too large either. I will go with 1024×768 you suggested first and see what Greg thinks.

SebinSong commented 2 days ago

Updated the PR again here with Ricardo's feedback:


So if the given image's physical size is beyond width 1024px and height 768px, the compressed image's dimension will be reduced to those max values. Otherwise, it doesn't change the dimension at all and only uses quality parameter to canvas.toBlob() for compression. (Just tested a few images after this update and it seems to work well too)

Let me know if we would like to change 1024X768 to something else here.