Android SurfaceViewRenderer: Don't rely on widthSpec/heightSpec after onMeasure() returns

Android SurfaceViewRenderer: Don't rely on widthSpec/heightSpec after onMeasure() returns

SurfaceViewRenderer currently stores widthSpec/heightSpec internally, and triggers requestLayout() from renderFrameOnRenderThread()->checkConsistentLayout() when it detects a change using widthSpec/heightSpec. This is not reliable, because onMeasure() might be called several times during the layout process negotiation. For example it might look like this:
-> onMeasure(at most 1920, at most 1080)
<- setMeasuredDimension(1080, 1080)
-> onMeasure(exactly 1080, exactly 1080)
<- setMeasuredDimension(1080, 1080)
Then we store (exactly 1080, exactly 1080) even though we are allowed to be bigger than this, and requestLayout() will never be triggered.

This CL moves the requestLayout() trigger to updateFrameDimensionsAndReportEvents() when the frame size changes.

Other small changes in this CL are:
* Replace with/height variables with Point.
* Add logging in updateFrameDimensionsAndReportEvents() even when rendererEvents is null.
* Use Math.round() in RendererCommon.getDisplaySize() instead of integer cast.

R=hbos@webrtc.org

Committed: https://crrev.com/4c5eea3c73e90b11bc17679d6b0943813e4c5038
Cr-Commit-Position: refs/heads/master@{#10774}

[magjed_webrtc, 2015-11-19 16:16:50]

Description was changed from

==========
Android SurfaceViewRenderer: Don't rely on widthSpec/heightSpec after
onMeasure() returns

SurfaceViewRenderer currently stores widthSpec/heightSpec internally, and
triggers requestLayout() from
renderFrameOnRenderThread()->checkConsistentLayout() when it detects a change
using widthSpec/heightSpec. This is not reliable, because onMeasure() might be
called several times during the layout process negotiation. For example it might
look like this:
-> onMeasure(at most 1920, at most 1080)
<- setMeasuredDimension(1080, 1080)
-> onMeasure(exactly 1080, exactly 1080)
<- setMeasuredDimension(1080, 1080)
Then we store (exactly 1080, exactly 1080) even though we are allowed to be
bigger than this, and requestLayout() will never be triggered.

This CL moves the requestLayout() trigger to
updateFrameDimensionsAndReportEvents() when the frame size changes.

Other small changes in this CL are:
* Replace with/height variables with Point.
* Add logging in updateFrameDimensionsAndReportEvents() even when rendererEvents
is null.
* Use Math.round() in RendererCommon.getDisplaySize() instead of integer cast.
==========

to

==========
Android SurfaceViewRenderer: Don't rely on widthSpec/heightSpec after
onMeasure() returns

SurfaceViewRenderer currently stores widthSpec/heightSpec internally, and
triggers requestLayout() from
renderFrameOnRenderThread()->checkConsistentLayout() when it detects a change
using widthSpec/heightSpec. This is not reliable, because onMeasure() might be
called several times during the layout process negotiation. For example it might
look like this:
-> onMeasure(at most 1920, at most 1080)
<- setMeasuredDimension(1080, 1080)
-> onMeasure(exactly 1080, exactly 1080)
<- setMeasuredDimension(1080, 1080)
Then we store (exactly 1080, exactly 1080) even though we are allowed to be
bigger than this, and requestLayout() will never be triggered.

This CL moves the requestLayout() trigger to
updateFrameDimensionsAndReportEvents() when the frame size changes.

Other small changes in this CL are:
* Replace with/height variables with Point.
* Add logging in updateFrameDimensionsAndReportEvents() even when rendererEvents
is null.
* Use Math.round() in RendererCommon.getDisplaySize() instead of integer cast.
==========

[magjed_webrtc, 2015-11-19 16:16:51]

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

[magjed_webrtc, 2015-11-19 16:19:02]

hbos - Please take a look.

[hbos, 2015-11-20 15:08:27]

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (left):

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:123: //
Runnable for posting frames to render thread..
I liked that second dot..

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:80: //
requestLayout() -> onMeasure() -> onLayout() -> surfaceChanged().
Optional, if you see fit: Can this by any chance be clarified?
I understand it's not a direct call chain and it's better than nothing but it's
a little bit unclear what's going on. Will every requestLayout() lead all the
way to a surfaceChanged() for example? If this is explained in the android
documentation though then you don't have to spell it out in too much detail.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:330:
renderThreadHandler.postAtFrontOfQueue(makeBlackRunnable);
With onMeasure being invoked multiple times will this cause multiple frames to
be black?

Regarding "onMeasure() -> onLayout()" comment. Does this mean we can move the
posting to onLayout() or does that add a delay to the process? (And if we move
it do we have to make sure it's only called if it changes?)

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:407: if
(eglBase.hasSurface()) {
Should this not be if (eglBase != null && eglBase.hasSurface())? release() nulls
it after release.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:423: //
requestLayout() -> onMeasure() -> onLayout() -> surfaceChanged().
nit: Confusing comment, there's no "waiting" (as in blocking).

[magjed_webrtc, 2015-11-20 15:49:25]

hbos - Please take another look.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (left):

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:123: //
Runnable for posting frames to render thread..
On 2015/11/20 15:08:26, hbos wrote:
> I liked that second dot..

Acknowledged.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:80: //
requestLayout() -> onMeasure() -> onLayout() -> surfaceChanged().
On 2015/11/20 15:08:26, hbos wrote:
> Optional, if you see fit: Can this by any chance be clarified?
> I understand it's not a direct call chain and it's better than nothing but
it's
> a little bit unclear what's going on. Will every requestLayout() lead all the
> way to a surfaceChanged() for example? If this is explained in the android
> documentation though then you don't have to spell it out in too much detail.

Done.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:330:
renderThreadHandler.postAtFrontOfQueue(makeBlackRunnable);
On 2015/11/20 15:08:26, hbos wrote:
> With onMeasure being invoked multiple times will this cause multiple frames to
> be black?
> 
> Regarding "onMeasure() -> onLayout()" comment. Does this mean we can move the
> posting to onLayout() or does that add a delay to the process? (And if we move
> it do we have to make sure it's only called if it changes?)

We only call makeBlack() if the measured dimension changes, and it's not a
problem if we happen to glClear() twice in some rare cases.

I could move it to onLayout(), but I want to post it as early as possible.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:407: if
(eglBase.hasSurface()) {
On 2015/11/20 15:08:26, hbos wrote:
> Should this not be if (eglBase != null && eglBase.hasSurface())? release()
nulls
> it after release.

Yes, done.

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:423: //
requestLayout() -> onMeasure() -> onLayout() -> surfaceChanged().
On 2015/11/20 15:08:26, hbos wrote:
> nit: Confusing comment, there's no "waiting" (as in blocking).

Done.

[hbos, 2015-11-23 08:25:14]

lgtm

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1453413005/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:330:
renderThreadHandler.postAtFrontOfQueue(makeBlackRunnable);
On 2015/11/20 15:49:25, magjed_webrtc wrote:
> On 2015/11/20 15:08:26, hbos wrote:
> > With onMeasure being invoked multiple times will this cause multiple frames
to
> > be black?
> > 
> > Regarding "onMeasure() -> onLayout()" comment. Does this mean we can move
the
> > posting to onLayout() or does that add a delay to the process? (And if we
move
> > it do we have to make sure it's only called if it changes?)
> 
> We only call makeBlack() if the measured dimension changes, and it's not a
> problem if we happen to glClear() twice in some rare cases.
> 
> I could move it to onLayout(), but I want to post it as early as possible.

Acknowledged. (I'm thinking if onMeasure is called multiple times with different
widthSpec/heightSpec you might get desiredLayoutSize != measured size between
calls of the same measure pass.)

[magjed_webrtc, 2015-11-24 15:53:02]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2015-11-24 15:53:02]

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

[commit-bot: I haz the power, 2015-11-24 15:53:13]

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

[magjed_webrtc, 2015-11-24 16:45:16]

The CQ bit was unchecked by magjed@webrtc.org

[magjed_webrtc, 2015-11-24 16:45:38]

Committed patchset #3 (id:40001) manually as
4c5eea3c73e90b11bc17679d6b0943813e4c5038 (presubmit successful).

[commit-bot: I haz the power, 2015-11-24 16:45:39]

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