[Remoting Android] Fix thread issue with OpenGL drawable

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"
@@ skipping 23 matching lines @@
   void OnFrameRendered() override;
   void OnSizeChanged(int width, int height) override;
 
   // CursorShapeStub interface.
   void SetCursorShape(const protocol::CursorShapeInfo& cursor_shape) override;
 
   // Returns the frame consumer for updating desktop frame. Can be called on any
   // thread but no more than once.
   std::unique_ptr<protocol::FrameConsumer> GrabFrameConsumer();
 
+  void OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
+                       const base::Closure& done);
+
   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:
+  // Initializes the core on the display thread. Can be called on any thread.
+  void Initialize();
+
   ChromotingJniRuntime* runtime_;
   base::WeakPtr<JniGlDisplayHandler> shell_;
 
   // Will be std::move'd when GrabFrameConsumer() is called.
   std::unique_ptr<DualBufferFrameConsumer> owned_frame_consumer_;
 
   base::WeakPtr<DualBufferFrameConsumer> frame_consumer_;
 
   ANativeWindow* window_ = nullptr;
   std::unique_ptr<EglThreadContext> egl_context_;
   std::unique_ptr<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) {
-  renderer_ = GlRenderer::CreateGlRendererWithDesktop();
   weak_ptr_ = weak_factory_.GetWeakPtr();
-  renderer_->SetDelegate(weak_ptr_);
+  Initialize();

[Sergey Ulanov, 2017/01/18 22:33:40]

runtime_->display_task_runner()->PostTask(
         FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize,
                               base::Unretained(this)));

[Yuwei, 2017/01/18 23:11:56]

Since we still need the weak pointer for other purposes, why not just use weak
pointer here so that we don't need to check the thread safety in mind when
reading this code?

[Sergey Ulanov, 2017/01/18 23:32:43]

The "Core" class used here to switch to a different thread is a common pattern
that is used in many other places. This pattern doesn't need WeakPtr. Some
issues I see with using weak_ptr here:
 1. Someone reads this code, recognizes the pattern, but gets confused by
WeakPtr.
 2. Someone removes weak_ptr dependency from DualBufferFrameConsumer, but
assumes it's still needed for this PostTask() keeps weak_factory_.
 3. Someone uses this code as reference for the pattern, assumes that weak_ptr
is meaningful here, and copies it in a new class.

Overall WeakPtr is not a silver-bullet. If it's used here it doesn't
automatically make this PostTask call safe (particularly because WeakPtr is not
thread-safe). You are just trading one set of potential issues for another.

[Yuwei, 2017/01/19 00:27:33]

Acknowledged. Done.

+  // Do not bind GlRenderer::OnFrameReceived. |renderer_| is not ready yet.
   owned_frame_consumer_.reset(new DualBufferFrameConsumer(
-      base::Bind(&GlRenderer::OnFrameReceived, renderer_->GetWeakPtr()),
+      base::Bind(&JniGlDisplayHandler::Core::OnFrameReceived, weak_ptr_),
       runtime_->display_task_runner(),
       protocol::FrameConsumer::PixelFormat::FORMAT_RGBA));
   frame_consumer_ = owned_frame_consumer_->GetWeakPtr();
 }
 
 JniGlDisplayHandler::Core::~Core() {}
 
 bool JniGlDisplayHandler::Core::CanRenderFrame() {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
   return egl_context_ && egl_context_->IsWindowBound();
@@ skipping 18 matching lines @@
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
   renderer_->OnCursorShapeChanged(cursor_shape);
 }
 
 std::unique_ptr<protocol::FrameConsumer>
 JniGlDisplayHandler::Core::GrabFrameConsumer() {
   DCHECK(owned_frame_consumer_) << "The frame consumer is already grabbed.";
   return std::move(owned_frame_consumer_);
 }
 
+void JniGlDisplayHandler::Core::OnFrameReceived(
+    std::unique_ptr<webrtc::DesktopFrame> frame,
+    const base::Closure& done) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_->OnFrameReceived(std::move(frame), done);
+}
+
 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_);
@@ skipping 42 matching lines @@
                                                    float diameter) {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
   renderer_->OnCursorInputFeedback(x, y, diameter);
 }
 
 base::WeakPtr<JniGlDisplayHandler::Core>
 JniGlDisplayHandler::Core::GetWeakPtr() {
   return weak_ptr_;
 }
 
+void JniGlDisplayHandler::Core::Initialize() {
+  if (!runtime_->display_task_runner()->BelongsToCurrentThread()) {

[Sergey Ulanov, 2017/01/18 22:33:40]

PostTask in the constructor and replace this code with
DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread()).

[Yuwei, 2017/01/19 00:27:33]

Done.

+    runtime_->display_task_runner()->PostTask(
+        FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize,
+                              weak_ptr_));

[Sergey Ulanov, 2017/01/18 22:33:40]

Don't need to use weak pointer here because Core is destroyed on the same
thread.

[Yuwei, 2017/01/19 00:27:33]

Done.

+    return;
+  }
+
+  renderer_ = GlRenderer::CreateGlRendererWithDesktop();
+  renderer_->SetDelegate(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();
@@ skipping 103 matching lines @@
                                 java_display_);
 }
 
 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