Bug 185764 – [WPE] Rendering on a HiDPI display looks scaled up instead of rendered at 2x

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Adrian Perez]

Created attachment 340688 [details]
Comparison of WPE and GTK+ ports rendering the same page on HiDPI

Running the Cog WPE launcher (https://github.com/Igalia/cog) with
the WPE FDO backend (https://github.com/Igalia/WPEBackend-fdo) in
a Wayland compositor which is configured for 2x scaling on a HiDPI
display, the resulting output looks like it was rendered at 1x and
then its size doubled (scaled up by the compositor, I think). The
expected behaviour would be to render at 2x.

For comparison, I am attaching a screenshot of the same webpage
side by side: Cog (WPE) on the left, and Epiphany (WebKitGTK+) on
the right.

Also, note that the compositor being used is the wlroots branch
of Sway (http://swaywm.org), but I doubt that's the cause. Note
that Cog will print out the following to standard output:

  % cog -P fdo perezdecastro.org
  Wayland: Got a wl_compositor interface
  Wayland: Got an xdg_shell interface
  Wayland: Got a wl_seat interface
  EGL initialized with version 1.4
  Seat name 'seat0'
  Seat caps: Pointer Keyboard Touch
  New XDG toplevel configuration: (0, 0)
  Cog-Message: 12:04:54.118: <http://perezdecastro.org/> Load started.
  Cog-Message: 12:04:54.119: <https://perezdecastro.org/> Redirected.
  New XDG toplevel configuration: (636, 695)
  ...

Note the 636x695 size for the Cog window/surface. The physical resolution
of the screen is 2560x1440, which translates into the 1280x720 logical
resolution, as Sway tells us: 

  % swaymsg -t get_outputs
  Output eDP-1 'Sharp Corporation LQ133T1JW19 0x00000000'
    Current mode: 1280x720 @ 59.970001 Hz
    Position: 0,0
    Scale factor: 2x
    Transform: normal
    Workspace: 1
    Available modes:
      2560x1440 @ 39.999001 Hz
      2560x1440 @ 47.918999 Hz
      2560x1440 @ 59.970001 Hz

So I *think* that the compositor is working correctly, and there has to be
something missing either in WPEBackend-fdo, in Cog, or WebKit (maybe some
changes are needed all across!) in order to use high-resolution rendering
when Wayland tells to the client that a scaling factor is being used for
a surface.

[Miguel Gomez]

Adrian, could you check if the version you're using is including the fix for https://bugs.webkit.org/show_bug.cgi?id=182673, please?

[Adrian Perez]

(In reply to Miguel Gomez from comment #1)
> Adrian, could you check if the version you're using is including the fix for
> https://bugs.webkit.org/show_bug.cgi?id=182673, please?

I have just tried with a build from yesterday's trunk (which indeed
includes the fix), but WPE is still rendering blurry upscaled frames
when the screen scaling factor is 2x (or higher). I have also updated
Sway to the latest Git commit, with no luck.

[Carlos Bentzen]

I can reproduce it with GNOME shell on Wayland as well. HiDPI 2x.

[Carlos Bentzen]

Created attachment 343499 [details]
Example wrong WPE 2x HiDPI

During creation of WebView in GTK+ (see webkitWebViewBaseCreateWebPage()), there is code for setting the device scale factor, but I see no such thing on WPEView constructor.

Trying to set it hard-coded in View::View() to 2x in my machine just to check and I got the following result attached. Not as expected. Used the same website for comparison.

Can Miguel or Zan shed some light on the matter? Maybe it would need a wl_surface::set_buffer_scale on the WPEBackend-fdo / Wayland side of things as well? I'm totally novice to the WPEBackend code.

Also, how to integrate this in the API?

[Carlos Bentzen]

Created attachment 343891 [details]
WIP

Adrian, can you test if this patch (very much WIP) works for you in Sway?

Just export MINIBROWSER_WPE_SCALE=2 and run the WPE MiniBrowser.

I'm not sure how we can integrate so for now I just used an env var.

[Adrian Perez]

(In reply to Carlos Eduardo Ramalho from comment #5)
> Created attachment 343891 [details]
> WIP
> 
> Adrian, can you test if this patch (very much WIP) works for you in Sway?
> 
> Just export MINIBROWSER_WPE_SCALE=2 and run the WPE MiniBrowser.
> 
> I'm not sure how we can integrate so for now I just used an env var.

I finally got round to try this. With your patch applied indeed
the rendering is being done at 2x, I'll attach a screenshot.

Something I have noticed when running the MiniBrowser under Sway
is that if a split is configured, the viewport reported to the
Web content does not get halved (this can be checked in a webpage
with responsive CSS like https://perezdecastro.org). I don't know
at the moment if this is because MiniBrowser is not handling well
the Wayland events to adapt to the new window size, or whether
it's caused by something in your patch. I'll add a screenshot as
well.

[Adrian Perez]

Created attachment 347582 [details]
MiniBrowser with WIP patch, wide window

[Adrian Perez]

Created attachment 347583 [details]
MiniBrowser with WIP patch, window width halved

[Adrian Perez]

(In reply to Adrian Perez from comment #6)

> Something I have noticed when running the MiniBrowser under Sway
> is that if a split is configured, the viewport reported to the
> Web content does not get halved (this can be checked in a webpage
> with responsive CSS like https://perezdecastro.org). I don't know
> at the moment if this is because MiniBrowser is not handling well
> the Wayland events to adapt to the new window size, or whether
> it's caused by something in your patch. I'll add a screenshot as
> well.

This is a separate bug, likely in MiniBrowser: using “trunk” without
your patch has the same behaviour. Let's split this in a separate bug
report :-)

[Carlos Bentzen]

Created attachment 347605 [details]
WIP

Progress: no need to set manually the scale. We can just get it from wl_output as it is done in GTK.

I don't know how to pass this value from WindowBackend to WPEView though. Left some comments in the WIP patch with suggestions.

Any help is appreciated.

[Carlos Bentzen]

(In reply to Adrian Perez from comment #9)
> This is a separate bug, likely in MiniBrowser: using “trunk” without
> your patch has the same behaviour. Let's split this in a separate bug
> report :-)

Opened bug 188774

[Ryan Walklin]

I have been working on this and think I have a solution, largely based on the current patch. I've added a libwpe function to set the WPE view's intrinsic scale factor (https://github.com/WebPlatformForEmbedded/libwpe/pull/44) and a patch to WPEView.cpp in WebKit largely derived from Adrian's patch: 

diff --git a/Source/WebKit/UIProcess/API/wpe/WPEView.cpp b/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
index 0bc7cb8d7..c90df3cc8 100644
--- a/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
+++ b/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
@@ -100,8 +100,13 @@ View::View(struct wpe_view_backend* backend, const API::PageConfiguration& baseC
                 flags.add(WebCore::ActivityState::IsInWindow);
             view.setViewState(flags);
         },
-        // padding
-        nullptr,
+        // device scale changed
+        [](void* data, float scale)
+        {
+            auto& view = *reinterpret_cast<View*>(data);
+            view.page().setIntrinsicDeviceScaleFactor(scale);
+        },
+        // padding,
         nullptr,
         nullptr
     };

This is enough to enable HiDPI support for me. IMHO the wl_output tracking should be done by the client, and it's enough to set the appropriate scale factor when a wl_surface enters an output, as this will only be set once on entry. It can be reset if the window moves to another output which doesn't support HiDPI or vice versa.

[Adrian Perez]

(In reply to Ryan Walklin from comment #12)
> I have been working on this and think I have a solution, largely based on
> the current patch. I've added a libwpe function to set the WPE view's
> intrinsic scale factor
> (https://github.com/WebPlatformForEmbedded/libwpe/pull/44) and a patch to
> WPEView.cpp in WebKit largely derived from Adrian's patch: 
> 
> diff --git a/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
> b/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
> index 0bc7cb8d7..c90df3cc8 100644
> --- a/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
> +++ b/Source/WebKit/UIProcess/API/wpe/WPEView.cpp
> @@ -100,8 +100,13 @@ View::View(struct wpe_view_backend* backend, const
> API::PageConfiguration& baseC
>                  flags.add(WebCore::ActivityState::IsInWindow);
>              view.setViewState(flags);
>          },
> -        // padding
> -        nullptr,
> +        // device scale changed
> +        [](void* data, float scale)
> +        {
> +            auto& view = *reinterpret_cast<View*>(data);
> +            view.page().setIntrinsicDeviceScaleFactor(scale);
> +        },
> +        // padding,
>          nullptr,
>          nullptr
>      };
> 
> This is enough to enable HiDPI support for me. IMHO the wl_output tracking
> should be done by the client, and it's enough to set the appropriate scale
> factor when a wl_surface enters an output, as this will only be set once on
> entry. It can be reset if the window moves to another output which doesn't
> support HiDPI or vice versa.

Hi Ryan! Thanks *a lot* for chiming in: after taking a quick look
around the WebKit code and Carlos' patch, I think you are correct
and that this change you suggest is what is missing.

I don't know whether Carlos Eduardo has time to finish... If he
doesn't, would you be interested to complete his WIP patch? I could
also finish it myself crediting you both.

[Ryan Walklin]

Created attachment 369485 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

[Adrian Perez]

Comment on attachment 369485 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

View in context: https://bugs.webkit.org/attachment.cgi?id=369485&action=review

The patch needs a small change before we can land it; other that
that, functionality looks good to me. Thanks Ryan!

> Source/WebKit/ChangeLog:9
> +        Wayland and calls setIntrinsicDeviceScaleFactor() for the current View.Page object. The

The scale factor can be from Wayland in the case where one us using a
WPE backend which uses Wayland… or it can from something else when not
using Wayland 🤔

What do you think of rephrasing this as “…scale factor from configured
through wpe_view_backend_dispatch_set_device_scale_factor() and calls…”?

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:111
>          nullptr

You will have to change this because now we also have a “get_accesible” function.
The current definition (as of commit f15b1d1) has the following definition:

  struct wpe_view_backend_client {
      void (*set_size)(void*, uint32_t, uint32_t);
      void (*frame_displayed)(void*);
      void (*activity_state_changed)(void*, uint32_t);
      void* (*get_accessible)(void*);
      void (*set_device_scale_factor)(void*, float);
      void (*_wpe_reserved0)(void);
  };

So we will need one of the “nullptr” items moved up before the lambda
which implementes the ”set_device_scale” callback.

[Ryan Walklin]

Created attachment 369525 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

[Adrian Perez]

Comment on attachment 369525 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

Patch looks good to me now. We need a WebKit reviewer now to
approve it before we can land it in the repository. Thanks for
making the requested changes! :)

[Carlos Garcia Campos]

Comment on attachment 369525 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

View in context: https://bugs.webkit.org/attachment.cgi?id=369525&action=review

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:110
> +        // set_device_scale_factor
> +        [](void* data, float scale)
> +        {
> +            auto& view = *reinterpret_cast<View*>(data);
> +            view.page().setIntrinsicDeviceScaleFactor(scale);
> +        },

This should be inside a #if WPE_BACKEND_CHECK_VERSION(1, 3, 0) block for backwards compatibility

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #18)
> Comment on attachment 369525 [details]
> Patch to add set_device_scale_factor implementation declared by libwpe
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369525&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:110
> > +        // set_device_scale_factor
> > +        [](void* data, float scale)
> > +        {
> > +            auto& view = *reinterpret_cast<View*>(data);
> > +            view.page().setIntrinsicDeviceScaleFactor(scale);
> > +        },
> 
> This should be inside a #if WPE_BACKEND_CHECK_VERSION(1, 3, 0) block for
> backwards compatibility

True! But WPE_BACKEND_CHECK_VERSION has been deprecated a while
ago, so let's use WPE_CHECK_VERSION instead :)

[Ryan Walklin]

Created attachment 369667 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

Added WPE version check.

[Ryan Walklin]

Patch for Cog here - https://github.com/Igalia/cog/pull/94

[Adrian Perez]

Comment on attachment 369667 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

Thanks Ryan! I think this now looks good for landing. Carlos,
could you give this one another look and hopefully flag it
with r+?

[WebKit Commit Bot]

Comment on attachment 369667 [details]
Patch to add set_device_scale_factor implementation declared by libwpe

Clearing flags on attachment: 369667

Committed r245394: <https://trac.webkit.org/changeset/245394>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Walklin]

Thanks Adrian and Carlos!