Validate fbo color image format and fragment shader output variable type.

gpu/command_buffer/service/framebuffer_manager.cc

Index: gpu/command_buffer/service/framebuffer_manager.cc
diff --git a/gpu/command_buffer/service/framebuffer_manager.cc b/gpu/command_buffer/service/framebuffer_manager.cc
index 0c5d96708e856d8e9c3d8578eb785c9f2d18c71e..3fcd013bf2778df0db1202f20f706316bd16d1cb 100644
--- a/gpu/command_buffer/service/framebuffer_manager.cc
+++ b/gpu/command_buffer/service/framebuffer_manager.cc
@@ -376,9 +376,17 @@ Framebuffer::Framebuffer(
   manager->StartTracking(this);
   DCHECK_GT(manager->max_draw_buffers_, 0u);
   draw_buffers_.reset(new GLenum[manager->max_draw_buffers_]);
+  adjusted_draw_buffers_.reset(new GLenum[manager->max_draw_buffers_]);
   draw_buffers_[0] = GL_COLOR_ATTACHMENT0;
-  for (uint32_t i = 1; i < manager->max_draw_buffers_; ++i)
-    draw_buffers_[i] = GL_NONE;
+  adjusted_draw_buffers_[0] = GL_COLOR_ATTACHMENT0;
+  for (uint32_t ii = 1; ii < manager->max_draw_buffers_; ++ii) {
+    draw_buffers_[ii] = GL_NONE;
+    adjusted_draw_buffers_[ii] = GL_NONE;
+  }
+
+  color_attachment_base_types_.reset(
+      new ShaderVariableBaseType[manager->max_draw_buffers_]);
+  ResetColorAttachmentBaseTypes();
 }
 
 Framebuffer::~Framebuffer() {
@@ -392,6 +400,12 @@ Framebuffer::~Framebuffer() {
   }
 }
 
+void Framebuffer::ResetColorAttachmentBaseTypes() {
+  for (size_t ii = 1; ii < manager_->max_draw_buffers_; ++ii) {
+    color_attachment_base_types_[ii] = SHADER_VARIABLE_UNDEFINED_TYPE;
+  }
+}
+
 bool Framebuffer::HasUnclearedAttachment(
     GLenum attachment) const {
   AttachmentMap::const_iterator it =
@@ -470,7 +484,8 @@ bool Framebuffer::HasSRGBAttachments() const {
   return false;
 }
 
-bool Framebuffer::PrepareDrawBuffersForClear() const {
+bool Framebuffer::PrepareDrawBuffersForClearingUninitializedAttachments(
+    ) const {
   std::unique_ptr<GLenum[]> buffers(new GLenum[manager_->max_draw_buffers_]);
   for (uint32_t i = 0; i < manager_->max_draw_buffers_; ++i)
     buffers[i] = GL_NONE;
@@ -490,7 +505,7 @@ bool Framebuffer::PrepareDrawBuffersForClear() const {
   }
   bool different = false;
   for (uint32_t i = 0; i < manager_->max_draw_buffers_; ++i) {
-    if (buffers[i] != draw_buffers_[i]) {
+    if (buffers[i] != adjusted_draw_buffers_[i]) {
       different = true;
       break;
     }
@@ -500,13 +515,67 @@ bool Framebuffer::PrepareDrawBuffersForClear() const {
   return different;
 }
 
-void Framebuffer::RestoreDrawBuffersAfterClear() const {
-  glDrawBuffersARB(manager_->max_draw_buffers_, draw_buffers_.get());
+void Framebuffer::RestoreDrawBuffers() const {
+  glDrawBuffersARB(manager_->max_draw_buffers_, adjusted_draw_buffers_.get());
+}
+
+bool Framebuffer::ValidateAndAdjustDrawBuffers(
+    const ShaderVariableBaseType* fragment_output_base_types) {
+  bool adjust = false;
+  for (size_t ii = 0; ii < manager_->max_draw_buffers_; ++ii) {

[piman, 2016/07/13 22:35:55]

This loop seems kind of expensive...
Truth is, we don't really handle more than 16 draw buffers in practice (since
validation disallows anything beyond GL_DRAW_BUFFER15), and there actually isn't
currently any GPU that handles more than 8 (according to
http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_...
)

I think we can make this significantly cheaper by coalescing the information
into bitfields, 2 bits per buffer (so 32 bits for the 16 buffers we support), so
that we can use bitwise logic operations to find whether the shader matches the
FBO:
- on the FBO side, one fbo_buffer_bound_mask for which the bit value is either
0x3 if the draw buffer is bound, 0x0 otherwise, and one fbo_buffer_type_mask
that contains the type (only relevant if the buffer is bound)
- on the shader side, similarly, shader_buffer_written_mask and
shader_buffer_type_mask

Then the logic for draw can be simplified to:
if (fbo_buffer_bound_mask & ~shader_buffer_written_mask)
  adjust = true;
if ((fbo_buffer_bound_mask & fbo_buffer_type_mask) != (fbo_buffer_bound_mask &
shader_buffer_type_mask))
  return false;

(and something equivalent for the clear case)

Then we only need a loop to generate the proper array for glDrawBuffers, in the
adjust case (which should be rare).

+    if (draw_buffers_[ii] == GL_NONE) {
+      DCHECK_EQ(static_cast<GLenum>(GL_NONE), adjusted_draw_buffers_[ii]);
+      continue;
+    }
+    if (color_attachment_base_types_[ii] == SHADER_VARIABLE_UNDEFINED_TYPE) {
+      if (adjusted_draw_buffers_[ii] != GL_NONE)
+        adjust = true;
+      continue;
+    }
+    if (fragment_output_base_types) {  // Draw call cases.
+      if (fragment_output_base_types[ii] == SHADER_VARIABLE_UNDEFINED_TYPE) {
+        if (adjusted_draw_buffers_[ii] != GL_NONE)
+          adjust = true;
+        continue;
+      }
+      if (fragment_output_base_types[ii] != color_attachment_base_types_[ii]) {
+        return false;
+      }
+    } else {  // Clear call cases.
+      if (color_attachment_base_types_[ii] != SHADER_VARIABLE_FLOAT) {
+        if (adjusted_draw_buffers_[ii] != GL_NONE)
+          adjust = true;
+        continue;
+      }
+    }
+  }
+  if (adjust) {
+    // This won't be reached in every draw/clear call - only when framebuffer
+    // or program has changed.
+    for (size_t ii = 0; ii < manager_->max_draw_buffers_; ++ii) {
+      if (draw_buffers_[ii] == GL_NONE)
+        continue;
+      if (color_attachment_base_types_[ii] == SHADER_VARIABLE_UNDEFINED_TYPE) {
+        adjusted_draw_buffers_[ii] = GL_NONE;
+        continue;
+      }
+      if (fragment_output_base_types) {  // Draw call cases.
+        if (fragment_output_base_types[ii] == SHADER_VARIABLE_UNDEFINED_TYPE) {
+          adjusted_draw_buffers_[ii] = GL_NONE;
+        }
+      } else {  // Clear call cases.
+        if (color_attachment_base_types_[ii] != SHADER_VARIABLE_FLOAT) {
+          adjusted_draw_buffers_[ii] = GL_NONE;
+        }
+      }
+    }
+    glDrawBuffersARB(manager_->max_draw_buffers_, adjusted_draw_buffers_.get());
+  }
+  return true;
 }
 
 void Framebuffer::ClearUnclearedIntOr3DTexturesOrPartiallyClearedTextures(
-    GLES2Decoder* decoder,
-    TextureManager* texture_manager) {
+    GLES2Decoder* decoder, TextureManager* texture_manager) {
   for (AttachmentMap::const_iterator it = attachments_.begin();
        it != attachments_.end(); ++it) {
     if (!it->second->IsTextureAttachment() || it->second->cleared())
@@ -524,10 +593,10 @@ void Framebuffer::ClearUnclearedIntOr3DTexturesOrPartiallyClearedTextures(
 }
 
 void Framebuffer::MarkAttachmentAsCleared(
-      RenderbufferManager* renderbuffer_manager,
-      TextureManager* texture_manager,
-      GLenum attachment,
-      bool cleared) {
+    RenderbufferManager* renderbuffer_manager,
+    TextureManager* texture_manager,
+    GLenum attachment,
+    bool cleared) {
   AttachmentMap::iterator it = attachments_.find(attachment);
   if (it != attachments_.end()) {
     Attachment* a = it->second.get();
@@ -540,9 +609,9 @@ void Framebuffer::MarkAttachmentAsCleared(
 }
 
 void Framebuffer::MarkAttachmentsAsCleared(
-      RenderbufferManager* renderbuffer_manager,
-      TextureManager* texture_manager,
-      bool cleared) {
+    RenderbufferManager* renderbuffer_manager,
+    TextureManager* texture_manager,
+    bool cleared) {
   for (AttachmentMap::iterator it = attachments_.begin();
        it != attachments_.end(); ++it) {
     Attachment* attachment = it->second.get();
@@ -745,8 +814,10 @@ GLenum Framebuffer::GetDrawBuffer(GLenum draw_buffer) const {
 
 void Framebuffer::SetDrawBuffers(GLsizei n, const GLenum* bufs) {
   DCHECK(n <= static_cast<GLsizei>(manager_->max_draw_buffers_));
-  for (GLsizei i = 0; i < n; ++i)
+  for (GLsizei i = 0; i < n; ++i) {
     draw_buffers_[i] = bufs[i];
+    adjusted_draw_buffers_[i] = bufs[i];
+  }
 }
 
 bool Framebuffer::HasAlphaMRT() const {
@@ -818,6 +889,27 @@ void Framebuffer::UnbindTexture(
   } while (!done);
 }
 
+void Framebuffer::UpdateColorAttachmentBaseTypes() {
+  ResetColorAttachmentBaseTypes();
+  for (AttachmentMap::const_iterator it = attachments_.begin();
+       it != attachments_.end(); ++it) {
+    if (it->first < GL_COLOR_ATTACHMENT0 ||
+        it->first >= GL_COLOR_ATTACHMENT0 + manager_->max_draw_buffers_) {

[piman, 2016/07/13 22:35:55]

this should be manager_->max_color_attachments_

+      continue;
+    }
+    size_t index = it->first - GL_COLOR_ATTACHMENT0;

[piman, 2016/07/13 22:35:55]

I think there's confusion about how color_attachment_base_types_ is indexed...
Here it's setup indexed by the color attachment index, but in
ValidateAndAdjustDrawBuffers,
it's looked up by draw buffer index.

I think it would make sense to keep the cache relative to the draw buffer index
- it would make the loop at draw time a bit cheaper. It means the loop in this
function needs to go over the draw buffers rather than the attachments. It means
that we have to run this again from SetDrawBuffers, but I think that's ok - I
prefer paying the cost at glDrawBuffers time rather than every draw call.

[piman, 2016/07/13 23:16:36]

Ok, so I now understand, this works in ES because it has an extra constraint
that draw_buffers[i] can only be GL_COLOR_ATTACHMENTi or GL_NONE (as opposed to
desktop GL where no such constraint exist).

Still, looping over the draw buffers instead of the attachments is a bit
simpler, and lets us do some of the processing (the GL_NONE case) ahead of time.

+    Attachment* attachment = it->second.get();
+    GLenum internal_format = attachment->internal_format();
+    if (GLES2Util::IsSignedIntegerFormat(internal_format)) {
+      color_attachment_base_types_[index] = SHADER_VARIABLE_INT;
+    } else if (GLES2Util::IsUnsignedIntegerFormat(internal_format)) {
+      color_attachment_base_types_[index] = SHADER_VARIABLE_UINT;
+    } else {
+      color_attachment_base_types_[index] = SHADER_VARIABLE_FLOAT;
+    }
+  }
+}
+
 Framebuffer* FramebufferManager::GetFramebuffer(
     GLuint client_id) {
   FramebufferMap::iterator it = framebuffers_.find(client_id);