Bug 106358 – [EFL][WebGL] WebGL content is not painted after resizing the viewport.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Joone Hur]

It seems that most WebGL contents are not painted after resizing the viewport.

[Viatcheslav Ostapenko]

Created attachment 181848 [details]
Patch

[Kalyan]

Comment on attachment 181848 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:481
> +{

What about the other GraphicsSurface implementations ?? i.e GraphicsSurfaceWin, GraphicsSurfaceMac etc

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:349
> +        assignImageBackingToLayer(layer, layerInfo.imageID);

Does this change effect GraphicsSurface usage on other platforms i.e using GraphicsSurface with Qt on Win, Mac for example. ??

[Viatcheslav Ostapenko]

Created attachment 183658 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #2)
> (From update of attachment 181848 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181848&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:481
> > +{
> 
> What about the other GraphicsSurface implementations ?? i.e GraphicsSurfaceWin, GraphicsSurfaceMac etc
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:349
> > +        assignImageBackingToLayer(layer, layerInfo.imageID);
> 
> Does this change effect GraphicsSurface usage on other platforms i.e using GraphicsSurface with Qt on Win, Mac for example. ??

I've added empty platformReset methods to make it compile on Qt Win and Mac. Qt doesn't need this because it doesn't have surface resize.

[Noam Rosenthal]

Comment on attachment 183658 [details]
Patch

This looks like a hack. 
I can't understand what the code does from reading it - but I'm guessing it resets all the backing store sizes forcing the GLX ones to get a new size from the GLXPixmap on the next draw, and this happens whenever anything changes in the layer? This doesn't seem like the most straightforward way to do what you're trying to do.

[Kalyan]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 181848 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181848&action=review
>Qt doesn't need this because it doesn't have surface resize.

Isn't the issue here that we are resizing the viewport and not the canvas itself ??

[Viatcheslav Ostapenko]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 181848 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=181848&action=review
> >Qt doesn't need this because it doesn't have surface resize.
> 
> Isn't the issue here that we are resizing the viewport and not the canvas itself ??

Canvas also.

[Viatcheslav Ostapenko]

(In reply to comment #5)
> (From update of attachment 183658 [details])
> This looks like a hack. 
> I can't understand what the code does from reading it - but I'm guessing it resets all the backing store sizes forcing the GLX ones to get a new size from the GLXPixmap on the next draw, and this happens whenever anything changes in the layer? This doesn't seem like the most straightforward way to do what you're trying to do.

Yes, resetting surface size is a part of the fix.
Also, setLayerState clears layer backing store by calling assingImageBackingToLayer with 0 imageID. My patch also prevents this.

What do you think would be better approach if we want to support surface resize?

Thanks.

[Kalyan]

(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 183658 [details] [details])
> > This looks like a hack. 
> > I can't understand what the code does from reading it - but I'm guessing it resets all the backing store sizes forcing the GLX ones to get a new size from the GLXPixmap on the next draw, and this happens whenever anything changes in the layer? This doesn't seem like the most straightforward way to do what you're trying to do.
> 
> Yes, resetting surface size is a part of the fix.
> Also, setLayerState clears layer backing store by calling assingImageBackingToLayer with 0 imageID. My patch also prevents this.
> 
> What do you think would be better approach if we want to support surface resize?
> 
> Thanks.

Looking at the demo, it doesn't resize the html canvas element after the initial setup is done. Demo seems broken after the main view size changes. Currently, any resize of Context3D platform layer(as a result of resizing the canvas element) would destroy the existing surface and create a new one. 
Still wondering about the issue here and why it is fixed after forcing a size query on graphicssurfaceglx side.

[Viatcheslav Ostapenko]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > (From update of attachment 183658 [details] [details] [details])
> > > This looks like a hack. 
> > > I can't understand what the code does from reading it - but I'm guessing it resets all the backing store sizes forcing the GLX ones to get a new size from the GLXPixmap on the next draw, and this happens whenever anything changes in the layer? This doesn't seem like the most straightforward way to do what you're trying to do.
> > 
> > Yes, resetting surface size is a part of the fix.
> > Also, setLayerState clears layer backing store by calling assingImageBackingToLayer with 0 imageID. My patch also prevents this.
> > 
> > What do you think would be better approach if we want to support surface resize?
> > 
> > Thanks.
> 
> Looking at the demo, it doesn't resize the html canvas element after the initial setup is done. Demo seems broken after the main view size changes. Currently, any resize of Context3D platform layer(as a result of resizing the canvas element) would destroy the existing surface and create a new one. 
> Still wondering about the issue here and why it is fixed after forcing a size query on graphicssurfaceglx side.

No, it doesn't destroy the surface. It just resizes it. Is it wrong?

[Kalyan]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #5)
> > > > (From update of attachment 183658 [details] [details] [details] [details])
> > > > This looks like a hack.  
> No, it doesn't destroy the surface. It just resizes it. Is it wrong?

Yes, CoordinateGraphicsLayer should recreate the surface(in UI Process side).
If we hold on to a surface with wrong size than that should be a bug here.

[Kalyan]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #5)
> > > > > (From update of attachment 183658 [details] [details] [details] [details] [details])
> > > > > This looks like a hack.  
> > No, it doesn't destroy the surface. It just resizes it. Is it wrong?
> 
> Yes, CoordinateGraphicsLayer should recreate the surface(in UI Process side).
> If we hold on to a surface with wrong size than that should be a bug here.

k, I think we are trying to solve two different things here. The issue with this demo and (some others I could replicate with) is clearing the backing store of the canvas layer(As you mentioned above and provided the fix). 

The other thing we are trying to solve here(atleast partly) is to avoid deletion of surface on UI side during canvas resize. IMHO should be part of a different discussion.

[Viatcheslav Ostapenko]

Created attachment 183892 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > (In reply to comment #5)
> > > > > > (From update of attachment 183658 [details] [details] [details] [details] [details] [details])
> > > > > > This looks like a hack.  
> > > No, it doesn't destroy the surface. It just resizes it. Is it wrong?
> > 
> > Yes, CoordinateGraphicsLayer should recreate the surface(in UI Process side).
> > If we hold on to a surface with wrong size than that should be a bug here.
> 
> k, I think we are trying to solve two different things here. The issue with this demo and (some others I could replicate with) is clearing the backing store of the canvas layer(As you mentioned above and provided the fix). 
> 
> The other thing we are trying to solve here(atleast partly) is to avoid deletion of surface on UI side during canvas resize. IMHO should be part of a different discussion.

It appears canvas resize problem is already solved. If layer/canvas size changed, canvas get recreated.
I uploaded patch which fixes layer update problem.

[Noam Rosenthal]

Comment on attachment 183892 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:345
> +#if USE(GRAPHICS_SURFACE)
> +    SurfaceBackingStoreMap::iterator it = m_surfaceBackingStores.find(id);
> +    if (it == m_surfaceBackingStores.end())
> +#endif

Can you move this into the assignImageBackingToLayer function, with an early return?

[Viatcheslav Ostapenko]

Created attachment 183903 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #15)
> (From update of attachment 183892 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183892&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:345
> > +#if USE(GRAPHICS_SURFACE)
> > +    SurfaceBackingStoreMap::iterator it = m_surfaceBackingStores.find(id);
> > +    if (it == m_surfaceBackingStores.end())
> > +#endif
> 
> Can you move this into the assignImageBackingToLayer function, with an early return?

Done

[Kalyan]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 183892 [details] [details])
> Done

Would be nice to have a layout test to replicate the issue. Would it be possible to provide one along with the fix ??

[Kalyan]

(In reply to comment #18)
> Would be nice to have a layout test to replicate the issue. Would it be possible to provide one along with the fix ??

or is it already covered by our tests(i.e pixel tests)??

[Viatcheslav Ostapenko]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > (From update of attachment 183892 [details] [details] [details])
> > Done
> 
> Would be nice to have a layout test to replicate the issue. Would it be possible to provide one along with the fix ??

Tried to do test today, cannot force it to re-layout the page.
Minibrowser uses fixed layout view and does setFixedLayout on window resize.
On webkittestrunner this doesn't happen by default.
It is possible to force fixed layout for webkittestrunner, but I'd have to do something like this:
https://trac.webkit.org/changeset/140262/trunk/Tools/WebKitTestRunner/TestInvocation.cpp
Any ideas?

[Viatcheslav Ostapenko]

Created attachment 184790 [details]
Patch

[Noam Rosenthal]

Comment on attachment 184790 [details]
Patch

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

Good catch! One nitpick but otherwise looks ok to me, if you can get a WK2 owner to review.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:553
> +    SurfaceBackingStoreMap::iterator surfacesIterator = m_surfaceBackingStores.find(id);
> +    if (surfacesIterator != m_surfaceBackingStores.end())
> +        return;

if (m_surfaceBackingStores.contains(id))

[Viatcheslav Ostapenko]

Created attachment 184798 [details]
Patch

[WebKit Review Bot]

Comment on attachment 184798 [details]
Patch

Attachment 184798 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16124532

New failing tests:
fast/canvas/webgl/webgl-composite-modes-update.html
platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes-update.html

[Viatcheslav Ostapenko]

(In reply to comment #24)
> (From update of attachment 184798 [details])
> Attachment 184798 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16124532
> 
> New failing tests:
> fast/canvas/webgl/webgl-composite-modes-update.html
> platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes-update.html

Have built chromium, have weird results:

fast/canvas/webgl/webgl-composite-modes-update.html [ ImageOnlyFailure ]

+fast/canvas/webgl/webgl-composite-modes-update.html		images diff (0%) 	IMAGE	PASS

I'll attach expected and actual images. For me look identical.

[Viatcheslav Ostapenko]

Created attachment 184853 [details]
webgl-composite-modes-update-expected.png

[Viatcheslav Ostapenko]

Created attachment 184854 [details]
webgl-composite-modes-update-actual.png

[Viatcheslav Ostapenko]

Created attachment 184855 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #28)
> Created an attachment (id=184855) [details]
> Patch

Use chromium test result as a baseline.

[Kalyan]

(In reply to comment #29)
> (In reply to comment #28)
> > Created an attachment (id=184855) [details] [details]
> > Patch
> 
> Use chromium test result as a baseline.

LGTM

[Hugo Parente Lima]

Comment on attachment 184855 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:551
> +    if (m_surfaceBackingStores.contains(id))

Would not be better to do this check outside the assignImageBackingToLayer function? so it remains intact and doesn't seems like it needs to be renamed.

[Viatcheslav Ostapenko]

(In reply to comment #31)
> (From update of attachment 184855 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184855&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:551
> > +    if (m_surfaceBackingStores.contains(id))
> 
> Would not be better to do this check outside the assignImageBackingToLayer function? so it remains intact and doesn't seems like it needs to be renamed.

https://bugs.webkit.org/show_bug.cgi?id=106358#c15

[Benjamin Poulain]

Comment on attachment 184855 [details]
Patch

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

I sign off on this for WebKit2. Please get Noam to review the changes for correctness.

In my opinion, the test is not good enough. Tests have a cost, especially with timers.

> LayoutTests/ChangeLog:14
> +        Add test checking that canvas painting is correct if layer parameters were changed,
> +        but webgl canvas didn't change.
> +
> +        * fast/canvas/webgl/webgl-composite-modes-update-expected.png: Added.
> +        * fast/canvas/webgl/webgl-composite-modes-update-expected.txt: Added.
> +        * fast/canvas/webgl/webgl-composite-modes-update.html: Added.
> +

I think this is not a great test. You should focus on the use case at hand, not have complex rendering for the sake of it. Drawing simple rect should be enough to reproduce the bug with a pixel test.

The test rely on two timers. It is unlikely required as most canvas rendering can be forced.

> LayoutTests/fast/canvas/webgl/webgl-composite-modes-update.html:28
> +This test is only useful as a pixel test. You should see two rows with 4 canvases in each. The top row of canvases should have a black background, the bottom row should have a dark blue background.
> +Each canvas should have a green triangle in it. If multisampling is supported, the odd columns should have smooth edges instead of jagged stair-stepping. Otherwise, all canvases within a row should be identical to each other.

This is not an accurate description of the test. You should have some context about what is being tested.

[Viatcheslav Ostapenko]

Comment on attachment 184855 [details]
Patch

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

>> LayoutTests/ChangeLog:14
>> +
> 
> I think this is not a great test. You should focus on the use case at hand, not have complex rendering for the sake of it. Drawing simple rect should be enough to reproduce the bug with a pixel test.
> 
> The test rely on two timers. It is unlikely required as most canvas rendering can be forced.

The problem is not in repaint of canvas, but in handling of layer update message for canvas layer on UI side.
I have to make sure, that layer flush happens and 1st set of updates is processed on UI side before the 2nd re-layout and layer flash.
I'll make test simpler, but probably will have to use several canvases. It was the most reliable way to reproduce the problem and I just modified existing layout test which always showed this problem.

[Viatcheslav Ostapenko]

Created attachment 185966 [details]
Patch

Simplified test case.

[Viatcheslav Ostapenko]

Created attachment 186075 [details]
Patch

Rebased patch.

[Noam Rosenthal]

Comment on attachment 186075 [details]
Patch

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

I'd rather not add a test at all than add a flaky test.

> Source/WebCore/ChangeLog:9
> +        When page size changes and layer parameters get updated LayerTreeRenderer::setLayerState
> +        clears layer backing store and detaches canvas surface from the layer. If layer size is

You're missing "the"s
clears the layer backing store and detaches the canvas surface. If the layer size

> Source/WebCore/ChangeLog:10
> +        not changed then canvas recreate is not invoked because canvas size is also not changed

then the canvas is not recreated. This leaves the canvas detached from the layer, but still referenced from m_surfaceBackingStores.

> Source/WebCore/ChangeLog:12
> +        Don't assign layer backing store to layer in assignImageBackingToLayer if there is canvas

if there is a canvas surface

> LayoutTests/ChangeLog:10
> +        Add test checking that canvas painting is correct if layer parameters were changed,
> +        but webgl canvas didn't change.
> +

This is not a good description of the test. Please explain why you need a test with 3 canvases just to reproduce this.

[Viatcheslav Ostapenko]

Created attachment 187684 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #37)
> (From update of attachment 186075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186075&action=review
> You're missing "the"s

Fixed.
 
> This is not a good description of the test. Please explain why you need a test with 3 canvases just to reproduce this.

It is not necessary now with use of testRunner.display()

[Noam Rosenthal]

Comment on attachment 187684 [details]
Patch

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

Looks good, but please clean up the test.

> LayoutTests/fast/canvas/webgl/webgl-layer-update.html:7
> +        var canvases = document.getElementsByTagName('canvas');
> +        for(var i = 0; i < canvases.length; i++) {

You have only 1 canvas in this test

> LayoutTests/fast/canvas/webgl/webgl-layer-update.html:13
> +            window.setTimeout("testRunner.notifyDone()", 0);

Is this timeout necessary?

> LayoutTests/fast/canvas/webgl/webgl-layer-update.html:24
> +            window.setTimeout("doUpdate()", 100);

window.setTimeout(doUpdate, 100);

> LayoutTests/fast/canvas/webgl/webgl-layer-update.html:32
> +if (window.testRunner) {
> +    testRunner.waitUntilDone();
> +}

No need for braces

> LayoutTests/fast/canvas/webgl/webgl-layer-update.html:72
> +var ctx;
> +function addCanvas() {
> +    var can = document.createElement('canvas');
> +    can.width = can.height = 100;
> +    can.style.position = "absolute";
> +    can.style.left = "40px";
> +    can.style.top = "40px";
> +    document.body.appendChild(can);
> +    ctx = can.getContext("experimental-webgl");
> +    draw(ctx, 1, 0, 0, 1);
> +}
> +
> +function draw(gl, red, green, blue, alpha) {
> +    gl.clearColor(red, green, blue, alpha);
> +    gl.clear(gl.COLOR_BUFFER_BIT);
> +}
> +
> +addCanvas();

This can all be one function or without a function at all... it's hard to read.

> LayoutTests/fast/canvas/webgl/webgl-layer-update.html:76
> +    testRunner.display();
> +}

no need for braces.

[Viatcheslav Ostapenko]

Created attachment 187702 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187702 [details]
Patch

Clearing flags on attachment: 187702

Committed r142586: <http://trac.webkit.org/changeset/142586>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.