[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

BUG=385924

Review-Url: https://codereview.chromium.org/2100943004
Cr-Commit-Position: refs/heads/master@{#403272}

remoting/android/java/src/org/chromium/chromoting/DesktopView.java

@@ -23,6 +23,7 @@ import android.view.inputmethod.InputMethodManager;
 
 import org.chromium.base.Log;
 import org.chromium.chromoting.jni.Client;
+import org.chromium.chromoting.jni.Display;
 
 /**
  * The user interface for viewing and interacting with a specific remote host.
@@ -46,6 +47,8 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface,
     /** The Client connection, used to inject input and fetch the video frames. */
     private Client mClient;
 
+    private Display mDisplay;
+
 
     // Flag to prevent multiple repaint requests from being backed up. Requests for repainting will
     // be dropped if this is already set to true. This is used by the main thread and the painting
@@ -90,10 +93,14 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface,
     public void init(Desktop desktop, Client client) {
         Preconditions.isNull(mDesktop);
         Preconditions.isNull(mClient);
+        Preconditions.isNull(mDisplay);
         Preconditions.notNull(desktop);
         Preconditions.notNull(client);
+        Preconditions.notNull(client.getDisplay());
+        Preconditions.isTrue(client.getDisplay() instanceof Display);
         mDesktop = desktop;
         mClient = client;
+        mDisplay = (Display) client.getDisplay();
         mInputHandler.init(desktop, new InputEventSender(client));
     }
 
@@ -121,7 +128,7 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface,
             }
             mRepaintPending = true;
         }
-        mClient.getDisplay().redrawGraphics();
+        mDisplay.redrawGraphics();
     }
 
     /**
@@ -136,7 +143,7 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface,
             Log.w(TAG, "Canvas being redrawn on UI thread");
         }
 
-        Bitmap image = mClient.getDisplay().getVideoFrame();
+        Bitmap image = mDisplay.getVideoFrame();
         if (image == null) {
             // This can happen if the client is connected, but a complete video frame has not yet
             // been decoded.
@@ -191,9 +198,9 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface,
         mOnPaint.raise(new PaintEventParameter(cursorPosition, canvas, scaleFactor));
 
         if (drawCursor) {
-            Bitmap cursorBitmap = mClient.getDisplay().getCursorBitmap();
+            Bitmap cursorBitmap = mDisplay.getCursorBitmap();
             if (cursorBitmap != null) {
-                Point hotspot = mClient.getDisplay().getCursorHotspot();
+                Point hotspot = mDisplay.getCursorHotspot();
                 canvas.drawBitmap(cursorBitmap, cursorPosition.x - hotspot.x,
                         cursorPosition.y - hotspot.y, new Paint());
             }
@@ -243,7 +250,7 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface,
     }
 
     public void attachRedrawCallback() {
-        mClient.getDisplay().provideRedrawCallback(new Runnable() {
+        mDisplay.provideRedrawCallback(new Runnable() {
             @Override
             public void run() {
                 paint();

remoting/android/java/src/org/chromium/chromoting/cardboard/CardboardRenderer.java

@@ -16,6 +16,7 @@ import com.google.vrtoolkit.cardboard.HeadTransform;
 import com.google.vrtoolkit.cardboard.Viewport;
 
 import org.chromium.chromoting.jni.Client;
+import org.chromium.chromoting.jni.Display;
 
 import javax.microedition.khronos.egl.EGLConfig;
 
@@ -66,6 +67,7 @@ public class CardboardRenderer implements CardboardView.StereoRenderer {
 
     private final Activity mActivity;
     private final Client mClient;
+    private final Display mDisplay;
 
     private float mCameraPosition;
 
@@ -107,6 +109,7 @@ public class CardboardRenderer implements CardboardView.StereoRenderer {
     public CardboardRenderer(Activity activity, Client client) {
         mActivity = activity;
         mClient = client;
+        mDisplay = (Display) client.getDisplay();
         mCameraPosition = 0.0f;
 
         mCameraMatrix = new float[16];
@@ -124,7 +127,7 @@ public class CardboardRenderer implements CardboardView.StereoRenderer {
     private void initializeRedrawCallback() {
         mActivity.runOnUiThread(new Runnable() {
             public void run() {
-                mClient.getDisplay().provideRedrawCallback(new Runnable() {
+                mDisplay.provideRedrawCallback(new Runnable() {
                     @Override
                     public void run() {
                         mDesktop.reloadTexture();
@@ -132,7 +135,7 @@ public class CardboardRenderer implements CardboardView.StereoRenderer {
                     }
                 });
 
-                mClient.getDisplay().redrawGraphics();
+                mDisplay.redrawGraphics();
             }
         });
     }
@@ -148,7 +151,7 @@ public class CardboardRenderer implements CardboardView.StereoRenderer {
         // Enable depth testing.
         GLES20.glEnable(GLES20.GL_DEPTH_TEST);
 
-        mDesktop = new Desktop(mClient);
+        mDesktop = new Desktop(mDisplay);
         mMenuBar = new MenuBar(mActivity);
         mPhotosphere = new Photosphere(mActivity);
         mCursor = new Cursor(mClient);

remoting/android/java/src/org/chromium/chromoting/cardboard/Cursor.java

@@ -14,6 +14,7 @@ import android.opengl.GLES20;
 
 import org.chromium.chromoting.InputStub;
 import org.chromium.chromoting.jni.Client;
+import org.chromium.chromoting.jni.Display;
 
 import java.nio.FloatBuffer;
 
@@ -53,6 +54,7 @@ public class Cursor {
     private static final float CURSOR_MOVE_THRESHOLD = 1.0f;
 
     private final Client mClient;
+    private final Display mDisplay;
 
     private FloatBuffer mPositionCoordinates;
 
@@ -80,6 +82,7 @@ public class Cursor {
 
     public Cursor(Client client) {
         mClient = client;
+        mDisplay = (Display) client.getDisplay();
         mHalfFrameSize = new PointF(0.0f, 0.0f);
         mCursorPosition = new PointF(0.0f, 0.0f);
 
@@ -140,7 +143,7 @@ public class Cursor {
             }
         }
 
-        Bitmap cursorBitmap = mClient.getDisplay().getCursorBitmap();
+        Bitmap cursorBitmap = mDisplay.getCursorBitmap();
 
         if (cursorBitmap == mCursorBitmap) {
             // Case when cursor image has not changed.
@@ -151,7 +154,7 @@ public class Cursor {
         }
 
         mCursorBitmap = cursorBitmap;
-        updatePosition(desktop, mCursorBitmap, mClient.getDisplay().getCursorHotspot());
+        updatePosition(desktop, mCursorBitmap, mDisplay.getCursorHotspot());
 
         TextureHelper.linkTexture(mTextureDataHandle, cursorBitmap);
 

remoting/android/java/src/org/chromium/chromoting/cardboard/Desktop.java

@@ -11,7 +11,7 @@ import android.graphics.Bitmap;
 import android.graphics.Point;
 import android.opengl.GLES20;
 
-import org.chromium.chromoting.jni.Client;
+import org.chromium.chromoting.jni.Display;
 
 import java.nio.FloatBuffer;
 
@@ -61,7 +61,7 @@ public class Desktop {
     // Number of vertices passed to glDrawArrays().
     private static final int VERTICES_NUMBER = 6;
 
-    private final Client mClient;
+    private final Display mDisplay;
 
     private int mVertexShaderHandle;
     private int mFragmentShaderHandle;
@@ -89,8 +89,8 @@ public class Desktop {
     // Lock to allow multithreaded access to mReloadTexture.
     private final Object mReloadTextureLock = new Object();
 
-    public Desktop(Client client) {
-        mClient = client;
+    public Desktop(Display display) {
+        mDisplay = display;
         mVertexShaderHandle =
                 ShaderHelper.compileShader(GLES20.GL_VERTEX_SHADER, VERTEX_SHADER);
         mFragmentShaderHandle =
@@ -223,7 +223,7 @@ public class Desktop {
         }
 
         // TODO(shichengfeng): Record the time desktop drawing takes.
-        Bitmap bitmap = mClient.getDisplay().getVideoFrame();
+        Bitmap bitmap = mDisplay.getVideoFrame();
 
         if (bitmap == null) {
             // This can happen if the client is connected, but a complete video frame has not yet

remoting/android/java/src/org/chromium/chromoting/jni/Client.java

@@ -23,9 +23,8 @@ public class Client implements InputStub {
     // Pointer to the C++ object, cast to a |long|.
     private long mNativeJniClient;
 
-    // Reference has to be kept until the lifecycle of Client ends. Code are currently using
-    // getDisplay() without doing a null check.
-    private Display mDisplay;
+    // Implementation-dependent display object used by the desktop view.
+    private Object mDisplay;
 
     // The global Client instance (may be null). This needs to be a global singleton so that the
     // Client can be passed between Activities.
@@ -41,11 +40,20 @@ public class Client implements InputStub {
     }
 
     /**
-     * Returns the display object. It will be null before calling connectToHost() but won't be null
-     *          after calling disconnectFromHost().
+     * Sets the display object. Called by the native code when the connection starts.
+     * @param display the implementation-dependent object used by the desktop view.
+     */
+    @CalledByNative
+    private void setDisplay(Object display) {
+        mDisplay = display;
+    }
+
+    /**
+     * Returns the display object. It will be null before calling connectToHost() or after calling
+     * disconnectFromHost().
      * @return the display object.
      */
-    public Display getDisplay() {
+    public Object getDisplay() {
         return mDisplay;
     }
 
@@ -93,8 +101,7 @@ public class Client implements InputStub {
 
         mConnectionListener = listener;
         mAuthenticator = authenticator;
-        mDisplay = new Display();
-        nativeConnect(mNativeJniClient, mDisplay.getNativePointer(), username, authToken, hostJid,
+        nativeConnect(mNativeJniClient, username, authToken, hostJid,
                 hostId, hostPubkey, mAuthenticator.getPairingId(hostId),
                 mAuthenticator.getPairingSecret(hostId), mCapabilityManager.getLocalCapabilities(),
                 flags);
@@ -124,7 +131,7 @@ public class Client implements InputStub {
         mConnected = false;
         mCapabilityManager.onHostDisconnect();
 
-        mDisplay.destroy();
+        mDisplay = null;
     }
 
     /** Called whenever the connection status changes. */
@@ -296,7 +303,7 @@ public class Client implements InputStub {
     private native void nativeDestroy(long nativeJniClient);
 
     /** Performs the native portion of the connection. */
-    private native void nativeConnect(long nativeJniClient, long nativeJniDisplayHandler,
+    private native void nativeConnect(long nativeJniClient,
             String username, String authToken, String hostJid, String hostId, String hostPubkey,
             String pairId, String pairSecret, String capabilities, String flags);
 

remoting/android/java/src/org/chromium/chromoting/jni/Display.java

@@ -51,8 +51,8 @@ public class Display {
     /** Bitmap holding the cursor shape. Accessed on the graphics thread. */
     private Bitmap mCursorBitmap;
 
-    public Display() {
-        mNativeJniDisplayHandler = nativeInit();
+    private Display(long nativeDisplayHandler) {
+        mNativeJniDisplayHandler = nativeDisplayHandler;
     }
 
     /**
@@ -74,16 +74,17 @@ public class Display {
     }
 
     /**
-     * Destroys its resources and the native counterpart. Called on the UI thread.
+     * Invalidates this object and disconnects from the native display handler. Called on the
+     * display thread by the native code.
      */
-    public void destroy() {
+    @CalledByNative
+    private void invalidate() {
         // Drop the reference to free the Bitmap for GC.
         synchronized (mFrameLock) {
             mFrameBitmap = null;
         }
 
         provideRedrawCallback(null);
-        nativeDestroy(mNativeJniDisplayHandler);
         mNativeJniDisplayHandler = 0;
     }
 
@@ -171,9 +172,10 @@ public class Display {
         return mCursorBitmap;
     }
 
-    private native long nativeInit();
-
-    private native void nativeDestroy(long nativeJniDisplayHandler);
+    @CalledByNative
+    private static Display createJavaDisplayObject(long nativeDisplayHandler) {
+        return new Display(nativeDisplayHandler);
+    }
 
     /** Schedules a redraw on the native graphics thread. */
     private native void nativeScheduleRedraw(long nativeJniDisplayHandler);

remoting/client/jni/jni_client.cc

@@ -70,6 +70,10 @@ void JniClient::DisconnectFromHost() {
     runtime_->network_task_runner()->DeleteSoon(FROM_HERE,
                                                 secret_fetcher_.release());
   }
+  if (display_handler_) {
+    runtime_->display_task_runner()->DeleteSoon(FROM_HERE,
+                                                display_handler_.release());
+  }
 }
 
 void JniClient::OnConnectionState(protocol::ConnectionToHost::State state,
@@ -147,7 +151,6 @@ bool JniClient::RegisterJni(JNIEnv* env) {
 void JniClient::Connect(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller,
-    jlong display_handler,
     const base::android::JavaParamRef<jstring>& username,
     const base::android::JavaParamRef<jstring>& authToken,
     const base::android::JavaParamRef<jstring>& hostJid,
@@ -157,11 +160,15 @@ void JniClient::Connect(
     const base::android::JavaParamRef<jstring>& pairSecret,
     const base::android::JavaParamRef<jstring>& capabilities,
     const base::android::JavaParamRef<jstring>& flags) {
-  // TODO(yuweih): Remove this line when JniClient owns JniDisplayHandler.
-  DisplayUpdaterFactory* factory_ptr = reinterpret_cast<JniDisplayHandler*>(
-              display_handler);
-  DCHECK(factory_ptr);
-  ConnectToHost(factory_ptr,
+#if defined(REMOTING_ANDROID_ENABLE_OPENGL_RENDERER)
+#error Feature not implemented.
+#else
+  JniDisplayHandler* raw_display_handler = new JniDisplayHandler(runtime_);
+#endif  // defined(REMOTING_ANDROID_ENABLE_OPENGL_RENDERER)
+  Java_Client_setDisplay(env, java_client_.obj(),
+                         raw_display_handler->GetJavaDisplay().obj());
+  display_handler_.reset(raw_display_handler);
+  ConnectToHost(raw_display_handler,
                 ConvertJavaStringToUTF8(env, username),
                 ConvertJavaStringToUTF8(env, authToken),
                 ConvertJavaStringToUTF8(env, hostJid),

remoting/client/jni/jni_client.h

@@ -80,7 +80,6 @@ class JniClient {
 
   void Connect(JNIEnv* env,
                const base::android::JavaParamRef<jobject>& caller,
-               jlong display_handler_ptr,
                const base::android::JavaParamRef<jstring>& username,
                const base::android::JavaParamRef<jstring>& authToken,
                const base::android::JavaParamRef<jstring>& hostJid,
@@ -158,6 +157,8 @@ class JniClient {
   // Reference to the Java client object.
   base::android::ScopedJavaGlobalRef<jobject> java_client_;
 
+  std::unique_ptr<DisplayUpdaterFactory> display_handler_;
+
   // Deleted on UI thread.
   std::unique_ptr<JniPairingSecretFetcher> secret_fetcher_;
 

remoting/client/jni/jni_display_handler.cc

@@ -52,15 +52,22 @@ void DisplayCursorShapeStub::SetCursorShape(
 }
 
 // JniDisplayHandler definitions.
-JniDisplayHandler::JniDisplayHandler(
-    ChromotingJniRuntime* runtime,
-    base::android::ScopedJavaGlobalRef<jobject> java_display)
+JniDisplayHandler::JniDisplayHandler(ChromotingJniRuntime* runtime)
     : runtime_(runtime),
-      java_display_(java_display),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  java_display_.Reset(Java_Display_createJavaDisplayObject(
+      env, reinterpret_cast<intptr_t>(this)));
+}
 
 JniDisplayHandler::~JniDisplayHandler() {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  Java_Display_invalidate(base::android::AttachCurrentThread(),
+                          java_display_.obj());
+}
+
+base::android::ScopedJavaLocalRef<jobject> JniDisplayHandler::GetJavaDisplay() {
+  return base::android::ScopedJavaLocalRef<jobject>(java_display_);
 }
 
 void JniDisplayHandler::UpdateCursorShape(
@@ -124,16 +131,6 @@ bool JniDisplayHandler::RegisterJni(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
-void JniDisplayHandler::Destroy(
-    JNIEnv* env,
-    const base::android::JavaParamRef<jobject>& caller) {
-  if (runtime_->display_task_runner()->BelongsToCurrentThread()) {
-    delete this;
-  } else {
-    runtime_->display_task_runner()->DeleteSoon(FROM_HERE, this);
-  }
-}
-
 void JniDisplayHandler::ScheduleRedraw(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller) {
@@ -142,10 +139,4 @@ void JniDisplayHandler::ScheduleRedraw(
                             weak_factory_.GetWeakPtr()));
 }
 
-static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& caller) {
-  return reinterpret_cast<intptr_t>(new JniDisplayHandler(
-      ChromotingJniRuntime::GetInstance(),
-      base::android::ScopedJavaGlobalRef<jobject>(env, caller)));
-}
-
 }  // namespace remoting

remoting/client/jni/jni_display_handler.h

@@ -25,12 +25,13 @@ class ChromotingJniRuntime;
 // unless otherwise noted.
 class JniDisplayHandler : public DisplayUpdaterFactory {
  public:
-  JniDisplayHandler(ChromotingJniRuntime* runtime,
-                    base::android::ScopedJavaGlobalRef<jobject> java_display);
+  explicit JniDisplayHandler(ChromotingJniRuntime* runtime);
 
   // Must be deleted on the display thread.
   ~JniDisplayHandler() override;
 
+  base::android::ScopedJavaLocalRef<jobject> GetJavaDisplay();
+
   void UpdateCursorShape(const protocol::CursorShapeInfo& cursor_shape);
 
   // DisplayUpdaterFactory overrides (functions can be called on any thread).
@@ -51,10 +52,6 @@ class JniDisplayHandler : public DisplayUpdaterFactory {
 
   static bool RegisterJni(JNIEnv* env);
 
-  // Deletes this object on display thread. Can be called on any thread.
-  void Destroy(JNIEnv* env,
-               const base::android::JavaParamRef<jobject>& caller);
-
   // Schedule redraw. Can be called on any thread.
   void ScheduleRedraw(JNIEnv* env,
                       const base::android::JavaParamRef<jobject>& caller);