Bug 212647 – REGRESSION (r262366): [ Mac wk1 ] webgl/webgl-backing-store-size-update.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Truitt Savell]

webgl/webgl-backing-store-size-update.html

This test became a consistent failure on Mac wk1 after the changes in https://trac.webkit.org/changeset/262366/webkit

history:
https://results.webkit.org/?suite=layout-tests&test=webgl%2Fwebgl-backing-store-size-update.html

Diff:
https://build.webkit.org/results/Apple-Catalina-Release-WK1-Tests/r262400%20(6339)/webgl/webgl-backing-store-size-update-diffs.html

Reproduced with:
run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f -1 webgl/webgl-backing-store-size-update.html

[Radar WebKit Bug Importer]

<rdar://problem/63882960>

[Jason Lawrence]

This test appears to have started failing with the changes in r262366 also: webgl/webgl2-primitive-restart.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=webgl%2Fwebgl-backing-store-size-update.html&test=webgl%2Fwebgl2-primitive-restart.html

[Dean Jackson]

This test:

- starts with a canvas that has a composited size of 100x100
- sets the canvas drawing buffer size to 200x200
- renders into the canvas
- changes the composited size of the canvas to 50x50
- waits for a layout pass
- sets the canvas drawing buffer size to 50x50
- renders into the canvas

I'm not sure why the issues are specific to WK1.

[Dean Jackson]

For WK1, at least in the test harness, it appears to be possible for display to be called *before* prepare.

Notice that the "canvas is displayed" happens earlier.

Good case
---------

canvas size set to 200x200
GCGL reshape to 200x200
Paint happens - document takes note

canvas size set to 100x200 (via setting just .width)
GCGL reshape to 100x200

canvas size set to 100x100 (via setting just .height)
GCGL reshape to 100x100

Paint happens - document takes note

All JS finishes, Document looks for canvas elements needing prep
Document calls prepare on canvas

canvas is displayed

Bad case
---------

canvas size set to 200x200
GCGL reshape to 200x200
Paint happens - document takes note

canvas size set to 100x200 (via setting just .width)
GCGL reshape to 100x200

canvas size set to 100x100 (via setting just .height)
GCGL reshape to 100x100

Paint happens - document takes note

canvas is displayed

All JS finishes, Document looks for canvas elements needing prep
Document calls prepare on canvas

[Dean Jackson]

So either the logic of doing the preparation in Page::doAfterUpdateRendering() is incorrect for WebKit1, or something about the canvas backbuffer sizing causes a display to trigger early.

Calling setNeedsDisplay from prepare would probably fix this particular test, but would still be two paints - we need the prepare to happen before the paint.

Maybe it is the fact that the resize and paint happens inside a rAF?

[Dean Jackson]

It's the test app that triggers the issue:

dino> display 0x10d7c06b8
1   0x11ef0ad84 -[WebGLLayer display]
2   0x108da1224 CA::Layer::display_if_needed(CA::Transaction*)
3   0x108ed35b5 CA::Context::commit_transaction(CA::Transaction*, double, double*)
4   0x108d7ec59 CA::Transaction::commit()
5   0x10b3d5772 -[WebView(WebPrivate) _forceRepaintForTesting]
6   0x1013a2d87 updateDisplay()
7   0x1013a2753 dump()
8   0x10140e892 TestRunner::notifyDone()

TestRunner forces a repaint.

[Dean Jackson]

The backing store test fix is easy.

Now for the primitive restart issue....

[Dean Jackson]

Same problem. Rather than fixing the tests I should probably call the document's canvas preparation stuff from _forceRepaintForTesting

[Dean Jackson]

I was hoping I could just call Page::updateRendering() inside the forceRepaint, but that doesn't appear to help - display is still called before prepare. I'll work out why this is.

[Dean Jackson]

Ah. _forceRepaintForTesting already triggers the Page::updateRendering code to run, but it exits early because it is already updating the rendering.

[Dean Jackson]

updateRendering calls document.serviceRequestAnimationFrameCallbacks(timestamp) and this particular test is triggering _forceRepaintForTesting from within the callback. 

I could try calling Page::doAfterUpdateRendering() from within _forceRepaintForTesting. Under normal circumstances it would have run through whatever triggered updateRendering.

However, rendering definitely hasn't been updated by this point, so it could be a mistake.

All this is to avoid having to edit the tests, but it would be more accurate.

[Dean Jackson]

Created attachment 401108 [details]
Patch

[Kenneth Russell]

Comment on attachment 401108 [details]
Patch

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

Looks good to me - one small comment.

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:223
> +    [self setNeedsDisplay];

Is this call cheap? If not, should it be done only if _preparedForDisplay == NO?

[Dean Jackson]

Comment on attachment 401108 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:223
>> +    [self setNeedsDisplay];
> 
> Is this call cheap? If not, should it be done only if _preparedForDisplay == NO?

This is probably the most concerning part of the patch. We really shouldn't have to do this, and it is only necessary because the test infrastructure can bypass the normal rendering cycle. Changing that might impact a lot of tests across all features, but probably should be done anyway.

However, my understanding is that this call is cheap. It's just setting a bit that will be detected in CoreAnimation's rendering loop that will cause it to execute `display`. Technically we shouldn't have even got into this method without it already being called, since the same logic that tells the system that this method is necessary also calls setNeedsDisplay itself. The problem is that the test infrastructure can trigger a `display` before the code in Page that would make preparation happen.

[Dean Jackson]

Committed r262643: <https://trac.webkit.org/changeset/262643>

[Darin Adler]

Comment on attachment 401108 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8597
> +        auto protectedCanvas = makeRefPtr(canvas);

I would have done makeRef(*canvas)

> LayoutTests/webgl/webgl-backing-store-size-update.html:35
> +            window.requestAnimationFrame(function() {

This "window." is not needed and in future I would suggest leaving it out.

[Dean Jackson]

Follow-up in r262652.