Summary: | [GTK] Support transient zoom | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | raffaele.tranquillini | ||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | aakash.kapoor, achristensen, alicem, berto, bugs-noreply, cdumez, cgarcia, cmarcelo, ews-watchlist, gustavo, gyuyoung.kim, luiz, mcatanzaro, ryuan.choi, sergio, svillar, thorton, youennf, zeno | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||
Attachments: |
|
Description
raffaele.tranquillini
2019-04-17 01:57:00 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. 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. :/ 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. 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. *** Bug 209858 has been marked as a duplicate of this bug. *** Sounds like you wrote a lot of code for this that never made it into Bugzilla, yes? 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. 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. (In reply to Michael Catanzaro from comment #8) > WebKitGTK > no longer exists AC mode once a process has entered AC mode. I meant: "exits" 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... So... I don't know anything about this, but sounds like doing what Safari does would be good...? 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. 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. 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. 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?
The patch depends on the patch from https://bugs.webkit.org/show_bug.cgi?id=212327 and won't apply without it. 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 Created attachment 430824 [details]
Patch
Created attachment 430826 [details]
Patch
Looks like API tests fail because of network issues. 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. Created attachment 430844 [details]
Patch
Fixed.
Created attachment 430846 [details]
Patch
Oh oops, the last one didn't contain the fixes from the first 2 reuploads.
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? (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. (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 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.) Created attachment 431424 [details]
Patch
Moved the constants to the header.
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. 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.
(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. Created attachment 431428 [details]
Patch
Made it static too.
(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 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; Created attachment 431429 [details]
Patch
Oh whoops, indeed.
Created attachment 431431 [details]
Patch
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]. |