Open Bug 1554850 Opened 1 year ago Updated 3 months ago

Fractional DPI change not detected on Linux

Categories

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

67 Branch
defect

Tracking

()

People

(Reporter: bernat, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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)

Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Priority: -- → P3
Assignee: nobody → stransky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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.

Assignee: stransky → nobody
Status: ASSIGNED → NEW

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.

It's also Wayland related issue.

Blocks: wayland
See Also: → 1543035

(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.

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.

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().

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

Flags: needinfo?(bernat)

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 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
Attachment #9078711 - Flags: feedback?(stransky)
Attachment #9078711 - Flags: feedback?(karlt)
Attachment #9078711 - Flags: feedback?(jmuizelaar)
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.
Attachment #9078711 - Flags: feedback?(stransky)
Attachment #9078711 - Flags: feedback?(jmuizelaar)
Duplicate of this bug: 1251432
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.
Attachment #9078711 - Flags: feedback?(karlt) → feedback+

(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.

You need to log in before you can comment on or make changes to this bug.