Test gfx::GpuMemoryBuffer::BGRA_8888 GpuMemoryBuffer lifecycle.

gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc

Index: gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc
diff --git a/gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc b/gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc
index 48f8ba878ee5aabc2a84a32815bbf143027d5b3d..01f92877414e9aa2170ae07e7d7c00f511140ec1 100644
--- a/gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc
+++ b/gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc
@@ -33,9 +33,9 @@ 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());
@@ -70,12 +70,50 @@ class GpuMemoryBufferTest : public testing::Test {
   GLuint framebuffer_id_;
 };
 
-// An end to end test that tests the whole GpuMemoryBuffer lifecycle.
-TEST_F(GpuMemoryBufferTest, Lifecycle) {
-  uint8 pixels[1 * 4] = { 255u, 0u, 0u, 255u };
+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_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;
@@ -86,13 +124,14 @@ TEST_F(GpuMemoryBufferTest, Lifecycle) {
   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]);
     }
   }
 
@@ -118,7 +157,8 @@ TEST_F(GpuMemoryBufferTest, Lifecycle) {
   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.
@@ -128,5 +168,10 @@ TEST_F(GpuMemoryBufferTest, Lifecycle) {
   glDestroyImageCHROMIUM(image_id);
 }
 
+INSTANTIATE_TEST_CASE_P(GpuMemoryBufferTests,
+                        GpuMemoryBufferTest,
+                        ::testing::Values(gfx::GpuMemoryBuffer::RGBA_8888,
+                                          gfx::GpuMemoryBuffer::BGRA_8888));
+
 }  // namespace gles2
 }  // namespace gpu