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

Thumbnail API is called with non-integer width #9690

Closed
spantaleev opened this issue May 13, 2019 · 9 comments
Closed

Thumbnail API is called with non-integer width #9690

spantaleev opened this issue May 13, 2019 · 9 comments
Labels
A-Media P1 T-Defect Z-Community-PR Issue is solved by a community member's PR

Comments

@spantaleev
Copy link
Contributor

Description

Using riot-web v1.1.0, I'm observing that the media thumbnail API is invoked with query parameters like this: ?width=1066.6666666666665&height=800.

Synapse responds with a 400 HTTP error response (Query parameter b'width' must be an integer), which results in no thumbnail rendering in the timeline.

The event's content is like this:

{
    "info": {
      "h": 3648,
      "mimetype": "image/jpeg",
      "orientation": 0,
      "size": 1800190,
      "thumbnail_info": {
        "h": 827,
        "mimetype": "image/jpeg",
        "size": 312435,
        "w": 620
      },
      "thumbnail_url": "mxc://........",
      "w": 2736
    },
    "url": "mxc://........",
    "body": "IMG.jpg",
    "msgtype": "m.image"
  }

The thumbnail API specifically calls for an integer, so having it respond with an error seems reasonable.

I'm guessing matrix-js-sdk's getHttpUriForMxc is invoked with a non-integer width parameter.

Fixing both riot-web (to round its numbers) and matrix-js-sdk (to either implicitly force-round or throw an exception) is probably ideal.

Log: not sent

Version information

  • Platform: web (in-browser), v1.1.0

For the web app:

  • Browser: Firefox
  • OS: Arch Linux
  • URL: somewhere else
@rkfg
Copy link
Contributor

rkfg commented May 20, 2019

Same. It seems it's caused by non-integer DPI scaling. I have GDK_SCALE and GDK_DPI_SCALE env vars set to 1.2 and this bug indeed happens. When I unset them everything works again.

@spantaleev
Copy link
Contributor Author

I don't have any GDK_ variables set, running on KDE Plasma.

I do have layout.css.devPixelsPerPx set to 1.2 in Firefox's about:config though.
But, resetting that (to 1) doesn't seem to fix the problem.

@rkfg
Copy link
Contributor

rkfg commented May 24, 2019

There are other variables for Qt: QT_SCALE_FACTOR and QT_AUTO_SCREEN_SCALE_FACTOR for example. What's your display resolution and do you use system-wide scaling?

And just like you said, rounding width and height fixes the issue:

diff --git a/src/content-repo.js b/src/content-repo.js
index 2a176515..8a5f9f06 100644
--- a/src/content-repo.js
+++ b/src/content-repo.js
@@ -51,10 +51,10 @@ module.exports = {
         const params = {};
 
         if (width) {
-            params.width = width;
+            params.width = Math.round(width);
         }
         if (height) {
-            params.height = height;
+            params.height = Math.round(height);
         }
         if (resizeMethod) {
             params.method = resizeMethod;

@spantaleev
Copy link
Contributor Author

I have QT_AUTO_SCREEN_SCALE_FACTOR=0.

Given that Force Fonts DPI is set to 115:

  • with layout.css.devPixelsPerPx = 1.2, window.innerWidth is 1440.
  • with layout.css.devPixelsPerPx = 1, window.innerWidth is 1728.
  • with layout.css.devPixelsPerPx = -1, window.innerWidth is 1440.

When Force Fonts DPI is disabled:

  • with layout.css.devPixelsPerPx = 1.2, window.innerWidth is 1440.
  • with layout.css.devPixelsPerPx = 1, window.innerWidth is 1760.
  • with layout.css.devPixelsPerPx = -1, window.innerWidth is 1760.

Regardless of which combination I use, the thumbnail never loads.
I'm logging in to riot-web from a fresh container tab each time, to prevent it having memorized some weird sizes.

@rkfg
Copy link
Contributor

rkfg commented May 24, 2019

Hmm, then it's not related. I was asking because it was never an issue (if memory serves me right) at work on a 1920x1080 display and I had thumbnails occasionally not loading at home on a 2560x1440 display with scaling enabled (1.2 just as yours).

@spantaleev
Copy link
Contributor Author

Maybe there's something else at play here, because my display resolution is 1920x1080.
Having window.innerWidth max out at 1760 sounds weird.

I am usually on a 2560x1440 monitor as well, so it may be that it's not exhibiting this problem. I'll be able to check on that one some next day.

@spantaleev
Copy link
Contributor Author

Looks like I can reproduce this on my 2560x1440 display as well.

Besides Force Fonts DPI (on KDE), and Firefox's layout.css.devPixelsPerPx, it looks like another important thing that affects window.innerWidth (and screen.width alike, it having the same value) is the Zoom level for the page.

On my 2560x1440 monitor, with Force Fonts DPI disabled and with layout.css.devPixelsPerPx disabled (-1.0):

  • without Zoom (at 100%), window.innerWidth is 2560. Thumbnails are requested with integer widths and load correctly.
  • with +10% Zoom (at 110%), window.innerWidth has a weird non-even value of 2347. Why such a number and not something like 2304, I don't know. In any case, riot-web doesn't work well with that.

@spantaleev
Copy link
Contributor Author

I've finally managed to dig into the code and find out that it's just window.devicePixelRatio which is taken into account.

For me, window.devicePixelRatio would sometimes have a weird value (e.g. 1.5789473684210527) and that would cause matrix-react-sdk to calculate some non-integer thumbWidth / thumbHeight values.

I've patched both:

This should fix both riot-web and any other client which may be invoking mxcUrlToHttp() or getHttpUriForMxc() with non-integer width/height.

@turt2live turt2live added the Z-Community-PR Issue is solved by a community member's PR label May 27, 2019
@turt2live
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Media P1 T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

No branches or pull requests

4 participants