[webkit-reviews] review granted: [Bug 211156] [WebGL2] Implement multiple render target entry points : [Attachment 404833] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Kenneth Russell
<kbr at google.com>'s request for review:
Bug 211156: [WebGL2] Implement multiple render target entry points
https://bugs.webkit.org/show_bug.cgi?id=211156

Attachment 404833: Patch

https://bugs.webkit.org/attachment.cgi?id=404833&action=review




--- Comment #5 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 404833
  --> https://bugs.webkit.org/attachment.cgi?id=404833
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404833&action=review

> Source/WebCore/html/canvas/WebGL2RenderingContext.h:321
> +    enum ClearBufferCaller {

enum class ClearBufferCaller : uint8_t

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:-649
> -#if ENABLE(WEBGL2)
> -    // FIXME: The logic here seems wrong. If we don't have WebGL 2 enabled
at all, then
> -    // we skip the m_webglDrawBuffers check. But if we do have WebGL 2
enabled, then we
> -    // perform this check, for WebGL 1 contexts only.
> -    if (!context()->m_webglDrawBuffers && !context()->isWebGL2())
> -	   return;
> -#endif
> -    bool reset = force;
> -    // This filtering works around graphics driver bugs on Mac OS X.
> -    for (size_t i = 0; i < m_drawBuffers.size(); ++i) {
> -	   if (m_drawBuffers[i] != GraphicsContextGL::NONE &&
getAttachment(m_drawBuffers[i])) {
> -	       if (m_filteredDrawBuffers[i] != m_drawBuffers[i]) {
> -		   m_filteredDrawBuffers[i] = m_drawBuffers[i];
> -		   reset = true;
> +    if (context()->isWebGL2() || context()->m_webglDrawBuffers) {
> +	   bool reset = force;
> +	   // This filtering works around graphics driver bugs on Mac OS X.
> +	   for (size_t i = 0; i < m_drawBuffers.size(); ++i) {
> +	       if (m_drawBuffers[i] != GraphicsContextGL::NONE &&
getAttachment(m_drawBuffers[i])) {
> +		   if (m_filteredDrawBuffers[i] != m_drawBuffers[i]) {
> +		       m_filteredDrawBuffers[i] = m_drawBuffers[i];
> +		       reset = true;
> +		   }
> +	       } else {
> +		   if (m_filteredDrawBuffers[i] != GraphicsContextGL::NONE) {
> +		       m_filteredDrawBuffers[i] = GraphicsContextGL::NONE;
> +		       reset = true;
> +		   }
>	       }
> -	   } else {
> -	       if (m_filteredDrawBuffers[i] != GraphicsContextGL::NONE) {
> -		   m_filteredDrawBuffers[i] = GraphicsContextGL::NONE;
> -		   reset = true;
> +	   }
> +	   if (reset) {
> +	       if (context()->isWebGL2()) {
> +		   context()->graphicsContextGL()->drawBuffers(
> +		       m_filteredDrawBuffers.size(),
m_filteredDrawBuffers.data());
> +	       } else {
> +		  
context()->graphicsContextGL()->getExtensions().drawBuffersEXT(
> +		       m_filteredDrawBuffers.size(),
m_filteredDrawBuffers.data());
>	       }
>	   }
>      }
> -    if (reset) {
> -	   context()->graphicsContextGL()->getExtensions().drawBuffersEXT(
> -	       m_filteredDrawBuffers.size(), m_filteredDrawBuffers.data());
> -    }

Dean will have to review this for me.