Use GLHelper support to validate format before processing texture.

content/browser/compositor/delegated_frame_host.cc

 // Copyright 2014 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/browser/compositor/delegated_frame_host.h"
 
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/compositor_frame_ack.h"
@@ skipping 593 matching lines @@
     if (gl_helper)
       sync_point = gl_helper->InsertSyncPoint();
   }
   bool lost_resource = sync_point == 0;
   release_callback->Run(sync_point, lost_resource);
 
   callback.Run(*bitmap,
                result ? content::READBACK_SUCCESS : content::READBACK_FAILED);
 }
 
+//// static

[danakj, 2015/01/06 16:16:19]

just one // is enough

[sivag, 2015/01/07 09:33:32]

Done.

+bool DelegatedFrameHost::IsReadbackConfigSupported(SkColorType color_type) {
+  ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
+  GLHelper* gl_helper = factory->GetGLHelper();
+  if (!gl_helper)
+    return false;
+  if (!gl_helper->IsReadbackConfigSupported(color_type))
+    return false;
+  return true;
+}
+
 // static
 void DelegatedFrameHost::PrepareTextureCopyOutputResult(
     const gfx::Size& dst_size_in_pixel,
     const SkColorType color_type,
     ReadbackRequestCallback& callback,
     scoped_ptr<cc::CopyOutputResult> result) {
   DCHECK(result->HasTexture());
+
+  if (!IsReadbackConfigSupported(color_type))

[danakj, 2015/01/06 16:16:19]

This makes us GetGLHelper() twice, IWBN to only call that function once.

Also previously if GetGLHelper() gave null, we would READBACK_FAILED. Now we
READBACK_FORMAT_NOT_SUPPORTED, which maybe is not right?

[sivag, 2015/01/07 09:33:32]

Done.

+    callback.Run(SkBitmap(), content::READBACK_FORMAT_NOT_SUPPORTED);
+
   base::ScopedClosureRunner scoped_callback_runner(
       base::Bind(callback, SkBitmap(), content::READBACK_FAILED));
 
-  // TODO(sikugu): We should be able to validate the format here using
-  // GLHelper::IsReadbackConfigSupported before we processs the result.
-  // See crbug.com/415682.
   scoped_ptr<SkBitmap> bitmap(new SkBitmap);
   if (!bitmap->tryAllocPixels(SkImageInfo::Make(dst_size_in_pixel.width(),
                                                 dst_size_in_pixel.height(),
                                                 color_type,
                                                 kOpaque_SkAlphaType)))
     return;
 
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   GLHelper* gl_helper = factory->GetGLHelper();
   if (!gl_helper)
@@ skipping 396 matching lines @@
     cc::SurfaceManager* manager = factory->GetSurfaceManager();
     new_layer->SetShowSurface(
         surface_id_, base::Bind(&SatisfyCallback, base::Unretained(manager)),
         base::Bind(&RequireCallback, base::Unretained(manager)),
         current_surface_size_, current_scale_factor_,
         current_frame_size_in_dip_);
   }
 }
 
 }  // namespace content