AndroidVideoCapturerJni: Fix threading issues

AndroidVideoCapturerJni: Fix threading issues

The primary fix in this CL is to remove the dangling |thread_| pointer in AndroidVideoCapturerJni. That thread is not safe to use after Stop() has been called. Even after Stop() has been called, we must still be able to return late frames to Java in order to not leak them, so that path has been made thread safe instead. To make sure that we always return frames, the Java frame should be wrapped in a scoped_refptr as quickly as possible, so this CL moves the wrapping from AndroidVideoCapturer to AndroidVideoCapturerJni. This also removes the need for the interface function AndroidVideoCapturerDelegate::ReturnBuffer().

Some other minor changes are:
 * Remove |valid_global_refs_| and all logic related to that. Now that rtc::Bind() captures method objects as scoped_refptr, the destructor of AndroidVideoCapturerJni will not be called before all frames are returned.
 * Remove global ref |j_frame_observer_|. No need for this, we don’t call it and it is kept alive with standard Java memory management.
 * Add helper function ShallowCenterCrop() for VideoFrameBuffers. This functionality already exists in the constructor of WrappedI420Buffer, but it’s more convenient to have it as a separate function.

BUG=webrtc:4742, webrtc:4909
R=glaznev@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/c464f504dcb40ad40b5258875493f12783bd5fda

[magjed_webrtc, 2015-08-21 14:28:57]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-21 14:29:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307973002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307973002/1

[magjed_webrtc, 2015-08-22 18:03:23]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-08-22 18:03:30]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-08-22 18:56:13]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-22 18:56:23]

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

[magjed_webrtc, 2015-08-22 19:22:40]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-22 19:22:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307973002/60001

[commit-bot: I haz the power, 2015-08-22 20:15:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-22 20:15:42]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2015-08-23 08:42:47]

Patchset #1 (id:40001) has been deleted

[magjed_webrtc, 2015-08-23 09:00:34]

magjed@webrtc.org changed reviewers:
+ glaznev@webrtc.org, tommi@webrtc.org

[magjed_webrtc, 2015-08-24 07:33:22]

Please take a look.

[tommi, 2015-08-24 09:14:42]

lgtm

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
File talk/app/webrtc/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
talk/app/webrtc/androidvideocapturer.cc:92: : frame.release();
can we at some point change CreateAliasedFrame to return a scoped_ptr?
(add a TODO if so?)

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
File talk/app/webrtc/androidvideocapturer.h (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
talk/app/webrtc/androidvideocapturer.h:70: void
OnIncomingFrame(rtc::scoped_refptr<webrtc::VideoFrameBuffer> buffer,
const &

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (left):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:94:
invoker_.Flush(thread_);
great that this isn't needed anymore

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:96:
CHECK(thread_checker_.CalledOnValidThread());
FYI - I don't think there's much point in having these thread checks be CHECKs
and not DCHECKs.

See here what ThreadChecker will do in builds where DCHECK isn't enabled:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:146: return;
return false? and return true if the method call was queued?

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.h (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidvideocapturer_jni.h:82: template <typename...
Args>
ah, it was only a matter of time...

[magjed_webrtc, 2015-08-24 11:27:19]

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
File talk/app/webrtc/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
talk/app/webrtc/androidvideocapturer.cc:92: : frame.release();
On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> can we at some point change CreateAliasedFrame to return a scoped_ptr?
> (add a TODO if so?)

rtc::scoped_ptr supports rvalue references so it is possible. I added a TODO.
However, I'm plannig to trying to remove the cricket::VideoFrameFactory
altogether.

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
File talk/app/webrtc/androidvideocapturer.h (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
talk/app/webrtc/androidvideocapturer.h:70: void
OnIncomingFrame(rtc::scoped_refptr<webrtc::VideoFrameBuffer> buffer,
On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> const &

The reason for making it by-value is that rtc::Bind will capture it exactly as
the function declaration. See the comment above.

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (right):

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:96:
CHECK(thread_checker_.CalledOnValidThread());
On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> FYI - I don't think there's much point in having these thread checks be CHECKs
> and not DCHECKs.
> 
> See here what ThreadChecker will do in builds where DCHECK isn't enabled:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Done.

https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:146: return;
On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> return false? and return true if the method call was queued?

It's a private method and there is nothing I can do if it returns false. I will
add that functionality if it's needed.

[tommi, 2015-08-24 11:46:04]

On 2015/08/24 11:27:19, magjed_webrtc wrote:
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
> File talk/app/webrtc/androidvideocapturer.cc (right):
> 
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
> talk/app/webrtc/androidvideocapturer.cc:92: : frame.release();
> On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> > can we at some point change CreateAliasedFrame to return a scoped_ptr?
> > (add a TODO if so?)
> 
> rtc::scoped_ptr supports rvalue references so it is possible. I added a TODO.
> However, I'm plannig to trying to remove the cricket::VideoFrameFactory
> altogether.
> 
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
> File talk/app/webrtc/androidvideocapturer.h (right):
> 
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/androidvi...
> talk/app/webrtc/androidvideocapturer.h:70: void
> OnIncomingFrame(rtc::scoped_refptr<webrtc::VideoFrameBuffer> buffer,
> On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> > const &
> 
> The reason for making it by-value is that rtc::Bind will capture it exactly as
> the function declaration. See the comment above.
> 
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
> File talk/app/webrtc/java/jni/androidvideocapturer_jni.cc (right):
> 
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
> talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:96:
> CHECK(thread_checker_.CalledOnValidThread());
> On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> > FYI - I don't think there's much point in having these thread checks be
CHECKs
> > and not DCHECKs.
> > 
> > See here what ThreadChecker will do in builds where DCHECK isn't enabled:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> Done.
> 
>
https://codereview.webrtc.org/1307973002/diff/60001/talk/app/webrtc/java/jni/...
> talk/app/webrtc/java/jni/androidvideocapturer_jni.cc:146: return;
> On 2015/08/24 09:14:42, tommi (webrtc) wrote:
> > return false? and return true if the method call was queued?
> 
> It's a private method and there is nothing I can do if it returns false. I
will
> add that functionality if it's needed.

sgtm lgtm

[AlexG, 2015-08-25 18:08:51]

lgtm

[magjed_webrtc, 2015-08-25 21:22:30]

Committed patchset #2 (id:80001) manually as
c464f504dcb40ad40b5258875493f12783bd5fda (presubmit successful).