Add R_8 GPU memory buffers format.

ui/gl/gl_image_memory.cc

Index: ui/gl/gl_image_memory.cc
diff --git a/ui/gl/gl_image_memory.cc b/ui/gl/gl_image_memory.cc
index ca106b7cb2941bdb202e00c0632b40a1cb128a5e..914e0c7c1524a8aa20d8e2844fbb6d05bfbd9d0a 100644
--- a/ui/gl/gl_image_memory.cc
+++ b/ui/gl/gl_image_memory.cc
@@ -17,16 +17,7 @@
 namespace gfx {
 namespace {
 
-bool ValidInternalFormat(unsigned internalformat) {
-  switch (internalformat) {
-    case GL_RGBA:
-      return true;
-    default:
-      return false;
-  }
-}
-
-bool ValidFormat(gfx::GpuMemoryBuffer::Format format) {
+bool ValidFormat(unsigned internalformat, gfx::GpuMemoryBuffer::Format format) {

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

why not two levels of switch statements like GLImageLinuxDMABuffer?

switch (internalformat) {
  case GL_RGBA:
    ...
    switch (format) {
      case ...:

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

Done.

   switch (format) {
     case gfx::GpuMemoryBuffer::ATC:
     case gfx::GpuMemoryBuffer::ATCIA:
@@ -35,11 +26,22 @@ bool ValidFormat(gfx::GpuMemoryBuffer::Format format) {
     case gfx::GpuMemoryBuffer::ETC1:
     case gfx::GpuMemoryBuffer::RGBA_8888:
     case gfx::GpuMemoryBuffer::BGRA_8888:
+      if (internalformat != GL_RGBA) {
+        LOG(ERROR) << "Invalid internal format " << internalformat

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

I prefer if you keep logging in GLImageMemory::Initialize to be consistent with
other GLImage implementations such as GLImageLinuxDMABuffer.

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

Done.

In this way the error message can't be as specific as in the previous patch
though.

+                   << "for the specified format: " << format;
+        return false;
+      }
+      return true;
+    case gfx::GpuMemoryBuffer::R_8:
+      if (internalformat != GL_LUMINANCE && internalformat != GL_R8) {
+        LOG(ERROR) << "Invalid internal format " << internalformat
+                   << "for the specified format: " << format;
+      }
       return true;
     case gfx::GpuMemoryBuffer::RGBX_8888:
+      LOG(ERROR) << "Invalid format: " << format;
       return false;
   }
-
   NOTREACHED();
   return false;
 }
@@ -52,6 +54,7 @@ bool IsCompressedFormat(gfx::GpuMemoryBuffer::Format format) {
     case gfx::GpuMemoryBuffer::DXT5:
     case gfx::GpuMemoryBuffer::ETC1:
       return true;
+    case gfx::GpuMemoryBuffer::R_8:
     case gfx::GpuMemoryBuffer::RGBA_8888:
     case gfx::GpuMemoryBuffer::BGRA_8888:
     case gfx::GpuMemoryBuffer::RGBX_8888:
@@ -78,23 +81,39 @@ GLenum TextureFormat(gfx::GpuMemoryBuffer::Format format) {
       return GL_RGBA;
     case gfx::GpuMemoryBuffer::BGRA_8888:
       return GL_BGRA_EXT;
+    case gfx::GpuMemoryBuffer::R_8:
     case gfx::GpuMemoryBuffer::RGBX_8888:
       NOTREACHED();
       return 0;
   }
-

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

heh, you don't like my use of blank lines? fine, I'll add them back next time I
touch this file :)

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

As discussed on IM, I was trying to minimize vertical space as suggested in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Vertical_Whit....

Putting it back.

+  NOTREACHED();
+  return 0;
+}
+GLenum InternalTextureFormat(unsigned internalformat,
+                             gfx::GpuMemoryBuffer::Format format) {
+  switch (internalformat) {
+    case GL_RGB:
+    case GL_RGBA:
+      return TextureFormat(format);
+    case GL_LUMINANCE:
+    case GL_R8:
+      return internalformat;
+  }
   NOTREACHED();
   return 0;
 }
 
-GLenum DataFormat(gfx::GpuMemoryBuffer::Format format) {
-  return TextureFormat(format);
+GLenum DataFormat(unsigned internalformat,

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

hm, I think the need for internalformat here is a good indication that we should
add a gfx::GpuMemoryBuffer::L_8 format. I think that would make the code a bit
cleaner. Wdyt?

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

LUMINANCE and LUMINANCE_ALPHA have been deprecated in OpengGL 3.0:
https://www.opengl.org/wiki/Image_Format#Legacy_Image_Formats


I think it'd be better to leave this here rather than exposing a deprecated
format in GpuMemoryBuffers.

+                  gfx::GpuMemoryBuffer::Format format) {
+  GLenum data_format = InternalTextureFormat(internalformat, format);

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

If we add gfx::GpuMemoryBuffer::L_8 then we can use a switch statement here and
TextureFormat instead of InternalTextureFormat.

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

See previous comment.

+  return data_format == GL_R8 ? GL_RED : data_format;
 }
 
 GLenum DataType(gfx::GpuMemoryBuffer::Format format) {
   switch (format) {
     case gfx::GpuMemoryBuffer::RGBA_8888:
     case gfx::GpuMemoryBuffer::BGRA_8888:
+    case gfx::GpuMemoryBuffer::R_8:
       return GL_UNSIGNED_BYTE;
     case gfx::GpuMemoryBuffer::ATC:
     case gfx::GpuMemoryBuffer::ATCIA:
@@ -150,48 +169,40 @@ GLImageMemory::~GLImageMemory() {
 bool GLImageMemory::StrideInBytes(size_t width,
                                   gfx::GpuMemoryBuffer::Format format,
                                   size_t* stride_in_bytes) {
-  base::CheckedNumeric<size_t> s = width;
+  base::CheckedNumeric<size_t> checked_stride = width;
   switch (format) {
-    case gfx::GpuMemoryBuffer::ATCIA:
-    case gfx::GpuMemoryBuffer::DXT5:
+    case GpuMemoryBuffer::R_8:
+      checked_stride += 3;
+      if (!checked_stride.IsValid())
+        return false;
+      *stride_in_bytes = checked_stride.ValueOrDie() & ~0x3;
+      return true;
+    case GpuMemoryBuffer::ATCIA:
+    case GpuMemoryBuffer::DXT5:
       *stride_in_bytes = width;
       return true;
-    case gfx::GpuMemoryBuffer::ATC:
-    case gfx::GpuMemoryBuffer::DXT1:
-    case gfx::GpuMemoryBuffer::ETC1:
-      DCHECK_EQ(width % 2, 0U);
-      s /= 2;
-      if (!s.IsValid())
-        return false;
-
-      *stride_in_bytes = s.ValueOrDie();
+    case GpuMemoryBuffer::ATC:
+    case GpuMemoryBuffer::DXT1:
+    case GpuMemoryBuffer::ETC1:
+      DCHECK_EQ(width % 2, 0u);
+      *stride_in_bytes = width / 2;
       return true;
-    case gfx::GpuMemoryBuffer::RGBA_8888:
-    case gfx::GpuMemoryBuffer::BGRA_8888:
-      s *= 4;
-      if (!s.IsValid())
+    case GpuMemoryBuffer::RGBA_8888:
+    case GpuMemoryBuffer::RGBX_8888:
+    case GpuMemoryBuffer::BGRA_8888:
+      checked_stride *= 4;
+      if (!checked_stride.IsValid())
         return false;
-
-      *stride_in_bytes = s.ValueOrDie();
+      *stride_in_bytes = checked_stride.ValueOrDie();
       return true;
-    case gfx::GpuMemoryBuffer::RGBX_8888:
-      NOTREACHED();
-      return false;
   }
-
   NOTREACHED();
   return false;
 }
 
 bool GLImageMemory::Initialize(const unsigned char* memory,
                                gfx::GpuMemoryBuffer::Format format) {
-  if (!ValidInternalFormat(internalformat_)) {
-    LOG(ERROR) << "Invalid internalformat: " << internalformat_;
-    return false;
-  }
-
-  if (!ValidFormat(format)) {
-    LOG(ERROR) << "Invalid format: " << format;
+  if (!ValidFormat(internalformat_, format)) {
     return false;
   }
 
@@ -256,14 +267,15 @@ bool GLImageMemory::CopyTexImage(unsigned target) {
                               0,  // x-offset
                               0,  // y-offset
                               size_.width(), size_.height(),
-                              DataFormat(format_), SizeInBytes(size_, format_),
-                              memory_);
+                              DataFormat(internalformat_, format_),
+                              SizeInBytes(size_, format_), memory_);
   } else {
     glTexSubImage2D(target, 0,  // level
                     0,          // x
                     0,          // y
-                    size_.width(), size_.height(), DataFormat(format_),
-                    DataType(format_), memory_);
+                    size_.width(), size_.height(),
+                    DataFormat(internalformat_, format_), DataType(format_),
+                    memory_);
   }
 
   return true;
@@ -314,21 +326,20 @@ void GLImageMemory::DoBindTexImage(unsigned target) {
         glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
         glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
         if (IsCompressedFormat(format_)) {
-          glCompressedTexImage2D(GL_TEXTURE_2D,
-                                 0,  // mip level
-                                 TextureFormat(format_), size_.width(),
-                                 size_.height(),
-                                 0,  // border
-                                 SizeInBytes(size_, format_), memory_);
+          glCompressedTexImage2D(
+              GL_TEXTURE_2D,
+              0,  // mip level
+              InternalTextureFormat(internalformat_, format_), size_.width(),
+              size_.height(),
+              0,  // border
+              SizeInBytes(size_, format_), memory_);
         } else {
           glTexImage2D(GL_TEXTURE_2D,
                        0,  // mip level
-                       TextureFormat(format_),
-                       size_.width(),
-                       size_.height(),
+                       InternalTextureFormat(internalformat_, format_),
+                       size_.width(), size_.height(),
                        0,  // border
-                       DataFormat(format_),
-                       DataType(format_),
+                       DataFormat(internalformat_, format_), DataType(format_),
                        memory_);
         }
       }
@@ -353,18 +364,15 @@ void GLImageMemory::DoBindTexImage(unsigned target) {
                                   0,  // x-offset
                                   0,  // y-offset
                                   size_.width(), size_.height(),
-                                  DataFormat(format_),
-                                  SizeInBytes(size_, format_),
-                                  memory_);
+                                  DataFormat(internalformat_, format_),
+                                  SizeInBytes(size_, format_), memory_);
       } else {
         glTexSubImage2D(GL_TEXTURE_2D,
                         0,  // mip level
                         0,  // x-offset
                         0,  // y-offset
-                        size_.width(),
-                        size_.height(),
-                        DataFormat(format_),
-                        DataType(format_),
+                        size_.width(), size_.height(),
+                        DataFormat(internalformat_, format_), DataType(format_),
                         memory_);
       }
     }
@@ -379,19 +387,17 @@ void GLImageMemory::DoBindTexImage(unsigned target) {
   if (IsCompressedFormat(format_)) {
     glCompressedTexImage2D(target,
                            0,  // mip level
-                           TextureFormat(format_), size_.width(),
-                           size_.height(),
+                           InternalTextureFormat(internalformat_, format_),
+                           size_.width(), size_.height(),
                            0,  // border
                            SizeInBytes(size_, format_), memory_);
   } else {
     glTexImage2D(target,
                  0,  // mip level
-                 TextureFormat(format_),
-                 size_.width(),
+                 InternalTextureFormat(internalformat_, format_), size_.width(),
                  size_.height(),
                  0,  // border
-                 DataFormat(format_),
-                 DataType(format_),
+                 DataFormat(internalformat_, format_), DataType(format_),
                  memory_);
   }
 }