Android RendererCommon: Refactor getSamplingMatrix()

Android RendererCommon: Refactor getSamplingMatrix()

This CL refactors RendererCommon.getSamplingMatrix() so it does not have any dependecy to SurfaceTeture. The purpose is to prepare for a change in how texture frames are represented - only the texture matrix will be exposed, not the SurfaceTexture itself. This CL also adds an extra test for RendererCommon.rotateTextureMatrix().

R=hbos@webrtc.org

Committed: https://crrev.com/27551c95744be6e888652b3292b4130cc804f59f
Cr-Commit-Position: refs/heads/master@{#10118}

[magjed_webrtc, 2015-09-29 09:01:40]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-09-29 09:02:14]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-09-29 09:02:24]

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

[magjed_webrtc, 2015-09-29 09:02:34]

hbos - Please take a look.

[hbos, 2015-09-29 15:17:31]

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

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java:177: // v' =
u.
If (0,0) is bottom-left this is a counter-clockwise rotation (as expected), but
if top-left it is clockwise.
You and
http://www.learnopengles.com/android-lesson-four-introducing-basic-texturing/
suggests bottom-left so I guess it is correct.

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:394: // For
ByteBuffers, row 0 specifies the top row, but for a texture, row 0 specifies the
wtf really? :P

Oh, but I assume the ByteBuffer in question is frame.yuvPlanes and that this is
transferred to the texture in glTexImage2D @ GlRectDrawer.uploadYuvData?
Seeing patterns: Could this be because in normal OpenGL land, with plain ol'
OpenGL functions like glTexImage.., (0,0) is *in fact* top-left and not
bottom-left? But in the land of Android/SurfaceTexture they made it
up-side-down? Tin foil hat.

You know, if this is why there is a discrepancy, would it not be worth
considering changing uploadYuvData to create the texture in the same
up-side-down format as SurfaceTexture, and that way all the lovely rendering
code, old and new, could render without if-else's like this one?
Could that be done without copying the buffer? Or do we want to if-else because
matrix flip is cheaper than pixel copy?

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

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:259: "Unexpected
texture object type: " + pendingFrame.textureObject.getClass());
If textureObject is null the instanceof check will be false and this will throw
a NullPointerException instead (without the message).

[magjed_webrtc, 2015-09-30 14:52:52]

Patchset #3 (id:30008) has been deleted

[magjed_webrtc, 2015-09-30 14:52:59]

Patchset #3 (id:90001) has been deleted

[magjed_webrtc, 2015-09-30 15:15:19]

hbos - Please take another look. I also changed a lot of incorrect uses of
assertEquals() - the expected value should be the first argument, not last.

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

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/RendererCommonTest.java:177: // v' =
u.
On 2015/09/29 15:17:30, hbos wrote:
> If (0,0) is bottom-left this is a counter-clockwise rotation (as expected),
but
> if top-left it is clockwise.
> You and
> http://www.learnopengles.com/android-lesson-four-introducing-basic-texturing/
> suggests bottom-left so I guess it is correct.

Yes, (0, 0) is bottom-left for textures. rotateTextureMatrix() will have the
effect of rotating the rendered image clockwise. This is not obvious, because it
is not the vertex coordinates we transform, but how we sample from the matrix.

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:394: // For
ByteBuffers, row 0 specifies the top row, but for a texture, row 0 specifies the
On 2015/09/29 15:17:30, hbos wrote:
> wtf really? :P
> 
> Oh, but I assume the ByteBuffer in question is frame.yuvPlanes and that this
is
> transferred to the texture in glTexImage2D @ GlRectDrawer.uploadYuvData?
> Seeing patterns: Could this be because in normal OpenGL land, with plain ol'
> OpenGL functions like glTexImage.., (0,0) is *in fact* top-left and not
> bottom-left? But in the land of Android/SurfaceTexture they made it
> up-side-down? Tin foil hat.
> 
> You know, if this is why there is a discrepancy, would it not be worth
> considering changing uploadYuvData to create the texture in the same
> up-side-down format as SurfaceTexture, and that way all the lovely rendering
> code, old and new, could render without if-else's like this one?
> Could that be done without copying the buffer? Or do we want to if-else
because
> matrix flip is cheaper than pixel copy?

I have confirmed that this is the case for all OpenGL. The official
documentation of glTexImage2D()
https://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexImage2D.xml clearly
say:
"The first element corresponds to the lower left corner of the texture image.
Subsequent elements progress left-to-right through the remaining texels in the
lowest row of the texture image, and then in successively higher rows of the
texture image. The final element corresponds to the upper right corner of the
texture image."
We don't want to make a pixel copy, so correcting with a sampling matrix is a
good solution I think.

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

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:259: "Unexpected
texture object type: " + pendingFrame.textureObject.getClass());
On 2015/09/29 15:17:30, hbos wrote:
> If textureObject is null the instanceof check will be false and this will
throw
> a NullPointerException instead (without the message).

Problem solved.

[hbos, 2015-09-30 16:03:58]

lgtm

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1375593002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:394: // For
ByteBuffers, row 0 specifies the top row, but for a texture, row 0 specifies the
On 2015/09/30 15:15:19, magjed_webrtc wrote:
> On 2015/09/29 15:17:30, hbos wrote:
> > wtf really? :P
> > 
> > Oh, but I assume the ByteBuffer in question is frame.yuvPlanes and that this
> is
> > transferred to the texture in glTexImage2D @ GlRectDrawer.uploadYuvData?
> > Seeing patterns: Could this be because in normal OpenGL land, with plain ol'
> > OpenGL functions like glTexImage.., (0,0) is *in fact* top-left and not
> > bottom-left? But in the land of Android/SurfaceTexture they made it
> > up-side-down? Tin foil hat.
> > 
> > You know, if this is why there is a discrepancy, would it not be worth
> > considering changing uploadYuvData to create the texture in the same
> > up-side-down format as SurfaceTexture, and that way all the lovely rendering
> > code, old and new, could render without if-else's like this one?
> > Could that be done without copying the buffer? Or do we want to if-else
> because
> > matrix flip is cheaper than pixel copy?
> 
> I have confirmed that this is the case for all OpenGL. The official
> documentation of glTexImage2D()
> https://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexImage2D.xml clearly
> say:
> "The first element corresponds to the lower left corner of the texture image.
> Subsequent elements progress left-to-right through the remaining texels in the
> lowest row of the texture image, and then in successively higher rows of the
> texture image. The final element corresponds to the upper right corner of the
> texture image."
> We don't want to make a pixel copy, so correcting with a sampling matrix is a
> good solution I think.

Agreed! Suddenly everything makes sense.

https://codereview.webrtc.org/1375593002/diff/110001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/RendererCommon.java (right):

https://codereview.webrtc.org/1375593002/diff/110001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:31: import
android.graphics.SurfaceTexture;
Unnecessary import.

[magjed_webrtc, 2015-09-30 16:11:40]

https://codereview.webrtc.org/1375593002/diff/110001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/RendererCommon.java (right):

https://codereview.webrtc.org/1375593002/diff/110001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:31: import
android.graphics.SurfaceTexture;
On 2015/09/30 16:03:58, hbos wrote:
> Unnecessary import.

Nice find, thanks.

[magjed_webrtc, 2015-09-30 16:23:43]

Patchset #4 (id:130001) has been deleted

[commit-bot: I haz the power, 2015-09-30 16:25:36]

Patchset 4 (id:??) landed as
https://crrev.com/27551c95744be6e888652b3292b4130cc804f59f
Cr-Commit-Position: refs/heads/master@{#10118}

[magjed_webrtc, 2015-09-30 16:25:36]

Committed patchset #4 (id:150001) manually as
27551c95744be6e888652b3292b4130cc804f59f (presubmit successful).