Android: Refactor renderers to allow apps to inject custom shaders

Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this information may be needed by some shaders. This also moves the responsibility of calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new VideoRendererGui.create() function that takes a custom drawer as argument. Each YuvImageRenderer in VideoRendererGui now has their own drawer instead of a common one.

BUG=b/25694445
R=nisse@webrtc.org, perkj@webrtc.org

Committed: https://crrev.com/51254331ccb3838b03ed0c630f7e3d5d402d1919
Cr-Commit-Position: refs/heads/master@{#11031}

[magjed_webrtc, 2015-12-15 08:35:10]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

BUG=b/25694445
==========

[magjed_webrtc, 2015-12-15 09:16:11]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders

BUG=b/25694445
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Moves uploadYuvData() into a separate helper class.
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Add new SurfaceViewRenderer.init() function and new VideoRendererGui.create()
function that takes a custom drawer as argument. Each YuvImageRenderer in
VideoRendererGui now has their own drawer instead of a common.


BUG=b/25694445
==========

[magjed_webrtc, 2015-12-15 09:17:09]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Moves uploadYuvData() into a separate helper class.
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Add new SurfaceViewRenderer.init() function and new VideoRendererGui.create()
function that takes a custom drawer as argument. Each YuvImageRenderer in
VideoRendererGui now has their own drawer instead of a common.


BUG=b/25694445
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Add new SurfaceViewRenderer.init() function and new VideoRendererGui.create()
function that takes a custom drawer as argument. Each YuvImageRenderer in
VideoRendererGui now has their own drawer instead of a common.


BUG=b/25694445
==========

[magjed_webrtc, 2015-12-15 09:17:56]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Add new SurfaceViewRenderer.init() function and new VideoRendererGui.create()
function that takes a custom drawer as argument. Each YuvImageRenderer in
VideoRendererGui now has their own drawer instead of a common.


BUG=b/25694445
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common.


BUG=b/25694445
==========

[magjed_webrtc, 2015-12-15 09:18:23]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common.


BUG=b/25694445
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common one.

BUG=b/25694445
==========

[magjed_webrtc, 2015-12-15 09:18:37]

Patchset #3 (id:40001) has been deleted

[magjed_webrtc, 2015-12-15 09:18:44]

Patchset #1 (id:1) has been deleted

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

magjed@webrtc.org changed reviewers:
+ nisse@webrtc.org, perkj@webrtc.org

[magjed_webrtc, 2015-12-15 09:41:29]

Patchset #1 (id:20001) has been deleted

[magjed_webrtc, 2015-12-15 09:41:41]

Please take a look.

[nisse-webrtc, 2015-12-15 10:37:08]

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

https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/EglBase.java:85: EGL10.EGL_BLUE_SIZE, 8,
Rename to CONFIG_RGBA, for consistency with CONFIG_PIXEL_RGBA_BUFFER. Only
difference is the surface type. Or rename the other way around.

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

https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:89: final int
planeWidth = (i == 0) ? width : width / 2;
Really should be (width + 1)/2, and similarly where repeated below. Maybe easier
to compute it once, something like

planeWidths = new int[] { width, (width+1)/2, (width+1)/2) };

[nisse-webrtc, 2015-12-15 10:37:22]

lgtm

[magjed_webrtc, 2015-12-15 12:59:00]

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

https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/EglBase.java:85: EGL10.EGL_BLUE_SIZE, 8,
On 2015/12/15 10:37:08, nisse-webrtc wrote:
> Rename to CONFIG_RGBA, for consistency with CONFIG_PIXEL_RGBA_BUFFER. Only
> difference is the surface type. Or rename the other way around.

Done.

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

https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:89: final int
planeWidth = (i == 0) ? width : width / 2;
On 2015/12/15 10:37:08, nisse-webrtc wrote:
> Really should be (width + 1)/2, and similarly where repeated below. Maybe
easier
> to compute it once, something like
> 
> planeWidths = new int[] { width, (width+1)/2, (width+1)/2) };

Actually, both width and (width+1)/2 will be rendered slightly incorrect when
width%2 != 0. The reason is that we always sample the textures in [0,1], but
when width%2 != 0, we should adjust it to something like [0,1-1/width] for the U
and V planes. The current behaviour is to drop the last pixel for the chroma
planes and stretch them. No one will notice that the chrome planes are out of
sync by 0.5 pixels compared to the luminance plane. We can fix this in the
future, but I think it's low priority. Also, I have only moved this code in this
CL, so I won't change it here.

I changed planeWidths and planeHeights to arrays as you suggest.

[perkj_webrtc, 2015-12-15 12:59:05]

lgtm

[nisse-webrtc, 2015-12-15 13:17:35]

On 2015/12/15 12:59:00, magjed_webrtc wrote:
>
https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
> File talk/app/webrtc/java/android/org/webrtc/EglBase.java (right):
> 
>
https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
> talk/app/webrtc/java/android/org/webrtc/EglBase.java:85: EGL10.EGL_BLUE_SIZE,
8,
> On 2015/12/15 10:37:08, nisse-webrtc wrote:
> > Rename to CONFIG_RGBA, for consistency with CONFIG_PIXEL_RGBA_BUFFER. Only
> > difference is the surface type. Or rename the other way around.
> 
> Done.
> 
>
https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
> File talk/app/webrtc/java/android/org/webrtc/RendererCommon.java (right):
> 
>
https://codereview.webrtc.org/1520243003/diff/60001/talk/app/webrtc/java/andr...
> talk/app/webrtc/java/android/org/webrtc/RendererCommon.java:89: final int
> planeWidth = (i == 0) ? width : width / 2;
> On 2015/12/15 10:37:08, nisse-webrtc wrote:
> > Really should be (width + 1)/2, and similarly where repeated below. Maybe
> easier
> > to compute it once, something like
> > 
> > planeWidths = new int[] { width, (width+1)/2, (width+1)/2) };
> 
> Actually, both width and (width+1)/2 will be rendered slightly incorrect when
> width%2 != 0. The reason is that we always sample the textures in [0,1], but
> when width%2 != 0, we should adjust it to something like [0,1-1/width] for the
U
> and V planes. The current behaviour is to drop the last pixel for the chroma
> planes and stretch them. No one will notice that the chrome planes are out of
> sync by 0.5 pixels compared to the luminance plane. We can fix this in the
> future, but I think it's low priority. Also, I have only moved this code in
this
> CL, so I won't change it here.

I was a bit worried about getting off-by-one errors in the stride. But on second

look, strides are separate, so I guess that's no problem.

Sub-pixel shifts of the planes will surely be hard to notice, and it's not clear

to me what's the correct behaviour if you have, e.g, a y-width of 5 and 
a uv-width of 3.

[magjed_webrtc, 2015-12-15 14:21:40]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2015-12-15 14:21:41]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1520243003/#ps100001 (title:
"Address Nisses comments")

[commit-bot: I haz the power, 2015-12-15 14:21:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1520243003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1520243003/100001

[magjed_webrtc, 2015-12-15 15:22:42]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common one.

BUG=b/25694445
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common one.

BUG=b/25694445
R=nisse@webrtc.org, perkj@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/51254331ccb3838b03ed0c630...
==========

[magjed_webrtc, 2015-12-15 15:22:43]

Committed patchset #3 (id:100001) manually as
51254331ccb3838b03ed0c630f7e3d5d402d1919 (presubmit successful).

[commit-bot: I haz the power, 2015-12-15 15:22:44]

Description was changed from

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common one.

BUG=b/25694445
R=nisse@webrtc.org, perkj@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/51254331ccb3838b03ed0c630...
==========

to

==========
Android: Refactor renderers to allow apps to inject custom shaders

This CL:
 * Abstracts the functions in GlRectDrawer to an interface.
 * Adds viewport location as argument to the draw() functions, because this
information may be needed by some shaders. This also moves the responsibility of
calling GLES20.glViewport() to the drawer.
 * Moves uploadYuvData() into a separate helper class.
 * Adds new SurfaceViewRenderer.init() function and new
VideoRendererGui.create() function that takes a custom drawer as argument. Each
YuvImageRenderer in VideoRendererGui now has their own drawer instead of a
common one.

BUG=b/25694445
R=nisse@webrtc.org, perkj@webrtc.org

Committed: https://crrev.com/51254331ccb3838b03ed0c630f7e3d5d402d1919
Cr-Commit-Position: refs/heads/master@{#11031}
==========

[commit-bot: I haz the power, 2015-12-15 15:22:45]

Patchset 3 (id:??) landed as
https://crrev.com/51254331ccb3838b03ed0c630f7e3d5d402d1919
Cr-Commit-Position: refs/heads/master@{#11031}