1554850 - Fractional DPI change not detected on Linux

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Vincent Bernat]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

When changing DPI from 96 DPI to 144 DPI (1.5 scale factor), Firefox stays unaffected.

Actual results:

No change.

Expected results:

Firefox should scale to the new DPI setting. Currently, on Linux, Firefox is listening to "notify::scale-factor" event. However, scale factor is only an integer value. I think Firefox should also listen to "notify::gtk-xft-dpi" which is used to compute scale factor. This would be something like:

g_signal_connect_after(default_settings, "notify::gtk-xft-dpi",
                       G_CALLBACK(scale_changed_cb), this);

(but it would need another callback as it is not attached to the widget)

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Yes, I can reproduce it. When fractional scaling changes and firefox is restarted it works fine. Without the restart we have wrong display metrics. But it looks like Gtk3 bug to me as we use Gtk3 function to place the popups.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

notify::gtk-xft-dpi is not related and it's not issued when scale changes. Also when the scale changes (from 100% to 200% and vice versa) the scale_change_cb is correctly called.

The fractional scaling renders by 100% or 200% scale to backbuffer and then applies the scale (1.5 for instance) on the backbuffer so the application can't get info about the 1.5 scale.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

It's also Wayland related issue.

Comment 4

[Vincent Bernat]

(In reply to Martin Stránský [:stransky] from comment #2)

notify::gtk-xft-dpi is not related and it's not issued when scale changes. Also when the scale changes (from 100% to 200% and vice versa) the scale_change_cb is correctly called.

The fractional scaling renders by 100% or 200% scale to backbuffer and then applies the scale (1.5 for instance) on the backbuffer so the application can't get info about the 1.5 scale.

I mentioned gtk-xft-dpi because just changing XSETTINGS' Xft/DPI value is enough for Firefox to pick the change (after a restart). GTK applications also handle correctly the change (without a restart), at least on X11, so I suppose they listen to this property as well. I am not using Gnome directly, I am using xsettingsd, modify its configuration file to change the value of Xft/DPI and send a HUP signal to trigger the change. Firefox is the only application not taking the change without a restart (but also the only one to support fractional scaling for non-text stuff, which is great!). GDK also has Gdk/UnscaledDPI which is mostly here to cancel Xft/DPI when using Gdk/WindowScalingFactor, but there seems to be no way to listen to it (or it's not documented). Maybe Gnome has another property to handle fractional scale, but I am not aware of it.

Comment 5

[Vincent Bernat]

It seems the situation is more complex. When invoking scale_changed_cb(), only the scaling factor is used, while on start, both the scaling factor and Xft/DPI are used. So, just invoking this callback doesn't help.

Comment 6

[Vincent Bernat]

After investigating a bit more, gfxPlatformGtk::GetFontScaleDPI() always return the initial value, preventing the scaling factor to be computed correctly when it is governed by font size (which is the only way to make it work when using fractional DPI):

static int32_t sDPI = 0;

int32_t gfxPlatformGtk::GetFontScaleDPI() {
  if (!sDPI) {
    // Make sure init is run so we have a resolution
    GdkScreen* screen = gdk_screen_get_default();
    gtk_settings_get_for_screen(screen);
    sDPI = int32_t(round(gdk_screen_get_resolution(screen)));
    if (sDPI <= 0) {
      // Fall back to something sane
      sDPI = 96;
    }
  }
  return sDPI;
}

Behind the scene, GTK/GDK are relying on gtk-xft-dpi and GDK_DPI_SCALE to compute the screen "resolution":

static void
settings_update_resolution (GtkSettings *settings)
{
  GtkSettingsPrivate *priv = settings->priv;
  gint dpi_int;
  gdouble dpi;
  const char *scale_env;
  double scale;

  /* We handle this here in the case that the dpi was set on the GtkSettings
   * object by the application. Other cases are handled in
   * xsettings-client.c:read-settings(). See comment there for the rationale.
   */
  if (priv->property_values[PROP_XFT_DPI - 1].source == GTK_SETTINGS_SOURCE_APPLICATION)
    {
      g_object_get (settings,
                    "gtk-xft-dpi", &dpi_int,
                    NULL);

      if (dpi_int > 0)
        dpi = dpi_int / 1024.;
      else
        dpi = -1.;

      scale_env = g_getenv ("GDK_DPI_SCALE");
      if (scale_env)
        {
          scale = g_ascii_strtod (scale_env, NULL);
          if (scale != 0 && dpi > 0)
            dpi *= scale;
        }

      gdk_screen_set_resolution (priv->screen, dpi);
    }
}

So, I would suggest to listen to notify::gtk-xft-dpi, reset sDPI value, then invoke the regular scale_changed_cb().

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Great, Thanks for your investigation. Do you mind to create a patch for it? I'll happily review it.
There's how-to build Firefox on Linux from sources:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation

Comment 8

[Samuel Kalbfleisch]

Attachment: dpi-dynamic-update_ff68.0.diff

I've attached a patch (based on Ubuntu 18.10's firefox-68.0+build3) which adds handling of changes of xsetting's Xft.DPI value. (I have the same use case as the original reporter, I update the DPI with my desktop environment's xsettings daemon when switching between high DPI and low DPI screens).

Without the window->Resize() call in nsWindow.cpp's settings_xft_dpi_changed_cb, the "viewport" of the UI becomes wrong after a DPI change. E.g. when DPI is reduced, part of the window becomes black (see attached screenshot; as if the "viewport" size is not calculated in device pixels). The call to Resize() fixes this, I'm not sure what exactly this triggers that actually fixes it though. So one might want to get rid of this workaround if anyone knows how.
(Without the Resize() call, the bug is worked around manually by just resizing the window).
nsWindow.cpp's existing scale_changed_cb calls OnSizeAllocate which calls (besides OnDPIChanged) window->OnScaleChanged(&allocation), I'm not sure why though, and in a quick test adding it to settings_xft_dpi_changed_cb didn't seem to fix the "viewport" problem (with Resize() removed).

In gfxPlatformGtk::gfxPlatformGtk() I have added a NULL check for gtk_settings_get_default() because according to its documentation it may return NULL under certain circumstances.
However in nsWindow::Create(), there is no NULL check for gtk_settings_get_default(). I'm not sure if this is a bug or if we can be sure that it will always be non-null. In the latter case my NULL-check in gfxPlatformGtk::gfxPlatformGtk() may be removed.

I've only tested this on X. I'm not sure if this change will have any effect on Wayland; there is no xsettings, but I can't rule out that GTK maps "notify::gtk-xft-dpi" to something else under Wayland.
I've also assumed that all code I've written is only run on Linux platforms.

I'm new to Firefox's code base, feel free to criticize as much as you like about the patch, I'll try to improve it.
If it's easier for you feel free to fix/reimplement the change on your own.

Credits go to Vincent Bernat for implementation suggestions, and to a similar change in Epiphany [0], both of which inspired my patch.
[0]: https://bugzilla.gnome.org/show_bug.cgi?id=744796, actual commit in https://gitlab.gnome.org/GNOME/epiphany/commit/e576aee616245907f86c773f75f1ca4b7446f686 -- The bug report includes an interesting discussion about this topic.

Comment 9

[Samuel Kalbfleisch]

Attachment: viewport-issue-without-Resize.png

Comment 10

[Samuel Kalbfleisch]

Comment on attachment 9078711 [details] [diff] [review]
dpi-dynamic-update_ff68.0.diff

Adding reviewers as per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Module_ownership

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 9078711 [details] [diff] [review]
dpi-dynamic-update_ff68.0.diff

I checking this out thanks.  It's the DispatchResized() that triggers the viewport size.  That shouldn't be necessary, so I'll see if I can find out what's up there.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 9078711 [details] [diff] [review]
dpi-dynamic-update_ff68.0.diff

Thank you for this.  Would you be able to [submit a patch through phabricator](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch), please?  That is how patches are landed now and it is usually easier to review there.

>+    g_signal_connect(gtk_settings, "notify::gtk-xft-dpi",
>+                     G_CALLBACK(gtk_settings_xft_dpi_changed_cb), nullptr);

A "notify::resolution" handler on the `GdkScreen` would be a little more consistent with the use of `gdk_screen_get_resolution()`, but I'm not aware of much difference in practice.  Disconnection of the `GtkSettings` handler is a little simpler in nsWindow, but disconnection of the `GdkScreen` handler would be a little simpler in `ScreenHelperGTK`.  I'm not too concerned which you use, but the ordering of signals might be relevant.  On XSETTINGS changes, it looks like "notify::resolution" is signalled before "notify::gtk-xft-dpi".

>+  // Font DPI has changed, so reset our cached DPI value.
>+  sDPI = 0;

An alternative option in gfxPlatformGtk.cpp is to just remove sDPI, and not cache results.  The `GdkScreen` already caches the value for `gdk_screen_get_resolution()`.

>+  // Without a call to Resize, the UI's viewport becomes too small/big.
>+  LayoutDeviceIntRect bounds = window->GetBounds();
>+  window->Resize(bounds.width, bounds.height, true);

Rather than doing a full `Resize()`, just `DispatchResized()`.  For an explanation comment "Even though the window size in screen pixels has not changed, nsViewManager stores the dimensions in app units.  DispatchResized() updates those."

There is a remaining issue when reducing dpi from a larger value at Firefox startup: the hamburger menu drop-down is limited in size unnecessarily because Firefox still has the initial concept of screen size.  This derives from the [use of `gfxPlatformGtk::GetFontScaleFactor()` in initializing screen data](https://searchfox.org/mozilla-central/rev/8b7aa8af652f87d39349067a5bc9c0256bf6dedc/widget/gtk/ScreenHelperGTK.cpp#151).  A RefreshScreens() on dpi change resolves that.

I'm pretty confident that `RefreshScreens()` before the update from nsWindow.cpp will be fine.  If the order is reversed, everything may still be OK, but I'm not sure.

Comment 14

[Teoh Han Hui]

Duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1214470

Comment 15

[Samuel Kalbfleisch]

(In reply to Teoh Han Hui from comment #14)

Duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1214470

I don't see how this is a duplicate of bug 1214470 in any way. If anything, bug 1604761 is related.

(In reply to Karl Tomlinson (:karlt) from comment #13)

Would you be able to [submit a patch through
phabricator](https://developer.mozilla.org/en-US/docs/Mozilla/
Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch), please? That
is how patches are landed now and it is usually easier to review there.

Thanks for the detailed feedback and sorry for taking so long to respond.
I intend to submit a new patch to phabricator in the near future.

Comment 16

[Vincent Bernat]

Attachment: patch

Since there is no activity, I have tried to implement Karl's feedback. DispatchResized() is protected, so I cannot call it. The original patch didn't work for me (switching between 96 and 144 dpi). However, if I update the patch to also remove use of sDPI cache, it works just fine. I am attaching the patch, dunno if I should forward it to formal review if there are still known problems in it (no use of DispatchResized()).

I have tried to determine how much overhead there was for not caching sDPI by timing the cumulative time spent in GetFontScaleDPI() and it started at 0.02279s (first call, no cache by GTK) and got to 0.02861s after loading 2 pages. So, 8ms. Highly unscientific. I was thinking may be could keep sDPI but provide a method to invalidate it (this way no need to configure the same signal at two places).

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Salut Vincent,

We don't use bugzilla anymore.
To have more chances to get it reviewed, please use phabricator. Simple tutorial:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Comment 18

[Vincent Bernat]

Attachment: Bug 1554850 - handle fractional DPI change on Linux. r?karlt

On Linux, Firefox is listening on notify::scale-factor to detect DPI
change. However, scale-factor is an int and on the lower-end side of
the DPI scale, some devices are using fractional scale factors encoded
into Xft/DPI setting. Changing from ×1 to ×1.5 scale is therefore
undetected.

The proposed change is two-folds:

• remove use of a cached sDPI value and rely on GTK being the cache

• listening on notify::gtk-xft-dpi to trigger a DPI change

What is missing:

• performance evaluation of not caching sDPI (on a 10s session
  loading 2 pages, there is an "overhead" of 6ms on my setup, nothing
  visible from my point of view)

• when changing Xft/DPI and scale, the change is done twice, this
  seems harmless

Comment 19

[Vincent Bernat]

OK, I managed to post the patch (using arc, hope it's good enough). I have used DispatchResized() by moving its prototype to the public part of the nsWindow class.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d85aaf27109c
handle fractional DPI change on Linux. r=karlt

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d85aaf27109c

Comment 22

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Hi Vincent. Makes sense that the regression below was cause by this patch, as the graph suggests?
== Change summary for alert #27267 (as of Tue, 20 Oct 2020 06:11:23 GMT) ==

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
16%	perf_reftest_singletons	bloom-basic-2.html	linux64-shippable-qr	e10s stylo	38.55 -> 44.80
15%	perf_reftest_singletons	bloom-basic.html	linux64-shippable-qr	e10s stylo	38.80 -> 44.71
6%	perf_reftest_singletons	scrollbar-styles-1.html	linux64-shippable	e10s stylo	435.24 -> 462.18
6%	perf_reftest_singletons	scrollbar-styles-1.html	linux64-shippable-qr	e10s stylo	461.49 -> 487.37

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
1%	perf_reftest_singletons	scrollbar-styles-1.html	linux64-shippable-qr	e10s stylo	485.32 -> 480.74

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27267

Comment 23

[Vincent Bernat]

Yes, it may be. We didn't try to optimize until we see such regressions. A first optimization would be to turn the screen variable into a static variable. I can do that if we can quickly run the regression tests to see how it improves.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to Vincent Bernat from comment #23)

A first optimization would be to turn the screen variable into a static variable. I can do that if we can quickly run the regression tests to see how it improves.

Thanks. I felt guilty for asking you to remove your optimization, so I did some experiments on bug 1675908.
It seems that optimizing away both screen and settings is not sufficient to completely remove the regression, so I'll use an approach similar to that in attachment 9078711 [details] [diff] [review].