Bug 154070 – [Threaded Compositor] Flickering and rendering artifacts when resizing the web view

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Open MiniBrowser and resize the window to see the effects. There's flickering, and rendering artifacts at the top

[Carlos Garcia Campos]

I've been investigating this and I think I know what's going on. First lets see how resize works in non-threaded compositor case. 

1- The web view is resized (size-allocate), and it notifies the DrawingArea.
2.- DrawingAreaProxyImpl creates a new backing store state ID and notifies the web process about it asking it to reply immediately. It waits for the reply up to 500ms.
3.- In the web process DrawingAreaImpl updates its backing store state ID to the given one, and sets the new size in the web page and layer tree host. then sends a message to the UI process that stops waiting.

This way, while the web process is updating for the new size, the UI process doesn't render anything, and when it finishes waiting, the backing stores in both web and UI are in sync in terms of size. 

This doesn't work with the threaded compositor and we end up rendering in the UI process with the new size while the web process is still using the old size. The problem is in the step 3), in non-threaded compositor case, the layer tree host updates everything (layer sizes), and it repaints so that when we reply back to the UI process, the layers are already using the new size and new contents have been rendered. In the threaded-compositor case this happens asynchronously, we have to go notify the compositing thread and when updated this notifies the main thread back. But when we reply to the UI process, the compositing thread hasn't been notified yet or it hasn't painted anything yet. So, I think we have to wait until everything has been updated to the new size and contents have been repainted to reply back to the UI process. In other words, we need to make ThreadedCoordinatedLayerTreeHost::viewportSizeChanged() sync and wait until everything has been updated, or make DrawingAreaImpl not reply back to the UI process after calling viewportSizeChanged() and add a wayt to be notified by the layer tree host when everything is ready to send the message back.

[Sergio Villar Senin]

(In reply to comment #1)
> I've been investigating this and I think I know what's going on. First lets
> see how resize works in non-threaded compositor case. 
> 
> 1- The web view is resized (size-allocate), and it notifies the DrawingArea.
> 2.- DrawingAreaProxyImpl creates a new backing store state ID and notifies
> the web process about it asking it to reply immediately. It waits for the
> reply up to 500ms.
> 3.- In the web process DrawingAreaImpl updates its backing store state ID to
> the given one, and sets the new size in the web page and layer tree host.
> then sends a message to the UI process that stops waiting.
> 
> This way, while the web process is updating for the new size, the UI process
> doesn't render anything, and when it finishes waiting, the backing stores in
> both web and UI are in sync in terms of size. 
> 
> This doesn't work with the threaded compositor and we end up rendering in
> the UI process with the new size while the web process is still using the
> old size. The problem is in the step 3), in non-threaded compositor case,
> the layer tree host updates everything (layer sizes), and it repaints so
> that when we reply back to the UI process, the layers are already using the
> new size and new contents have been rendered. In the threaded-compositor
> case this happens asynchronously, we have to go notify the compositing
> thread and when updated this notifies the main thread back. But when we
> reply to the UI process, the compositing thread hasn't been notified yet or
> it hasn't painted anything yet. So, I think we have to wait until everything
> has been updated to the new size and contents have been repainted to reply
> back to the UI process. In other words, we need to make
> ThreadedCoordinatedLayerTreeHost::viewportSizeChanged() sync and wait until
> everything has been updated, or make DrawingAreaImpl not reply back to the
> UI process after calling viewportSizeChanged() and add a wayt to be notified
> by the layer tree host when everything is ready to send the message back.

This is why I think that UI side compositing is much more appealing from the user POV, because you see immediate response to your actions like resizing or scrolling. It does not really matter that you don't have the final rendering very quickly because you could still do things like scrolling, zooming or resizing with the last rendered content while waiting for the webprocess to do all the heavy rendering stuff.

[Carlos Garcia Campos]

We plan to move compositing to the UI process eventually, but at this point it's easier to do the transition in two steps, first move to the threaded compositor, and then move the compositor to a thread in the UI process. But I could be wrong and it easier to do this in one step.

[Carlos Garcia Campos]

Created attachment 280998 [details]
Patch

[Carlos Garcia Campos]

The patch doesn't apply because it depends on bugs #158564 and #158562

[Carlos Garcia Campos]

Created attachment 281008 [details]
Rebased patch

I should apply now

[WebKit Commit Bot]

Attachment 281008 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:49:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

I know that for non-threaded compositor task the resize is already an almost sync operation but making resizing in the threaded compositor sync as well seems a step in the wrong direction.

I think that being out of sync for some time is OK-ish. Isn't it possible to ask the WP to resize the backing store and return immediately?

[Carlos Garcia Campos]

(In reply to comment #8)
> I know that for non-threaded compositor task the resize is already an almost
> sync operation

It doesn't really matter whether to use the threaded compositor or not, the UI process sends a messages and keeps waiting until the resize is done, so it's *always* a sync operation.

> but making resizing in the threaded compositor sync as well
> seems a step in the wrong direction.

My first attempt to fix this was to wait until the resize is done in the compositor thread to reply back to the UI process, but it didn't really work. Between the time the task is scheduled and when it's done all other updates that happens in the compositor thread end upo producing flickering and rendering artifacts. Note that resizing is not a so common operation and it's usually done by user interaction, so it's impossible that the user notices any block or hang.

> I think that being out of sync for some time is OK-ish.

Have you seen the effects of that? The flickering and rendering artifacts are embarrassing and annoying. Doing it sync makes everything smooth.

> Isn't it possible to
> ask the WP to resize the backing store and return immediately?

The main problem of this is that the backing store in this case is not in the web process, but in the UI process. In non-accelerated compositing mode there are two backing stores, one in the UI process and one in the web process. When there's a resize a message is sent to the web process that resizes its backing store and redraws. When it replies back to the UI process the backing store in the UI process is then resized and the new image is rendered. In AC mode the backing store in the Ui process is the redirected window, and the web process renders directly into it. So, when a resize happens, we need to resize the redirected window before sending the message the web process, then we send the message that updates the GL viewport to the new size and schedules a layer flush.

[Zan Dobersek]

Comment on attachment 281008 [details]
Rebased patch

This should probably be revisited in the future, probably by changing the DrawingArea implementation.

[Carlos Garcia Campos]

Committed r202037: <http://trac.webkit.org/changeset/202037>