Android video rendering: Apply SurfaceTexture.getTransformationMatrix()

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

Index: talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java
diff --git a/talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java b/talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java
index 85680073fd4d7e712a0bd23a0cb2e328413ea7be..51dce59f17ce62846a4ece288080aa625c11df58 100644
--- a/talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java
+++ b/talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java
@@ -41,6 +41,7 @@ import android.opengl.EGL14;
 import android.opengl.EGLContext;
 import android.opengl.GLES20;
 import android.opengl.GLSurfaceView;
+import android.opengl.Matrix;
 import android.util.Log;
 
 import org.webrtc.VideoRenderer.I420Frame;
@@ -133,12 +134,14 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
     // The actual view area in pixels. It is a centered subrectangle of the rectangle defined by
     // |layoutInPercentage|.
     private final Rect displayLayout = new Rect();
-    // Cached texture transformation matrix, calculated from current layout parameters.
-    private final float[] texMatrix = new float[16];
-    // Flag if texture vertices or coordinates update is needed.
-    private boolean updateTextureProperties;
+    // Cached layout transformation matrix, calculated from current layout parameters.
+    private float[] layoutMatrix = new float[16];

[hbos, 2015/09/08 14:08:47]

This probably shouldn't be used at all until it has been properly set (and could
be null). But if it does have a default value, wouldn't the identity matrix make
more sense (setIdentity in constr)?

[magjed_webrtc, 2015/09/08 15:24:28]

True, I will default to null instead.

[hbos, 2015/09/09 08:27:54]

Acknowledged.

+    // Flag if layout transformation matrix update is needed.
+    private boolean updateLayoutProperties;
     // Texture properties update lock.
-    private final Object updateTextureLock = new Object();
+    private final Object updateLayoutLock = new Object();

[hbos, 2015/09/08 14:08:47]

What variables are guarded by this lock? Can you list them in the comment?

[magjed_webrtc, 2015/09/08 15:24:28]

Done.

+    // Texture sampling matrix.
+    private float[] samplingMatrix;
     // Viewport dimensions.
     private int screenWidth;
     private int screenHeight;
@@ -160,7 +163,7 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
       this.scalingType = scalingType;
       this.mirror = mirror;
       layoutInPercentage = new Rect(x, y, Math.min(100, x + width), Math.min(100, y + height));
-      updateTextureProperties = false;
+      updateLayoutProperties = false;
       rotationDegree = 0;
     }
 
@@ -185,8 +188,8 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
     }
 
     private void checkAdjustTextureCoords() {

[hbos, 2015/09/08 14:08:47]

Rename to checkAdjustLayoutMatrix?

[magjed_webrtc, 2015/09/08 15:24:28]

Renamed to getLayoutMatrix() and changed it a bit.

-      synchronized(updateTextureLock) {
-        if (!updateTextureProperties) {
+      synchronized(updateLayoutLock) {
+        if (!updateLayoutProperties) {
           return;
         }
         // Initialize to maximum allowed area. Round to integer coordinates inwards the layout
@@ -209,9 +212,9 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
                             (displayLayout.height() - displaySize.y) / 2);
         Log.d(TAG, "  Adjusted display size: " + displayLayout.width() + " x "
             + displayLayout.height());
-        RendererCommon.getTextureMatrix(texMatrix, rotationDegree, mirror, videoAspectRatio,
-            (float) displayLayout.width() / displayLayout.height());
-        updateTextureProperties = false;
+        layoutMatrix = RendererCommon.getLayoutMatrix(
+            mirror, videoAspectRatio, (float) displayLayout.width() / displayLayout.height());

[hbos, 2015/09/08 14:08:47]

What are the implications of mirroring? I'm thinking about trying to rotate or
some other operation before or after mirroring and how that might reverse the
direction.
But the layout matrix is both mirrored and rotated in the correct order here and
then not modified?

What if we want to mirror afterwards? Or do we simply not do that?
Thinking about how we might mirror in order to avoid the back-facing camera's
default mirroring. Is this different if textures are used or not? If we want to
do the final decision about whether or not we want to mirror later does the
matrix operation order bite us in the ass?

[magjed_webrtc, 2015/09/08 15:24:28]

We need to take care when we multiply matrices together, i.e. A * B is not
always the same as B * A, but we can still do whatever we want. The final
decision about mirroring is done in the Matrix.multiplyMM() call a few lines
below.

[hbos, 2015/09/09 08:27:54]

Acknowledged.

+        updateLayoutProperties = false;
         Log.d(TAG, "  AdjustTextureCoords done");
       }
     }
@@ -261,6 +264,8 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
         }
       }
 
+      final float[] texMatrix = new float[16];
+      Matrix.multiplyMM(texMatrix, 0, samplingMatrix, 0, layoutMatrix, 0);

[hbos, 2015/09/08 14:08:47]

Is reading from samplingMatrix thread-safe without synchronized on this?

Is reading from layoutMatrix thread-safe without synchronized on
updateLayoutLock?

[magjed_webrtc, 2015/09/08 15:24:28]

Thanks, this is a bug. I moved all usage of |samplingMatrix| into this function.
I also should not call getTransformationMatrix() before updateTexImage().

[hbos, 2015/09/09 08:27:54]

You're still updating and returning a reference to layoutMatrix. We're safe
since layoutMatrix is only updated in draw() but I think the name
"getLayoutMatrix" is misleading since it is not a normal getter and the return
value is not thread-safe. What influences if the matrix is updated or not is
guarded by a lock, but the actual matrix being updated is otherwise not guarded.

In my opinion it is more clear that we are updating and dealing with a member
variable if we use the member variable directly and make the updater a "void
updateLayoutMatrix()" or similar.

       if (rendererType == RendererType.RENDERER_YUV) {
         drawer.drawYuv(yuvTextures, texMatrix);
       } else {
@@ -291,7 +296,7 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
     }
 
     public void setScreenSize(final int screenWidth, final int screenHeight) {
-      synchronized(updateTextureLock) {
+      synchronized(updateLayoutLock) {
         if (screenWidth == this.screenWidth && screenHeight == this.screenHeight) {
           return;
         }
@@ -299,7 +304,7 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
             screenWidth + " x " + screenHeight);
         this.screenWidth = screenWidth;
         this.screenHeight = screenHeight;
-        updateTextureProperties = true;
+        updateLayoutProperties = true;
       }
     }
 
@@ -307,7 +312,7 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
         RendererCommon.ScalingType scalingType, boolean mirror) {
       final Rect layoutInPercentage =
           new Rect(x, y, Math.min(100, x + width), Math.min(100, y + height));
-      synchronized(updateTextureLock) {
+      synchronized(updateLayoutLock) {
         if (layoutInPercentage.equals(this.layoutInPercentage) && scalingType == this.scalingType
             && mirror == this.mirror) {
           return;
@@ -318,7 +323,7 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
         this.layoutInPercentage.set(layoutInPercentage);
         this.scalingType = scalingType;
         this.mirror = mirror;
-        updateTextureProperties = true;
+        updateLayoutProperties = true;
       }
     }
 
@@ -333,14 +338,14 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
         rendererEvents.onFrameResolutionChanged(videoWidth, videoHeight, rotation);
       }
 
-      synchronized (updateTextureLock) {
+      synchronized (updateLayoutLock) {
         Log.d(TAG, "ID: " + id + ". YuvImageRenderer.setSize: " +
             videoWidth + " x " + videoHeight + " rotation " + rotation);
 
         this.videoWidth = videoWidth;
         this.videoHeight = videoHeight;
         rotationDegree = rotation;
-        updateTextureProperties = true;
+        updateLayoutProperties = true;
         Log.d(TAG, "  YuvImageRenderer.setSize done.");
       }
     }
@@ -378,6 +383,8 @@ public class VideoRendererGui implements GLSurfaceView.Renderer {
         }
         pendingFrame = frame;
       }
+      samplingMatrix = RendererCommon.getSamplingMatrix(
+          (SurfaceTexture) frame.textureObject, frame.rotationDegree);
       setSize(frame.width, frame.height, frame.rotationDegree);
       seenFrame = true;