DialogSurfaceManager implementation.

media/base/android/dialog_surface_callback.cc

Index: media/base/android/dialog_surface_callback.cc
diff --git a/media/base/android/dialog_surface_callback.cc b/media/base/android/dialog_surface_callback.cc
new file mode 100644
index 0000000000000000000000000000000000000000..718e9a54fb42ab6a1b9967ab77e3b6a4076b2e26
--- /dev/null
+++ b/media/base/android/dialog_surface_callback.cc
@@ -0,0 +1,204 @@
+// Copyright 2016 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 "media/base/android/dialog_surface_callback.h"
+
+#include <algorithm>
+#include <limits>
+#include <map>
+#include <memory>
+#include <utility>
+
+#include "base/android/build_info.h"
+#include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
+#include "base/bind.h"
+#include "base/lazy_instance.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "jni/DialogSurfaceCallback_jni.h"
+
+using base::android::AttachCurrentThread;
+
+namespace media {
+
+static long next_id_ = 1;
+
+// Map of ids to DialogSurfaceCallbacks.  This is called from multiple threads.
+class DialogCallbackMap {
+ public:
+  // Associate |thiz| with |id| from any thread.
+  void Insert(long id, DialogSurfaceCallback* thiz) {
+    base::AutoLock _l(lock_);
+    map_[id] = thiz;

[boliu, 2017/01/04 01:48:44]

check nothing is being overridden here?

[liberato (no reviews please), 2017/01/11 22:17:56]

Done.

+  }
+
+  // Forget about |id| from any thread.  It's okay if it's not present.
+  // if it's not present.
+  void Erase(long id) {
+    base::AutoLock _l(lock_);
+    map_.erase(id);
+  }
+
+  // Return the associated callback for |id|, or null, from any thread.
+  DialogSurfaceCallback* Lookup(long id) {
+    base::AutoLock _l(lock_);
+    return LookupLocked(id);
+  }
+
+  // Map |id| to the correct task runner for callbacks, or null.  Thread safe.
+  scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(long id) {
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner;
+    base::AutoLock _l(lock_);
+    DialogSurfaceCallback* thiz = LookupLocked(id);
+    // While under lock, |thiz| cannot be removed from the map.  Get a strong
+    // ref to it.
+    if (thiz)
+      task_runner = thiz->GetTaskRunner();

[boliu, 2017/01/04 01:48:44]

is it overkill to support callbacks from multiple arbitrary threads?

maybe should make task_runner_ const to avoid threading issues

[liberato (no reviews please), 2017/01/11 22:17:56]

there's a replacement for AVDA that's in the works which will likely use a
private thread to do most work, including process callbacks.  i didn't want this
to memorize the task runner of the first thing that uses it.

whether the private thread is "private to all decoders" or "private to one
instance of a decoder" is up in the air.  either, though, "not the gpu main
thread" seems flaky if we ever want to use this for something other than video
decoding.

[boliu, 2017/01/12 20:24:18]

But this CL does exactly that right now..
One thread per encoder? seems excessive to me..
Can we add the complexity when it's needed, and not before?

[liberato (no reviews please), 2017/02/03 21:28:32]

Done.

+
+    return task_runner;
+  }
+
+ private:
+  // Lookup |id|.  The caller must hold |lock_|.
+  DialogSurfaceCallback* LookupLocked(long id) {
+    DialogSurfaceCallback* thiz = nullptr;

[boliu, 2017/01/04 01:48:44]

lock_.AssertAcquired()

[liberato (no reviews please), 2017/01/11 22:17:56]

Done.

+    Map::const_iterator it = map_.find(id);
+    if (it != map_.end())
+      thiz = it->second;
+
+    return thiz;
+  }
+
+  // Alternatively, we could put in a callback that is bound to the correct
+  // thread, and use a weak ref.  However, since we still need a default
+  // action when the weak ref is cleared to unblock the callback, it ends up
+  // not being any simpler.
+  using Map = std::map<long, DialogSurfaceCallback*>;
+  Map map_;
+
+  base::Lock lock_;
+};
+
+static base::LazyInstance<DialogCallbackMap>::Leaky g_map =
+    LAZY_INSTANCE_INITIALIZER;
+
+DialogSurfaceCallback::DialogSurfaceCallback(
+    const DialogSurface::Callback& callback)
+    : task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      id_(next_id_++),
+      callback_(callback) {
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
+  j_surface_callback_.Reset(Java_DialogSurfaceCallback_create(env, id_));
+
+  g_map.Pointer()->Insert(id_, this);
+}
+
+DialogSurfaceCallback::~DialogSurfaceCallback() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  g_map.Pointer()->Erase(id_);
+}
+
+void DialogSurfaceCallback::SetAndroidSurfaceCB(
+    const AndroidSurfaceCB& android_surface_cb) {
+  android_surface_cb_ = android_surface_cb;
+}
+
+bool DialogSurfaceCallback::RegisterDialogSurfaceCallback(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+DialogSurfaceCallback::GetTaskRunner() const {
+  // May be called on any thread.
+  return task_runner_;
+}
+
+bool DialogSurfaceCallback::CalledOnValidThread() const {
+  return thread_checker_.CalledOnValidThread();
+}
+
+void DialogSurfaceCallback::OnCallbackProperThread(

[boliu, 2017/01/04 01:48:44]

hmm, if we could have passed a WeakPtr opaquely through java and back, how much
of this stuff becomes unnecessary?

[liberato (no reviews please), 2017/01/11 22:17:56]

good question.  never thought of it.

i think that if we still need to remember a task runner, then not too much is
saved.  if we could use a single task runner for all, then yeah, pretty much all
of it can be taken out.

but, see above for why i think that we can't get around remembering a task
runner.

[boliu, 2017/01/12 20:24:18]

What if DialogSurfaceCallback is refcoutnedthreadsafe, and java takes a single
reference? Java has an explicit onDestroy to drop the reference?

[liberato (no reviews please), 2017/02/03 21:28:32]

it can work.

there are lots of thread hopping issues, though.  random thread calls into java
to start the callback => jni posts to native thread, returns or waits.  native
thread processes the callback which might onDestroy on a different thread (gpu
main thread).

we need to make sure that onDestroy doesn't happen while the native post is
still pending.  that can be done by duplicating the ref in the jni call, and
passing it along with the post.

we also need to make sure that onDestroy is synchronized with the random threads
that come into the callback on java.  that would be easy, but we have to make
sure that one can still cause onDestroy from a native callback on a native
thread, which might happen while the (sync) callback is still waiting on a
random jni thread.  could deadlock, i'd suspect, unless we're careful.  probably
onDestroy has to post in java, to prevent a deadlock.

i'll give it a try, and see if it ends up being more or less code.

(later)

ownership gets to be very messy.  i had the callback native object hand a newly
allocated ref to java.  somebody (DialogSurface) has to call a method on the
callback to destroy the extra ref, else it'll live on forever.  i don't see
anything on the java side that we could use to do it automatically.

plus, there are the thread hops mentioned above.

unless you feel strongly, i'd rather keep it this way.  ownership is clearer and
it's hard to leak.

[liberato (no reviews please), 2017/02/07 00:20:20]

ah, the problem with my attempt with WeakRefs was that i'm silly.  :)  i tried
this again today, and it seems to fit much better this time.

still re-writing, so maybe i'm being too optimistic.  seems likely to work,
though.

thanks!

+    long id,
+    DialogSurface::CallbackOp what,
+    base::android::ScopedJavaGlobalRef<jobject> jobject_opt,
+    base::WaitableEvent* event) {
+  // A callback has happened on some other thread.  We're run on the correct
+  // thread in response to it.
+  DialogSurfaceCallback* thiz = g_map.Pointer()->Lookup(id);
+  // |thiz| cannot be deleted here, since we're on the main thread.  However,
+  // it might have been deleted earlier, so it can be null.
+
+  // If this is SURFACE_DESTROYED, then no further callbacks are permitted.
+  // Erase the map entry in case the IDialogSurface decides to send another.
+  if (what == DialogSurface::CallbackOp::SURFACE_DESTROYED)
+    g_map.Pointer()->Erase(id);
+
+  if (thiz) {
+    DCHECK(thiz->CalledOnValidThread());
+    // Send the java surface to the DialogSurface before the callback.
+    if (what == DialogSurface::CallbackOp::SURFACE_CREATED &&
+        !thiz->android_surface_cb_.is_null()) {
+      thiz->android_surface_cb_.Run(jobject_opt);
+    }
+
+    // Run the client-supplied callback.
+    thiz->callback_.Run(what);
+  }
+
+  // If this is a synchronous callback, then signal that we've completed.
+  if (event)
+    event->Signal();
+}
+
+void OnDialogSurfaceCallbackCreated(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jclass>& jcaller,
+    jlong id,
+    const base::android::JavaParamRef<jobject>& jsurface) {
+  // Called from some random thread in java.
+  // Post back to the main thread.  We get a strong ref here to the task
+  // runner, so that it's okay if the controller is deleted.  Holding the
+  // lock during the callback is not safe; the caller might require it to
+  // complete the callback.
+  // Remember that we're on a random thread, but |g_map| is thread safe.
+  if (scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+          g_map.Pointer()->GetTaskRunner(id)) {
+    base::Closure cb = base::Bind(
+        &DialogSurfaceCallback::OnCallbackProperThread, id,
+        DialogSurface::CallbackOp::SURFACE_CREATED,
+        base::android::ScopedJavaGlobalRef<jobject>(jsurface), nullptr);
+    task_runner->PostTask(FROM_HERE, cb);
+  }
+}
+
+void OnDialogSurfaceCallbackDestroyed(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jclass>& jcaller,
+    jlong id) {
+  // Called from some random thread in java.
+  // SURFACE_DESTROYED must be synchronous, so that the client can stop using
+  // the surface before Android destroys it.
+  if (scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+          g_map.Pointer()->GetTaskRunner(id)) {
+    // TODO(liberato): check the thread, and just call if needed.  That way,
+    // we'll work regardless of how the java classes handle threading.
+    base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
+                              base::WaitableEvent::InitialState::NOT_SIGNALED);
+    base::Closure cb = base::Bind(
+        &DialogSurfaceCallback::OnCallbackProperThread, id,
+        DialogSurface::CallbackOp::SURFACE_DESTROYED, nullptr, &event);
+    task_runner->PostTask(FROM_HERE, cb);
+    event.Wait();

[boliu, 2017/01/04 01:48:44]

why does this need to be synchronous?

I think blocking a binder thread is probably like blocking the IO thread in
chrome, it's bad bad bad

[liberato (no reviews please), 2017/01/11 22:17:56]

it has to be synchronous because android's surfaceDestroyed() callback requires
it.

as long as one doesn't run out of threads in the binder pool, i think that this
should be okay.

[boliu, 2017/01/12 20:24:18]

But what if that happens?

Can emulate chrome's sync IPC which does not block the IO thread. Make the
caller block for an async reply back from here.

[liberato (no reviews please), 2017/02/03 21:28:32]

Done.  added IAndroidOverlayCompletion which gets sent to the callback for sync
destroy.

+  }
+}
+
+}  // namespace media