command_buffer: correctly use glGetFramebufferAttachmentParameter

gpu/command_buffer/service/gles2_cmd_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <stdio.h>
@@ skipping 6049 matching lines @@
     case GL_ALPHA_BITS:
       *num_written = 1;
       if (params) {
         GLint v = 0;
         Framebuffer* framebuffer =
             GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER_EXT);
         if (framebuffer) {
           if (framebuffer->HasAlphaMRT() &&
               framebuffer->HasSameInternalFormatsMRT()) {
             if (feature_info_->gl_version_info().is_desktop_core_profile) {
-              glGetFramebufferAttachmentParameterivEXT(
-                  GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
-                  GL_FRAMEBUFFER_ATTACHMENT_ALPHA_SIZE, &v);
+              if (framebuffer->HasColorAttachment(0)) {

[Zhenyao Mo, 2016/07/04 23:16:06]

Here we have a pre-existing issue: we should query any attachment since they all
have the same format.  Here we limit to COLOR_ATTACHMENT0, but we don't have to
have an image attached to COLOR_ATTACHMENT0.

+                glGetFramebufferAttachmentParameterivEXT(
+                    GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
+                    GL_FRAMEBUFFER_ATTACHMENT_ALPHA_SIZE, &v);
+              }
             } else {
               glGetIntegerv(GL_ALPHA_BITS, &v);
             }
           }
         } else {
           v = (ClientExposedBackBufferHasAlpha() ? 8 : 0);
         }
         params[0] = v;
       }
       return true;
     case GL_DEPTH_BITS:
       *num_written = 1;
       if (params) {
         GLint v = 0;
         if (feature_info_->gl_version_info().is_desktop_core_profile) {
           Framebuffer* framebuffer =
               GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER_EXT);
-          if (framebuffer) {
+          if (framebuffer && framebuffer->HasDepthAttachment()) {

[Zhenyao Mo, 2016/07/04 23:16:06]

This should be an inter if statement.  Because if framebuffer is not null, and
you don't have depth attachment, you should return 0 (already set by default)
rather than querying the back_buffer.

             glGetFramebufferAttachmentParameterivEXT(
                 GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT,
                 GL_FRAMEBUFFER_ATTACHMENT_DEPTH_SIZE, &v);
           } else {
             v = (back_buffer_has_depth_ ? 24 : 0);
           }
         } else {
           glGetIntegerv(GL_DEPTH_BITS, &v);
         }
         params[0] = BoundFramebufferHasDepthAttachment() ? v : 0;
       }
       return true;
     case GL_RED_BITS:
     case GL_GREEN_BITS:
     case GL_BLUE_BITS:
       *num_written = 1;
       if (params) {
         GLint v = 0;
         if (feature_info_->gl_version_info().is_desktop_core_profile) {
           Framebuffer* framebuffer =
               GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER_EXT);
-          if (framebuffer) {
+          if (framebuffer && framebuffer->HasColorAttachment(0)) {

[Zhenyao Mo, 2016/07/04 23:16:06]

Here we should do similar things as GL_ALPHA_BITS, checking color formats are
the same, checking at least one color attachment exists, and put this condition
as an inner loop.

             GLenum framebuffer_enum = 0;
             switch (pname) {
               case GL_RED_BITS:
                 framebuffer_enum = GL_FRAMEBUFFER_ATTACHMENT_RED_SIZE;
                 break;
               case GL_GREEN_BITS:
                 framebuffer_enum = GL_FRAMEBUFFER_ATTACHMENT_GREEN_SIZE;
                 break;
               case GL_BLUE_BITS:
                 framebuffer_enum = GL_FRAMEBUFFER_ATTACHMENT_BLUE_SIZE;
@@ skipping 10 matching lines @@
         params[0] = v;
       }
       return true;
     case GL_STENCIL_BITS:
       *num_written = 1;
       if (params) {
         GLint v = 0;
         if (feature_info_->gl_version_info().is_desktop_core_profile) {
           Framebuffer* framebuffer =
               GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER_EXT);
-          if (framebuffer) {
+          if (framebuffer && framebuffer->HasStencilAttachment()) {

[Zhenyao Mo, 2016/07/04 23:16:06]

Same here, should be an inner if statement.

             glGetFramebufferAttachmentParameterivEXT(
                 GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
                 GL_FRAMEBUFFER_ATTACHMENT_STENCIL_SIZE, &v);
           } else {
             v = (back_buffer_has_stencil_ ? 8 : 0);
           }
         } else {
           glGetIntegerv(GL_STENCIL_BITS, &v);
         }
         params[0] = BoundFramebufferHasStencilAttachment() ? v : 0;
@@ skipping 10926 matching lines @@
 }
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "base/macros.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu