[Remoting Android] Separate the display core from JniGlDisplayHandler

remoting/client/jni/jni_gl_display_handler.cc

 // 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 "remoting/client/jni/jni_gl_display_handler.h"
 
 #include <android/native_window_jni.h>
 #include <array>
 
 #include "base/android/jni_android.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "jni/GlDisplay_jni.h"
 #include "remoting/client/cursor_shape_stub_proxy.h"
 #include "remoting/client/dual_buffer_frame_consumer.h"
 #include "remoting/client/jni/chromoting_jni_runtime.h"
 #include "remoting/client/jni/egl_thread_context.h"
-#include "remoting/client/queued_task_poster.h"
 #include "remoting/client/software_video_renderer.h"
 #include "remoting/protocol/frame_consumer.h"
 
 namespace remoting {
 
-JniGlDisplayHandler::JniGlDisplayHandler(ChromotingJniRuntime* runtime)
-    : runtime_(runtime), weak_factory_(this) {
+// The core that lives on the display thread.
+class JniGlDisplayHandler::Core : public protocol::CursorShapeStub,
+                                  public GlRendererDelegate {
+ public:
+  Core(ChromotingJniRuntime* runtime, base::WeakPtr<JniGlDisplayHandler> shell);
+  ~Core();
+
+  // GlRendererDelegate interface.
+  bool CanRenderFrame() override;
+  void OnFrameRendered() override;
+  void OnSizeChanged(int width, int height) override;
+
+  // CursorShapeStub interface.
+  void SetCursorShape(const protocol::CursorShapeInfo& cursor_shape) override;
+
+  // Creates the frame consumer for updating desktop frame. Should be called
+  // exactly once.
+  std::unique_ptr<protocol::FrameConsumer> CreateFrameConsumer();
+
+  void SurfaceCreated(base::android::ScopedJavaGlobalRef<jobject> surface);
+  void SurfaceChanged(int width, int height);
+  void SurfaceDestroyed();
+
+  void SetTransformation(const std::array<float, 9>& matrix);
+  void MoveCursor(float x, float y);
+  void SetCursorVisibility(bool visible);
+  void StartInputFeedback(float x, float y, float diameter);
+
+  base::WeakPtr<Core> GetWeakPtr();
+
+ private:
+  ChromotingJniRuntime* runtime_;
+  base::WeakPtr<JniGlDisplayHandler> shell_;
+  base::WeakPtr<DualBufferFrameConsumer> frame_consumer_;
+
+  ANativeWindow* window_ = nullptr;
+  std::unique_ptr<EglThreadContext> egl_context_;
+  GlRenderer renderer_;
+
+  // Used on display thread.
+  base::WeakPtr<Core> weak_ptr_;
+  base::WeakPtrFactory<Core> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+JniGlDisplayHandler::Core::Core(ChromotingJniRuntime* runtime,
+                                base::WeakPtr<JniGlDisplayHandler> shell)
+    : runtime_(runtime), shell_(shell), weak_factory_(this) {
   weak_ptr_ = weak_factory_.GetWeakPtr();
-  java_display_.Reset(Java_GlDisplay_createJavaDisplayObject(
-      base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this)));
   renderer_.SetDelegate(weak_ptr_);
-  ui_task_poster_.reset(new QueuedTaskPoster(runtime->display_task_runner()));
 }
 
-JniGlDisplayHandler::~JniGlDisplayHandler() {
+JniGlDisplayHandler::Core::~Core() {}
+
+bool JniGlDisplayHandler::Core::CanRenderFrame() {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  DCHECK(!ui_task_poster_) << "Invalidate() must be called on the UI thread "
-                              "before deleting this object.";
+  return egl_context_ && egl_context_->IsWindowBound();
 }
 
-void JniGlDisplayHandler::Initialize(
-    const base::android::JavaRef<jobject>& java_client) {
-  Java_GlDisplay_initializeClient(base::android::AttachCurrentThread(),
-                                  java_display_, java_client);
+void JniGlDisplayHandler::Core::OnFrameRendered() {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  egl_context_->SwapBuffers();
+  runtime_->ui_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&JniGlDisplayHandler::OnRenderDone, shell_));
 }
 
-void JniGlDisplayHandler::Invalidate() {
-  DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
-  Java_GlDisplay_invalidate(base::android::AttachCurrentThread(),
-                            java_display_);
-  ui_task_poster_.reset();
+void JniGlDisplayHandler::Core::OnSizeChanged(int width, int height) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  runtime_->ui_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&JniGlDisplayHandler::OnCanvasSizeChanged, shell_,
+                            width, height));
 }
 
-std::unique_ptr<protocol::CursorShapeStub>
-JniGlDisplayHandler::CreateCursorShapeStub() {
-  return base::MakeUnique<CursorShapeStubProxy>(
-      weak_ptr_, runtime_->display_task_runner());
+void JniGlDisplayHandler::Core::SetCursorShape(
+    const protocol::CursorShapeInfo& cursor_shape) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_.OnCursorShapeChanged(cursor_shape);
 }
 
-std::unique_ptr<protocol::VideoRenderer>
-JniGlDisplayHandler::CreateVideoRenderer() {
-  DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
+std::unique_ptr<protocol::FrameConsumer>
+JniGlDisplayHandler::Core::CreateFrameConsumer() {
   DCHECK(!frame_consumer_);

[Sergey Ulanov, 2016/10/03 17:33:44]

DCHECK here that this function is called on UI thread

[Yuwei, 2016/10/03 19:03:39]

I think calling on any thread may be fine as long as it is only called once at
the beginning?

[Yuwei, 2016/10/03 20:33:04]

Added comment about threading.

   std::unique_ptr<DualBufferFrameConsumer> consumer =
       base::MakeUnique<DualBufferFrameConsumer>(
           base::Bind(&GlRenderer::OnFrameReceived, renderer_.GetWeakPtr()),
           runtime_->display_task_runner(),
           protocol::FrameConsumer::PixelFormat::FORMAT_RGBA);
   frame_consumer_ = consumer->GetWeakPtr();

[Sergey Ulanov, 2016/10/03 17:33:44]

frame_consumer_ is set on UI thread, but then it's used on display thread in
SurfaceCreated(). Is there anything to ensure there is no race?

[Yuwei, 2016/10/03 19:03:39]

I think it's mostly fine since it is only called once before SurfaceCreated().

Alternatively I can create the consumer in ctor and provide a function for
transferring the ownership of the consumer to the caller.

[Sergey Ulanov, 2016/10/03 19:42:10]

Problem is that there is nothing in this class to verify that this function is
called before SurfaceCreated().
Maybe add DCHECK in JniGlDisplayHandler::OnSurfaceCreated() to verify that it's
called after CreateFrameConsumer()
That would work as well.

[Yuwei, 2016/10/03 20:33:04]

Acknowledged.
I think it's not necessary now. |frame_consumer_| is now set inside the ctor so
the caller can grab the ownership any time they want.
Done.

-  return base::MakeUnique<SoftwareVideoRenderer>(std::move(consumer));
+  return std::move(consumer);
+}
+
+void JniGlDisplayHandler::Core::SurfaceCreated(
+    base::android::ScopedJavaGlobalRef<jobject> surface) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  DCHECK(!egl_context_);
+  DCHECK(!window_);
+  renderer_.RequestCanvasSize();
+  window_ = ANativeWindow_fromSurface(base::android::AttachCurrentThread(),
+                                      surface.obj());
+  egl_context_.reset(new EglThreadContext());
+  egl_context_->BindToWindow(window_);
+  renderer_.OnSurfaceCreated(static_cast<int>(egl_context_->client_version()));
+  runtime_->network_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&DualBufferFrameConsumer::RequestFullDesktopFrame,
+                            frame_consumer_));
+}
+
+void JniGlDisplayHandler::Core::SurfaceChanged(int width, int height) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_.OnSurfaceChanged(width, height);
+}
+
+void JniGlDisplayHandler::Core::SurfaceDestroyed() {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  DCHECK(egl_context_);
+  DCHECK(window_);
+  renderer_.OnSurfaceDestroyed();
+  egl_context_.reset();
+  ANativeWindow_release(window_);

[Sergey Ulanov, 2016/10/03 17:33:44]

previously window was released in SurfaceCreatedOnDisplayThread(). Is there any
reason this window can't be released in Core::SurfaceCreated()? Then it should
be possible to remove window_

[Yuwei, 2016/10/03 19:03:39]

Looks likes releasing window_ right after eglCreateWindowSurface still work.
Since the Android specific behavior of eglCreateWindowSurface is not well
documented, my concern is if eglCreateWindowSurface doesn't acquire a reference
to |window_| then |window_| might be destroyed before |egl_context_| is detached
once the view is destroyed on the UI thread. I remembered that I saw warning
saying failed to swap buffer when the surface is being destroyed, couldn't repro
it with latest build on Android N though...

I also did a code search on ANativeWindow_release and looks like most people
will keep the window until some destroy function is called.

I'm okay with releasing the window right after eglCreateWindowSurface as long as
it doesn't crash the app.

+  window_ = nullptr;
+}
+
+void JniGlDisplayHandler::Core::SetTransformation(
+    const std::array<float, 9>& matrix) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_.OnPixelTransformationChanged(matrix);
+}
+
+void JniGlDisplayHandler::Core::MoveCursor(float x, float y) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_.OnCursorMoved(x, y);
+}
+
+void JniGlDisplayHandler::Core::SetCursorVisibility(bool visible) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_.OnCursorVisibilityChanged(visible);
+}
+
+void JniGlDisplayHandler::Core::StartInputFeedback(float x,
+                                                   float y,
+                                                   float diameter) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_.OnCursorInputFeedback(x, y, diameter);
+}
+
+base::WeakPtr<JniGlDisplayHandler::Core>
+JniGlDisplayHandler::Core::GetWeakPtr() {
+  return weak_ptr_;
+}
+
+// Shell implementations.
+
+JniGlDisplayHandler::JniGlDisplayHandler(
+    ChromotingJniRuntime* runtime,
+    const base::android::JavaRef<jobject>& java_client)
+    : runtime_(runtime),
+      ui_task_poster_(runtime->display_task_runner()),
+      weak_factory_(this) {
+  core_.reset(new Core(runtime_, weak_factory_.GetWeakPtr()));
+  JNIEnv* env = base::android::AttachCurrentThread();
+  java_display_.Reset(Java_GlDisplay_createJavaDisplayObject(
+      env, reinterpret_cast<intptr_t>(this)));
+  Java_GlDisplay_initializeClient(env, java_display_, java_client);
+}
+
+JniGlDisplayHandler::~JniGlDisplayHandler() {
+  DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
+  Java_GlDisplay_invalidate(base::android::AttachCurrentThread(),
+                            java_display_);
+  runtime_->display_task_runner()->DeleteSoon(FROM_HERE, core_.release());
+}
+
+std::unique_ptr<protocol::CursorShapeStub>
+JniGlDisplayHandler::CreateCursorShapeStub() {
+  return base::MakeUnique<CursorShapeStubProxy>(
+      core_->GetWeakPtr(), runtime_->display_task_runner());
+}
+
+std::unique_ptr<protocol::VideoRenderer>
+JniGlDisplayHandler::CreateVideoRenderer() {
+  return base::MakeUnique<SoftwareVideoRenderer>(core_->CreateFrameConsumer());
 }
 
 // static
 bool JniGlDisplayHandler::RegisterJni(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 void JniGlDisplayHandler::OnSurfaceCreated(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
     const base::android::JavaParamRef<jobject>& surface) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
   runtime_->display_task_runner()->PostTask(
       FROM_HERE,
-      base::Bind(&JniGlDisplayHandler::SurfaceCreatedOnDisplayThread, weak_ptr_,
+      base::Bind(&Core::SurfaceCreated, core_->GetWeakPtr(),
                  base::android::ScopedJavaGlobalRef<jobject>(env, surface)));
 }
 
 void JniGlDisplayHandler::OnSurfaceChanged(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
     int width,
     int height) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
   runtime_->display_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&GlRenderer::OnSurfaceChanged,
-                            renderer_.GetWeakPtr(), width, height));
+      FROM_HERE,
+      base::Bind(&Core::SurfaceChanged, core_->GetWeakPtr(), width, height));
 }
 
 void JniGlDisplayHandler::OnSurfaceDestroyed(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
   runtime_->display_task_runner()->PostTask(
-      FROM_HERE,
-      base::Bind(&JniGlDisplayHandler::SurfaceDestroyedOnDisplayThread,
-                 weak_ptr_));
+      FROM_HERE, base::Bind(&Core::SurfaceDestroyed, core_->GetWeakPtr()));
 }
 
 void JniGlDisplayHandler::OnPixelTransformationChanged(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
     const base::android::JavaParamRef<jfloatArray>& jmatrix) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
   DCHECK(env->GetArrayLength(jmatrix.obj()) == 9);
   std::array<float, 9> matrix;
   env->GetFloatArrayRegion(jmatrix.obj(), 0, 9, matrix.data());
-  PostSequentialTaskOnDisplayThread(
-      base::Bind(&GlRenderer::OnPixelTransformationChanged,
-                 renderer_.GetWeakPtr(), matrix));
+  ui_task_poster_.AddTask(
+      base::Bind(&Core::SetTransformation, core_->GetWeakPtr(), matrix));
 }
 
 void JniGlDisplayHandler::OnCursorPixelPositionChanged(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
     float x,
     float y) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
-  PostSequentialTaskOnDisplayThread(
-      base::Bind(&GlRenderer::OnCursorMoved, renderer_.GetWeakPtr(), x, y));
+  ui_task_poster_.AddTask(
+      base::Bind(&Core::MoveCursor, core_->GetWeakPtr(), x, y));
 }
 
 void JniGlDisplayHandler::OnCursorVisibilityChanged(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
     bool visible) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
-  PostSequentialTaskOnDisplayThread(base::Bind(
-      &GlRenderer::OnCursorVisibilityChanged, renderer_.GetWeakPtr(), visible));
+  ui_task_poster_.AddTask(
+      base::Bind(&Core::SetCursorVisibility, core_->GetWeakPtr(), visible));
 }
 
 void JniGlDisplayHandler::OnCursorInputFeedback(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
     float x,
     float y,
     float diameter) {
   DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
-  PostSequentialTaskOnDisplayThread(
-      base::Bind(&GlRenderer::OnCursorInputFeedback, renderer_.GetWeakPtr(), x,
-                 y, diameter));
+  ui_task_poster_.AddTask(base::Bind(&Core::StartInputFeedback,
+                                     core_->GetWeakPtr(), x, y, diameter));
 }
 
-void JniGlDisplayHandler::PostSequentialTaskOnDisplayThread(
-    const base::Closure& task) {
-  if (!ui_task_poster_) {
-    return;
-  }
-  ui_task_poster_->AddTask(task);
+void JniGlDisplayHandler::OnRenderDone() {
+  DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
+  Java_GlDisplay_canvasRendered(base::android::AttachCurrentThread(),
+                                java_display_);
 }
 
-bool JniGlDisplayHandler::CanRenderFrame() {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  return egl_context_ && egl_context_->IsWindowBound();
-}
-
-void JniGlDisplayHandler::OnFrameRendered() {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  egl_context_->SwapBuffers();
-  runtime_->ui_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&JniGlDisplayHandler::NotifyRenderDoneOnUiThread,
-                            java_display_));
-}
-
-void JniGlDisplayHandler::OnSizeChanged(int width, int height) {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  runtime_->ui_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&JniGlDisplayHandler::ChangeCanvasSizeOnUiThread,
-                            java_display_, width, height));
-}
-
-void JniGlDisplayHandler::SetCursorShape(
-    const protocol::CursorShapeInfo& cursor_shape) {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  renderer_.OnCursorShapeChanged(cursor_shape);
-}
-
-// static
-void JniGlDisplayHandler::NotifyRenderDoneOnUiThread(
-    base::android::ScopedJavaGlobalRef<jobject> java_display) {
-  Java_GlDisplay_canvasRendered(base::android::AttachCurrentThread(),
-                                java_display);
-}
-
-void JniGlDisplayHandler::SurfaceCreatedOnDisplayThread(
-    base::android::ScopedJavaGlobalRef<jobject> surface) {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  renderer_.RequestCanvasSize();
-  ANativeWindow* window = ANativeWindow_fromSurface(
-      base::android::AttachCurrentThread(), surface.obj());
-  egl_context_.reset(new EglThreadContext());
-  egl_context_->BindToWindow(window);
-  ANativeWindow_release(window);
-  renderer_.OnSurfaceCreated(static_cast<int>(egl_context_->client_version()));
-  runtime_->network_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&DualBufferFrameConsumer::RequestFullDesktopFrame,
-                            frame_consumer_));
-}
-
-void JniGlDisplayHandler::SurfaceDestroyedOnDisplayThread() {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  renderer_.OnSurfaceDestroyed();
-  egl_context_.reset();
-}
-
-// static
-void JniGlDisplayHandler::ChangeCanvasSizeOnUiThread(
-    base::android::ScopedJavaGlobalRef<jobject> java_display,
-    int width,
-    int height) {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_GlDisplay_changeCanvasSize(env, java_display, width, height);
+void JniGlDisplayHandler::OnCanvasSizeChanged(int width, int height) {
+  DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
+  Java_GlDisplay_changeCanvasSize(base::android::AttachCurrentThread(),
+                                  java_display_, width, height);
 }
 
 }  // namespace remoting