Add C++ SurfaceTextureHandler

Add C++ SurfaceTextureHandler
This cl adds a C++ counterpart of the Java SurfaceTextureHandler. It can be used for creating a webrtc::VideoFrames from a native handle and also guarantee that the Java SurfaceTexture is notified when the VideoFrame is no longer in use.

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

Committed: https://crrev.com/3e9eb4ba01acf8a49fa1949305fb30a7daf51964
Cr-Commit-Position: refs/heads/master@{#10084}

[perkj_webrtc, 2015-09-27 09:46:58]

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

[perkj_webrtc, 2015-09-27 09:46:58]

Can you please review?

[magjed_webrtc, 2015-09-27 11:26:41]

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

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: //  Start
to stream textures to the given |listener|.
Use Javadoc block comment like the rest of the class. Also you should mention
that the listener may only be set once.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:105: throw new
RuntimeException("SurfaceTextureHelper listener has already been set.");
s/RuntimeException/IllegalStateException/g

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:127: * the
textureFrame have been returned by a call to  returnTextureFrame.
s/textureFrame/texture frame/g.
s/have/has/g.
s/to  returnTextureFrame/to returnTextureFrame()/g.
Also, reflow comments to 100 chars per row.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:145:
barrier.await();
This is duplicated boilerplate code from ThreadUtils. I have made similar
changes in https://codereview.webrtc.org/1368093003/.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:198: public
void returnTextureFrame() {
Why have you moved this public method below the private methods? I think the
order of the methods are good as they are.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc (right):

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:37: static JNIEnv* jni() {
return AttachCurrentThreadIfNeeded(); }
I don't think this is a good practice. Pass the JNIEnv down through the methods
instead. Change the ctor to: SurfaceTextureHelper::SurfaceTextureHelper(JNIEnv*
jni, jobject egl_shared_context).

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:54: RTC_CHECK(false)
s/RTC_CHECK/RTC_NOTREACHED/g

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:61:
rtc::scoped_refptr<SurfaceTextureHelper> pool_;
Make this const if ReturnTextureFrame() can be made const.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:65:
SurfaceTextureHelper::Create(jobject shared_egl_context) {
There is no need for a factory method, use the constructor directly instead.
Otherwise all call sites need to worry about getting nullptr in return.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:73: FindClass(jni(),
"org/webrtc/SurfaceTextureHelper")),
Replace FindClass(...) with:
jni->FindClass("org/webrtc/SurfaceTextureHelper");
to remove the dependency to ClassReferenceHolder.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:79: void
ReturnTextureFrame();
Can this be made: 'void ReturnTextureFrame() const'?

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:81: const
ScopedGlobalRef<jclass> j_surface_texture_helper_class_;
FYI - You don't need to hold the class yourself if you are using FindClass() in
classreferenceholder.h. Actually, I think it is better to hold the class
ourselves so that org.webrtc.SurfaceTextureHelper is only loaded if it's used.

[perkj_webrtc, 2015-09-28 07:22:47]

PTAL

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

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:102: //  Start
to stream textures to the given |listener|.
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> Use Javadoc block comment like the rest of the class. Also you should mention
> that the listener may only be set once.

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:105: throw new
RuntimeException("SurfaceTextureHelper listener has already been set.");
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> s/RuntimeException/IllegalStateException/g

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:127: * the
textureFrame have been returned by a call to  returnTextureFrame.
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> s/textureFrame/texture frame/g.
> s/have/has/g.
> s/to  returnTextureFrame/to returnTextureFrame()/g.
> Also, reflow comments to 100 chars per row.

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:145:
barrier.await();
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> This is duplicated boilerplate code from ThreadUtils. I have made similar
> changes in https://codereview.webrtc.org/1368093003/.

ok, either land yours first or maybe you can refactor this in your cl?

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/android/...
talk/app/webrtc/java/android/org/webrtc/SurfaceTextureHelper.java:198: public
void returnTextureFrame() {
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> Why have you moved this public method below the private methods? I think the
> order of the methods are good as they are.

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc (right):

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:37: static JNIEnv* jni() {
return AttachCurrentThreadIfNeeded(); }
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> I don't think this is a good practice. Pass the JNIEnv down through the
methods
> instead. Change the ctor to:
SurfaceTextureHelper::SurfaceTextureHelper(JNIEnv*
> jni, jobject egl_shared_context).

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:54: RTC_CHECK(false)
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> s/RTC_CHECK/RTC_NOTREACHED/g

Well, that is a DCHECK. I wanted to make sure it always crash, but OK.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:61:
rtc::scoped_refptr<SurfaceTextureHelper> pool_;
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> Make this const if ReturnTextureFrame() can be made const.

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:65:
SurfaceTextureHelper::Create(jobject shared_egl_context) {
On 2015/09/27 11:26:41, magjed_webrtc wrote:
> There is no need for a factory method, use the constructor directly instead.
> Otherwise all call sites need to worry about getting nullptr in return.

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:73: FindClass(jni(),
"org/webrtc/SurfaceTextureHelper")),
On 2015/09/27 11:26:40, magjed_webrtc wrote:
> Replace FindClass(...) with:
> jni->FindClass("org/webrtc/SurfaceTextureHelper");
> to remove the dependency to ClassReferenceHolder.

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:79: void
ReturnTextureFrame();
On 2015/09/27 11:26:41, magjed_webrtc wrote:
> Can this be made: 'void ReturnTextureFrame() const'?

Done.

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:81: const
ScopedGlobalRef<jclass> j_surface_texture_helper_class_;
On 2015/09/27 11:26:41, magjed_webrtc wrote:
> FYI - You don't need to hold the class yourself if you are using FindClass()
in
> classreferenceholder.h. Actually, I think it is better to hold the class
> ourselves so that org.webrtc.SurfaceTextureHelper is only loaded if it's used.

Acknowledged.

[magjed_webrtc, 2015-09-28 08:19:38]

lgtm

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc (right):

https://codereview.webrtc.org/1366413003/diff/1/talk/app/webrtc/java/jni/surf...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.cc:54: RTC_CHECK(false)
On 2015/09/28 07:22:47, perkj1 wrote:
> On 2015/09/27 11:26:40, magjed_webrtc wrote:
> > s/RTC_CHECK/RTC_NOTREACHED/g
> 
> Well, that is a DCHECK. I wanted to make sure it always crash, but OK. 

I see. It's up to you.

https://codereview.webrtc.org/1366413003/diff/20001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/1366413003/diff/20001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:63: return
*j_surface_texture_helper_;
Maybe this should be moved in the .cc file?

[perkj_webrtc, 2015-09-28 08:52:41]

Committed patchset #2 (id:20001) manually as
3e9eb4ba01acf8a49fa1949305fb30a7daf51964 (presubmit successful).

[commit-bot: I haz the power, 2015-09-28 08:52:44]

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