Add R_8 GPU memory buffers format.

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..41cef24e9568aa10d5aa62f1fe181e9c9c879398 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());
@@ -71,11 +71,11 @@ class GpuMemoryBufferTest : public testing::Test {
 };
 
 // An end to end test that tests the whole GpuMemoryBuffer lifecycle.
-TEST_F(GpuMemoryBufferTest, Lifecycle) {
+TEST_P(GpuMemoryBufferTest, Lifecycle) {
   uint8 pixels[1 * 4] = { 255u, 0u, 0u, 255u };
 
   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,22 +86,28 @@ TEST_F(GpuMemoryBufferTest, Lifecycle) {
   ASSERT_TRUE(mapped_buffer != NULL);
 
   // Assign a value to each pixel.
-  int stride = kImageWidth * kImageBytesPerPixel;
+  int imageBytesPerPixel = GetParam() == gfx::GpuMemoryBuffer::R_8 ? 1 : 4;

[reveman, 2015/04/01 23:38:55]

Can you refactor this test a bit so this conditional is not needed? Maybe just
memset(mapped_buffer + y * stride, 0xff, ..) instead

[Daniele Castagna, 2015/04/03 01:08:15]

Refactored, refer to crrev.com/1055143003.

+  uint32 stride = 0;
+  buffer->GetStride(&stride);
   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];
+      for (int p = 0; p < imageBytesPerPixel; ++p) {
+        mapped_buffer[y * stride + x * imageBytesPerPixel + p] = pixels[p];
+      }
     }
   }
 
   // Unmap the buffer.
   buffer->Unmap();
 
+  GLenum internalformat = GL_RGBA;
+  if (GetParam() == gfx::GpuMemoryBuffer::R_8) {

[reveman, 2015/04/01 23:38:55]

I prefer a switch statement where you explicitly handle all formats. and I guess
you can NOTREACHED if the format is invalid even though this is just a test...

[Daniele Castagna, 2015/04/03 01:08:15]

This will change after crrev.com/1055143003

+    internalformat =
+        gl_.decoder()->GetCapabilities().texture_rg ? GL_R8 : GL_LUMINANCE;
+  }
   // Create the image. This should add the image ID to the ImageManager.
-  GLuint image_id = glCreateImageCHROMIUM(
-      buffer->AsClientBuffer(), kImageWidth, kImageHeight, GL_RGBA);
+  GLuint image_id = glCreateImageCHROMIUM(buffer->AsClientBuffer(), kImageWidth,
+                                          kImageHeight, internalformat);
   EXPECT_NE(0u, image_id);
   EXPECT_TRUE(gl_.decoder()->GetImageManager()->LookupImage(image_id) != NULL);
 
@@ -128,5 +134,15 @@ TEST_F(GpuMemoryBufferTest, Lifecycle) {
   glDestroyImageCHROMIUM(image_id);
 }
 
+static const gfx::GpuMemoryBuffer::Format kMemoryBufferFormats[] = {

[reveman, 2015/04/01 23:38:55]

It would be nice to test all formats here and not just R_8 and RGBA_8888. How
about landing a patch that refactor this test to try all formats prior to this
CL?

[Daniele Castagna, 2015/04/03 01:08:15]

crrev.com/1055143003

+    gfx::GpuMemoryBuffer::RGBA_8888,
+    gfx::GpuMemoryBuffer::R_8};

[reveman, 2015/04/01 23:38:55]

nit: use the same order as where the enum is defined

[Daniele Castagna, 2015/04/03 01:08:15]

Refer to crrev.com/1055143003

+INSTANTIATE_TEST_CASE_P(
+    GpuMemoryBufferTests,
+    GpuMemoryBufferTest,
+    ::testing::ValuesIn(std::vector<gfx::GpuMemoryBuffer::Format>(
+        kMemoryBufferFormats,
+        kMemoryBufferFormats + arraysize(kMemoryBufferFormats))));

[reveman, 2015/04/01 23:38:55]

How about:

INSTANTIATE_TEST_CASE_P(
    GpuMemoryBufferTests,
    GpuMemoryBufferTest,
    ::testing::Values(gfx::GpuMemoryBuffer::R_8,
                      gfx::GpuMemoryBuffer::RGBA_8888))

[Daniele Castagna, 2015/04/03 01:08:15]

Much better.
Used it in crrev.com/1055143003

+
 }  // namespace gles2
 }  // namespace gpu