Android media: VideoFrame should not store so many sync points.

content/browser/renderer_host/image_transport_factory_android.cc

Index: content/browser/renderer_host/image_transport_factory_android.cc
diff --git a/content/browser/renderer_host/image_transport_factory_android.cc b/content/browser/renderer_host/image_transport_factory_android.cc
index cf3ff8b1b7c2e8f6be68cb89a2d001935f55a6b3..64805124924ba1f9fa5dad95beadcbaa8114fd29 100644
--- a/content/browser/renderer_host/image_transport_factory_android.cc
+++ b/content/browser/renderer_host/image_transport_factory_android.cc
@@ -6,6 +6,7 @@
 
 #include "base/lazy_instance.h"
 #include "base/strings/stringprintf.h"
+#include "cc/output/context_provider.h"
 #include "content/browser/gpu/browser_gpu_channel_host_factory.h"
 #include "content/common/gpu/client/gl_helper.h"
 #include "content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h"
@@ -13,6 +14,7 @@
 #include "gpu/command_buffer/client/gles2_implementation.h"
 #include "third_party/WebKit/public/platform/WebGraphicsContext3D.h"
 #include "third_party/khronos/GLES2/gl2.h"
+#include "ui/compositor/compositor.h"
 #include "ui/gfx/android/device_display_info.h"
 
 namespace content {
@@ -31,6 +33,7 @@ class GLContextLostListener
 
 namespace {
 
+bool g_initialized_for_unit_tests = false;
 static ImageTransportFactoryAndroid* g_factory = NULL;
 
 class CmdBufferImageTransportFactory : public ImageTransportFactoryAndroid {
@@ -101,13 +104,50 @@ GLHelper* CmdBufferImageTransportFactory::GetGLHelper() {
   return gl_helper_.get();
 }
 
+class NoImageTransportFactory : public ImageTransportFactoryAndroid {

[dshwang, 2014/06/24 14:04:21]

This class is very similar to NoTransportImageTransportFactory except for
inheriting ImageTransportFactoryAndroid.

[danakj, 2014/07/02 21:43:03]

one class per file, can you move this out?

+ public:
+  explicit NoImageTransportFactory(
+      scoped_ptr<ui::ContextFactory> context_factory)
+      : context_factory_(context_factory.Pass()) {}
+  virtual ~NoImageTransportFactory() {}
+
+  virtual GLHelper* GetGLHelper() OVERRIDE {
+    if (!gl_helper_) {
+      context_provider_ = context_factory_->SharedMainThreadContextProvider();
+      gl_helper_.reset(new GLHelper(context_provider_->ContextGL(),
+                                    context_provider_->ContextSupport()));
+    }
+    return gl_helper_.get();
+  }
+  virtual uint32 GetChannelID() OVERRIDE {
+    NOTREACHED();
+    return 0;
+  }
+
+ private:
+  scoped_ptr<ui::ContextFactory> context_factory_;
+  scoped_refptr<cc::ContextProvider> context_provider_;
+  scoped_ptr<GLHelper> gl_helper_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(NoImageTransportFactory);
+};
+
 }  // anonymous namespace
 
 // static
+void ImageTransportFactoryAndroid::InitializeForUnitTests(

[dshwang, 2014/06/24 14:04:21]

Implement it like ImageTransportFactory::InitializeForUnitTests() in order to
use it in unittests.

+    scoped_ptr<ui::ContextFactory> test_factory) {
+  DCHECK(!g_factory);
+  g_initialized_for_unit_tests = true;
+  g_factory = new NoImageTransportFactory(test_factory.Pass());
+}
+
+// static
 ImageTransportFactoryAndroid* ImageTransportFactoryAndroid::GetInstance() {
-  if (!g_factory)
+  if (!g_factory) {
+    DCHECK(!g_initialized_for_unit_tests);

[danakj, 2014/07/02 21:43:03]

i don't think this DCHECK buys much, since there's no Initialize() for this
class unlike the other ITF. what are you trying to guard against? i think just
remove it and the global bool.

what would be better is if this whole NoImageTransportFactory class was only
compiled into test targets. can we do that instead?

     g_factory = new CmdBufferImageTransportFactory();
-
+  }
   return g_factory;
 }