Android VideoRendererGui: Refactor GLES rendering

Android VideoRendererGui: Refactor GLES rendering

This CL should not change any visible behaviour. It does the following:
 * Extract GLES rendering into separate class GlRectDrawer. This class is also needed for future video encode with OES texture input.
 * Clean up current ScalingType -> display size calculation and introduce new SCALE_ASPECT_BALANCED (b/21735609) and remove unused SCALE_FILL.
 * Replace current mirror/rotation index juggling with android.opengl.Matrix operations instead.

Committed: https://crrev.com/59a677ada27c660e9cd7486f0d702753dbeb6d39
Cr-Commit-Position: refs/heads/master@{#9496}

[magjed_webrtc, 2015-06-22 19:40:46]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-06-22 20:45:09]

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

[magjed_webrtc, 2015-06-22 20:49:14]

glaznev - Please take a look.

[AlexG, 2015-06-23 22:03:36]

lgtm

Mainly nits in comments

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

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:197:
oesShader.release();
add oesShader = null;

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:200:
yuvShader.release();
Add yuvShader = null;

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

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:214: private
static Point getDisplaySize(float minVisibleFraction, float videoAspectRatio,
non necessary alignment?

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:241: (screenWidth
* layoutInPercentage.left + 99) / 100,
use +50 insead of +99 for round() instead of floor()?

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:243: (screenWidth
* layoutInPercentage.right) / 100,
+50 for round() instead of ceil()?

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:250: final Point
displaySize = getDisplaySize(minVisibleFraction, videoAspectRatio,
non necessary alignment?

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:253:
(displayLayout.height() - displaySize.y) / 2);
Log displayLayout values before and after the adjustment

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:310:
GLES20.glActiveTexture(GLES20.GL_TEXTURE0 + i);
Is binding necessary here? Yo will do similar calls in drawer.drawYuv()

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:312: int w = (i ==
0) ?
alignment? and for below lines in this loop

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:336: if
(rendererType == RendererType.RENDERER_YUV) {
Better to use frameFromQueue.yuvFrame here - you are using it in above code to
determine frame type

[magjed_webrtc, 2015-06-24 10:17:18]

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

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:197:
oesShader.release();
On 2015/06/23 22:03:36, AlexG wrote:
> add oesShader = null;

Done.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/GlRectDrawer.java:200:
yuvShader.release();
On 2015/06/23 22:03:36, AlexG wrote:
> Add yuvShader = null;

Done.

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

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:214: private
static Point getDisplaySize(float minVisibleFraction, float videoAspectRatio,
On 2015/06/23 22:03:36, AlexG wrote:
> non necessary alignment?

Done.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:223: if
(videoAspectRatio > (float) maxDisplayWidth / maxDisplayHeight) {
I realized this branching is not necessary so I removed it.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:241: (screenWidth
* layoutInPercentage.left + 99) / 100,
On 2015/06/23 22:03:36, AlexG wrote:
> use +50 insead of +99 for round() instead of floor()?

I interpret layoutInPercentage as a bounding box so I want to have e.g.
screenWidth * layoutInPercentage.left / 100.0 <= displayLayout.left &&
displayLayout.right <= screenWidth * layoutInPercentage.right / 100.0. I added a
comment for this.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:243: (screenWidth
* layoutInPercentage.right) / 100,
On 2015/06/23 22:03:36, AlexG wrote:
> +50 for round() instead of ceil()?

ditto

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:250: final Point
displaySize = getDisplaySize(minVisibleFraction, videoAspectRatio,
On 2015/06/23 22:03:36, AlexG wrote:
> non necessary alignment?

Done.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:253:
(displayLayout.height() - displaySize.y) / 2);
On 2015/06/23 22:03:36, AlexG wrote:
> Log displayLayout values before and after the adjustment

Done.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:310:
GLES20.glActiveTexture(GLES20.GL_TEXTURE0 + i);
On 2015/06/23 22:03:36, AlexG wrote:
> Is binding necessary here? Yo will do similar calls in drawer.drawYuv()

Yes, it's necessary for glTexImage2D below. I have to do it in drawer.drawYuv as
well, because this code path (frameFromQueue != null) does not happen every
time.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:312: int w = (i ==
0) ?
On 2015/06/23 22:03:36, AlexG wrote:
> alignment? and for below lines in this loop

Done.

https://codereview.webrtc.org/1191243005/diff/20001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:336: if
(rendererType == RendererType.RENDERER_YUV) {
On 2015/06/23 22:03:36, AlexG wrote:
> Better to use frameFromQueue.yuvFrame here - you are using it in above code to
> determine frame type

I agree that would be better, but frameFromQueue might be null and we are
redrawing an old frame here.

[magjed_webrtc, 2015-06-24 10:17:28]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2015-06-24 10:17:28]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1191243005/#ps40001 (title:
"glaznev@s comments")

[commit-bot: I haz the power, 2015-06-24 10:17:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191243005/40001

[commit-bot: I haz the power, 2015-06-24 10:59:41]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-06-24 10:59:50]

Patchset 2 (id:??) landed as
https://crrev.com/59a677ada27c660e9cd7486f0d702753dbeb6d39
Cr-Commit-Position: refs/heads/master@{#9496}