Bug 197002 - [GTK] Support transient zoom
Summary: [GTK] Support transient zoom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 209858 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-17 01:57 PDT by raffaele.tranquillini
Modified: 2021-06-15 10:21 PDT (History)
19 users (show)

See Also:


Attachments
Patch (46.41 KB, patch)
2021-06-08 02:36 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (46.40 KB, patch)
2021-06-08 02:39 PDT, Alexander Mikhaylenko
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.40 KB, patch)
2021-06-08 02:58 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (46.58 KB, patch)
2021-06-08 08:08 PDT, Alexander Mikhaylenko
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.57 KB, patch)
2021-06-08 08:46 PDT, Alexander Mikhaylenko
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (46.53 KB, patch)
2021-06-15 05:12 PDT, Alexander Mikhaylenko
mcatanzaro: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (47.64 KB, patch)
2021-06-15 05:49 PDT, Alexander Mikhaylenko
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (47.65 KB, patch)
2021-06-15 05:53 PDT, Alexander Mikhaylenko
mcatanzaro: review+
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Patch (47.63 KB, patch)
2021-06-15 06:09 PDT, Alexander Mikhaylenko
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (47.61 KB, patch)
2021-06-15 06:24 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description raffaele.tranquillini 2019-04-17 01:57:00 PDT
Pinch to zoom currently renders content while the gesture is being performed, at every single step, making it laggy on mildly complex pages. The rendering should probably be deferred or triggered in larger zoom "steps" while being combined with cached and scaled previous rendering buffers. Does Safari handle this differently than on Gtk? It looks way smoother even on less powerful CPUs.
Comment 1 Alexander Mikhaylenko 2019-04-17 10:36:11 PDT
Completely agree.

Safari (well, WebKit on macOS) simply scales a snapshot while the gesture is performed, so it is more smooth, right. And it also uses GPU for rendering AFAIK.

I am going to work on this when I have a bit more free time/energy than I do right now.
Comment 2 Alexander Mikhaylenko 2019-04-27 01:16:15 PDT
I've been looking at this, and it's more complex than I assumed.

At any given time, only the directly visible part of the page is rendered. Which is fine if you're zoom in, but makes it impossible to just reuse it if you're zooming out, and you actually need to render the newly shown parts of the page.

Now, this may or may not be problematic wrt performance. While I'm pretty sure it won't be slower than status quo, it may still be somewhat laggy. :/
Comment 3 Alexander Mikhaylenko 2019-05-07 02:57:57 PDT
Ok, so I got pretty reasonable performance scaling the web page to 1.0 scale and zooming the output on UI process side, then scaling the page to the desired size at the end.

It's still not perfect, when you're zooming from non-1.0 scale, it might lag once  at the beginning on certain pages, such as YouTube (where the status quo zooming is basically unusable), but it's IMO _a lot_ better than what we have now. The zooming itself is predictably perfectly smooth on every page.

Video still plays during the zooming :p It is a bit blurry, but it would be same if you zoom in anyway, now it's just same when zooming in or out.

I'd still love to find a way to do it without the initial re-scaling, to eliminate the initial lag completely and reduce blur. But honestly, seeing as rendering is very tightly coupled to the viewport, I'm not sure it's possible without major changes in the rendering :/

Another problem: scrollbars are scaled with the content. Well, that was expected. Maybe I can hide those scrollbars and draw them separately during the gesture.
Comment 4 Alexander Mikhaylenko 2019-05-07 06:30:14 PDT
Ugh, and the initial lag is pretty bad on some pages. I need to try again to reuse the page in its current scale, but a different viewport size.
Comment 5 Michael Catanzaro 2020-04-01 15:44:27 PDT
*** Bug 209858 has been marked as a duplicate of this bug. ***
Comment 6 Michael Catanzaro 2020-04-01 15:57:55 PDT
Sounds like you wrote a lot of code for this that never made it into Bugzilla, yes?
Comment 7 Alexander Mikhaylenko 2020-04-01 16:01:01 PDT
Yes, but it was pretty bad and for non-AC mode only. I tried multiple times to make something work without initial page scaling, but wasn't able to at the end.

Maybe I should give up and be content with 1 page rescale at the beginning if the initial scale ≠ 1 and one at the end if the final scale ≠ 1 like it was in that patch.
Comment 8 Michael Catanzaro 2020-04-01 16:18:03 PDT
I think most users will be happy that we have pinch-to-zoom at all. I understand Firefox and Chrome cannot do this. ;)

One thing that has changed since you last worked on WebKit is that WebKitGTK no longer exists AC mode once a process has entered AC mode. There's no longer much point in doing that now that PSON exists. So since AC mode is now a permanent thing, it does become arguably more important than ever before.
Comment 9 Michael Catanzaro 2020-04-01 16:18:35 PDT
(In reply to Michael Catanzaro from comment #8)
> WebKitGTK
> no longer exists AC mode once a process has entered AC mode.

I meant: "exits"
Comment 10 Alexander Mikhaylenko 2020-04-02 11:36:56 PDT
My main concern about this method is that if a page takes forever to reload (e.g. paste.gnome.org with a huge paste) _and_ the initial scale ≠ 1, it may take multiple seconds to rescale to 1 before it starts smoothly animating with pinch.

E.g. Safari only does rescale at the end, so mitigates this completely: the gesture is responsive, if there's lag, it's after it's done.

Of course it's a moot point considering now this multi-second layout is done on every frame, so it's still an improvement, but...
Comment 11 Michael Catanzaro 2020-05-01 16:36:48 PDT
So... I don't know anything about this, but sounds like doing what Safari does would be good...?
Comment 12 Alexander Mikhaylenko 2020-05-01 16:38:33 PDT
What Safari does is impossible, it's completely different drawing model.

I have a proof of concept code locally using the scaling-to-1.0-and-back approach I described above, but I have flickers when it scales to 1.0 and back, so I need to fix that and I have no idea how.
Comment 13 Alexander Mikhaylenko 2020-12-03 00:16:27 PST
Ok, what I did wouldn't be nearly enough for 4K screens.

So, I think it's safe to say it can't be done in GTK 3 really, we need 4 and then naybe rework the whole drawing to do composition on UI process side, as it is doable in GTK4 unlike 3.
Comment 14 Alexander Mikhaylenko 2021-06-07 06:10:45 PDT
Update: I managed to do it for AC mode only: https://twitter.com/alexm_gnome/status/1400578267099324416

Despite what the previous tweet says, it does work on GTK3 as well, though it's less smooth than on 4. Still way better than status quo though.

However, the non-AC mode is still a problem and everything above applies to it.

One possibility is to use transient zoom for AC mode and the current  real-time one otherwise and call it a day - long term (e.g. in GTK4) we're always using AC mode anyway.
Comment 15 Alexander Mikhaylenko 2021-06-08 02:36:32 PDT
Created attachment 430823 [details]
Patch

Patch.

Some questions:

1. The non-AC mode zooming is still slow, I wonder if we should switch to AC mode with ondemand when trying to zoom?

2. We always pass the same flags to flushPendingLayerChanges() with the same condition.
   Maybe it should be a boolean/enum parameter similarly to the `enum class UpdateRenderingType { Normal, TransientZoom }` in `TiledCoreAnimationDrawingArea`?

3. We currently don't handle AC mode changes during the gesture, and honestly I don't particularly want to go into that rabbit hole. IIUC if we go with 1., it doesn't matter?
Comment 16 Alexander Mikhaylenko 2021-06-08 02:37:13 PDT
The patch depends on the patch from https://bugs.webkit.org/show_bug.cgi?id=212327 and won't apply without it.
Comment 17 EWS Watchlist 2021-06-08 02:37:30 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 18 Alexander Mikhaylenko 2021-06-08 02:39:33 PDT
Created attachment 430824 [details]
Patch
Comment 19 Alexander Mikhaylenko 2021-06-08 02:58:42 PDT
Created attachment 430826 [details]
Patch
Comment 20 Alexander Mikhaylenko 2021-06-08 03:19:05 PDT
Looks like API tests fail because of network issues.
Comment 21 Michael Catanzaro 2021-06-08 07:30:27 PDT
Comment on attachment 430826 [details]
Patch

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

LGTM, but I'm afraid we're going to need to find an owner willing to review this due to extensive changes in cross-platform WK2.

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:153
> +#if PLATFORM(GTK)
> +    if (!m_transientZoom)
> +#endif
> +        flags.add(FinalizeRenderingUpdateFlags::ApplyScrollingTreeLayerPositions);

Style nit: don't do this please, just write it out twice:

#if PLATFORM(GTK)
    if (!m_transientZoom)
        flags.add(FinalizeRenderingUpdateFlags::ApplyScrollingTreeLayerPositions);
#else
    flags.add(FinalizeRenderingUpdateFlags::ApplyScrollingTreeLayerPositions);
#endif

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:207
> +#if PLATFORM(GTK)
> +        if (!m_transientZoom)
> +#endif

Ditto.
Comment 22 Alexander Mikhaylenko 2021-06-08 08:08:44 PDT
Created attachment 430844 [details]
Patch

Fixed.
Comment 23 Alexander Mikhaylenko 2021-06-08 08:46:51 PDT
Created attachment 430846 [details]
Patch

Oh oops, the last one didn't contain the fixes from the first 2 reuploads.
Comment 24 Tim Horton 2021-06-14 15:28:42 PDT
Comment on attachment 430846 [details]
Patch

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

The Mac/iOS/shared VGC changes seem fine to me, but final review should come from a GTK person since I can't speak to those parts.

> Source/WebKit/UIProcess/ViewGestureController.cpp:65
> +static const double minMagnification = 1;
> +static const double maxMagnification = 3;

Maybe you can expose these on the class, so that...

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:67
> +static const double minMagnification = 1;
> +static const double maxMagnification = 3;

... we don't have to repeat ourselves?
Comment 25 Tim Horton 2021-06-14 15:33:23 PDT
(In reply to Alexander Mikhaylenko from comment #10)
> E.g. Safari only does rescale at the end, so mitigates this completely: the
> gesture is responsive, if there's lag, it's after it's done.

*Some*day I hope we can do something more like iOS, where the zooming zooms painted content, but new tiles come in asynchronously as they are available. Best of both worlds. But right now, we're a good ways away from that.
Comment 26 Sergio Villar Senin 2021-06-15 04:04:36 PDT
(In reply to Tim Horton from comment #25)
> (In reply to Alexander Mikhaylenko from comment #10)
> > E.g. Safari only does rescale at the end, so mitigates this completely: the
> > gesture is responsive, if there's lag, it's after it's done.
> 
> *Some*day I hope we can do something more like iOS, where the zooming zooms
> painted content, but new tiles come in asynchronously as they are available.
> Best of both worlds. But right now, we're a good ways away from that.

Didn't the old Qt port use to do that?. As far as I remember they're compositing in the UI thread so you could do zooming, scrolling, etc even if the WebProcess was busy (I remember stopping it completely with the debugger while pinch zooming).
Comment 27 Michael Catanzaro 2021-06-15 04:27:38 PDT
Comment on attachment 430846 [details]
Patch

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

Thanks Tim!

>> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:67
>> +static const double maxMagnification = 3;
> 
> ... we don't have to repeat ourselves?

(You can move them to the private section of the class declaration.)
Comment 28 Alexander Mikhaylenko 2021-06-15 05:12:16 PDT
Created attachment 431424 [details]
Patch

Moved the constants to the header.
Comment 29 Michael Catanzaro 2021-06-15 05:42:23 PDT
Comment on attachment 431424 [details]
Patch

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

> Source/WebKit/UIProcess/ViewGestureController.h:488
> +    static constexpr double minMagnification { 1 };
> +    static constexpr double maxMagnification { 3 };

I think const will suffice here. You probably don't need the constexpr?

> Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm:-54
> -static const double minMagnification = 1;
> -static const double maxMagnification = 3;

OK, EWS is sad. Looks like resistanceForDelta would need to become a static class function, since it uses these but no longer has access to them.
Comment 30 Alexander Mikhaylenko 2021-06-15 05:49:57 PDT
Created attachment 431426 [details]
Patch

Hopefully fixed macOS build, minMagnification and maxMagnification were used in resistanceForDelta which was a static func. It's a private method now instead.
Comment 31 Alexander Mikhaylenko 2021-06-15 05:52:21 PDT
(In reply to Michael Catanzaro from comment #29)
> I think const will suffice here. You probably don't need the constexpr?

../../Source/WebKit/UIProcess/ViewGestureController.h:489:25: error: ‘constexpr’ needed for in-class initialization of static data member ‘const double WebKit::ViewGestureController::minMagnification’ of non-integral type [-fpermissive]

> OK, EWS is sad. Looks like resistanceForDelta would need to become a static
> class function, since it uses these but no longer has access to them.

Indeed, missed that. And indeed, it can be static as well.
Comment 32 Alexander Mikhaylenko 2021-06-15 05:53:25 PDT
Created attachment 431428 [details]
Patch

Made it static too.
Comment 33 Michael Catanzaro 2021-06-15 05:59:42 PDT
(In reply to Alexander Mikhaylenko from comment #31)
> ../../Source/WebKit/UIProcess/ViewGestureController.h:489:25: error:
> ‘constexpr’ needed for in-class initialization of static data member ‘const
> double WebKit::ViewGestureController::minMagnification’ of non-integral type
> [-fpermissive]

Hm, well today I learned!
Comment 34 Michael Catanzaro 2021-06-15 06:00:51 PDT
Comment on attachment 431428 [details]
Patch

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

> Source/WebKit/UIProcess/ViewGestureController.h:294
> +    static double ViewGestureController::resistanceForDelta(double deltaScale, double currentScale) const;

Next build failure is due to the extra qualification, should be:

static double resistanceForDelta(double deltaScale, double currentScale) const;
Comment 35 Alexander Mikhaylenko 2021-06-15 06:09:54 PDT
Created attachment 431429 [details]
Patch

Oh whoops, indeed.
Comment 36 Alexander Mikhaylenko 2021-06-15 06:24:10 PDT
Created attachment 431431 [details]
Patch
Comment 37 EWS 2021-06-15 10:21:03 PDT
Committed r278880 (238823@main): <https://commits.webkit.org/238823@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431431 [details].