[Webkit-unassigned] [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76248





--- Comment #12 from ChangSeok Oh <kevin.cs.oh at gmail.com>  2012-02-03 01:52:41 PST ---
(From update of attachment 124068)
View in context: https://bugs.webkit.org/attachment.cgi?id=124068&action=review

Thank you for review. The codes look more simple and better! :)
BTW, I have a small question, please see below.

>> Source/WebCore/platform/graphics/GraphicsContext3D.h:902
>> +    void resolveMultisamplingIfNecessary(GC3Dint x, GC3Dint y, GC3Dsizei width, GC3Dsizei height);
> 
> Better to pass an IntRect here.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:46
>> +bool GraphicsContext3D::resizeFBOs(int width, int height)
> 
> Better to pass an const IntSize& here. reshapeFBOs might be a better name to match the caller.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:74
>> +                extensions->ensureEnabled("GL_EXT_packed_depth_stencil");
> 
> Here it might be better to do something like this:
> 
> const const char* packedDepthStencilExtension = isGLES2Compliant() ? "GL_OES_packed_depth_stencil" : "GL_EXT_packed_depth_stencil";
> if (extensions->supports(packedDepthStencilExtension)) {
>     extensions->ensureEnabled(packedDepthStencilExtension);

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:160
>> +        resolveMultisamplingIfNecessary(0, 0, m_currentWidth, m_currentHeight);
> 
> It seems like this should be checking whether m_boundFBO == m_multisampleFBO, but we need to ask the original author to answer that question, I think. Later it might be good to suck more of the logic into resolveMultisamplingIfNecessary. Perhaps it would be better as bool resolveMultisamplingIfNecessaryAndBindSingleSampleFramebuffer() or something like that. :)

Reasonable. When I test with "if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)" in here, no issue found. So, do you want to deal with the change you mentioned in this bug? Please let me know. :)

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204
>> +            std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA
> 
> Nit: Missing a period at the end of this comment.

Done. Also I did it at some lines else. :)

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:59
>> +        supportPackedDepthStencilBuffer = extensions->supports("GL_OES_packed_depth_stencil");
> 
> This could simply be:
> 
> // We don't allow the logic where stencil is required and depth is not.
> // See GraphicsContext3D::validateAttributes.
> supportsPackedDepthStencilBuffer = (m_attrs.stencil || m_attrs.depth) && getExtensions()->->supports("GL_OES_packed_depth_stencil");

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:77
>> +    if (!m_attrs.antialias && (m_attrs.stencil || m_attrs.depth)) {
> 
> Better to ASSERT(!m_attrs.antialias) instead of checking it when you know it's always false.

Done. Inserted it at the upper row.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.