Bug 104664

Created attachment 179975 [details] Patch

(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);

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.

(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.

(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.

Created attachment 180183 [details] Patch

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.

(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.

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()?

(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.

(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.

(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 :)

Created attachment 180199 [details] Patch

Comment on attachment 180199 [details] Patch Please update the changelog with the test information as discussed.

(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".

(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.

Created attachment 180353 [details] Patch for landing

Comment on attachment 180353 [details] Patch for landing Clearing flags on attachment: 180353 Committed r138265: <http://trac.webkit.org/changeset/138265>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 105586

Created attachment 180431 [details] Patch for landing

Comment on attachment 180431 [details] Patch for landing Clearing flags on attachment: 180431 Committed r138327: <http://trac.webkit.org/changeset/138327>

All reviewed patches have been landed. Closing bug.