Summary: | [Qt][EFL][WebGL] Minor refactoring of GraphicsSurface/GraphicsSurfaceGLX | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Viatcheslav Ostapenko <ostap73> | ||||||||||
Component: | WebGL | Assignee: | Viatcheslav Ostapenko <ostap73> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarcelo, kalyan.kondapally, kenneth, noam, rafael.lobo, webkit.review.bot, zeno | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Viatcheslav Ostapenko
2013-02-01 13:44:58 PST
Created attachment 186128 [details]
Patch
Please cc kalyan Comment on attachment 186128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:440 > + > + GLuint texture() const > + { > + if (!m_texture) { > + GLXPixmap pixmap = glxPixmap(); > + if (!pixmap) > + return 0; > + > + GraphicsSurfacePrivate* that = const_cast<GraphicsSurfacePrivate*>(this); Why make this const? (In reply to comment #3) > (From update of attachment 186128 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review > > > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:440 > > + > > + GLuint texture() const > > + { > > + if (!m_texture) { > > + GLXPixmap pixmap = glxPixmap(); > > + if (!pixmap) > > + return 0; > > + > > + GraphicsSurfacePrivate* that = const_cast<GraphicsSurfacePrivate*>(this); > > Why make this const? Texture is allocated once, but read often. Just to give compiler chance to optimize it. Have seen this pattern in many places in webkit&Qt. Comment on attachment 186128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:363 > + pGlXBindTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT, 0); Should we bind and release the texture immediately?? Is there any reason why we dont release it till next swapbuffer call?? > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:447 > + pGlXBindTexImageEXT(display(), pixmap, GLX_FRONT_EXT, 0); If we agree that it is k to bind and release the texture in swapbuffer call, we could call swapbuffer here to have the updated content. > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:474 > + pGlXReleaseTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT); we dont have to do this, incase we already release the texture in swapbuffer call Comment on attachment 186128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:363 >> + pGlXBindTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT, 0); > > Should we bind and release the texture immediately?? Is there any reason why we dont release it till next swapbuffer call?? To clarify, what I meant was to reverse the order. Instead of release -> bind we should have bind -> release (In reply to comment #6) > (From update of attachment 186128 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review > > >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:363 > >> + pGlXBindTexImageEXT(display(), glxPixmap(), GLX_FRONT_EXT, 0); > > > > Should we bind and release the texture immediately?? Is there any reason why we dont release it till next swapbuffer call?? > > To clarify, what I meant was to reverse the order. Instead of release -> bind we should have bind -> release Originally it was like this in my patch, but somebody had concerns about it. It works either way on ATI fglrx and mesa, but rebind makes problems on nvidia. It doesn't work either way on xorg-radeon ;) Comment on attachment 186128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:364 > + k, LGTM. Comment on attachment 186128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:358 > + // re-bind texture the XWindow. I'm not sure what you meant to say here. " ... has to re-bind the texture to the XWindow." may be? Comment on attachment 186128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186128&action=review >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:358 >> + // re-bind texture the XWindow. > > I'm not sure what you meant to say here. " ... has to re-bind the texture to the XWindow." may be? Oh! Yes! Thanks! Created attachment 186390 [details]
Patch
Comment on attachment 186390 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186390&action=review > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:357 > + // The buffers are being switched on the writing side, the reading side has to switched, you mean swapped? > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:361 > + // Release previous lock and rebind texture to surface to get frame update. "to get frame update", you mean "to ensure the frame updates"? or "facilitate a frame update"? > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:450 > + if (!m_texture) { > + GLXPixmap pixmap = glxPixmap(); > + if (!pixmap) > + return 0; > + > + GraphicsSurfacePrivate* that = const_cast<GraphicsSurfacePrivate*>(this); > + glGenTextures(1, &(that->m_texture)); > + glBindTexture(GL_TEXTURE_2D, that->m_texture); > + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > + pGlXBindTexImageEXT(display(), pixmap, GLX_FRONT_EXT, 0); > + } > + > + return m_texture; I'd prefer to construct this method differently: 1. return early if there is an m_texture. 2. use a local texture variable, and then at the end do the const_cast thing to save it to a member. This way the area where you const_cast is easy to read and understand, without "that" :) Created attachment 187751 [details]
Patch
Created attachment 187755 [details]
Patch
Comment on attachment 187755 [details] Patch Clearing flags on attachment: 187755 Committed r142587: <http://trac.webkit.org/changeset/142587> All reviewed patches have been landed. Closing bug. |