Use GLHelper for glReadPixels and format detection

content/renderer/media/renderer_gpu_video_accelerator_factories.cc

Index: content/renderer/media/renderer_gpu_video_accelerator_factories.cc
diff --git a/content/renderer/media/renderer_gpu_video_accelerator_factories.cc b/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
index 94e0d78217fa621974b945fd79bbaee2756e8fd3..3cb5bbd4f6d1ed34c21347f9cb1c9e8c15716f4e 100644
--- a/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
+++ b/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
@@ -69,6 +69,18 @@ RendererGpuVideoAcceleratorFactories::GetContext3d() {
   return context_provider_->WebContext3D();
 }
 
+GLHelper* RendererGpuVideoAcceleratorFactories::GetGLHelper() {
+  if (!GetContext3d())
+    return NULL;
+
+  if (gl_helper_.get() == NULL) {
+    gl_helper_.reset(new GLHelper(GetContext3d()->GetImplementation(),
+                                  GetContext3d()->GetContextSupport()));
+  }
+
+  return gl_helper_.get();
+}
+
 scoped_ptr<media::VideoDecodeAccelerator>
 RendererGpuVideoAcceleratorFactories::CreateVideoDecodeAccelerator() {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -178,59 +190,20 @@ void RendererGpuVideoAcceleratorFactories::ReadPixels(
     const SkBitmap& pixels) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  WebGraphicsContext3DCommandBufferImpl* context = GetContext3d();
-  if (!context)
+  GLHelper* gl_helper = GetGLHelper();
+  if (!gl_helper)
     return;
 
-  gpu::gles2::GLES2Implementation* gles2 = context->GetImplementation();
+  unsigned char* pixel_data =
+      static_cast<unsigned char*>(pixels.pixelRef()->pixels());
+
+  if (gl_helper->IsReadbackConfigSupported(pixels.colorType())) {
+    gl_helper->ReadbackTextureSync(
+        texture_id, visible_rect, pixel_data, pixels.colorType());
+  } else {
+    gl_helper->ReadbackTextureSync(
+        texture_id, visible_rect, pixel_data, kRGBA_8888_SkColorType);
 
-  GLuint tmp_texture;
-  gles2->GenTextures(1, &tmp_texture);
-  gles2->BindTexture(GL_TEXTURE_2D, tmp_texture);
-  gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
-  gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
-  gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
-  gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
-  context->copyTextureCHROMIUM(
-      GL_TEXTURE_2D, texture_id, tmp_texture, 0, GL_RGBA, GL_UNSIGNED_BYTE);
-
-  GLuint fb;
-  gles2->GenFramebuffers(1, &fb);
-  gles2->BindFramebuffer(GL_FRAMEBUFFER, fb);
-  gles2->FramebufferTexture2D(
-      GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, tmp_texture, 0);
-  gles2->PixelStorei(GL_PACK_ALIGNMENT, 4);
-
-#if SK_B32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_R32_SHIFT == 16 && \
-    SK_A32_SHIFT == 24
-  GLenum skia_format = GL_BGRA_EXT;
-  GLenum read_format = GL_BGRA_EXT;
-  GLint supported_format = 0;
-  GLint supported_type = 0;
-  gles2->GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT, &supported_format);
-  gles2->GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE, &supported_type);
-  if (supported_format != GL_BGRA_EXT || supported_type != GL_UNSIGNED_BYTE) {
-    read_format = GL_RGBA;
-  }
-#elif SK_R32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_B32_SHIFT == 16 && \
-    SK_A32_SHIFT == 24
-  GLenum skia_format = GL_RGBA;
-  GLenum read_format = GL_RGBA;
-#else
-#error Unexpected Skia ARGB_8888 layout!
-#endif
-  gles2->ReadPixels(visible_rect.x(),
-                    visible_rect.y(),
-                    visible_rect.width(),
-                    visible_rect.height(),
-                    read_format,
-                    GL_UNSIGNED_BYTE,
-                    pixels.pixelRef()->pixels());
-  gles2->DeleteFramebuffers(1, &fb);
-  gles2->DeleteTextures(1, &tmp_texture);
-
-  if (skia_format != read_format) {
-    DCHECK(read_format == GL_RGBA);
     int pixel_count = visible_rect.width() * visible_rect.height();
     uint32_t* pixels_ptr = static_cast<uint32_t*>(pixels.pixelRef()->pixels());
     for (int i = 0; i < pixel_count; ++i) {
@@ -238,14 +211,9 @@ void RendererGpuVideoAcceleratorFactories::ReadPixels(
         uint32_t g = (pixels_ptr[i] >> 8) & 0xFF;
         uint32_t b = (pixels_ptr[i] >> 16) & 0xFF;
         uint32_t a = (pixels_ptr[i] >> 24) & 0xFF;
-        pixels_ptr[i] = (r << SK_R32_SHIFT) |
-                        (g << SK_G32_SHIFT) |
-                        (b << SK_B32_SHIFT) |
-                        (a << SK_A32_SHIFT);
+        pixels_ptr[i] = (b << 0) | (g << 8) | (r << 16) | (a << 24);

[piman, 2014/08/08 21:02:06]

nit: why not keep the SK_*_SHIFT here? That way it works for any layout.

[robert.bradford, 2014/08/08 21:32:31]

Those macros, AFAICT, represent the native representation which may not map to
the format of the bitmap. We know we need to just swap B & R so I think this is
safer.

[piman, 2014/08/08 21:57:12]

How do we know that?

We can check that the format is kN32_SkColorType and fail if it isn't. Then
whatever platform, it will be correct to unpack the colors from RGBA (the format
that GL gives us) and repack them with the SK_*_SHIFTs into what Skia expects.

     }
   }
-
-  DCHECK_EQ(gles2->GetError(), static_cast<GLenum>(GL_NO_ERROR));
 }
 
 base::SharedMemory* RendererGpuVideoAcceleratorFactories::CreateSharedMemory(