[Remoting Android] Refactor GlDesktopView

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

 // 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.
 
 package org.chromium.chromoting.jni;
 
 import android.graphics.Matrix;
 import android.graphics.PointF;
 import android.view.Surface;
 import android.view.SurfaceHolder;
 
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
-import org.chromium.chromoting.Desktop;
 import org.chromium.chromoting.DesktopView;
-import org.chromium.chromoting.DesktopViewFactory;
 import org.chromium.chromoting.Event;
-import org.chromium.chromoting.GlDesktopView;
 import org.chromium.chromoting.InputFeedbackRadiusMapper;
 import org.chromium.chromoting.RenderStub;
 import org.chromium.chromoting.SizeChangedEventParameter;
 
 /**
- * This class is the JNI interface class that helps bridging GlDesktopView with the OpenGL renderer
- * in native code. The lifetime of this class is managed by the native JniGlDisplayHandler.
+ * This class is a RenderStub implementation that uses the OpenGL renderer in native code to render
+ * the remote desktop. The lifetime of this class is managed by the native JniGlDisplayHandler.
  *
  * This class works entirely on the UI thread:
  *  Functions should all be called on UI.
  *  Events will only be triggered on UI.
  */
 @JNINamespace("remoting")
 public class GlDisplay implements SurfaceHolder.Callback, RenderStub {
     private volatile long mNativeJniGlDisplay;

[Hzj_jie, 2016/09/10 02:06:32]

Is this volatile useful? I have not seen this pointer been changed somewhere.
And you can make this final.

[Yuwei, 2016/09/10 05:29:14]

There is a small design issue with JniGlDisplayHandler. invalidate() is called
by the native code from the display thread. volatile in Java can guarantee
assignment and referencing are atomic.

This isn't very thread-safe though if invalidate() is called between the nullptr
check and calling the native function. I'm considering making
JniGlDisplayHandler call invalidate() from UI.

[Yuwei, 2016/09/12 18:56:14]

I'll consider fixing this problem after this CL is submitted. I can either:

* Make JniGlDisplay destroy on the UI thread and call invalidate() on UI
* Or reverse the ownership direction and control the lifetime of the native
class from Java like Client does. IIRC the reason we made the native Display
control the lifetime of the Java counterpart was to make it easier for the
native code to decide which renderer instance to create, but now it's
unnecessary since we only have one renderer.

[Yuwei, 2016/09/12 19:13:14]

Looks like I was wrong with #2. The reason was to avoid passing native pointer
of JniGlDisplay back and forth between GlDisplay and Client.

[Hzj_jie, 2016/09/12 21:28:04]

Emmm, interesting, code search won't display all references when searching
mNativeJniGlDisplay, so I lost two references.

[Hzj_jie, 2016/09/12 21:28:04]

I have had a look at JniGlDisplayHandler, and have several questions,
1. JniGlDisplayHandler should only be destroyed on display thread, otherwise all
other On* functions are not safe. Is that correct?
2. Even you invalidate the pointer in UI thread, it's still not safe. As the
invalidate function may be executed after the destructor. i.e. You may execute
some renderer_.GetWeakPtr() after renderer_ has been released.
3. jni.Client.destroy() should call Desktop.onDestroy() and
DesktopView.onDestroy(), JniClient.DisconnectFromHost(),
JniGlDisplayHandler.~JniGlDisplayHandler sequentially. So you probably be safe
without need to have the invalidate function in GlDisplay. i.e. Nobody is able
to call GlDisplay functions after JniGlDisplayHandler's destructor. Would you
mind to confirm it?

[Yuwei, 2016/09/12 21:50:04]

Something like that. Basically its WeakFactory lives on the display thread and
therefore must be destroyed on destroyed on the display thread.
You may check my fix: https://codereview.chromium.org/2338473002/

Destroy() is called directly on UI and it calls the destructor. The destructor
is marked as private.
Desktop.onDestroy() is called by android and may be called after invalidate().
Also from the standpoint of GlDisplay it is better to design it that way to
prevent possible mistake.

[Hzj_jie, 2016/09/13 18:00:13]

I just guessed Activity.onDestroy() and View.onDestroy() should be called in the
same thread without interruption. But since you have implemented a safer
solution without this assumption, I am good with it.

[Yuwei, 2016/09/13 18:21:09]

Should be now GlDisplay.invalidate() vs View.onDetachedFromWindow(). They don't
need to have causality.
Acknowledged.

     private final Event.Raisable<SizeChangedEventParameter> mOnClientSizeChanged =
             new Event.Raisable<>();
     private final Event.Raisable<SizeChangedEventParameter> mOnHostSizeChanged =
             new Event.Raisable<>();
-    private final Event.Raisable<Void> mOnCanvasRendered =
-            new Event.Raisable<>();
+    private final Event.Raisable<Void> mOnCanvasRendered = new Event.Raisable<>();
     private InputFeedbackRadiusMapper mFeedbackRadiusMapper;

[Hzj_jie, 2016/09/10 02:06:32]

Move regular fields after final fields.

[Yuwei, 2016/09/12 18:56:14]

Done. Just moving mNativeJniGlDisplay below?

[Hzj_jie, 2016/09/12 21:28:04]

Since mNativeJniGlDisplay needs to be mutable, this comment is out-of-date.

[Yuwei, 2016/09/12 21:50:04]

Acknowledged.

     private float mScaleFactor = 0;
 
     private GlDisplay(long nativeJniGlDisplay) {
         mNativeJniGlDisplay = nativeJniGlDisplay;
     }
 
     /**
      * Invalidates this object and disconnects from the native display handler. Called on the
      * display thread by the native code.
      */
@@ skipping 25 matching lines @@
         if (mNativeJniGlDisplay != 0) {
             nativeOnSurfaceChanged(mNativeJniGlDisplay, width, height);
         }
     }
 
     /**
      * Notifies the OpenGL renderer that the current surface being used is about to be destroyed.
      */
     @Override
     public void surfaceDestroyed(SurfaceHolder holder) {
         if (mNativeJniGlDisplay != 0) {

[Hzj_jie, 2016/09/10 02:06:32]

Is there any chance mNativeJniGlDisplay == 0 / nullptr? I have not found a
native caller will send nullptr for this variable.

[Yuwei, 2016/09/10 05:29:14]

invalidate()

             nativeOnSurfaceDestroyed(mNativeJniGlDisplay);
         }
     }
 
     @Override
     public void setDesktopView(DesktopView view) {
+        view.getHolder().addCallback(this);
         mFeedbackRadiusMapper = new InputFeedbackRadiusMapper(view);
     }
 
     /**
      * Sets the transformation matrix (in pixel coordinates).
      * @param matrix the transformation matrix
      */
     @Override
     public void setTransformation(Matrix matrix) {
         if (mNativeJniGlDisplay != 0) {
@@ skipping 69 matching lines @@
      * Called by native code when a render request has been done by the OpenGL renderer. This
      * will only be called when the render event callback is enabled.
      */
     @CalledByNative
     private void canvasRendered() {
         mOnCanvasRendered.raise(null);
     }
 
     @CalledByNative
     private void initializeClient(Client client) {
-        client.setDesktopViewFactory(new DesktopViewFactory() {
-            @Override
-            public DesktopView createDesktopView(Desktop desktop, Client client) {
-                return new GlDesktopView(GlDisplay.this, desktop, client);
-            }
-        });
+        client.setRenderStub(this);
     }
 
     @CalledByNative
     private static GlDisplay createJavaDisplayObject(long nativeDisplayHandler) {
         return new GlDisplay(nativeDisplayHandler);
     }
 
     private native void nativeOnSurfaceCreated(long nativeJniGlDisplayHandler, Surface surface);
     private native void nativeOnSurfaceChanged(long nativeJniGlDisplayHandler,
                                                int width, int height);
     private native void nativeOnSurfaceDestroyed(long nativeJniGlDisplayHandler);
     private native void nativeOnPixelTransformationChanged(long nativeJniGlDisplayHandler,
                                                            float[] matrix);
     private native void nativeOnCursorPixelPositionChanged(long nativeJniGlDisplayHandler,
                                                            float x, float y);
     private native void nativeOnCursorInputFeedback(long nativeJniGlDisplayHandler,
                                                     float x, float y, float diameter);
     private native void nativeOnCursorVisibilityChanged(long nativeJniGlDisplayHandler,
                                                         boolean visible);
 }