Bug 197002 – [GTK] Support transient zoom

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

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.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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. :/

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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.

[Michael Catanzaro]

*** Bug 209858 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Sounds like you wrote a lot of code for this that never made it into Bugzilla, yes?

[Alice Mikhaylenko]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> WebKitGTK
> no longer exists AC mode once a process has entered AC mode.

I meant: "exits"

[Alice Mikhaylenko]

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

[Michael Catanzaro]

So... I don't know anything about this, but sounds like doing what Safari does would be good...?

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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?

[Alice Mikhaylenko]

The patch depends on the patch from https://bugs.webkit.org/show_bug.cgi?id=212327 and won't apply without it.

[EWS Watchlist]

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

[Alice Mikhaylenko]

Created attachment 430824 [details]
Patch

[Alice Mikhaylenko]

Created attachment 430826 [details]
Patch

[Alice Mikhaylenko]

Looks like API tests fail because of network issues.

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

Created attachment 430844 [details]
Patch

Fixed.

[Alice Mikhaylenko]

Created attachment 430846 [details]
Patch

Oh oops, the last one didn't contain the fixes from the first 2 reuploads.

[Tim Horton]

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?

[Tim Horton]

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

[Sergio Villar Senin]

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

[Michael Catanzaro]

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

[Alice Mikhaylenko]

Created attachment 431424 [details]
Patch

Moved the constants to the header.

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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

[Alice Mikhaylenko]

Created attachment 431428 [details]
Patch

Made it static too.

[Michael Catanzaro]

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

[Michael Catanzaro]

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;

[Alice Mikhaylenko]

Created attachment 431429 [details]
Patch

Oh whoops, indeed.

[Alice Mikhaylenko]

Created attachment 431431 [details]
Patch

[EWS]

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