Android: Add unittests for SurfaceTextureHelper

Android: Add unittests for SurfaceTextureHelper

BUG=webrtc:4993
R=hbos@webrtc.org

Committed: https://crrev.com/8e9cb09506b4a076bc097324e8b79a72d3124615
Cr-Commit-Position: refs/heads/master@{#10099}

[magjed_webrtc, 2015-09-25 13:22:56]

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

[magjed_webrtc, 2015-09-28 09:18:04]

hbos - Please take a look.

[hbos, 2015-09-28 15:08:03]

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
File talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java
(right):

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:74: }
Due to rare spurious wakeup this should technically be in a while loop checking
that we have waited long enough if hasNewFrame is still false...

You'll have to like check the time and wait(timeRemaining) as long as
timeRemaining > 1 (because wait(0) means wait indef). (You'll want make sure
wait(1+) is invoked at least once regardless of timing inprecisions so maybe
do-while.)
\o/ JAVA wait/notify! Where the correct solution looks like an ugly hack.

I don't know if spurious wakeup is an issue in practice or not but it would be a
shame if a unittest turned flakey due to it. What do you think?

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:107:
assertEquals(eglOesBase.surfaceWidth(), height);
surfaceHeight()

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:118:
eglOesBase.swapBuffers();
Here and other swapBuffers(): Add a comment saying that this ultimately triggers
onTextureFrameAvailable.

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:172:
assertEquals(eglOesBase.surfaceWidth(), height);
surfaceHeight()

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:227:
// Assert new frame yet.
Incomplete sentence

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:229:
// Draw and wait for one frame.
nit: Have an empty line of code between each "frame"?

[magjed_webrtc, 2015-09-28 15:35:16]

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
File talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java
(right):

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:74: }
On 2015/09/28 15:08:03, hbos wrote:
> Due to rare spurious wakeup this should technically be in a while loop
checking
> that we have waited long enough if hasNewFrame is still false...
> 
> You'll have to like check the time and wait(timeRemaining) as long as
> timeRemaining > 1 (because wait(0) means wait indef). (You'll want make sure
> wait(1+) is invoked at least once regardless of timing inprecisions so maybe
> do-while.)
> \o/ JAVA wait/notify! Where the correct solution looks like an ugly hack.
> 
> I don't know if spurious wakeup is an issue in practice or not but it would be
a
> shame if a unittest turned flakey due to it. What do you think?

Done, but I don't make sure wait() is invoked at least once. Why would that be
necessary?

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:107:
assertEquals(eglOesBase.surfaceWidth(), height);
On 2015/09/28 15:08:03, hbos wrote:
> surfaceHeight()

Lol, done.

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:118:
eglOesBase.swapBuffers();
On 2015/09/28 15:08:03, hbos wrote:
> Here and other swapBuffers(): Add a comment saying that this ultimately
triggers
> onTextureFrameAvailable.

Done.

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:172:
assertEquals(eglOesBase.surfaceWidth(), height);
On 2015/09/28 15:08:03, hbos wrote:
> surfaceHeight()

Lol, done.

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:227:
// Assert new frame yet.
On 2015/09/28 15:08:03, hbos wrote:
> Incomplete sentence

Lol, done.

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:229:
// Draw and wait for one frame.
On 2015/09/28 15:08:03, hbos wrote:
> nit: Have an empty line of code between each "frame"?

Done.

[hbos, 2015-09-28 15:57:33]

lgtm

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
File talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java
(right):

https://codereview.webrtc.org/1371643002/diff/20001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/SurfaceTextureHelperTest.java:74: }
On 2015/09/28 15:35:16, magjed_webrtc wrote:
> On 2015/09/28 15:08:03, hbos wrote:
> > Due to rare spurious wakeup this should technically be in a while loop
> checking
> > that we have waited long enough if hasNewFrame is still false...
> > 
> > You'll have to like check the time and wait(timeRemaining) as long as
> > timeRemaining > 1 (because wait(0) means wait indef). (You'll want make sure
> > wait(1+) is invoked at least once regardless of timing inprecisions so maybe
> > do-while.)
> > \o/ JAVA wait/notify! Where the correct solution looks like an ugly hack.
> > 
> > I don't know if spurious wakeup is an issue in practice or not but it would
be
> a
> > shame if a unittest turned flakey due to it. What do you think?
> 
> Done, but I don't make sure wait() is invoked at least once. Why would that be
> necessary?

Because brainfart.

[magjed_webrtc, 2015-09-28 21:15:30]

Committed patchset #3 (id:40001) manually as
8e9cb09506b4a076bc097324e8b79a72d3124615 (presubmit successful).

[commit-bot: I haz the power, 2015-09-28 21:15:33]

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