Add check if depth and stencil attachments are same image

gpu/command_buffer/service/framebuffer_manager_unittest.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 <stddef.h>
 #include <stdint.h>
 
 #include "gpu/command_buffer/service/error_state_mock.h"
 #include "gpu/command_buffer/service/feature_info.h"
 #include "gpu/command_buffer/service/framebuffer_manager.h"
@@ skipping 187 matching lines @@
 
 TEST_F(FramebufferInfoTest, AttachRenderbuffer) {
   const GLuint kRenderbufferClient1Id = 33;
   const GLuint kRenderbufferService1Id = 333;
   const GLuint kRenderbufferClient2Id = 34;
   const GLuint kRenderbufferService2Id = 334;
   const GLuint kRenderbufferClient3Id = 35;
   const GLuint kRenderbufferService3Id = 335;
   const GLuint kRenderbufferClient4Id = 36;
   const GLuint kRenderbufferService4Id = 336;
+  const GLuint kRenderbufferClient5Id = 37;
+  const GLuint kRenderbufferService5Id = 337;
   const GLsizei kWidth1 = 16;
   const GLsizei kHeight1 = 32;
   const GLenum kFormat1 = GL_RGBA4;
   const GLenum kBadFormat1 = GL_DEPTH_COMPONENT16;
   const GLsizei kSamples1 = 0;
   const GLsizei kWidth2 = 16;
   const GLsizei kHeight2 = 32;
   const GLenum kFormat2 = GL_DEPTH_COMPONENT16;
   const GLsizei kSamples2 = 0;
   const GLsizei kWidth3 = 16;
   const GLsizei kHeight3 = 32;
   const GLenum kFormat3 = GL_STENCIL_INDEX8;
   const GLsizei kSamples3 = 0;
   const GLsizei kWidth4 = 16;
   const GLsizei kHeight4 = 32;
-  const GLenum kFormat4 = GL_STENCIL_INDEX8;
+  const GLenum kFormat4 = GL_DEPTH24_STENCIL8;
   const GLsizei kSamples4 = 0;
-  const GLsizei kDifferentSamples4 = 1;
+  const GLsizei kWidth5 = 16;
+  const GLsizei kHeight5 = 32;
+  const GLenum kFormat5 = GL_DEPTH24_STENCIL8;
+  const GLsizei kSamples5 = 0;
+  const GLsizei kDifferentSamples5 = 1;
 
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_COLOR_ATTACHMENT0));
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT));
   EXPECT_FALSE(
       framebuffer_->HasUnclearedAttachment(GL_DEPTH_STENCIL_ATTACHMENT));
 
   renderbuffer_manager_->CreateRenderbuffer(
       kRenderbufferClient1Id, kRenderbufferService1Id);
   Renderbuffer* renderbuffer1 =
       renderbuffer_manager_->GetRenderbuffer(kRenderbufferClient1Id);
   ASSERT_TRUE(renderbuffer1 != NULL);
 
-  // check adding one attachment
+  // Check adding one attachment.
   framebuffer_->AttachRenderbuffer(GL_COLOR_ATTACHMENT0, renderbuffer1);
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_COLOR_ATTACHMENT0));
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
   EXPECT_EQ(static_cast<GLenum>(GL_RGBA4),
             framebuffer_->GetReadBufferInternalFormat());
   EXPECT_FALSE(framebuffer_->HasDepthAttachment());
   EXPECT_FALSE(framebuffer_->HasStencilAttachment());
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   EXPECT_TRUE(framebuffer_->IsCleared());
 
   // Try a format that's not good for COLOR_ATTACHMENT0.
   renderbuffer_manager_->SetInfo(
       renderbuffer1, kSamples1, kBadFormat1, kWidth1, kHeight1);
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
 
   // Try a good format.
   renderbuffer_manager_->SetInfo(
       renderbuffer1, kSamples1, kFormat1, kWidth1, kHeight1);
   EXPECT_EQ(static_cast<GLenum>(kFormat1),
             framebuffer_->GetReadBufferInternalFormat());
   EXPECT_FALSE(framebuffer_->HasDepthAttachment());
   EXPECT_FALSE(framebuffer_->HasStencilAttachment());
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   EXPECT_FALSE(framebuffer_->IsCleared());
 
-  // check adding another
+  // Check adding another.
   renderbuffer_manager_->CreateRenderbuffer(
       kRenderbufferClient2Id, kRenderbufferService2Id);
   Renderbuffer* renderbuffer2 =
       renderbuffer_manager_->GetRenderbuffer(kRenderbufferClient2Id);
   ASSERT_TRUE(renderbuffer2 != NULL);
   framebuffer_->AttachRenderbuffer(GL_DEPTH_ATTACHMENT, renderbuffer2);
   EXPECT_TRUE(framebuffer_->HasUnclearedAttachment(GL_COLOR_ATTACHMENT0));
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
   EXPECT_EQ(static_cast<GLenum>(kFormat1),
             framebuffer_->GetReadBufferInternalFormat());
   EXPECT_TRUE(framebuffer_->HasDepthAttachment());
   EXPECT_FALSE(framebuffer_->HasStencilAttachment());
   // The attachment has a size of 0,0 so depending on the order of the map
   // of attachments it could either get INCOMPLETE_ATTACHMENT because it's 0,0
   // or INCOMPLETE_DIMENSIONS because it's not the same size as the other
   // attachment.
   GLenum status = framebuffer_->IsPossiblyComplete(feature_info_.get());
   EXPECT_TRUE(
       status == GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT ||
       status == GL_FRAMEBUFFER_INCOMPLETE_DIMENSIONS_EXT);
   EXPECT_FALSE(framebuffer_->IsCleared());
 
   renderbuffer_manager_->SetInfo(
       renderbuffer2, kSamples2, kFormat2, kWidth2, kHeight2);
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   EXPECT_FALSE(framebuffer_->IsCleared());
   EXPECT_TRUE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
 
-  // check marking them as cleared.
+  // Check marking them as cleared.
   manager_.MarkAttachmentsAsCleared(
       framebuffer_, renderbuffer_manager_.get(), texture_manager_.get());
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_COLOR_ATTACHMENT0));
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   EXPECT_TRUE(framebuffer_->IsCleared());
 
-  // Check adding one that is already cleared.
+  // Add another one to stencil attachment point.
   renderbuffer_manager_->CreateRenderbuffer(
       kRenderbufferClient3Id, kRenderbufferService3Id);
   Renderbuffer* renderbuffer3 =
       renderbuffer_manager_->GetRenderbuffer(kRenderbufferClient3Id);
   ASSERT_TRUE(renderbuffer3 != NULL);
   renderbuffer_manager_->SetInfo(
       renderbuffer3, kSamples3, kFormat3, kWidth3, kHeight3);
   renderbuffer_manager_->SetCleared(renderbuffer3, true);
 
   framebuffer_->AttachRenderbuffer(GL_STENCIL_ATTACHMENT, renderbuffer3);
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT));
   EXPECT_EQ(static_cast<GLenum>(kFormat1),
             framebuffer_->GetReadBufferInternalFormat());
   EXPECT_TRUE(framebuffer_->HasDepthAttachment());
   EXPECT_TRUE(framebuffer_->HasStencilAttachment());
+  // Binding different images to depth and stencil attachment points should
+  // return FRAMEBUFFER_UNSUPPORTED.
+  EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_UNSUPPORTED),
+            framebuffer_->IsPossiblyComplete(feature_info_.get()));
+  EXPECT_TRUE(framebuffer_->IsCleared());

[qiankun, 2016/07/21 09:29:21]

We don't need to check isCleared for unsupported framebuffer.

+
+  // Bind a renderbufer in format DEPTH_STENCIL to depth and stencil

[qiankun, 2016/07/21 09:29:21]

typo: renderbuffer

+  // attachment points.
+  renderbuffer_manager_->CreateRenderbuffer(
+      kRenderbufferClient4Id, kRenderbufferService4Id);
+  Renderbuffer* renderbuffer4 =
+      renderbuffer_manager_->GetRenderbuffer(kRenderbufferClient4Id);
+  ASSERT_TRUE(renderbuffer4 != NULL);
+  renderbuffer_manager_->SetInfo(
+      renderbuffer4, kSamples4, kFormat4, kWidth4, kHeight4);
+  renderbuffer_manager_->SetCleared(renderbuffer4, true);
+
+  EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
+  EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT));
+  framebuffer_->AttachRenderbuffer(GL_DEPTH_ATTACHMENT, renderbuffer4);
+  framebuffer_->AttachRenderbuffer(GL_STENCIL_ATTACHMENT, renderbuffer4);
+  EXPECT_EQ(static_cast<GLenum>(kFormat1),
+            framebuffer_->GetReadBufferInternalFormat());
+  EXPECT_TRUE(framebuffer_->HasDepthAttachment());
+  EXPECT_TRUE(framebuffer_->HasStencilAttachment());
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   EXPECT_TRUE(framebuffer_->IsCleared());
 
-  // Check marking the renderbuffer as unclared.
+  // Check marking the renderbuffer as uncleared.
   renderbuffer_manager_->SetInfo(
       renderbuffer1, kSamples1, kFormat1, kWidth1, kHeight1);
   EXPECT_EQ(static_cast<GLenum>(kFormat1),
             framebuffer_->GetReadBufferInternalFormat());
   EXPECT_TRUE(framebuffer_->HasDepthAttachment());
   EXPECT_TRUE(framebuffer_->HasStencilAttachment());
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   EXPECT_FALSE(framebuffer_->IsCleared());
 
   const Framebuffer::Attachment* attachment =
       framebuffer_->GetAttachment(GL_COLOR_ATTACHMENT0);
   ASSERT_TRUE(attachment != NULL);
   EXPECT_EQ(kWidth1, attachment->width());
   EXPECT_EQ(kHeight1, attachment->height());
   EXPECT_EQ(kSamples1, attachment->samples());
   EXPECT_EQ(kFormat1, attachment->internal_format());
   EXPECT_FALSE(attachment->cleared());
 
   EXPECT_TRUE(framebuffer_->HasUnclearedAttachment(GL_COLOR_ATTACHMENT0));
 
   // Clear it.
   manager_.MarkAttachmentsAsCleared(
       framebuffer_, renderbuffer_manager_.get(), texture_manager_.get());
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_COLOR_ATTACHMENT0));
   EXPECT_TRUE(framebuffer_->IsCleared());
 
-  // Check replacing an attachment
+  // Check replacing one attachment when both depth and stencil attachments
+  // are present.
   renderbuffer_manager_->CreateRenderbuffer(
-      kRenderbufferClient4Id, kRenderbufferService4Id);
-  Renderbuffer* renderbuffer4 =
-      renderbuffer_manager_->GetRenderbuffer(kRenderbufferClient4Id);
-  ASSERT_TRUE(renderbuffer4 != NULL);
+      kRenderbufferClient5Id, kRenderbufferService5Id);
+  Renderbuffer* renderbuffer5 =
+      renderbuffer_manager_->GetRenderbuffer(kRenderbufferClient5Id);
+  ASSERT_TRUE(renderbuffer5 != NULL);
   renderbuffer_manager_->SetInfo(
-      renderbuffer4, kSamples4, kFormat4, kWidth4, kHeight4);
+      renderbuffer5, kSamples5, kFormat5, kWidth5, kHeight5);
 
-  framebuffer_->AttachRenderbuffer(GL_STENCIL_ATTACHMENT, renderbuffer4);
+  framebuffer_->AttachRenderbuffer(GL_STENCIL_ATTACHMENT, renderbuffer5);
   EXPECT_TRUE(framebuffer_->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT));
   EXPECT_FALSE(framebuffer_->IsCleared());
 
   attachment = framebuffer_->GetAttachment(GL_STENCIL_ATTACHMENT);
   ASSERT_TRUE(attachment != NULL);
-  EXPECT_EQ(kWidth4, attachment->width());
-  EXPECT_EQ(kHeight4, attachment->height());
-  EXPECT_EQ(kSamples4, attachment->samples());
-  EXPECT_EQ(kFormat4, attachment->internal_format());
+  EXPECT_EQ(kWidth5, attachment->width());
+  EXPECT_EQ(kHeight5, attachment->height());
+  EXPECT_EQ(kSamples5, attachment->samples());
+  EXPECT_EQ(kFormat5, attachment->internal_format());
+  EXPECT_FALSE(attachment->cleared());
+  // Binding different images to depth and stencil attachment points should
+  // return FRAMEBUFFER_UNSUPPORTED.
+  EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_UNSUPPORTED),
+            framebuffer_->IsPossiblyComplete(feature_info_.get()));
+
+  // Check replacing both depth and stencil attachments.
+  framebuffer_->AttachRenderbuffer(GL_DEPTH_ATTACHMENT, renderbuffer5);
+  EXPECT_TRUE(framebuffer_->HasUnclearedAttachment(GL_DEPTH_ATTACHMENT));
+  EXPECT_TRUE(framebuffer_->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT));
+  EXPECT_FALSE(framebuffer_->IsCleared());
+
+  attachment = framebuffer_->GetAttachment(GL_DEPTH_ATTACHMENT);
+  EXPECT_EQ(kWidth5, attachment->width());
+  EXPECT_EQ(kHeight5, attachment->height());
+  EXPECT_EQ(kSamples5, attachment->samples());
+  EXPECT_EQ(kFormat5, attachment->internal_format());
   EXPECT_FALSE(attachment->cleared());
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
 
   // Change samples.
   ASSERT_FALSE(
       feature_info_->feature_flags().chromium_framebuffer_mixed_samples);
   renderbuffer_manager_->SetInfo(
-      renderbuffer4, kDifferentSamples4, kFormat4, kWidth4, kHeight4);
+      renderbuffer5, kDifferentSamples5, kFormat5, kWidth5, kHeight5);
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
   renderbuffer_manager_->SetInfo(
-      renderbuffer4, kSamples4, kFormat4, kWidth4, kHeight4);
+      renderbuffer5, kSamples5, kFormat5, kWidth5, kHeight5);
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
 
   // Check changing an attachment.
   renderbuffer_manager_->SetInfo(
-      renderbuffer4, kSamples4, kFormat4, kWidth4 + 1, kHeight4);
+      renderbuffer5, kSamples5, kFormat5, kWidth5 + 1, kHeight5);
 
   attachment = framebuffer_->GetAttachment(GL_STENCIL_ATTACHMENT);
   ASSERT_TRUE(attachment != NULL);
-  EXPECT_EQ(kWidth4 + 1, attachment->width());
-  EXPECT_EQ(kHeight4, attachment->height());
-  EXPECT_EQ(kSamples4, attachment->samples());
-  EXPECT_EQ(kFormat4, attachment->internal_format());
+  EXPECT_EQ(kWidth5 + 1, attachment->width());
+  EXPECT_EQ(kHeight5, attachment->height());
+  EXPECT_EQ(kSamples5, attachment->samples());
+  EXPECT_EQ(kFormat5, attachment->internal_format());
   EXPECT_FALSE(attachment->cleared());
   EXPECT_FALSE(framebuffer_->IsCleared());
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_INCOMPLETE_DIMENSIONS_EXT),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
 
   // Check removing it.
   framebuffer_->AttachRenderbuffer(GL_STENCIL_ATTACHMENT, NULL);
   EXPECT_FALSE(framebuffer_->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT));
   EXPECT_EQ(static_cast<GLenum>(kFormat1),
             framebuffer_->GetReadBufferInternalFormat());
   EXPECT_TRUE(framebuffer_->HasDepthAttachment());
   EXPECT_FALSE(framebuffer_->HasStencilAttachment());
-
-  EXPECT_TRUE(framebuffer_->IsCleared());
-  EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
+  EXPECT_FALSE(framebuffer_->IsCleared());
+  EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_INCOMPLETE_DIMENSIONS_EXT),

[qiankun, 2016/07/21 09:29:21]

After STENCIL attachment is removed, depth attachment and stencil attachment are
not the same one. Why it doesn't return FRAMEBUFFER_UNSUPPORTED?

[qiankun, 2016/07/21 15:21:39]

You should reply in the review context. 
So the validation code in framebuffer_manager.cc is wrong. It should be moved to
the beginning of IsPossiblyComplete().

[Jiawei, 2016/07/22 00:38:55]

After stencil attachment is removed, only depth attachment is present, which
does not meet the condition of the added validation code.

             framebuffer_->IsPossiblyComplete(feature_info_.get()));
 
   // Remove depth, Set color to 0 size.
   framebuffer_->AttachRenderbuffer(GL_DEPTH_ATTACHMENT, NULL);
   renderbuffer_manager_->SetInfo(renderbuffer1, kSamples1, kFormat1, 0, 0);
   EXPECT_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT),
             framebuffer_->IsPossiblyComplete(feature_info_.get()));
 
   // Remove color.
   framebuffer_->AttachRenderbuffer(GL_COLOR_ATTACHMENT0, NULL);
@@ skipping 1181 matching lines @@
       renderbuffer_manager_->GetRenderbuffer(kRenderbufferClientId);
   ASSERT_TRUE(renderbuffer != NULL);
   framebuffer_->AttachRenderbuffer(GL_COLOR_ATTACHMENT1, renderbuffer);
   EXPECT_TRUE(framebuffer_->GetReadBufferAttachment());
 }
 
 }  // namespace gles2
 }  // namespace gpu