Android SurfaceViewRenderer: Add tests for onMeasure()

Android SurfaceViewRenderer: Add tests for onMeasure()

BUG=webrtc:4742
R=hbos@webrtc.org, perkj@webrtc.org

Committed: https://crrev.com/e76fb369825382cdd3cc136c0bc22a9b10e8caee
Cr-Commit-Position: refs/heads/master@{#10224}

[magjed_webrtc, 2015-10-03 11:12:02]

Patchset #3 (id:40001) has been deleted

[magjed_webrtc, 2015-10-03 11:12:08]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-10-03 11:12:15]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-10-04 07:33:28]

Patchset #1 (id:60001) has been deleted

[magjed_webrtc, 2015-10-05 08:22:15]

hbos - Please take a look.

[magjed_webrtc, 2015-10-06 06:06:21]

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

[magjed_webrtc, 2015-10-06 06:06:34]

hbos - Please take a look.

[hbos, 2015-10-06 08:15:17]

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java
(right):

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:52:
Arrays.asList(MeasureSpec.EXACTLY, MeasureSpec.AT_MOST);
In the below tests, why do we expect the AT_MOST mode to give the exact value of
the spec size and not <=?

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:65:
}
Remember multiple of 16 stride alignments and
VideoCapturerAndroid.CaptureFormat.frameSize... should that apply here as well
and if so can we reuse calc size code? (Even though the tests happen to already
be mul-16)

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:76:
surfaceViewRenderer.onMeasure(widthSpec, heightSpec);
I looked at onMeasure. The way it obtains and releases and then reobtains the
layoutLock, setting and getting size but not as a ONE-OP... it looks racey.

[magjed_webrtc, 2015-10-07 08:43:53]

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java
(right):

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:52:
Arrays.asList(MeasureSpec.EXACTLY, MeasureSpec.AT_MOST);
On 2015/10/06 08:15:16, hbos wrote:
> In the below tests, why do we expect the AT_MOST mode to give the exact value
of
> the spec size and not <=?

After a frame is delivered we know exactly how big SurfaceViewRenderer should
be, even for AT_MOST constraints.

Before a frame is delivered, SurfaceViewRenderer is coded to be as big as
possible, and that's what I test in testNoFrame(). That choice is a bit
arbitrary, but I think it's ok. I don't want the size to be 0x0 before a frame
arrives, because then no surface is created.

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:65:
}
On 2015/10/06 08:15:16, hbos wrote:
> Remember multiple of 16 stride alignments and
> VideoCapturerAndroid.CaptureFormat.frameSize... should that apply here as well
> and if so can we reuse calc size code? (Even though the tests happen to
already
> be mul-16)

Yeah, that's specific to the camera. We can use whatever width and stride we
want here, as long as stride >= width.

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:76:
surfaceViewRenderer.onMeasure(widthSpec, heightSpec);
On 2015/10/06 08:15:16, hbos wrote:
> I looked at onMeasure. The way it obtains and releases and then reobtains the
> layoutLock, setting and getting size but not as a ONE-OP... it looks racey.

Hmm. onMeasure() is only called from UI thread in non-test code. The lock is for
communicating with the render thread. I put the whole onMeasure() in a
synchronized(layoutLock) to be sure it's safe in both real and test code.

[hbos, 2015-10-07 09:47:09]

lgtm by addressing comment, or BY COMBAT.

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java
(right):

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:52:
Arrays.asList(MeasureSpec.EXACTLY, MeasureSpec.AT_MOST);
On 2015/10/07 08:43:52, magjed_webrtc wrote:
> On 2015/10/06 08:15:16, hbos wrote:
> > In the below tests, why do we expect the AT_MOST mode to give the exact
value
> of
> > the spec size and not <=?
> 
> After a frame is delivered we know exactly how big SurfaceViewRenderer should
> be, even for AT_MOST constraints.
> 
> Before a frame is delivered, SurfaceViewRenderer is coded to be as big as
> possible, and that's what I test in testNoFrame(). That choice is a bit
> arbitrary, but I think it's ok. I don't want the size to be 0x0 before a frame
> arrives, because then no surface is created.

Acknowledged.

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:65:
}
On 2015/10/07 08:43:53, magjed_webrtc wrote:
> On 2015/10/06 08:15:16, hbos wrote:
> > Remember multiple of 16 stride alignments and
> > VideoCapturerAndroid.CaptureFormat.frameSize... should that apply here as
well
> > and if so can we reuse calc size code? (Even though the tests happen to
> already
> > be mul-16)
> 
> Yeah, that's specific to the camera. We can use whatever width and stride we
> want here, as long as stride >= width.

Acknowledged.

https://codereview.webrtc.org/1379793003/diff/70001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:76:
surfaceViewRenderer.onMeasure(widthSpec, heightSpec);
On 2015/10/07 08:43:52, magjed_webrtc wrote:
> On 2015/10/06 08:15:16, hbos wrote:
> > I looked at onMeasure. The way it obtains and releases and then reobtains
the
> > layoutLock, setting and getting size but not as a ONE-OP... it looks racey.
> 
> Hmm. onMeasure() is only called from UI thread in non-test code. The lock is
for
> communicating with the render thread. I put the whole onMeasure() in a
> synchronized(layoutLock) to be sure it's safe in both real and test code.

Acknowledged.

https://codereview.webrtc.org/1379793003/diff/90001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java
(right):

https://codereview.webrtc.org/1379793003/diff/90001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:194:
frameDimensions, 720, 720, widthSpec, heightSpec);
Is 720x720 specifically tested for based on the value that
BALANCED_VISIBLE_FRACTION now has? (Expected height = width *
BALANCED_VISIBLE_FRACTION?) If so there is a hidden assumption about its value
and I'd prefer the expected WxH to be calculated based on
BALANCED_VISIBLE_FRACTION (here and below). Also, BALANCED_VISIBLE_FRACTION
should be final, no?

[magjed_webrtc, 2015-10-08 06:04:22]

https://codereview.webrtc.org/1379793003/diff/90001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java
(right):

https://codereview.webrtc.org/1379793003/diff/90001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceViewRendererOnMeasureTest.java:194:
frameDimensions, 720, 720, widthSpec, heightSpec);
On 2015/10/07 09:47:09, hbos wrote:
> Is 720x720 specifically tested for based on the value that
> BALANCED_VISIBLE_FRACTION now has? (Expected height = width *
> BALANCED_VISIBLE_FRACTION?) If so there is a hidden assumption about its value
> and I'd prefer the expected WxH to be calculated based on
> BALANCED_VISIBLE_FRACTION (here and below). Also, BALANCED_VISIBLE_FRACTION
> should be final, no?

True, thanks. I have updated the tests.

[hbos, 2015-10-08 08:04:26]

lgtm

[hbos, 2015-10-08 08:04:56]

On 2015/10/08 08:04:26, hbos wrote:
> lgtm

would have preferred by combat though

[magjed_webrtc, 2015-10-08 09:46:55]

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

[magjed_webrtc, 2015-10-08 09:47:58]

perkj@ - Please review VideoRenderer.java as an owner.

[perkj_webrtc, 2015-10-08 11:06:22]

https://codereview.webrtc.org/1379793003/diff/130001/talk/app/webrtc/java/src...
File talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java (right):

https://codereview.webrtc.org/1379793003/diff/130001/talk/app/webrtc/java/src...
talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java:61: public I420Frame(
use package scope if this is not intended to be used by an app.

[perkj_webrtc, 2015-10-08 13:09:24]

On 2015/10/08 11:06:22, perkj1 wrote:
>
https://codereview.webrtc.org/1379793003/diff/130001/talk/app/webrtc/java/src...
> File talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java (right):
> 
>
https://codereview.webrtc.org/1379793003/diff/130001/talk/app/webrtc/java/src...
> talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java:61: public I420Frame(
> use package scope if this is not intended to be used by an app.

lgtm

[magjed_webrtc, 2015-10-08 17:05:40]

Committed patchset #5 (id:150001) manually as
e76fb369825382cdd3cc136c0bc22a9b10e8caee (presubmit successful).

[commit-bot: I haz the power, 2015-10-08 17:05:42]

Patchset 5 (id:??) landed as
https://crrev.com/e76fb369825382cdd3cc136c0bc22a9b10e8caee
Cr-Commit-Position: refs/heads/master@{#10224}