Android SurfaceViewRenderer: Enable hardware scaler

talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java

Index: talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java
diff --git a/talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java b/talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java
index c9999473542bda084a7be4e3a019c361f5dba654..b0df7b914e0b4db6ce0b0024fdf1f55b56eb0a39 100644
--- a/talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java
+++ b/talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java
@@ -56,6 +56,42 @@ public class SurfaceViewRenderer extends SurfaceView
     implements SurfaceHolder.Callback, VideoRenderer.Callbacks {
   private static final String TAG = "SurfaceViewRenderer";
 
+  /**
+   * Convenience class for layout plus surface size with equals() and toString() functionality.
+   */
+  private static class LayoutConfiguration {

[hbos, 2015/11/12 11:35:47]

If two objects are considered equal in java they should produce the same
hashCode() so when overriding equals() you should generally override hashCode()
as well.
  Considering this is a private helper class not used in any hash tables or the
like I'm fine with not overriding hashCode if you don't want to bother with
that. But if you break the hashCode contract I think there should be a comment
about it above the class.

+    public int layoutWidth;
+    public int layoutHeight;
+    public int surfaceWidth;
+    public int surfaceHeight;
+
+    public LayoutConfiguration() {}
+
+    public LayoutConfiguration(
+        int layoutWidth, int layoutHeight, int surfaceWidth, int surfaceHeight) {
+      this.layoutWidth = layoutWidth;
+      this.layoutHeight = layoutHeight;
+      this.surfaceWidth = surfaceWidth;
+      this.surfaceHeight = surfaceHeight;
+    }
+
+    @Override
+    public boolean equals(Object that) {
+      if (!(that instanceof LayoutConfiguration)) {
+        return false;
+      }
+      final LayoutConfiguration lc = (LayoutConfiguration) that;
+      return layoutWidth == lc.layoutWidth && layoutHeight == lc.layoutHeight
+          && surfaceWidth == lc.surfaceWidth && surfaceHeight == lc.surfaceHeight;
+    }
+
+    @Override
+    public String toString() {
+      return "Layout: " + layoutWidth + "x" + layoutHeight
+          + ", Surface: " + surfaceWidth + "x" + surfaceHeight;
+    }
+  }
+
   // Dedicated render thread.
   private HandlerThread renderThread;
   // |renderThreadHandler| is a handler for communicating with |renderThread|, and is synchronized
@@ -76,23 +112,16 @@ public class SurfaceViewRenderer extends SurfaceView
 
   // These variables are synchronized on |layoutLock|.
   private final Object layoutLock = new Object();
-  // These three different dimension values are used to keep track of the state in these functions:
-  // requestLayout() -> onMeasure() -> onLayout() -> surfaceChanged().
-  // requestLayout() is triggered internally by frame size changes, but can also be triggered
-  // externally by layout update requests.
-  // Most recent measurement specification from onMeasure().
+  // |widthSpec|/|heightSpec| is the most recent measurement specification from onMeasure().
   private int widthSpec;
   private int heightSpec;
-  // Current size on screen in pixels. Updated in onLayout(), and should be consistent with
-  // |widthSpec|/|heightSpec| after that.
-  private int layoutWidth;
-  private int layoutHeight;
-  // Current surface size of the underlying Surface. Updated in surfaceChanged(), and should be
-  // consistent with |layoutWidth|/|layoutHeight| after that.
-  // TODO(magjed): Enable hardware scaler with SurfaceHolder.setFixedSize(). This will decouple
-  // layout and surface size.
-  private int surfaceWidth;
-  private int surfaceHeight;
+  // |isBlack| is true if the current surface is completely black. Changing layout aspect ratio or
+  // surface size is only allowed while the surface is black to avoid render artifacts.
+  private boolean isBlack = true;
+  // |desiredConfig| is the layout configuration we want and are waiting for.
+  private LayoutConfiguration desiredConfig;
+  // |currentConfig| is the actual current layout configuration.
+  private final LayoutConfiguration currentConfig = new LayoutConfiguration();
   // |isSurfaceCreated| keeps track of the current status in surfaceCreated()/surfaceDestroyed().
   private boolean isSurfaceCreated;
   // Last rendered frame dimensions, or 0 if no frame has been rendered yet.
@@ -296,20 +325,46 @@ public class SurfaceViewRenderer extends SurfaceView
   }
 
   // Returns desired layout size given current measure specification and video aspect ratio.
-  private Point getDesiredLayoutSize() {
-    synchronized (layoutLock) {
-      final int maxWidth = getDefaultSize(Integer.MAX_VALUE, widthSpec);
-      final int maxHeight = getDefaultSize(Integer.MAX_VALUE, heightSpec);
-      final Point size =
-          RendererCommon.getDisplaySize(scalingType, frameAspectRatio(), maxWidth, maxHeight);
-      if (MeasureSpec.getMode(widthSpec) == MeasureSpec.EXACTLY) {
-        size.x = maxWidth;
-      }
-      if (MeasureSpec.getMode(heightSpec) == MeasureSpec.EXACTLY) {
-        size.y = maxHeight;
-      }
-      return size;
+  private static Point getDesiredLayoutSize(
+      int widthSpec, int heightSpec,
+      RendererCommon.ScalingType scalingType, float frameAspectRatio) {
+    final int maxWidth = getDefaultSize(Integer.MAX_VALUE, widthSpec);
+    final int maxHeight = getDefaultSize(Integer.MAX_VALUE, heightSpec);
+    final Point size =
+        RendererCommon.getDisplaySize(scalingType, frameAspectRatio, maxWidth, maxHeight);
+    if (MeasureSpec.getMode(widthSpec) == MeasureSpec.EXACTLY) {
+      size.x = maxWidth;
+    }
+    if (MeasureSpec.getMode(heightSpec) == MeasureSpec.EXACTLY) {
+      size.y = maxHeight;
     }

[hbos, 2015/11/12 11:35:47]

What about MeasureSpec.AT_MOST? What if X is restricted but not Y, will it be
stretched/squished?

+    return size;
+  }
+
+  /**
+   * Calculate desired LayoutConfiguration based on measure specification, scaling type,
+   * and frame size.
+   */
+  private static LayoutConfiguration getDesiredLayoutConfiguration(int widthSpec, int heightSpec,
+      RendererCommon.ScalingType scalingType, int frameWidth, int frameHeight) {
+    final Point layoutSize = getDesiredLayoutSize(
+        widthSpec, heightSpec, scalingType, (float) frameWidth / frameHeight);
+    // Calculate at what scale we are rendering the frame.
+    final float displayScale =
+        Math.max((float) layoutSize.x / frameWidth, (float) layoutSize.y / frameHeight);

[hbos, 2015/11/12 11:35:47]

Do we always want the max scale? Thinking about black borders vs scale-to-fill
etc.

+    final int surfaceWidth;
+    final int surfaceHeight;
+    if (displayScale > 1) {
+      // Upscaling - decrease surface size and let the HW scaler take care of the upscaling
+      // instead of the GPU.
+      surfaceWidth = Math.round(layoutSize.x / displayScale);
+      surfaceHeight = Math.round(layoutSize.y / displayScale);

[hbos, 2015/11/12 11:35:47]

How does the HW scaler know to take care of the upscaling?

+    } else {
+      // Downscaling - render at layout resolution.
+      surfaceWidth = layoutSize.x;
+      surfaceHeight = layoutSize.y;
+    }
+    return new LayoutConfiguration(layoutSize.x, layoutSize.y, surfaceWidth, surfaceHeight);
   }
 
   // View layout interface.
@@ -318,7 +373,19 @@ public class SurfaceViewRenderer extends SurfaceView
     synchronized (layoutLock) {
       this.widthSpec = widthSpec;
       this.heightSpec = heightSpec;
-      final Point size = getDesiredLayoutSize();
+
+      final RendererCommon.ScalingType scalingType;
+      final float aspectRatio;
+      if (isBlack) {
+        // Unconstrained layout change - update to latest.
+        scalingType = this.scalingType;
+        aspectRatio = frameAspectRatio();
+      } else {
+        // Lock aspect of currently rendered frame with |SCALE_ASPECT_FIT|.
+        scalingType = RendererCommon.ScalingType.SCALE_ASPECT_FIT;
+        aspectRatio = (float) currentConfig.surfaceWidth / currentConfig.surfaceHeight;
+      }
+      final Point size = getDesiredLayoutSize(widthSpec, heightSpec, scalingType, aspectRatio);
       setMeasuredDimension(size.x, size.y);
     }
   }
@@ -326,8 +393,8 @@ public class SurfaceViewRenderer extends SurfaceView
   @Override
   protected void onLayout(boolean changed, int left, int top, int right, int bottom) {
     synchronized (layoutLock) {
-      layoutWidth = right - left;
-      layoutHeight = bottom - top;
+      currentConfig.layoutWidth = right - left;
+      currentConfig.layoutHeight = bottom - top;
     }
     // Might have a pending frame waiting for a layout of correct size.
     runOnRenderThread(renderFrameRunnable);
@@ -348,8 +415,8 @@ public class SurfaceViewRenderer extends SurfaceView
     Logging.d(TAG, getResourceName() + "Surface destroyed.");
     synchronized (layoutLock) {
       isSurfaceCreated = false;
-      surfaceWidth = 0;
-      surfaceHeight = 0;
+      currentConfig.surfaceWidth = 0;
+      currentConfig.surfaceHeight = 0;
     }
     runOnRenderThread(new Runnable() {
       @Override public void run() {
@@ -362,8 +429,8 @@ public class SurfaceViewRenderer extends SurfaceView
   public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
     Logging.d(TAG, getResourceName() + "Surface changed: " + width + "x" + height);
     synchronized (layoutLock) {
-      surfaceWidth = width;
-      surfaceHeight = height;
+      currentConfig.surfaceWidth = width;
+      currentConfig.surfaceHeight = height;
     }
     // Might have a pending frame waiting for a surface of correct size.
     runOnRenderThread(renderFrameRunnable);
@@ -395,6 +462,7 @@ public class SurfaceViewRenderer extends SurfaceView
     GLES20.glClearColor(0, 0, 0, 0);
     GLES20.glClear(GLES20.GL_COLOR_BUFFER_BIT);
     eglBase.swapBuffers();
+    isBlack = true;
   }
 
   /**
@@ -402,21 +470,26 @@ public class SurfaceViewRenderer extends SurfaceView
    */
   private boolean checkConsistentLayout() {

[hbos, 2015/11/12 11:35:47]

if (Thread.currentThread() != renderThread) throw exception

     synchronized (layoutLock) {
-      final Point desiredLayoutSize = getDesiredLayoutSize();
-      if (desiredLayoutSize.x != layoutWidth || desiredLayoutSize.y != layoutHeight) {
-        Logging.d(TAG, getResourceName() + "Requesting new layout with size: "
-            + desiredLayoutSize.x + "x" + desiredLayoutSize.y);
+      final int rotatedFrameWidth = (frameRotation % 180 == 0) ? frameWidth : frameHeight;
+      final int rotatedFrameHeight = (frameRotation % 180 == 0) ? frameHeight : frameWidth;
+      final LayoutConfiguration newDesiredConfig = getDesiredLayoutConfiguration(
+          widthSpec, heightSpec, scalingType, rotatedFrameWidth, rotatedFrameHeight);
+      if (!newDesiredConfig.equals(this.desiredConfig)) {
+        Logging.d(TAG, getResourceName() + "Requesting new config: " + newDesiredConfig);
+        this.desiredConfig = newDesiredConfig;
+        // Output intermediate black frame while the layout is updated.
+        makeBlack();
         // Request layout update on UI thread.
         post(new Runnable() {
           @Override public void run() {
+            getHolder().setFixedSize(newDesiredConfig.surfaceWidth, newDesiredConfig.surfaceHeight);
+            // requestLayout() triggers onMeasure() -> onLayout().
             requestLayout();
           }
         });
         return false;
       }
-      // Wait for requestLayout() to propagate through this sequence before returning true:
-      // requestLayout() -> onMeasure() -> onLayout() -> surfaceChanged().
-      return surfaceWidth == layoutWidth && surfaceHeight == layoutHeight;
+      return desiredConfig.equals(currentConfig);
     }
   }
 
@@ -431,16 +504,21 @@ public class SurfaceViewRenderer extends SurfaceView
       Logging.d(TAG, getResourceName() + "No surface to draw on");
       return;
     }
+    synchronized (frameLock) {
+      if (pendingFrame == null) {
+        return;
+      }
+    }
     if (!checkConsistentLayout()) {
-      // Output intermediate black frames while the layout is updated.
-      makeBlack();
+      // Wait until layout update is done.
       return;
     }
     // After a surface size change, the EGLSurface might still have a buffer of the old size in the
     // pipeline. Querying the EGLSurface will show if the underlying buffer dimensions haven't yet
     // changed. Such a buffer will be rendered incorrectly, so flush it with a black frame.
     synchronized (layoutLock) {
-      if (eglBase.surfaceWidth() != surfaceWidth || eglBase.surfaceHeight() != surfaceHeight) {
+      if (eglBase.surfaceWidth() != currentConfig.surfaceWidth
+          || eglBase.surfaceHeight() != currentConfig.surfaceHeight) {
         makeBlack();
       }
     }
@@ -459,12 +537,14 @@ public class SurfaceViewRenderer extends SurfaceView
     synchronized (layoutLock) {
       final float[] rotatedSamplingMatrix =
           RendererCommon.rotateTextureMatrix(frame.samplingMatrix, frame.rotationDegree);
-      final float[] layoutMatrix = RendererCommon.getLayoutMatrix(
-          mirror, frameAspectRatio(), (float) layoutWidth / layoutHeight);
+      final float[] layoutMatrix = RendererCommon.getLayoutMatrix(mirror, frameAspectRatio(),
+          (float) currentConfig.surfaceWidth / currentConfig.surfaceHeight);
       texMatrix = RendererCommon.multiplyMatrices(rotatedSamplingMatrix, layoutMatrix);
+      isBlack = false;
     }
 
-    GLES20.glViewport(0, 0, surfaceWidth, surfaceHeight);
+    GLES20.glViewport(0, 0, currentConfig.surfaceWidth, currentConfig.surfaceHeight);
+    GLES20.glClear(GLES20.GL_COLOR_BUFFER_BIT);
     if (frame.yuvFrame) {
       // Make sure YUV textures are allocated.
       if (yuvTextures == null) {