Test gfx::GpuMemoryBuffer::BGRA_8888 GpuMemoryBuffer lifecycle.

gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc

 // Copyright 2014 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 <GLES2/gl2.h>
 #include <GLES2/gl2chromium.h>
 #include <GLES2/gl2ext.h>
 #include <GLES2/gl2extchromium.h>
 
 #include "base/bind.h"
@@ skipping 15 matching lines @@
 using testing::Invoke;
 using testing::Return;
 using testing::SetArgPointee;
 using testing::StrictMock;
 
 namespace gpu {
 namespace gles2 {
 
 static const int kImageWidth = 32;
 static const int kImageHeight = 32;
-static const int kImageBytesPerPixel = 4;
 
-class GpuMemoryBufferTest : public testing::Test {
+class GpuMemoryBufferTest
+    : public testing::TestWithParam<gfx::GpuMemoryBuffer::Format> {
  protected:
   void SetUp() override {
     gl_.Initialize(GLManager::Options());
     gl_.MakeCurrent();
 
     glGenTextures(2, texture_ids_);
     glBindTexture(GL_TEXTURE_2D, texture_ids_[1]);
 
     glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
     glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
@@ skipping 14 matching lines @@
     glDeleteFramebuffers(1, &framebuffer_id_);
 
     gl_.Destroy();
   }
 
   GLManager gl_;
   GLuint texture_ids_[2];
   GLuint framebuffer_id_;
 };
 
+namespace {

[reveman, 2015/04/03 02:20:16]

nit: we use a blank line between namespace and code in most chromium code

[Daniele Castagna, 2015/04/03 02:56:14]

Ack. It'll be there at the next iteration.

[Daniele Castagna, 2015/04/03 04:26:46]

Done.

+std::vector<uint8> GetTexturePixel(const gfx::GpuMemoryBuffer::Format format) {
+  std::vector<uint8> pixel;
+  switch (format) {
+    case gfx::GpuMemoryBuffer::RGBA_8888:
+      pixel.push_back(255u);
+      pixel.push_back(0u);
+      pixel.push_back(0u);
+      pixel.push_back(255u);
+      return pixel;
+    case gfx::GpuMemoryBuffer::BGRA_8888:
+      pixel.push_back(0u);
+      pixel.push_back(0u);
+      pixel.push_back(255u);
+      pixel.push_back(255u);
+      return pixel;
+    default:

[reveman, 2015/04/03 02:20:16]

Please avoid a default case and explicitly list all formats. I'd like to make
sure that when we add a new format we update this code.

[Daniele Castagna, 2015/04/03 02:56:14]

I don't see why if someone adds a new format (to GpuMemoryBuffer), they'd have
to touch this code.

Listing all the formats in all the places where format is used make it so the
number of lines of code we have to write is O(max_enum * places_where_its_used).

Wouldn't it be better, at least in this case where we're in a test, to assert
that the default case is not executed?

If someone adds a new format to GpuMemoryBuffer, but not to this test, I don't
see why this test should change.
On the other side, if someone adds a new format to the test, I'd expect they'd
run the test at least once, and they would easily figure out what to change.

If you're OK with that, I can just change NOTREACHED() with ASSERT_TRUE(false);

[reveman, 2015/04/03 03:41:02]

If someone adds a new format to GpuMemoryBuffer, they might not know or remember
that we have these tests. I'd like the compiler to remind me so please change
this.

Having a default case to save ourselves from typing a few lines of code is the
wrong trade-off imo.

[Daniele Castagna, 2015/04/03 04:26:46]

Done.

+      NOTREACHED();
+      return std::vector<uint8>();
+  }
+}
+
+std::vector<uint8> GetFramebufferPixel(
+    const gfx::GpuMemoryBuffer::Format format) {
+  std::vector<uint8> pixel;
+  switch (format) {
+    case gfx::GpuMemoryBuffer::RGBA_8888:
+    case gfx::GpuMemoryBuffer::BGRA_8888:
+      pixel.push_back(255u);
+      pixel.push_back(0u);
+      pixel.push_back(0u);
+      pixel.push_back(255u);
+      return pixel;
+    default:

[reveman, 2015/04/03 02:20:16]

ditto

[Daniele Castagna, 2015/04/03 02:56:14]

Ack.

[Daniele Castagna, 2015/04/03 04:26:46]

Done.

+      NOTREACHED();
+      return std::vector<uint8>();
+  }
+}
+}  // namespace

[reveman, 2015/04/03 02:20:16]

nit: we use a blank line between namespace and code in most chromium code

[Daniele Castagna, 2015/04/03 02:56:14]

Ack. I'll add it at the next iteration.

[Daniele Castagna, 2015/04/03 04:26:46]

Done.

+
 // An end to end test that tests the whole GpuMemoryBuffer lifecycle.
-TEST_F(GpuMemoryBufferTest, Lifecycle) {
-  uint8 pixels[1 * 4] = { 255u, 0u, 0u, 255u };
-
+TEST_P(GpuMemoryBufferTest, Lifecycle) {
   scoped_ptr<gfx::GpuMemoryBuffer> buffer(gl_.CreateGpuMemoryBuffer(
-      gfx::Size(kImageWidth, kImageHeight), gfx::GpuMemoryBuffer::RGBA_8888));
+      gfx::Size(kImageWidth, kImageHeight), GetParam()));
 
   // Map buffer for writing.
   void* data;
   bool rv = buffer->Map(&data);
   DCHECK(rv);
 
   uint8* mapped_buffer = static_cast<uint8*>(data);
   ASSERT_TRUE(mapped_buffer != NULL);
 
   // Assign a value to each pixel.
-  int stride = kImageWidth * kImageBytesPerPixel;
-  for (int x = 0; x < kImageWidth; ++x) {
-    for (int y = 0; y < kImageHeight; ++y) {
-      mapped_buffer[y * stride + x * kImageBytesPerPixel + 0] = pixels[0];
-      mapped_buffer[y * stride + x * kImageBytesPerPixel + 1] = pixels[1];
-      mapped_buffer[y * stride + x * kImageBytesPerPixel + 2] = pixels[2];
-      mapped_buffer[y * stride + x * kImageBytesPerPixel + 3] = pixels[3];
+  uint32 stride = 0;
+  buffer->GetStride(&stride);
+  ASSERT_GE(stride, 0u);
+  std::vector<uint8> pixel = GetTexturePixel(GetParam());
+  for (int y = 0; y < kImageHeight; ++y) {
+    for (uint32 x_offset = 0; x_offset < stride; x_offset += pixel.size()) {

[reveman, 2015/04/03 02:20:16]

I don't think we should be writing past kImageWidth. We should also avoid logic
that prevents a negative stride from working correctly as I think we need to
support that in the future.

[Daniele Castagna, 2015/04/03 02:56:14]

Isn't this similar to what you were suggesting in crrev.com/1051503003 with
memset(mapped_buffer + y * stride, 0xff, ..)?

Do we support negative stride anywhere right now? There is an assert just above,
to make sure that stride is positive. As soon as we add a new format with a
negative stride to this test, it would be pretty clear what to change.

If you're not OK with this, what would you suggest to do instead?

[reveman, 2015/04/03 03:41:02]

memset(mapped_buffer + y * stride, 0xff, ..) works with a negative stride
I'm not aware of any gmb type independent code that doesn't support this right
now. If you know of some let me know as it looks like we'll have to change this
soon.

Let's get it right now instead of having to fix it later.
for (int y = 0; y < kImageHeight; ++y) {
  for (int x = 0; x < kImageWidth; ++x) {
    std::copy(pixel.begin(), pixel.end(),
              mapped_buffer + y * stride + x * pixel.size());
  }
}

+      std::copy(pixel.begin(), pixel.end(),
+                &mapped_buffer[y * stride + x_offset]);
     }
   }
 
   // Unmap the buffer.
   buffer->Unmap();
 
   // Create the image. This should add the image ID to the ImageManager.
   GLuint image_id = glCreateImageCHROMIUM(
       buffer->AsClientBuffer(), kImageWidth, kImageHeight, GL_RGBA);
   EXPECT_NE(0u, image_id);
   EXPECT_TRUE(gl_.decoder()->GetImageManager()->LookupImage(image_id) != NULL);
 
   // Bind the texture and the image.
   glBindTexture(GL_TEXTURE_2D, texture_ids_[0]);
   glBindTexImage2DCHROMIUM(GL_TEXTURE_2D, image_id);
 
   // Copy texture so we can verify result using CheckPixels.
   glCopyTextureCHROMIUM(GL_TEXTURE_2D,
                         texture_ids_[0],
                         texture_ids_[1],
                         GL_RGBA,
                         GL_UNSIGNED_BYTE);
   EXPECT_TRUE(glGetError() == GL_NO_ERROR);
 
   // Check if pixels match the values that were assigned to the mapped buffer.
-  GLTestHelper::CheckPixels(0, 0, kImageWidth, kImageHeight, 0, pixels);
+  GLTestHelper::CheckPixels(0, 0, kImageWidth, kImageHeight, 0,
+                            &GetFramebufferPixel(GetParam()).front());
   EXPECT_TRUE(GL_NO_ERROR == glGetError());
 
   // Release the image.
   glReleaseTexImage2DCHROMIUM(GL_TEXTURE_2D, image_id);
 
   // Destroy the image.
   glDestroyImageCHROMIUM(image_id);
 }
 
+INSTANTIATE_TEST_CASE_P(GpuMemoryBufferTests,
+                        GpuMemoryBufferTest,
+                        ::testing::Values(gfx::GpuMemoryBuffer::RGBA_8888,
+                                          gfx::GpuMemoryBuffer::BGRA_8888));
+
 }  // namespace gles2
 }  // namespace gpu