Android: Add SurfaceTextureHelper for creating and managing SurfaceTextures

Android: Add SurfaceTextureHelper for creating and managing SurfaceTextures

Add new helper class to create and synchronize access to SurfaceTextures. The plan is replace the SurfaceTexture in MediaCodecVideoDecoder in a follow-up CL and remove the SurfaceTexture.updateTexImage() call in VideoRendererGui.

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

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

[magjed_webrtc, 2015-09-14 09:36:42]

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

[magjed_webrtc, 2015-09-14 09:36:53]

hbos - Please take a look.

[hbos, 2015-09-14 11:53:46]

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: return;
Optional, stats: How about logging and/or increment a counter when a frame is
dropped?

You would have to watch out for tryDeliverTextureFrame being called multiple
times for the same frame in the case of a texture being delivered before
returnTextureFrame. And if failed to deliver the first time it could still
succeed in delivering the second time. But the second invocation might otherwise
be the next frame. So adding this would be tricker than it would first seem,
thus optional.

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:158:
eglBase.makeCurrent();
I'm guessing/hoping makeCurrent is cheap if already current. Otherwise: do we
need to do this in every tryDeliverTextureFrame?

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:164: :
TimeUnit.MILLISECONDS.toNanos(SystemClock.elapsedRealtime());
Why different timestamps on different versions?
There's a SystemClock.elapsedRealtimeNanos().

[magjed_webrtc, 2015-09-14 15:07:48]

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: return;
On 2015/09/14 11:53:46, hbos wrote:
> Optional, stats: How about logging and/or increment a counter when a frame is
> dropped?
> 
> You would have to watch out for tryDeliverTextureFrame being called multiple
> times for the same frame in the case of a texture being delivered before
> returnTextureFrame. And if failed to deliver the first time it could still
> succeed in delivering the second time. But the second invocation might
otherwise
> be the next frame. So adding this would be tricker than it would first seem,
> thus optional.

We never drop frames, at least we can't see them being dropped. We will not
receive new onFrameAvailable() until updateTexImage() is called. If a texture is
delivered in onFrameAvailable() before returnTextureFrame(), we will not drop
the frame, we will wait for returnTextureFrame() and then deliver the new frame.

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:158:
eglBase.makeCurrent();
On 2015/09/14 11:53:46, hbos wrote:
> I'm guessing/hoping makeCurrent is cheap if already current. Otherwise: do we
> need to do this in every tryDeliverTextureFrame?

I was also hoping/guessing it's cheap if it's already current. It is most robust
this way, because the onTextureFrameAvailable() receiver can potentially make
another context current on this thread. Anyway, I changed to set it once and for
all in the ctor and added a comment for the interface to NOT make another
context current.

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:164: :
TimeUnit.MILLISECONDS.toNanos(SystemClock.elapsedRealtime());
On 2015/09/14 11:53:46, hbos wrote:
> Why different timestamps on different versions?
> There's a SystemClock.elapsedRealtimeNanos().

SurfaceTexture.getTimestamp() was added in API level 14 (ICE_CREAM_SANDWICH),
and SystemClock.elapsedRealtimeNanos() was added in API level 17.
TimeUnit.toNanos() and SystemClock.elapsedRealtime() was added in API level 1.
This way, we get a well defined timestamp for all API levels.

[hbos, 2015-09-14 15:29:27]

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: return;
On 2015/09/14 15:07:48, magjed_webrtc wrote:
> On 2015/09/14 11:53:46, hbos wrote:
> > Optional, stats: How about logging and/or increment a counter when a frame
is
> > dropped?
> > 
> > You would have to watch out for tryDeliverTextureFrame being called multiple
> > times for the same frame in the case of a texture being delivered before
> > returnTextureFrame. And if failed to deliver the first time it could still
> > succeed in delivering the second time. But the second invocation might
> otherwise
> > be the next frame. So adding this would be tricker than it would first seem,
> > thus optional.
> 
> We never drop frames, at least we can't see them being dropped. We will not
> receive new onFrameAvailable() until updateTexImage() is called. If a texture
is
> delivered in onFrameAvailable() before returnTextureFrame(), we will not drop
> the frame, we will wait for returnTextureFrame() and then deliver the new
frame.

Ok OK. I thought onFrameAvailable() would be called periodically regardless of
updateTexImage().

Hmmm... Let's say X is the time between one updateTexImage() and the subsequent
onFrameAvailable(). Even though there are no *deliverable* frames being dropped,
if you're slower than X at returning a frame you would be throttling the next
frame after that, right? So this would negatively affect the framerate, correct?
So you would indirectly be losing "frames", but the measurable loss would be in
time not spent waiting for the next frame instead of number of ready frames
dropped.

It might still make sense to log "slower than X frame borrows" as a warning for
slow rendering. Or maybe not? What do you think?

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:158:
eglBase.makeCurrent();
On 2015/09/14 15:07:48, magjed_webrtc wrote:
> On 2015/09/14 11:53:46, hbos wrote:
> > I'm guessing/hoping makeCurrent is cheap if already current. Otherwise: do
we
> > need to do this in every tryDeliverTextureFrame?
> 
> I was also hoping/guessing it's cheap if it's already current. It is most
robust
> this way, because the onTextureFrameAvailable() receiver can potentially make
> another context current on this thread. Anyway, I changed to set it once and
for
> all in the ctor and added a comment for the interface to NOT make another
> context current.

Acknowledged.

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:164: :
TimeUnit.MILLISECONDS.toNanos(SystemClock.elapsedRealtime());
On 2015/09/14 15:07:48, magjed_webrtc wrote:
> On 2015/09/14 11:53:46, hbos wrote:
> > Why different timestamps on different versions?
> > There's a SystemClock.elapsedRealtimeNanos().
> 
> SurfaceTexture.getTimestamp() was added in API level 14 (ICE_CREAM_SANDWICH),
> and SystemClock.elapsedRealtimeNanos() was added in API level 17.
> TimeUnit.toNanos() and SystemClock.elapsedRealtime() was added in API level 1.
> This way, we get a well defined timestamp for all API levels.

Acknowledged.

[magjed_webrtc, 2015-09-14 16:00:21]

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: return;
On 2015/09/14 15:29:26, hbos wrote:
> On 2015/09/14 15:07:48, magjed_webrtc wrote:
> > On 2015/09/14 11:53:46, hbos wrote:
> > > Optional, stats: How about logging and/or increment a counter when a frame
> is
> > > dropped?
> > > 
> > > You would have to watch out for tryDeliverTextureFrame being called
multiple
> > > times for the same frame in the case of a texture being delivered before
> > > returnTextureFrame. And if failed to deliver the first time it could still
> > > succeed in delivering the second time. But the second invocation might
> > otherwise
> > > be the next frame. So adding this would be tricker than it would first
seem,
> > > thus optional.
> > 
> > We never drop frames, at least we can't see them being dropped. We will not
> > receive new onFrameAvailable() until updateTexImage() is called. If a
texture
> is
> > delivered in onFrameAvailable() before returnTextureFrame(), we will not
drop
> > the frame, we will wait for returnTextureFrame() and then deliver the new
> frame.
> 
> Ok OK. I thought onFrameAvailable() would be called periodically regardless of
> updateTexImage().
> 
> Hmmm... Let's say X is the time between one updateTexImage() and the
subsequent
> onFrameAvailable(). Even though there are no *deliverable* frames being
dropped,
> if you're slower than X at returning a frame you would be throttling the next
> frame after that, right? So this would negatively affect the framerate,
correct?
> So you would indirectly be losing "frames", but the measurable loss would be
in
> time not spent waiting for the next frame instead of number of ready frames
> dropped.
> 
> It might still make sense to log "slower than X frame borrows" as a warning
for
> slow rendering. Or maybe not? What do you think?

The camera/decoder will eventually start dropping frames, but we don't get
notified about this, and we don't know how many buffers they have. I agree it
might make sense to log a warning if we don't get a returnTextureFrame() ~1s
after onTextureFrameAvailable(). I can add that in a follow-up CL once this
class is used. Is that ok?

[hbos, 2015-09-15 07:33:42]

lgtm

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1342713003/diff/10004/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:153: return;
On 2015/09/14 16:00:20, magjed_webrtc wrote:
> On 2015/09/14 15:29:26, hbos wrote:
> > On 2015/09/14 15:07:48, magjed_webrtc wrote:
> > > On 2015/09/14 11:53:46, hbos wrote:
> > > > Optional, stats: How about logging and/or increment a counter when a
frame
> > is
> > > > dropped?
> > > > 
> > > > You would have to watch out for tryDeliverTextureFrame being called
> multiple
> > > > times for the same frame in the case of a texture being delivered before
> > > > returnTextureFrame. And if failed to deliver the first time it could
still
> > > > succeed in delivering the second time. But the second invocation might
> > > otherwise
> > > > be the next frame. So adding this would be tricker than it would first
> seem,
> > > > thus optional.
> > > 
> > > We never drop frames, at least we can't see them being dropped. We will
not
> > > receive new onFrameAvailable() until updateTexImage() is called. If a
> texture
> > is
> > > delivered in onFrameAvailable() before returnTextureFrame(), we will not
> drop
> > > the frame, we will wait for returnTextureFrame() and then deliver the new
> > frame.
> > 
> > Ok OK. I thought onFrameAvailable() would be called periodically regardless
of
> > updateTexImage().
> > 
> > Hmmm... Let's say X is the time between one updateTexImage() and the
> subsequent
> > onFrameAvailable(). Even though there are no *deliverable* frames being
> dropped,
> > if you're slower than X at returning a frame you would be throttling the
next
> > frame after that, right? So this would negatively affect the framerate,
> correct?
> > So you would indirectly be losing "frames", but the measurable loss would be
> in
> > time not spent waiting for the next frame instead of number of ready frames
> > dropped.
> > 
> > It might still make sense to log "slower than X frame borrows" as a warning
> for
> > slow rendering. Or maybe not? What do you think?
> 
> The camera/decoder will eventually start dropping frames, but we don't get
> notified about this, and we don't know how many buffers they have. I agree it
> might make sense to log a warning if we don't get a returnTextureFrame() ~1s
> after onTextureFrameAvailable(). I can add that in a follow-up CL once this
> class is used. Is that ok?

Yes.

[magjed_webrtc, 2015-09-15 07:44:25]

Committed patchset #3 (id:30001) manually as
4ae28a10701c82e7bcdd32f075bb89f43d745c95 (presubmit successful).

[commit-bot: I haz the power, 2015-09-15 07:44:27]

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