Android video rendering: Apply SurfaceTexture.getTransformationMatrix()

Android video rendering: Apply SurfaceTexture.getTransformationMatrix()

This CL applies the transformation matrix instead of assuming it is always a vertical flip. This should fix the issue where pixels at one of the edges look stretched out and repeated.

BUG=webrtc:4968, webrtc:4742
R=hbos@webrtc.org, pbos@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/529528cc36d2c34f886e34165a1635285db11b8a

[magjed_webrtc, 2015-09-08 11:56:44]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-09-08 12:00:16]

magjed@webrtc.org changed reviewers:
+ hbos@webrtc.org, pbos@webrtc.org

[magjed_webrtc, 2015-09-08 12:00:54]

The bos brothers - please take a look.

[pbos-webrtc, 2015-09-08 12:18:50]

Like a bos.

rs-lgtm but I'm really not well-versed in matrix magic, so go on hbos@' review
instead. :)

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

bos-bros unite!

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/androidte...
File talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java
(right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java:127: 1, 0,
0, 1});
I don't fully understand the matrix values in these unittests, like why the "1"
in the bottom-left corner? Just me being bad at transformation matrix math. Can
you briefly explain offline?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/RendererCommon.java (right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:69: 0,  1, 0, 1};
nit: Could make this final (even though it wouldn't make its contents const)

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:118:
adjustOrigin(matrix);
Does "adjustOrigin" have any effect on a scale matrix?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:143: }
What assumptions, if any, about |matrix| enable this method to do the
translation transform without a fullblown matrix multiplication?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java (right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:138: private
float[] layoutMatrix = new float[16];
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)?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:142: private final
Object updateLayoutLock = new Object();
What variables are guarded by this lock? Can you list them in the comment?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:190: private void
checkAdjustTextureCoords() {
Rename to checkAdjustLayoutMatrix?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:216: mirror,
videoAspectRatio, (float) displayLayout.width() / displayLayout.height());
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?

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:268:
Matrix.multiplyMM(texMatrix, 0, samplingMatrix, 0, layoutMatrix, 0);
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:16:13]

Patchset #2 (id:40001) has been deleted

[magjed_webrtc, 2015-09-08 15:23:15]

Patchset #2 (id:60001) has been deleted

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

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/androidte...
File talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java
(right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java:127: 1, 0,
0, 1});
On 2015/09/08 14:08:47, hbos wrote:
> I don't fully understand the matrix values in these unittests, like why the
"1"
> in the bottom-left corner? Just me being bad at transformation matrix math.
Can
> you briefly explain offline?

Done. Also added comment.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/RendererCommon.java (right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:69: 0,  1, 0, 1};
On 2015/09/08 14:08:47, hbos wrote:
> nit: Could make this final (even though it wouldn't make its contents const)

Done.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:118:
adjustOrigin(matrix);
On 2015/09/08 14:08:47, hbos wrote:
> Does "adjustOrigin" have any effect on a scale matrix?

Yes. Otherwise it will zoom in/out around the corner of the texture instead of
the center of the texture.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:143: }
On 2015/09/08 14:08:47, hbos wrote:
> What assumptions, if any, about |matrix| enable this method to do the
> translation transform without a fullblown matrix multiplication?

No assumptions really, this is just two inlined and simplified matrix
multiplications.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java (right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:138: private
float[] layoutMatrix = new float[16];
On 2015/09/08 14:08:47, hbos wrote:
> 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)?

True, I will default to null instead.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:142: private final
Object updateLayoutLock = new Object();
On 2015/09/08 14:08:47, hbos wrote:
> What variables are guarded by this lock? Can you list them in the comment?

Done.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:190: private void
checkAdjustTextureCoords() {
On 2015/09/08 14:08:47, hbos wrote:
> Rename to checkAdjustLayoutMatrix?

Renamed to getLayoutMatrix() and changed it a bit.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:216: mirror,
videoAspectRatio, (float) displayLayout.width() / displayLayout.height());
On 2015/09/08 14:08:47, hbos wrote:
> 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?

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.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:268:
Matrix.multiplyMM(texMatrix, 0, samplingMatrix, 0, layoutMatrix, 0);
On 2015/09/08 14:08:47, hbos wrote:
> Is reading from samplingMatrix thread-safe without synchronized on this?
> 
> Is reading from layoutMatrix thread-safe without synchronized on
> updateLayoutLock?

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]

Fix my "updateLayoutMatrix()" comment or fight about it with me in-person :)
Otherwise, lgtm.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/androidte...
File talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java
(right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java:127: 1, 0,
0, 1});
On 2015/09/08 15:24:28, magjed_webrtc wrote:
> On 2015/09/08 14:08:47, hbos wrote:
> > I don't fully understand the matrix values in these unittests, like why the
> "1"
> > in the bottom-left corner? Just me being bad at transformation matrix math.
> Can
> > you briefly explain offline?
> 
> Done. Also added comment.

Acknowledged.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/RendererCommon.java (right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:118:
adjustOrigin(matrix);
On 2015/09/08 15:24:28, magjed_webrtc wrote:
> On 2015/09/08 14:08:47, hbos wrote:
> > Does "adjustOrigin" have any effect on a scale matrix?
> 
> Yes. Otherwise it will zoom in/out around the corner of the texture instead of
> the center of the texture.

Acknowledged.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:143: }
On 2015/09/08 15:24:28, magjed_webrtc wrote:
> On 2015/09/08 14:08:47, hbos wrote:
> > What assumptions, if any, about |matrix| enable this method to do the
> > translation transform without a fullblown matrix multiplication?
> 
> No assumptions really, this is just two inlined and simplified matrix
> multiplications.

Acknowledged.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java (right):

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:138: private
float[] layoutMatrix = new float[16];
On 2015/09/08 15:24:28, magjed_webrtc wrote:
> On 2015/09/08 14:08:47, hbos wrote:
> > 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)?
> 
> True, I will default to null instead.

Acknowledged.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:216: mirror,
videoAspectRatio, (float) displayLayout.width() / displayLayout.height());
On 2015/09/08 15:24:28, magjed_webrtc wrote:
> On 2015/09/08 14:08:47, hbos wrote:
> > 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?
> 
> 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.

Acknowledged.

https://codereview.webrtc.org/1318153007/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:268:
Matrix.multiplyMM(texMatrix, 0, samplingMatrix, 0, layoutMatrix, 0);
On 2015/09/08 15:24:28, magjed_webrtc wrote:
> On 2015/09/08 14:08:47, hbos wrote:
> > Is reading from samplingMatrix thread-safe without synchronized on this?
> > 
> > Is reading from layoutMatrix thread-safe without synchronized on
> > updateLayoutLock?
> 
> Thanks, this is a bug. I moved all usage of |samplingMatrix| into this
function.
> I also should not call getTransformationMatrix() before updateTexImage().

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.

[magjed_webrtc, 2015-09-09 09:00:22]

Committed patchset #3 (id:100001) manually as
529528cc36d2c34f886e34165a1635285db11b8a (presubmit successful).