Summary: | [WPE] Rendering on a HiDPI display looks scaled up instead of rendered at 2x | ||
---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> |
Component: | WPE WebKit | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | bugs-noreply, cadubentzen, cgarcia, commit-queue, elima, magomez, ryan, zan |
Priority: | P2 | ||
Version: | Other | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: |
https://github.com/Igalia/cog/issues/16 https://github.com/WebPlatformForEmbedded/libwpe/pull/44 |
||
Attachments: |
Description
Adrian Perez
2018-05-18 04:26:50 PDT
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? (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. I can reproduce it with GNOME shell on Wayland as well. HiDPI 2x. 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?
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.
(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. Created attachment 347582 [details]
MiniBrowser with WIP patch, wide window
Created attachment 347583 [details]
MiniBrowser with WIP patch, window width halved
(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 :-) 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.
(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 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. (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. Created attachment 369485 [details]
Patch to add set_device_scale_factor implementation declared by libwpe
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. Created attachment 369525 [details]
Patch to add set_device_scale_factor implementation declared by libwpe
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! :)
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 (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 :) Created attachment 369667 [details]
Patch to add set_device_scale_factor implementation declared by libwpe
Added WPE version check.
Patch for Cog here - https://github.com/Igalia/cog/pull/94 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+?
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> All reviewed patches have been landed. Closing bug. Thanks Adrian and Carlos! |