[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