Bug 104535 - [EFL [WebGL] [Wk2] Resizing the canvas breaks WebGL
Summary: [EFL [WebGL] [Wk2] Resizing the canvas breaks WebGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kalyan
URL:
Keywords:
Depends on:
Blocks: 104532
  Show dependency treegraph
 
Reported: 2012-12-10 04:19 PST by Kalyan
Modified: 2012-12-12 07:56 PST (History)
6 users (show)

See Also:


Attachments
WIP (5.59 KB, patch)
2012-12-12 01:19 PST, Kalyan
no flags Details | Formatted Diff | Diff
patch (7.04 KB, patch)
2012-12-12 07:02 PST, Kalyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalyan 2012-12-10 04:19:10 PST
Try https://www.khronos.org/registry/webgl/sdk/demos/webkit/SpiritBox.html in MiniBrowser.

Resize the view.

Expected Result:
View is resized properly with blue background and the cube is spinning.

Actual Result:
View is resized but we dont see any blue background or spinning cube.
Comment 1 Kalyan 2012-12-12 01:19:21 PST
Created attachment 178995 [details]
WIP

This fixes the bug. There is not flickering while resizing though especially when trying to make the window smaller.
Comment 2 Zeno Albisser 2012-12-12 01:38:00 PST
Comment on attachment 178995 [details]
WIP

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

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:227
>      ::glFlush(); // Make sure all GL calls have been committed before resizing.

Are you sure that EFL does not need to flush here?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:326
> +#if (!PLATFORM(EFL))

I think we could unify this whole hunk for Qt and EFL.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:332
> +        else if (m_canvasSize != platformLayer->platformLayerSize()) {

I am not convinced this is completely correct. The canvas could theoretically change (different token) but have the same size, couldn't it?
On the other hand it can never change size, but keep the same token. Or am i missing something the EFL port does?
So may be we should check for both?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:348
> +    m_canvasSize = m_canvasPlatformLayer ? m_canvasPlatformLayer->platformLayerSize() : IntSize();

Same here. If you update the size, don't you need to update the token as well?
Comment 3 Kalyan 2012-12-12 01:49:44 PST
(In reply to comment #2)
> (From update of attachment 178995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178995&action=review
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:227
> >      ::glFlush(); // Make sure all GL calls have been committed before resizing.
>
> Are you sure that EFL does not need to flush here?
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:326
> > +#if (!PLATFORM(EFL))
> 
> I think we could unify this whole hunk for Qt and EFL.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:332
> > +        else if (m_canvasSize != platformLayer->platformLayerSize()) {
> 
> I am not convinced this is completely correct. The canvas could theoretically change (different token) but have the same size, couldn't it?
> On the other hand it can never change size, but keep the same token. Or am i missing something the EFL port does?
> So may be we should check for both?
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:348
> > +    m_canvasSize = m_canvasPlatformLayer ? m_canvasPlatformLayer->platformLayerSize() : IntSize();
> 
> Same here. If you update the size, don't you need to update the token as well?

One main difference is that in EFL port, we dont actually recreate the surface for every resize( or delete any associated GL resources). We only resize the window.
On Efl port Token never changes. I will remove the ifdefs in the coordinatedgraphicslayer
Comment 4 Simon Hausmann 2012-12-12 04:02:43 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 178995 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178995&action=review
> > 
> > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:227
> > >      ::glFlush(); // Make sure all GL calls have been committed before resizing.
> >
> > Are you sure that EFL does not need to flush here?
> > 
> > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:326
> > > +#if (!PLATFORM(EFL))
> > 
> > I think we could unify this whole hunk for Qt and EFL.
> > 
> > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:332
> > > +        else if (m_canvasSize != platformLayer->platformLayerSize()) {
> > 
> > I am not convinced this is completely correct. The canvas could theoretically change (different token) but have the same size, couldn't it?
> > On the other hand it can never change size, but keep the same token. Or am i missing something the EFL port does?
> > So may be we should check for both?
> > 
> > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:348
> > > +    m_canvasSize = m_canvasPlatformLayer ? m_canvasPlatformLayer->platformLayerSize() : IntSize();
> > 
> > Same here. If you update the size, don't you need to update the token as well?
> 
> One main difference is that in EFL port, we dont actually recreate the surface for every resize( or delete any associated GL resources). We only resize the window.
> On Efl port Token never changes. I will remove the ifdefs in the coordinatedgraphicslayer

The GraphicsSurface API is the lowest common denominator between different platforms. On Mac OS X re-sizing requires re-creating the underlying surface and therefore triggers a token change. As the lowest common denominator that behaviour has become part of the GraphicsSurface API contract, even if some platforms don't actually _do_ require a token change / surface re-creation.

Therefore I think we should write code that _uses_ GraphicsSurface based on the API assumptions that hold for all platforms, given that resizing the GL canvas is a case that I would qualify as happening rarely and not worth the optimization, compared to the benefit of less #ifdefs in WebKit code and more code sharing across ports.
Comment 5 Kalyan 2012-12-12 07:02:37 PST
Created attachment 179031 [details]
patch
Comment 6 WebKit Review Bot 2012-12-12 07:56:47 PST
Comment on attachment 179031 [details]
patch

Clearing flags on attachment: 179031

Committed r137467: <http://trac.webkit.org/changeset/137467>
Comment 7 WebKit Review Bot 2012-12-12 07:56:52 PST
All reviewed patches have been landed.  Closing bug.