-
Notifications
You must be signed in to change notification settings - Fork 337
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
[Bug]: Image expand broken in 0.19.6-beta #2796
Comments
I don't think this is necessarily a backend issue. I can see this being a bit challenging from the ui perspective, as you'd ideally want to know whether it's an image before you try to load it, as thumbnails can include metadata from non-image URLs. |
I spose our use of the term thumbnail here is confusing. This is actually a picture resizing done whenever pictrs fetches a thumbnail from another site (IE images from metatags, or images from sites), and ppl don't use the pictrs image upload. This was done to decrease the amount of storage necessary. The web and app UI's should still be fine for their thumbnail displays for any image (because they use pictrs's on-demand resizing). The problem is that we have our max_resize set way too low. |
I really don't know what a good default should be here. This page discusses some different image sizes for various sites: https://www.istockphoto.com/blog/best-practices/design/what-are-standard-photo-sizes 1000-1200 seems like it would work. |
I know that PR, I linked it in the original issue. You'll have to either optimize for storage or for quality, you can't have both. It doesn't make a difference whether the image is uploaded to pictrs or external, as a separate thumbnail url will be generated regardless. Would-be thumbnail with current settings: https://lemmy.ml/pictrs/image/3c1ae8cf-8712-431a-9f5f-53457bbaf46d.png?thumbnail=256 The thumbnail is perfectly fine for the preview icon on the small square, but the expanded image should be full resolution. Additionally, lemmy-ui currently links to the thumbnail URL on the expanded image when it probably should link to the post URL, but that's a separate issue. |
I downloaded that image, and it's around 1800 width, so this one doesn't look distorted. https://lemmy.ml/pictrs/image/3c1ae8cf-8712-431a-9f5f-53457bbaf46d.png?thumbnail=1800 Basically if we use pictrs image resizing at all, we're guaranteed to create distortions with fetched images that are too large. I still think the solution is to just choose a sane default that's bigger than 256.
I'll have to check the code again for what value it puts in thumbnail url if its a local image upload. |
Are you talking about the image that gets expanded when you click on the thumbnail? That one is definitely too small. Here are the images:
And from the api response:
So its clear that the problem is with image expanding in lemmy-ui. Instead of loading the full-size image, it actually uses the thumbnail url. This used to work fine as thumbnails were not downscaled before, but now it obviously breaks. So lemmy-ui needs to be fixed to use the |
max_thumbnail_size
default of 256 is too small for most images.
Yes, that's exactly what I reported in #2793. |
Ok, I'll try to get this fixed now. |
* 0.19.6-beta.8 * 0.19.6-beta.9 * 0.19.6-beta.14 * Using full-size image column, not thumbnail. - Fixes #2796
Hi! Can this be reopened? This is still breaking apps such as Voyager that are using There are a few problems with the approach in #2797.
|
Before | After |
---|---|
Solution?
Honestly I'm not sure, but I think clients need exposed an image link that's not compressed, and is hosted/proxied locally to preserve privacy.
I do recognize the instance hosting size issues but there has to be a better way to clean up these cached high quality images after some period of time, and potentially re-fetch as needed later?
For the privacy aspect, instances do have the option to run an image proxy. |
As already mentioned, this can be handled by instance admins setting config value
How about increasing it to 512? I wouldnt go higher than that because it will result in unnecessary network traffic for most clients which display small thumbnails, and dont request a specific size from the server. |
I'd be good with upping it to 512, but ya as stated above, this was a bug in the lemmy-ui code that wrongly used the thumbnail url for the expanded image. The back-end proxyallimages setting is the way to do proxied images for security reasons. |
unfortunately this is still technically a breaking change |
I don't know if I'd call it breaking, there were no API changes in the thumbnail or url columns. Only changed the back-end resolution of the thumbnail column, which is entirely up to servers. I just fixed broken lemmy-ui code that incorrectly was using the thumbnail column where it should've used the url one for images. |
I agree, but using
This sounds like the best approach. Then, the
would be the I'd love that approach. I would suggest reverting the changes until this proposal is merged (and
Not in terms of the API model, but in terms of user experience it is :)
Social graph images are recommended to be 1200 x 630 on high resolution devices (facebook link warning). Again, |
I just learned that 0.19.6 was released, this is quite disappointing. |
I checked a few other apps (jerboa, thunder), and luckily they weren't using my bad lemmy-ui code for post images. I'd be up for using proxyallimages as the default in a future release, but that's a long way away yet. If I were you I'd just fix the code that's incorrectly using the thumnail column for full size images. There's no guarantee of the picture resolution for that column, for any server. It's a server-specific setting. |
It doesn't really matter. Voyager was broken, you guys were aware, and despite that a release was cut. I'll release an update, but come on. Releases take time to propagate! Even a week would have been good. Ugh! |
I apologize for that, we'd been delaying this release for months so we've been tryisg to get it out for a while. We also have very few ppl helping test our release builds, so the picture resolution change would've gotten discovered sooner if we had more testers. The next release has a lot of breaking api and federation changes, so we'll give at least a month or so for app devs to prepare for that one. |
^ Oops, misclick on mobile. |
Thanks for the apology. Another problem with this approach is regarding random remote instances going down while viewing your feed. I think originally Voyager used Another example is if a post goes viral from a small instance, with this change all those users are gonna hit the small instance. Versus using So I hope proxying (and caching) fullsize images on the local instance is prioritized. |
Requirements
Summary
Its reported here that the new max thumbnail size for fetched images has far too small a default.
Its good that we added this, but it should probably be increased, I'm not sure what to.
Real life example: https://lemmy.ml/post/22217391
Context: https://lemmy.ml/post/22203615
Steps to Reproduce
Technical Details
NA
Version
main, release/v0.19
Lemmy Instance URL
lemmy.ml
The text was updated successfully, but these errors were encountered: