Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

MImageBody thumbnail use thumbnail_info for max w and h #4718

Closed
wants to merge 1 commit into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jun 5, 2020

Fixes element-hq/element-web#13918
If the image was really wide such that it'd hit the Synapse limit of 800px wide but wasn't tall enough to hit our maxImageHeight of 600 then it'd get stretched horizontally causing blur.

Before:
image
image

After:
image
image

(shows both encrypted and unencrypted [relying on server thumbs])

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy requested a review from a team June 5, 2020 15:46
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm code-wise, though iirc the 1.0 redesign wanted images that grew with the timeline width. The blurry artifacts might have been an unintended side effect of this as after a point we might have been meant to use the raw image instead of trying to thumbnail to 1080p?

@t3chguy
Copy link
Member Author

t3chguy commented Jun 5, 2020

we always show thumbnails unless the user is on a HiDPI display. I am on a 5220 x 1440 display which is not hidpi but 4 times the width needed to see blurring in the image in the issue.

@matrix-org/design what do we want to do here:

  • stretch the thumbnail making it blurry
  • show the thumbnail at its maximal size
  • fetch the potentially insanely large image which could take forever to load but not be blurry

@t3chguy t3chguy requested a review from a team June 5, 2020 17:40
@nadonomy
Copy link
Contributor

  • show the thumbnail at its maximal size

Definite no go, as the varying widths on the timeline would feel terrible.

  • stretch the thumbnail making it blurry
  • fetch the potentially insanely large image which could take forever to load but not be blurry

Why don't we show the thumbnail to begin with for the best bandwidth and swap it out for the full size one if the user loads it?

@t3chguy
Copy link
Member Author

t3chguy commented Jun 18, 2020

@nadonomy

Why don't we show the thumbnail to begin with for the best bandwidth and swap it out for the full size one if the user loads it?

We do this, and it causes blurring due to stretching in the thumbnail. See element-hq/element-web#13918

@nadonomy
Copy link
Contributor

@nadonomy

Why don't we show the thumbnail to begin with for the best bandwidth and swap it out for the full size one if the user loads it?

We do this, and it causes blurring due to stretching in the thumbnail. See vector-im/riot-web#13918

I'm suggesting we swap out the thumbnail in the timeline once the user loads/views it— is that what we do? I assume not from the linked issue, but if we do already and users are confused we could look at exposing this better through the hover state.

@t3chguy
Copy link
Member Author

t3chguy commented Jun 18, 2020

No, we do not, but the complaint was about it ever getting stretched to the point it'd be blurry

@nadonomy
Copy link
Contributor

nadonomy commented Jun 18, 2020

Yup, my thinking though is:

  • It's disadvantageous to download large files pre-emptively, so better to use the thumbnail and expect it to be lower quality
  • Once we have the large file, that signifies user intent and the client has the full size, so we may as well use it
  • Even if not immediately obvious, I think users would learn what's going on over time, and we have easy options (e.g. hover states) to improve discovery if we need to

I've used other platforms that do this and it's a pretty pragmatic solution, so that'd be my preferred one if we're actioning this now.

@t3chguy
Copy link
Member Author

t3chguy commented Jun 29, 2020

Definite no go, as the varying widths on the timeline would feel terrible.

Why don't we just max-width 800px - a lot of users complain that the thumbnails are HUGE, 800px width is the limit Synapse imposes on thumbnailing

@SimonBrandner
Copy link
Contributor

Fixes #1520

@turt2live
Copy link
Member

@t3chguy this may be superseded by #7017 (sorry)

@t3chguy t3chguy closed this Oct 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Desktop zoom makes images look very blurry
4 participants