Bug 106358 - [EFL][WebGL] WebGL content is not painted after resizing the viewport.
Summary: [EFL][WebGL] WebGL content is not painted after resizing the viewport.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Viatcheslav Ostapenko
URL: https://www.webkit.org/blog-files/web...
Keywords:
Depends on:
Blocks: 104532
  Show dependency treegraph
 
Reported: 2013-01-08 11:47 PST by Joone Hur
Modified: 2013-02-11 23:26 PST (History)
14 users (show)

See Also:


Attachments
Patch (7.72 KB, patch)
2013-01-08 22:22 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2013-01-19 23:06 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2013-01-21 22:24 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2013-01-22 00:05 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (12.56 KB, patch)
2013-01-25 12:08 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (12.46 KB, patch)
2013-01-25 13:10 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
webgl-composite-modes-update-expected.png (4.50 KB, image/png)
2013-01-25 21:33 PST, Viatcheslav Ostapenko
no flags Details
webgl-composite-modes-update-actual.png (4.30 KB, image/png)
2013-01-25 21:38 PST, Viatcheslav Ostapenko
no flags Details
Patch (11.79 KB, patch)
2013-01-25 22:21 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (7.55 KB, patch)
2013-02-01 00:25 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (7.73 KB, patch)
2013-02-01 10:23 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (7.94 KB, patch)
2013-02-11 14:48 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (7.60 KB, patch)
2013-02-11 15:41 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2013-01-08 11:47:34 PST
It seems that most WebGL contents are not painted after resizing the viewport.
Comment 1 Viatcheslav Ostapenko 2013-01-08 22:22:23 PST
Created attachment 181848 [details]
Patch
Comment 2 Kalyan 2013-01-18 16:52:53 PST
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. ??
Comment 3 Viatcheslav Ostapenko 2013-01-19 23:06:07 PST
Created attachment 183658 [details]
Patch
Comment 4 Viatcheslav Ostapenko 2013-01-19 23:18:43 PST
(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.
Comment 5 Noam Rosenthal 2013-01-20 00:36:30 PST
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.
Comment 6 Kalyan 2013-01-20 07:20:57 PST
(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 ??
Comment 7 Viatcheslav Ostapenko 2013-01-20 13:08:52 PST
(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.
Comment 8 Viatcheslav Ostapenko 2013-01-20 13:15:08 PST
(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.
Comment 9 Kalyan 2013-01-20 19:05:02 PST
(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.
Comment 10 Viatcheslav Ostapenko 2013-01-20 19:15:52 PST
(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?
Comment 11 Kalyan 2013-01-20 20:10:01 PST
(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.
Comment 12 Kalyan 2013-01-21 15:00:06 PST
(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.
Comment 13 Viatcheslav Ostapenko 2013-01-21 22:24:54 PST
Created attachment 183892 [details]
Patch
Comment 14 Viatcheslav Ostapenko 2013-01-21 22:26:38 PST
(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.
Comment 15 Noam Rosenthal 2013-01-21 22:44:29 PST
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?
Comment 16 Viatcheslav Ostapenko 2013-01-22 00:05:54 PST
Created attachment 183903 [details]
Patch
Comment 17 Viatcheslav Ostapenko 2013-01-22 00:06:29 PST
(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
Comment 18 Kalyan 2013-01-22 00:15:48 PST
(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 ??
Comment 19 Kalyan 2013-01-22 00:23:43 PST
(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)??
Comment 20 Viatcheslav Ostapenko 2013-01-22 19:10:59 PST
(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?
Comment 21 Viatcheslav Ostapenko 2013-01-25 12:08:09 PST
Created attachment 184790 [details]
Patch
Comment 22 Noam Rosenthal 2013-01-25 13:02:31 PST
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))
Comment 23 Viatcheslav Ostapenko 2013-01-25 13:10:22 PST
Created attachment 184798 [details]
Patch
Comment 24 WebKit Review Bot 2013-01-25 16:00:03 PST
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
Comment 25 Viatcheslav Ostapenko 2013-01-25 21:31:55 PST
(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.
Comment 26 Viatcheslav Ostapenko 2013-01-25 21:33:08 PST
Created attachment 184853 [details]
webgl-composite-modes-update-expected.png
Comment 27 Viatcheslav Ostapenko 2013-01-25 21:38:34 PST
Created attachment 184854 [details]
webgl-composite-modes-update-actual.png
Comment 28 Viatcheslav Ostapenko 2013-01-25 22:21:34 PST
Created attachment 184855 [details]
Patch
Comment 29 Viatcheslav Ostapenko 2013-01-25 22:23:08 PST
(In reply to comment #28)
> Created an attachment (id=184855) [details]
> Patch

Use chromium test result as a baseline.
Comment 30 Kalyan 2013-01-26 03:56:01 PST
(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
Comment 31 Hugo Parente Lima 2013-01-28 14:28:58 PST
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.
Comment 32 Viatcheslav Ostapenko 2013-01-28 16:58:26 PST
(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
Comment 33 Benjamin Poulain 2013-01-29 14:34:24 PST
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.
Comment 34 Viatcheslav Ostapenko 2013-01-29 15:07:36 PST
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.
Comment 35 Viatcheslav Ostapenko 2013-02-01 00:25:22 PST
Created attachment 185966 [details]
Patch

Simplified test case.
Comment 36 Viatcheslav Ostapenko 2013-02-01 10:23:10 PST
Created attachment 186075 [details]
Patch

Rebased patch.
Comment 37 Noam Rosenthal 2013-02-09 08:54:41 PST
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.
Comment 38 Viatcheslav Ostapenko 2013-02-11 14:48:55 PST
Created attachment 187684 [details]
Patch
Comment 39 Viatcheslav Ostapenko 2013-02-11 15:00:34 PST
(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()
Comment 40 Noam Rosenthal 2013-02-11 15:27:56 PST
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.
Comment 41 Viatcheslav Ostapenko 2013-02-11 15:41:35 PST
Created attachment 187702 [details]
Patch
Comment 42 WebKit Review Bot 2013-02-11 23:26:24 PST
Comment on attachment 187702 [details]
Patch

Clearing flags on attachment: 187702

Committed r142586: <http://trac.webkit.org/changeset/142586>
Comment 43 WebKit Review Bot 2013-02-11 23:26:31 PST
All reviewed patches have been landed.  Closing bug.