Add R_8 GPU memory buffers format.

content/common/gpu/client/gpu_memory_buffer_impl.cc

 // Copyright 2013 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 "content/common/gpu/client/gpu_memory_buffer_impl.h"
 
 #include "base/logging.h"
 #include "base/numerics/safe_math.h"
 #include "content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h"
 #include "ui/gl/gl_bindings.h"
@@ skipping 62 matching lines @@
 // static
 GpuMemoryBufferImpl* GpuMemoryBufferImpl::FromClientBuffer(
     ClientBuffer buffer) {
   return reinterpret_cast<GpuMemoryBufferImpl*>(buffer);
 }
 
 // static
 bool GpuMemoryBufferImpl::StrideInBytes(size_t width,
                                         Format format,
                                         size_t* stride_in_bytes) {
-  base::CheckedNumeric<size_t> s = width;
+  base::CheckedNumeric<size_t> checked_stride = width;
   switch (format) {
-    case ATCIA:
-    case DXT5:
-      *stride_in_bytes = width;
-      return true;
-    case ATC:
-    case DXT1:
-    case ETC1:
+    case GpuMemoryBuffer::R_8:
+      checked_stride = (checked_stride + 3) % 4;

[reveman, 2015/04/01 13:14:33]

I assume this is a typo and should be "(width + 3) & ~0x3"

[Daniele Castagna, 2015/04/01 21:58:34]

Ops, I was trying to break some test to see if this code was tested anywhere
with the new enum and I left it in. After adding R_8 to
GpuMemoryBufferFactorySharedMemory::GetSupportedGpuMemoryBufferConfigurations
the GpuMemoryBufferFactoryTest correctly started to fail.

+      break;
+    case GpuMemoryBuffer::ATCIA:
+    case GpuMemoryBuffer::DXT5:  // 'checked_stride' is the same as 'width'.

[reveman, 2015/04/01 13:14:33]

nit: no need for checked_stride in this case

[Daniele Castagna, 2015/04/01 21:58:34]

Done.

+      break;
+    case GpuMemoryBuffer::ATC:
+    case GpuMemoryBuffer::DXT1:
+    case GpuMemoryBuffer::ETC1:
       DCHECK_EQ(width % 2, 0U);

[reveman, 2015/04/01 13:14:33]

nit: ..., 0u);

[Daniele Castagna, 2015/04/01 21:58:34]

Done.

-      s /= 2;
-      if (!s.IsValid())
-        return false;
-
-      *stride_in_bytes = s.ValueOrDie();
-      return true;
-    case RGBA_8888:
-    case RGBX_8888:
-    case BGRA_8888:
-      s *= 4;
-      if (!s.IsValid())
-        return false;
-
-      *stride_in_bytes = s.ValueOrDie();
-      return true;
+      checked_stride /= 2;

[reveman, 2015/04/01 13:14:33]

no need for checked_stride in this case.

[Daniele Castagna, 2015/04/01 21:58:34]

Done.

+      break;
+    case GpuMemoryBuffer::RGBA_8888:
+    case GpuMemoryBuffer::RGBX_8888:
+    case GpuMemoryBuffer::BGRA_8888:
+      checked_stride *= 4;
+      break;
   }
-
-  NOTREACHED();

[reveman, 2015/04/01 13:14:33]

I like to keep this condition. ie detect if format becomes an invalid value at
runtime instead of silently returning |width| as stride. I would prefer if the
base::CheckedNumeric was moved into only the cases where needed as I think that
makes it more clear when those checks are needed.

[Daniele Castagna, 2015/04/01 21:58:34]

Done as you suggested.

For the sake of arguing: I think leaving CheckedNumeric everywhere clearly
conveys the idea that 'width' is not to be trusted and has to be validated.

Good point about NOTREACHED(). Isn't 'format' potentially coming from the
renderer (from ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer) as for 'width'?
Should we validate it and return false instead of asserting?

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

Ok, fair enough.
The IPC system will automatically verify that format is a valid enum value as it
arrives. This NOTREACHED is only in case we mess it up in the GPU process after
the IPC arrives.

-  return false;
+  if (!checked_stride.IsValid())
+    return false;
+  *stride_in_bytes = checked_stride.ValueOrDie();
+  return true;
 }
 
 // static
 size_t GpuMemoryBufferImpl::NumberOfPlanesForGpuMemoryBufferFormat(
     gfx::GpuMemoryBuffer::Format format) {
   switch (format) {
     case gfx::GpuMemoryBuffer::Format::ATC:
     case gfx::GpuMemoryBuffer::Format::ATCIA:
     case gfx::GpuMemoryBuffer::Format::DXT1:
     case gfx::GpuMemoryBuffer::Format::DXT5:
     case gfx::GpuMemoryBuffer::Format::ETC1:
+    case gfx::GpuMemoryBuffer::Format::R_8:
     case gfx::GpuMemoryBuffer::Format::RGBA_8888:
     case gfx::GpuMemoryBuffer::Format::RGBX_8888:
     case gfx::GpuMemoryBuffer::Format::BGRA_8888:
       return 1;
     default:
       NOTREACHED();
       return 0;
   }
 }
 
 gfx::GpuMemoryBuffer::Format GpuMemoryBufferImpl::GetFormat() const {
   return format_;
 }
 
 bool GpuMemoryBufferImpl::IsMapped() const {
   return mapped_;
 }
 
 ClientBuffer GpuMemoryBufferImpl::AsClientBuffer() {
   return reinterpret_cast<ClientBuffer>(this);
 }
 
 }  // namespace content