Bug 104664 – [EFL [WebGL] [Wk2] Contents are not rendered properly.

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

[Kalyan]

Try loading http://aleksandarrodic.com/p/jellyfish/ in MiniBrowser.

Expected Result:
Page is loaded and Jelly Fish are rendered properly. Try moving them , Jelly Fish move around and no rendering artifacts are visible.

Actual Result:

Page is loaded but Jelly Fish are not rendered properly. This becomes more evident when we try to move them around.

[Viatcheslav Ostapenko]

Created attachment 179975 [details]
Patch

[Kalyan]

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

Instead of releasing the surface texture (after binding to it), we should probably track the original bound texture and restore it.

Something like:
    GLuint current_texture;
    glGetIntegerv(GL_TEXTURE_BINDING_2D, &current_texture);
    glBindTexture(GL_TEXTURE_2D, textureId);
    // do all relevant stuff
    glBindTexture(GL_TEXTURE_2D, current_texture);

[Kalyan]

Comment on attachment 179975 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:443
> +        glBindTexture(GL_TEXTURE_2D, platformGetTextureID());

We need to track the current bound texture and restore it once done with this texture.

[Kalyan]

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

Some other relevant info from the extension:

 5. Should users be required to re-bind the drawable to a texture after
    the drawable has been rendered to?

    It is difficult to define what the contents of the texture would be if
    we don't require this.  Also, requiring this would allow implementations
    to perform an implicit copy at this point if they could not support
    texturing directly out of renderable memory.

    The problem with defining the contents of the texture after rendering
    has occured to the associated drawable is that there is no way to
    synchronize the use of the buffer as a source and as a destination.
    Direct OpenGL rendering is not necessarily done in the same command
    stream as X rendering.  At the time the pixmap is used as the source
    for a texturing operation, it could be in a state halfway through a
    copyarea operation in which half of it is say, white, and half is the
    result of the copyarea operation.  How is this defined?  Worse, some
    other OpenGL application could be halfway through a frame of rendering
    when the composite manager sources from it.  The buffer might just
    contain the results of a "glClear" operation at that point.

    To gurantee tear-free rendering, a composite manager would run as follows:

    -receive request for compositing:
    XGrabServer()
    glXWaitX() or XSync()
    glXBindTexImageEXT()

    <Do rendering/compositing>

    glXReleaseTexImageEXT()
    XUngrabServer()

    Apps that don't synchronize like this would get what's available, 
    and that may or may not be what they expect.

[Viatcheslav Ostapenko]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=179975) [details] [details]
> > Patch
> 
> Instead of releasing the surface texture (after binding to it), we should probably track the original bound texture and restore it.
> 
> Something like:
>     GLuint current_texture;
>     glGetIntegerv(GL_TEXTURE_BINDING_2D, &current_texture);
>     glBindTexture(GL_TEXTURE_2D, textureId);
>     // do all relevant stuff
>     glBindTexture(GL_TEXTURE_2D, current_texture);

I looked around and don't think saving and restoring of bound texture is necessary:

1. It is called out of painting.
2. All painting code binds textures every time when necessary
3. We don't do it anywhere else.

[Viatcheslav Ostapenko]

Created attachment 180183 [details]
Patch

[Noam Rosenthal]

Comment on attachment 180183 [details]
Patch

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

There's no d

> Source/WebCore/ChangeLog:3
> +        [EFL [WebGL] [Wk2] Contents are not rendered properly.

When, in what circumstances, and how does that get reproduced?

> Source/WebCore/ChangeLog:17
> +        Covered by existing tests.

Which existing test, and how?

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:443
> +        // Release previous lock and rebind texture to surface to get frame update

. at end of sentence.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:45
>  void TextureMapperSurfaceBackingStore::swapBuffersIfNeeded(uint32_t frontBuffer)
>  {
> -    if (m_graphicsSurface && m_graphicsSurface->frontBuffer() != frontBuffer)
> +    UNUSED_PARAM(frontBuffer);

The style guides say to use void TextureMapperSurfaceBackingStore::swapBuffersIfNeeded(uint32_t), use UNUSED_PARAM only in an #ifdef clause case.

[Noam Rosenthal]

(In reply to comment #7)
> (From update of attachment 180183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180183&action=review
> 
> There's no d
OOPS...
There's no description of how this bug manifests itself, how we reproduce it, and how it's fixed.

[Viatcheslav Ostapenko]

Comment on attachment 180183 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [EFL [WebGL] [Wk2] Contents are not rendered properly.
> 
> When, in what circumstances, and how does that get reproduced?

All webgl pages that do canvas updates.
Currently on Qt/EFL only 1st frame is shown until resize.

>> Source/WebCore/ChangeLog:17
>> +        Covered by existing tests.
> 
> Which existing test, and how?

Webgl animations that do more than one frame render.
Currently it is still broken because of other issues (linked to the bug), but with this patch frame updates are coming.

>> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:45
>> +    UNUSED_PARAM(frontBuffer);
> 
> The style guides say to use void TextureMapperSurfaceBackingStore::swapBuffersIfNeeded(uint32_t), use UNUSED_PARAM only in an #ifdef clause case.

So, should I remove parameter and rename to swapBuffers()?

[Noam Rosenthal]

(In reply to comment #9)
> (From update of attachment 180183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180183&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        [EFL [WebGL] [Wk2] Contents are not rendered properly.
> > 
> > When, in what circumstances, and how does that get reproduced?
> 
> All webgl pages that do canvas updates.
> Currently on Qt/EFL only 1st frame is shown until resize.
> 
> >> Source/WebCore/ChangeLog:17
> >> +        Covered by existing tests.
> > 
> > Which existing test, and how?
> 
> Webgl animations that do more than one frame render.
> Currently it is still broken because of other issues (linked to the bug), but with this patch frame updates are coming.

And there is no test that covers that?
> >> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:45
> >> +    UNUSED_PARAM(frontBuffer);
> > 
> > The style guides say to use void TextureMapperSurfaceBackingStore::swapBuffersIfNeeded(uint32_t), use UNUSED_PARAM only in an #ifdef clause case.
> 
> So, should I remove parameter and rename to swapBuffers()?
No. Read my comment again.

[Viatcheslav Ostapenko]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 180183 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=180183&action=review
> > 
> > >> Source/WebCore/ChangeLog:3
> > >> +        [EFL [WebGL] [Wk2] Contents are not rendered properly.
> > > 
> > > When, in what circumstances, and how does that get reproduced?
> > 
> > All webgl pages that do canvas updates.
> > Currently on Qt/EFL only 1st frame is shown until resize.
> > 
> > >> Source/WebCore/ChangeLog:17
> > >> +        Covered by existing tests.
> > > 
> > > Which existing test, and how?
> > 
> > Webgl animations that do more than one frame render.
> > Currently it is still broken because of other issues (linked to the bug), but with this patch frame updates are coming.
> 
> And there is no test that covers that?

Yes, there is.
WebGL repaint layout tests for example.
I just mean, that they are still broken even with this fix because of issues 105325 and 105326.

[Noam Rosenthal]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 180183 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=180183&action=review
> > > 
> > > >> Source/WebCore/ChangeLog:3
> > > >> +        [EFL [WebGL] [Wk2] Contents are not rendered properly.
> > > > 
> > > > When, in what circumstances, and how does that get reproduced?
> > > 
> > > All webgl pages that do canvas updates.
> > > Currently on Qt/EFL only 1st frame is shown until resize.
> > > 
> > > >> Source/WebCore/ChangeLog:17
> > > >> +        Covered by existing tests.
> > > > 
> > > > Which existing test, and how?
> > > 
> > > Webgl animations that do more than one frame render.
> > > Currently it is still broken because of other issues (linked to the bug), but with this patch frame updates are coming.
> > 
> > And there is no test that covers that?
> 
> Yes, there is.
> WebGL repaint layout tests for example.
> I just mean, that they are still broken even with this fix because of issues 105325 and 105326.

OK - please update the Changelog to reflect this information :)

[Viatcheslav Ostapenko]

Created attachment 180199 [details]
Patch

[Noam Rosenthal]

Comment on attachment 180199 [details]
Patch

Please update the changelog with the test information as discussed.

[Viatcheslav Ostapenko]

(In reply to comment #14)
> (From update of attachment 180199 [details])
> Please update the changelog with the test information as discussed.

Actually, at the last patch I added that "webgl repaint tests fail because of this".

[Kalyan]

(In reply to comment #5)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=179975) [details] [details] [details]
> > > Patch

> 1. It is called out of painting.
> 2. All painting code binds textures every time when necessary
> 3. We don't do it anywhere else.
k, Thanks for verifying it.

[Viatcheslav Ostapenko]

Created attachment 180353 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180353 [details]
Patch for landing

Clearing flags on attachment: 180353

Committed r138265: <http://trac.webkit.org/changeset/138265>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 105586

[Viatcheslav Ostapenko]

Created attachment 180431 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180431 [details]
Patch for landing

Clearing flags on attachment: 180431

Committed r138327: <http://trac.webkit.org/changeset/138327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.