Android: Extend functionality of EglRenderer

Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470
Committed: https://crrev.com/9ab8a1884dc8326e2eb401cc30d35ca19f9b943f
Cr-Commit-Position: refs/heads/master@{#14698}

[magjed_webrtc, 2016-10-19 08:57:23]

Description was changed from

==========
Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470
==========

to

==========
Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470
==========

[magjed_webrtc, 2016-10-19 08:57:23]

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

[magjed_webrtc, 2016-10-19 08:57:35]

Sami - please take a look.

[sakal, 2016-10-19 10:52:47]

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:119: if
(renderThreadHandler != null) {
synchronized (handlerLock)?

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:162: if (sharedContext
== null) {
It is unclear to me why this code is needed. Can you please add a comment?

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:171:
postToRenderThread(eglSurfaceCreationRunnable);
Do we need to call eglSurfaceCreationRunnable.setSurface? Can we use
createEglSurface instead?

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:202:
renderThreadHandler.removeCallbacks(logStatisticsRunnable);
nit: Can you move this below renderThreadHandler = null?

It doesn't really matter because of the lock but it makes code clearer to me.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:307: // if (fps ==
Float.POSITIVE_INFINITY) {
Please remove commented out code.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:350: lastFrameTimeNs =
currentTimeNs;
With this behavior, if camera is running at 4 fps and we request 3 fps, we get 2
fps. Maybe we should change this?

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:516:
resetStatistics(currentTimeNs);
Also save statistics for first frame?

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:534: (framesRendered *
TimeUnit.SECONDS.toNanos(1) + elapsedTimeNs / 2) / elapsedTimeNs;
Can we just use Math.round, this is very confusing. Or maybe fps should be a
floating point number instead.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:538: +
TimeUnit.NANOSECONDS.toMicros(renderTimeNs / framesRendered) + " us, "
Would using μs cause problems?

[magjed_webrtc, 2016-10-19 12:15:38]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2016-10-19 12:19:07]

Patchset #2 (id:40001) has been deleted

[magjed_webrtc, 2016-10-19 12:21:05]

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:119: if
(renderThreadHandler != null) {
On 2016/10/19 10:52:47, sakal wrote:
> synchronized (handlerLock)?

Done.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:162: if (sharedContext
== null) {
On 2016/10/19 10:52:47, sakal wrote:
> It is unclear to me why this code is needed. Can you please add a comment?

Done.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:171:
postToRenderThread(eglSurfaceCreationRunnable);
On 2016/10/19 10:52:47, sakal wrote:
> Do we need to call eglSurfaceCreationRunnable.setSurface? Can we use
> createEglSurface instead?

No, the only place we get the Surface from is the argument in createEglSurface,
so that's the only place we call setSurface. The Surface is then stored in
eglSurfaceCreationRunnable. The reason for posting eglSurfaceCreationRunnable
here is that we allow the order to be both:
eglRenderer.setSurface(surface);
eglRenderer.init(...);
and
eglRenderer.init(...);
eglRenderer.setSurface(surface);

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:202:
renderThreadHandler.removeCallbacks(logStatisticsRunnable);
On 2016/10/19 10:52:47, sakal wrote:
> nit: Can you move this below renderThreadHandler = null?
> 
> It doesn't really matter because of the lock but it makes code clearer to me.

If I move this below renderThreadHandler = null then it will obviously give a
null pointer exception o_O.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:307: // if (fps ==
Float.POSITIVE_INFINITY) {
On 2016/10/19 10:52:47, sakal wrote:
> Please remove commented out code.

Ops, done. It was a local experiment if TimeUnit.SECONDS.toNanos(1) /
Float.POSITIVE_INFINITY would result in 0, which it does.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:350: lastFrameTimeNs =
currentTimeNs;
On 2016/10/19 10:52:47, sakal wrote:
> With this behavior, if camera is running at 4 fps and we request 3 fps, we get
2
> fps. Maybe we should change this?

Done.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:516:
resetStatistics(currentTimeNs);
On 2016/10/19 10:52:47, sakal wrote:
> Also save statistics for first frame?

Done.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:534: (framesRendered *
TimeUnit.SECONDS.toNanos(1) + elapsedTimeNs / 2) / elapsedTimeNs;
On 2016/10/19 10:52:47, sakal wrote:
> Can we just use Math.round, this is very confusing. Or maybe fps should be a
> floating point number instead.

Done, I rewrote the logging.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:538: +
TimeUnit.NANOSECONDS.toMicros(renderTimeNs / framesRendered) + " us, "
On 2016/10/19 10:52:47, sakal wrote:
> Would using μs cause problems?

It seems to work.

[sakal, 2016-10-19 13:24:05]

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:171:
postToRenderThread(eglSurfaceCreationRunnable);
On 2016/10/19 12:21:05, magjed_webrtc wrote:
> On 2016/10/19 10:52:47, sakal wrote:
> > Do we need to call eglSurfaceCreationRunnable.setSurface? Can we use
> > createEglSurface instead?
> 
> No, the only place we get the Surface from is the argument in
createEglSurface,
> so that's the only place we call setSurface. The Surface is then stored in
> eglSurfaceCreationRunnable. The reason for posting eglSurfaceCreationRunnable
> here is that we allow the order to be both:
> eglRenderer.setSurface(surface);
> eglRenderer.init(...);
> and
> eglRenderer.init(...);
> eglRenderer.setSurface(surface);

Acknowledged.

https://codereview.webrtc.org/2428933002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:202:
renderThreadHandler.removeCallbacks(logStatisticsRunnable);
On 2016/10/19 12:21:05, magjed_webrtc wrote:
> On 2016/10/19 10:52:47, sakal wrote:
> > nit: Can you move this below renderThreadHandler = null?
> > 
> > It doesn't really matter because of the lock but it makes code clearer to
me.
> 
> If I move this below renderThreadHandler = null then it will obviously give a
> null pointer exception o_O.

Sorry, didn't consider that.

https://codereview.webrtc.org/2428933002/diff/60001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2428933002/diff/60001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:119: synchronized
(renderThreadHandler) {
renderThreadHandler -> handlerLock

[magjed_webrtc, 2016-10-19 13:31:14]

https://codereview.webrtc.org/2428933002/diff/60001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/EglRenderer.java (right):

https://codereview.webrtc.org/2428933002/diff/60001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/EglRenderer.java:119: synchronized
(renderThreadHandler) {
On 2016/10/19 13:24:05, sakal wrote:
> renderThreadHandler -> handlerLock

Ops, thanks.

[sakal, 2016-10-19 13:56:46]

lgtm

[magjed_webrtc, 2016-10-19 14:02:00]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-10-19 14:02:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-19 16:02:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 16:02:43]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[magjed_webrtc, 2016-10-20 08:53:35]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-10-20 08:53:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-20 10:18:11]

Description was changed from

==========
Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470
==========

to

==========
Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470
==========

[commit-bot: I haz the power, 2016-10-20 10:18:12]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-10-20 10:18:21]

Description was changed from

==========
Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470
==========

to

==========
Android: Extend functionality of EglRenderer

The purpose is to prepare for a TextureViewRenderer that will need the
new functionality.

The new functionality is:
 * Be able to create an EglRenderer using a SurfaceTexture.
 * Fps reduction logic.
 * Log statistics every 4 seconds regardless of framerate.
 * Include swap buffer time in statistics.
 * Use EglBase10 if texture frames are disabled.
 * Function for printing stack trace of render thread.
 * Public clearImage() function for clearing the EGLSurface.

BUG=webrtc:6470

Committed: https://crrev.com/9ab8a1884dc8326e2eb401cc30d35ca19f9b943f
Cr-Commit-Position: refs/heads/master@{#14698}
==========

[commit-bot: I haz the power, 2016-10-20 10:18:22]

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