[Webkit-unassigned] [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 25 07:54:54 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=76248
--- Comment #7 from ChangSeok Oh <kevin.cs.oh at gmail.com> 2012-01-25 07:54:54 PST ---
(From update of attachment 122730)
View in context: https://bugs.webkit.org/attachment.cgi?id=122730&action=review
Thank you very much for review!
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:51
>> + // FIXME: We don't support antialiasing yet.
>
> See below: Here you could call resolveMultisamplingIfNecessary. I think with that change you could share this method entirely.
Added resolveMultisamplingIfNecessary as you commented. And also I moved readRenderingResults into GraphicsContext3DCommon.
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:70
>> + std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA
>
> If you put this function back in the shared file you could just add a member like m_convertRenderingResultsToBGRA and make it true for GLES and false for desktop GL.
Done. In my humble opinion, a new member is not needed. Because just using isGLES2Compliant() is enough to do that.
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:194
>> + ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_boundFBO);
>
> Is this bit of code exactly the same as the desktop counterpart? If so, you should split it off into a helper method and call it, rather than copy and pasting.
Done. I moved GC3D::reshape() into GC3DOpenGLCommon and newly added helper function resizeFBOs at GC3DOpenGL.cpp & GC3DOpenGLES.cpp for each files.
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:213
>> + m_layerComposited = true;
>
> Hrm. This is one case where it would have made more sense to use and #ifdef. Another approach is to move this to the common file and make a helper called resolveMultisamplingIfNecessary which has an empty implementation for OpenGLES.
Done.
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:223
>> + }
>
> This could be shared easily if you simply ensured that m_attrs.antialias was always false for OpenGL ES.
Done. Added condition in GC3D::validateAttributes to disable antialiasing for GLES.
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:229
>> + ::glCopyTexImage2D(target, level, internalformat, x, y, width, height, border);
>
> You can also share this if you make resolveMultisamplingIfNecessary take a rectangle.
Done.
>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:242
>> + ::glReadPixels(x, y, width, height, format, type, data);
>
> Hey, same thing here! :)
ditto. :)
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list