-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
#2411 - Compress large images attached in the chat #2428
#2411 - Compress large images attached in the chat #2428
Conversation
group-income Run #3494
Run Properties:
|
Project |
group-income
|
Branch Review |
sebin/task/#2411-compress-images-before-uploading
|
Run status |
Passed #3494
|
Run duration | 08m 46s |
Commit |
321d2baf89 ℹ️: Merge 1985f28feda882fd83e1a646bca81364e2486c7c into 5f68446576179fd93a5428ebfba3...
|
Committer | Sebin Song |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
10
|
Skipped |
0
|
Passing |
111
|
View all changes introduced in this branch ↗︎ |
const sizeDiff = blob.size - IMAGE_ATTACHMENT_MAX_SIZE | ||
|
||
if (sizeDiff <= 0 || // if the compressed image is already smaller than the max size, return the compressed image. | ||
quality <= 0.3) { // Do not sacrifice the image quality too much. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the multiple tests I've done, when the quality factor value passed to canvas.toBlob()
becomes <= 0.3
, the outcome image starts to lose the quality to an unpleasant level. 0.3
here is a value I chose subjectively, but feel free to try compression test yourself using this codepen I created for this task and let me know if you would like a change.
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)? |
// url here is an instance of URL.createObjectURL(), which needs to be converted to a 'Blob' | ||
const attachmentBlob = await objectURLtoBlob(url) | ||
const attachmentBlob = needsIamgeCompression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in Image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. fixed it.
frontend/utils/image.js
Outdated
export async function compressImage (imgUrl: string, sourceMimeType?: string): Promise<any> { | ||
// Takes a source image url and generate a blob of the compressed image. | ||
|
||
// According to the testing result, 0.8 is a good starting point for both resizingFactor and quality for .jpeg and .webp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this shouldn't be a 'resizing factor' anyhow, but it should resize images to some arbitrary maximum size. E.g., not the real suggestion, the max size could be 1024×768, and images are scaled not to exceed this. The dimensions would come from how the images are displayed in the chat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like I wrote in the desc, check this codepen I created for this task : https://codepen.io/freenaturalsoul/pen/vYoMWWv?editors=0100
I tried compressiing various large size images there. try it there.
this shouldn't be a 'resizing factor' anyhow, but it should resize images to some arbitrary maximum size.
It doesn't seem like you are suggesting this without knowing what the goal of the issue..? The goal of the issue Greg wrote is to reduce the image file-size down below a certain value (400KB). Sacrificing the quality and the size here is inevitable, at least when using Canvas.toBlob()
.
According to tests I've done, only changing the quality
argument of that canvas API without reducing the image dimension along with it have lead to the outcomes getting blurry in earlier stages. That's why I chose an approach to decrease the quality factor and the dimension at the same time. Plus, that is something this article suggested too.
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:
All major browsers (Chrome, Edge, Firefox, Safari) and their derivatives support WebP by default, although this can be configured.
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:
(*) 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. |
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. |
That idea was based on this can-I-use result: I saw The same article has Which web browsers natively support WebP? section there too and it appears the support level is quite good though. |
This is a subjective opinion. So not taking it unless Greg wants to drop it.
Like I explained in a comment above, reducing the
Try using canvas.toBlob(imageEl, type, quality) with specifying
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. |
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.
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, |
@corrideat |
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 Let me know if we would like to change |
frontend/utils/image.js
Outdated
// If image's physical size is greater than the max dimension, resize the image to the max dimension. | ||
const imageMaxDimension = { width: 1024, height: 768 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double this to 2048 x 1536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that doubling it quadruples the image size.
2048*1536 / (1<<20) === 3
I.e., an uncompressed image of that size would take up 3 MiB. Of course, this would generally be less with JPEG or WebP compression, but it'll come with some quality loss.
ETA: This comment is meant just as an observation. Also, the uncompressed size is actually 9 MiB, not 3 MiB as originally stated. The reason is that there are three channels (RGB) instead of just one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Adobe, JPEG typically has a 10:1 compression ratio over BMP (BMP is essentially uncompressed):
Nonetheless, JPEG files often achieve compression of 10:1 with no significant decrease in quality. (https://www.adobe.com/creativecloud/file-types/image/comparison/bmp-vs-jpeg.html)
Assuming that WebP is about 30% more efficient and a ratio of 13:1, this is about 0.70 MiB. So, a 2048*1536
will likely have reduced quality to reach the 400 KiB goal.
@taoeffect a) if the image file size is > b) if image file size is > I was worried increasing this max dimension might lead to noticeable low quality of the compression outcome. But so far, I haven't seen any visibly bad quality from the compressed images. So this adjustment seems okay. |
frontend/utils/constants.js
Outdated
@@ -20,6 +20,7 @@ export const CHAT_ATTACHMENT_SUPPORTED_EXTENSIONS = [ | |||
// TODO: fetch this value from a server API | |||
export const MEGABYTE = 1 << 20 | |||
export const CHAT_ATTACHMENT_SIZE_LIMIT = 30 * MEGABYTE // in byte. | |||
export const IMAGE_ATTACHMENT_MAX_SIZE = 400000 // 400KB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a new entry for KILOBYTE
just below MEGABYTE
?
export const KILOBYTE = 1 << 10
Then:
export const IMAGE_ATTACHMENT_MAX_SIZE = 400 * KILOBYTE
This is a slightly larger number than 400000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
frontend/utils/image.js
Outdated
return blob | ||
} else { | ||
// if the size difference is greater than 100KB, reduce the next compression factors by 10%, otherwise 5%. | ||
const minusFactor = sizeDiff > 100 * 1000 ? 0.1 : 0.05 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this can be edited to 100 * KILOBYTE
once that constant is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work @SebinSong! Two minor changes related to defining KILOBYTE
and this should be good to merge 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work @SebinSong! 👏
closes #2411
[NOTE]
As @taoeffect suggested in the issue,
400KB
will be the size value to determine compression is needed.The app uses HTML canvas API (toBlob(), drawImage()) to compress the image on the browser, which is a popular client-side image-compression npm package internally uses as well.
The compression outcome will be one of two most popular images formats on the web that support lossy-compression (
image/webp
orimage/jpeg
). According to tests I've done (pls feel free to check out this codepen I created for testing for yourself too), converting to 'webp' format apparently has a better compression efficiency than 'jpeg' (meaning it drops the file size below 400KB without sacrificing too much quality). So if the browser supportswebp
format, the outcome will always bewebp
and otherwisejpeg
.