Bug 189743 – [GTK][AC] Resizing the window doesn't always update the visible rect

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

I'm using Debian's 2.22.0

Not sure if it's the same case as the old https://bugs.webkit.org/show_bug.cgi?id=115096 but sometimes I enlarge a window and the new space for the visible rect is just filled with a grey color but the contents of the web page are not painted there.

I still haven't found a reliable way to reproduce it, but you just need to resize the window several times (perhaps not continuously but not sure) and the problem will eventually show up. I'm adding the [AC] tag because I suspect that's the culprit of the visual artifact I'm observing.

[Carlos Garcia Campos]

Yes, this is happening to me too, I had in my TODO to investigate the problem.

[Michael Catanzaro]

This has been broken since I started back in 2014.

[Carlos Garcia Campos]

This broke recently for me

[Sergio Villar Senin]

(In reply to Michael Catanzaro from comment #2)
> This has been broken since I started back in 2014.

I've contributed to WK since 2010 and using it for even longer time and this is the first time it happens to me.

I don't deny that it might have been a problem back then in some specific situations but now it's happening every day to me and several times each day.

[Michael Catanzaro]

There has always been a conspicuously-broken visual issue, when resizing in AC mode, where the newly-visible portion of the window is gray/white, then painted later (too late), a problem which does not exist in non-AC mode.

Seems that, as of recently, the newly-visible rect is sometimes never painted over at all, rather than just being painted late like before. I've noticed this a few times since you reported this bug and I agree that it's a new problem.

[Sergio Villar Senin]

(In reply to Michael Catanzaro from comment #5)
> There has always been a conspicuously-broken visual issue, when resizing in
> AC mode, where the newly-visible portion of the window is gray/white, then
> painted later (too late), a problem which does not exist in non-AC mode.

Well I think that's the price you pay for having WebProcess composition. The UI is resized, so it asks the WebProcess for new contents, but they cannot be served immediately so you end up with those unpainted areas. That does not happen with UIProcess compositing because the UI process can already resolve the resizing/zooming/etc... interactions because it has a local copy of the compositing tree.

A way to mitigate that is (I don't know if we're doing that) to render also content which is not currently in the visible rect to help with scrolling/resizing.

> Seems that, as of recently, the newly-visible rect is sometimes never
> painted over at all, rather than just being painted late like before. I've
> noticed this a few times since you reported this bug and I agree that it's a
> new problem.

Great. Let's see if we manage to bisect it and find the culprit.

[Michael Catanzaro]

(In reply to Sergio Villar Senin from comment #6)
> A way to mitigate that is (I don't know if we're doing that) to render also
> content which is not currently in the visible rect to help with
> scrolling/resizing.

Maybe we should do that then. This is one of the reasons we don't want to switch AC mode to always on: because resizing in AC mode looks so terrible....

[Michael Catanzaro]

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

[Michael Catanzaro]

Hey Adrian, let's consider this one a blocker for the 2.22.3 release.

(In reply to Sergio Villar Senin from comment #6)
> Great. Let's see if we manage to bisect it and find the culprit.

I agree we need to bisect this to make progress. I didn't have time this week.

Would be really nice if someone with a build machine could handle it.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #9)
> Hey Adrian, let's consider this one a blocker for the 2.22.3 release.

Sure thing. There's a good bunch of MSE patches which are already
merged in the release branch waiting for 2.22.3 to happen, but at
least we don't have anyone completely blocked out of being able to
watch videos so that's not in a rush anymore — we can wait for this.

[Zan Dobersek]

Created attachment 352559 [details]
Patch

[Zan Dobersek]

Comment on attachment 352559 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:206
> -    if (m_surface && m_surface->resize(size))
> +    // if (m_surface && m_surface->resize(size))

When the page and its connected layer tree host is being resized, the associated AcceleratedSurface is resized right here, on the main thread, whereas the composition (which would be affected by the resize) is all done in a separate thread. So this is just an inappropriate cross-thread access into the wl_egl_window object that's managed in the derived AcceleratedSurfaceWayland class. This patch moves the resizing operation over to the composition thread, with the underlying resizing issue disappearing.

Reason why this is done on the main thread is the X11-specific implementation of AcceleratedSurface. That one respawns a pixmap object on each resize and retrieves its address as the surface ID that's here assigned to the m_layerTreeContext.contextID variable, which I guess is necessary.

[Zan Dobersek]

Comment on attachment 352559 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:206
> -    if (m_surface && m_surface->resize(size))
> +    // if (m_surface && m_surface->resize(size))

When the page and its connected layer tree host is being resized, the associated AcceleratedSurface is resized right here, on the main thread, whereas the composition (which would be affected by the resize) is all done in a separate thread. So this is just an inappropriate cross-thread access into the wl_egl_window object that's managed in the derived AcceleratedSurfaceWayland class. This patch moves the resizing operation over to the composition thread, with the underlying resizing issue disappearing.

Reason why this is done on the main thread is the X11-specific implementation of AcceleratedSurface. That one respawns a pixmap object on each resize and retrieves its address as the surface ID that's here assigned to the m_layerTreeContext.contextID variable, which I guess is necessary.

[Michael Catanzaro]

I can confirm this patch fixes YouTube. Thanks a bunch.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #10)
> Sure thing. There's a good bunch of MSE patches which are already
> merged in the release branch waiting for 2.22.3 to happen, but at
> least we don't have anyone completely blocked out of being able to
> watch videos so that's not in a rush anymore — we can wait for this.

IMO this bug is so important, we should stall further releases until a final patch is ready.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #15)
> (In reply to Adrian Perez from comment #10)
> > Sure thing. There's a good bunch of MSE patches which are already
> > merged in the release branch waiting for 2.22.3 to happen, but at
> > least we don't have anyone completely blocked out of being able to
> > watch videos so that's not in a rush anymore — we can wait for this.
> 
> IMO this bug is so important, we should stall further releases until a final
> patch is ready.

Yes, for the moment I am holding the release. Nice to see progress here!

Note that if we have a patch that we think it's not fit to be landed
upstream *just yet*, but we that works and we are comfortable having
point release, it is also possible to merge the patch in its WIP form
to the release branch and update later on to its final version. I would
rather avoid doing that, but it's definitely an option (such a thing
has been done a couple of times before).

:-)

[Zan Dobersek]

Created attachment 352963 [details]
Patch

[Michael Catanzaro]

Comment on attachment 352963 [details]
Patch

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

Thanks a bunch!

If Miguel is available to review this before landing, that would be ideal.

> Source/WebKit/ChangeLog:3
> +        REGRESSION(r??????): [GTK][AC] Resizing the window doesn't always update the visible rect

Probably best to remove the REGRESSION bit from the commit message, since we never figured out at which point this regressed.

[Miguel Gomez]

(In reply to Michael Catanzaro from comment #18)
> Comment on attachment 352963 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352963&action=review
> 
> Thanks a bunch!
> 
> If Miguel is available to review this before landing, that would be ideal.

LGTM

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> I can confirm this patch fixes YouTube. Thanks a bunch.

The new version works too!

[Zan Dobersek]

Created attachment 353081 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 353081 [details]
Patch for landing

Clearing flags on attachment: 353081

Committed r237410: <https://trac.webkit.org/changeset/237410>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.