Add initial GpuMemoryBufferSurfaceTexture implementation.

content/common/android/surface_texture_lookup.cc

Index: content/common/android/surface_texture_lookup.cc
diff --git a/content/common/android/surface_texture_lookup.cc b/content/common/android/surface_texture_lookup.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1314b1f5c415d76929f61d58082cd438d6b1723c
--- /dev/null
+++ b/content/common/android/surface_texture_lookup.cc
@@ -0,0 +1,27 @@
+// 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/common/android/surface_texture_lookup.h"
+
+#include "base/logging.h"
+
+namespace content {
+
+namespace {
+SurfaceTextureLookup* g_instance_ = NULL;

[epennerAtGoogle, 2014/03/12 00:45:06]

It would be nice if someone could own this. Otherwise can you comment why
initialization races are impossible (or change to LazyInstance).

[reveman, 2014/03/12 16:07:06]

It's not the responsibility of this code to prevent races. It's the
responsibility of the user. Same as GpuSurfaceLookup..

Can't be a LazyInstance as this is just an interface. The user is free to use a
LazyInstance to protect against races though.

[epennerAtGoogle, 2014/03/12 19:46:28]

g_instance_ isn't an interface though right? It's a real piece of data. It could
as easily be a lazy-instance no?

For example, if you use this interface as the getter, it seems like you could
have set the pointer already on another thread long ago in the past, but still
get a stale NULL in the getter, even though you had every expectation that it
was already set. This would be a DCHECK.

Excuse me if I'm wrong here, I just like err on the side of extreme caution when
it comes to thread safety issues. There was a long discussion on another thread
about this so if anything I'll learn something here if I'm wrong ;)

[reveman, 2014/03/12 21:19:11]

While g_instance will be a pointer to a real implementation,
SurfaceTextureLookup is not. So there's nothing here I can lazily instantiate.
It's up to the client to create an implementation and instantiate it.
Usage of this API requires the client to set the instance in a thread safe
manner. It's actually preferred to not have this API thread safe as that would
prevent TSan from detecting incorrect usage by a client.

[epennerAtGoogle, 2014/03/12 23:10:14]

I get now why we can't use LazyInstance (we don't know the type to create! ;) ).
Sorry about that. However I still don't see how Initialization is synchronized.

When lazy instance accesses it's pointer (the equivalent of GetIntance()) it
needs an Acquire_load or equivalent, on every accesses to the pointer. It's
considerably more expensive than NoBarrier_load IIUC, and in Singleton it was a
recent bug to use NoBarrier_Load. I don't see how we are achieving that
synchronization if we allow calls to GetInstance on another thread. If other
code is synchronizing it currently, can you point me at what mechanism makes it
synchronized?

From my understanding we have:

SurfaceTextureLookup::InitInstance() // Main thread very early.
SurfaceTextureLookup::GetInstance() // Compositor thread later on.

SurfaceTextureTracker::InitInstance(); // Binder (bad client), otherwise GPU
thread.
SurfaceTextureTracker::GetInstance(); // GPU thread.

In the first case, the 'later on' by itself doesn't seem enough to ensure the
pointer (and the object) are correct on the compositor thread.

In the second case we seem mostly okay because Init + GetInstance _should_
happen on the GPU thread if the client behaves correctly, and the binder thread
access will then go through the lazy instance.

The order of these things could also change, and I don't like the idea that Tsan
is required to verify that we are correct (if we were to change the locations of
Init, or allow changing the global pointer later on, etc.)

[reveman, 2014/03/13 21:36:00]

You're making a valid point. While the previous patch is unlikely to cause
issues in practice I think we should get this right in theory too. There was at
least one issue with the previous patch that I would have overlooked if you
wouldn't have pointed this out so thanks for doing that.

These type of global instances are usually set very early before any threads are
spawned to ensure thread safe usage and I think that's what we should be doing
here too.

Latest patch should be doing that in both cases.
This is safe as InitInstance() happens before we spawn the compositor thread.
In previous patch, this was:
// Binder or GPU thread (bad client), otherwise Browser IO thread.

In latest patch, this happens on Browser UI thread very early (from
PreEarlyInitialization()) which should make it safe.
I don't think we should allow the global pointer to change. There are DCHECKs in
place to prevent this. If someone moves InitInstance around they might mess up
the ordering and we rely on TSan to detect that. That's good enough imo.

+}  // namespace
+
+// static
+SurfaceTextureLookup* SurfaceTextureLookup::GetInstance() {
+  DCHECK(g_instance_);
+  return g_instance_;
+}
+
+// static
+void SurfaceTextureLookup::InitInstance(SurfaceTextureLookup* instance) {
+  DCHECK(!g_instance_);
+  g_instance_ = instance;
+}
+
+}  // namespace content