Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java object it.

In addition, arrange so that the SurfaceTextureHelper.stopListening
method (in java) can be called from any thread.

BUG=

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

[nisse-webrtc, 2016-05-18 07:30:17]

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

[nisse-webrtc, 2016-05-18 07:30:17]

I don't quite understand the required ordering in this code.

The method MediaCodecVideoDecoder.TextureListener.release used to look like
this,

      surfaceTextureHelper.dispose();
      synchronized (newFrameLock) {
        if (renderedBuffer != null) {
          surfaceTextureHelper.returnTextureFrame();
          renderedBuffer = null;
        }
      }

I.e., first call surfaceTextureHelper.dispose, and then conditinally call
surfaceTextureHelper.returnTextureFrame. This looks a bit strange.

Maybe SurfaceTextureHelper.dispose ought to do that? The comments on that method
(and the related stopListening), are a bit confusing.

And if it really is important that dispose complete before other things are
cleaned up, it seems a bit brittle to do it in the C++ destructor of a
ref-counted object. It would perhaps be better with a wrapper dispose method
that can be called explicitly.

[nisse-webrtc, 2016-05-18 08:13:20]

Description was changed from

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

BUG=
==========

to

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.
 
BUG=
==========

[magjed_webrtc, 2016-05-18 08:57:25]

lgtm

It's ok to call SurfaceTextureHelper.returnTextureFrame() after
SurfaceTextureHelper.dispose(). The only important thing to make sure is that
all frames are eventually returned with returnTextureFrame(), otherwise the
resources in SurfaceTextureHelper will not be released.

[nisse-webrtc, 2016-05-18 11:14:41]

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

[commit-bot: I haz the power, 2016-05-18 11:14:50]

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

[commit-bot: I haz the power, 2016-05-18 13:20:12]

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

[commit-bot: I haz the power, 2016-05-18 13:20:13]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-05-23 13:07:47]

I'm looking into how to support calling stopListening on any thread.

The simplest approach would be to use a lock and use for synchronizing all of
startListening, stopListening, and listener.onTextureFrameAvailable.

But maybe it's better to use a similar trick as dispose uses? I.e., something
like

if (on handler thread) {
  listener = null;
} else {
  handler.postAtFrontOfQueue(... { listener = null; barrier.countDown() });
  wait(barrier)
}

[nisse-webrtc, 2016-05-23 13:31:57]

Tried to make stopListening work on any thread, using the same post-and-barrier
trick as dispose.

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (left):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:380:
handler.removeCallbacks(setListenerRunnable);
Is it correct to make this call up-front, regardless of which thread we're on?

[magjed_webrtc, 2016-05-24 09:09:55]

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (left):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:380:
handler.removeCallbacks(setListenerRunnable);
On 2016/05/23 13:31:57, nisse-webrtc wrote:
> Is it correct to make this call up-front, regardless of which thread we're on?

Yes? |setListenerRunnable| is not allowed to execute after stopListening(),
because calling startListening() followed by stopListening() should be
guaranteed to stop.

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:377: if
(handler.getLooper().getThread() == Thread.currentThread()) {
Please add a helper method invokeAtFrontUninterruptibly() in ThreadUtils that
does exactly this, and replace the use case here and in dispose(). dispose()
currently avoids waiting for release() to finish, but you can ignore that and
wait for the whole Runnable to finish instead.

public static void invokeUninterruptibly(final Handler handler, final Runnable
runnable) {
  if (handler.getLooper().getThread() == Thread.currentThread()) {
    runnable.run();
    return;
  }
  final CountDownLatch barrier = new CountDownLatch(1);
  handler.postAtFrontOfQueue(new Runnable() {
    @Override
    public void run() {
      runnable.run();
      barrier.countDown();
    }
  });
  awaitUninterruptibly(barrier);
}

[nisse-webrtc, 2016-05-24 10:18:00]

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (left):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:380:
handler.removeCallbacks(setListenerRunnable);
On 2016/05/24 09:09:55, magjed_webrtc wrote:
> On 2016/05/23 13:31:57, nisse-webrtc wrote:
> > Is it correct to make this call up-front, regardless of which thread we're
on?
> 
> Yes? |setListenerRunnable| is not allowed to execute after stopListening(),
> because calling startListening() followed by stopListening() should be
> guaranteed to stop.

Good. It used to only be called on the handler thread.

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:377: if
(handler.getLooper().getThread() == Thread.currentThread()) {
On 2016/05/24 09:09:55, magjed_webrtc wrote:
> Please add a helper method invokeAtFrontUninterruptibly() in ThreadUtils that
> does exactly this, and replace the use case here and in dispose(). dispose()
> currently avoids waiting for release() to finish, but you can ignore that and
> wait for the whole Runnable to finish instead.
> 
> public static void invokeUninterruptibly(final Handler handler, final Runnable
> runnable) {
>   if (handler.getLooper().getThread() == Thread.currentThread()) {
>     runnable.run();
>     return;
>   }
>   final CountDownLatch barrier = new CountDownLatch(1);
>   handler.postAtFrontOfQueue(new Runnable() {
>     @Override
>     public void run() {
>       runnable.run();
>       barrier.countDown();
>     }
>   });
>   awaitUninterruptibly(barrier);
> }

Done, except that I don't do the currentThread case. The other invoke methods
appear to not to do that, so I try to stay consistent and leave that check with
the caller, even if that's a somewhat ugly duplication of the code block. 

Not sure what's best, should all ThreadUtils.invoke* check for this case?

[magjed_webrtc, 2016-05-24 14:29:56]

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:377: if
(handler.getLooper().getThread() == Thread.currentThread()) {
> Not sure what's best, should all ThreadUtils.invoke* check for this case?
At least invokeAtFront() should have it. Otherwise it will surely deadlock. The
other invoke* should probably have a check, but I'm not sure if we should
execute the Runnable immediately or throw a RuntimeException. If we have code
like:
handler.post(A);
invokeUninterruptibly(handler, B);
A should probably be executed before B, so we can't execute the Runnable
immediately in B.

[nisse-webrtc, 2016-05-25 07:35:59]

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:377: if
(handler.getLooper().getThread() == Thread.currentThread()) {
On 2016/05/24 14:29:56, magjed_webrtc wrote:
> > Not sure what's best, should all ThreadUtils.invoke* check for this case?
> At least invokeAtFront() should have it.

I'll add that.

> Otherwise it will surely deadlock.

The alternative would be to require the caller to do that check, so I think it's
a matter of style. As far as I see, invokeInterruptibly will deadlock in the
same way if called on the handler's own thread.

> The other invoke* should probably have a check, 

I'm a little concerned about consistency, but maybe it makes sense to have the
atFront variant be special and try run it as soon as possible.

[magjed_webrtc, 2016-05-25 09:03:07]

lgtm

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:377: if
(handler.getLooper().getThread() == Thread.currentThread()) {
On 2016/05/25 07:35:59, nisse-webrtc wrote:
> On 2016/05/24 14:29:56, magjed_webrtc wrote:
> The alternative would be to require the caller to do that check, so I think
it's
> a matter of style.
Yes, but in the case of invokeAtFront, the util function can handle if it's the
same thread, so I don't see any reason not to handle it.

> As far as I see, invokeInterruptibly will deadlock in the
> same way if called on the handler's own thread.
Yes they will, so throwing an exception might facilitate debugging.

The invoke functions are only used from this class, so if you want, you can even
remove the non-at-front invoke functions and only use invokeAtFront.

[nisse-webrtc, 2016-05-25 11:35:51]

I'll try to land this now.

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/40001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:377: if
(handler.getLooper().getThread() == Thread.currentThread()) {
On 2016/05/25 09:03:07, magjed_webrtc wrote:
> On 2016/05/25 07:35:59, nisse-webrtc wrote:
> > On 2016/05/24 14:29:56, magjed_webrtc wrote:
> > The alternative would be to require the caller to do that check, so I think
> it's
> > a matter of style.
> Yes, but in the case of invokeAtFront, the util function can handle if it's
the
> same thread, so I don't see any reason not to handle it.
> 
> > As far as I see, invokeInterruptibly will deadlock in the
> > same way if called on the handler's own thread.
> Yes they will, so throwing an exception might facilitate debugging.
> 
> The invoke functions are only used from this class, so if you want, you can
even
> remove the non-at-front invoke functions and only use invokeAtFront.

Deleted the non-AtFront method. Renamed the variant with return value, made to
post at front, and work on the handler's thread.

Also updated tests.

[nisse-webrtc, 2016-05-25 11:35:58]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-05-25 11:35:58]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1988043002/#ps120001 (title:
"Delete non-AtFront method.")

[commit-bot: I haz the power, 2016-05-25 11:36:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988043002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988043002/120001

[commit-bot: I haz the power, 2016-05-25 11:42:39]

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

[commit-bot: I haz the power, 2016-05-25 11:42:40]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5843)

[nisse-webrtc, 2016-05-25 11:58:31]

Description was changed from

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.
 
BUG=
==========

to

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.

In addition, arrange so that the SurfaceTextureHelper.stopListening 
method (in java) can be called from any thread.

BUG=
==========

[nisse-webrtc, 2016-05-25 12:01:10]

nisse@webrtc.org changed reviewers:
+ tommi@webrtc.org
- perkj@webrtc.org

[nisse-webrtc, 2016-05-25 12:01:11]

Hi tommi,

I need owners approval, primarily for ThreadUtils.java and
MediaCodecVideoDecoder.java. I Per's absense, can you take a look?

[tommi, 2016-05-26 13:36:58]

lgtm

https://codereview.webrtc.org/1988043002/diff/140001/webrtc/api/java/android/...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/140001/webrtc/api/java/android/...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:382: @Override
public void run() {
is the indent off?

[nisse-webrtc, 2016-05-26 13:55:14]

Fixed indent now.

https://codereview.webrtc.org/1988043002/diff/140001/webrtc/api/java/android/...
File webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java (right):

https://codereview.webrtc.org/1988043002/diff/140001/webrtc/api/java/android/...
webrtc/api/java/android/org/webrtc/SurfaceTextureHelper.java:382: @Override
public void run() {
On 2016/05/26 13:36:58, tommi-webrtc wrote:
> is the indent off?

Possibly, the current code and cl format don't agree at all, so I'm not sure how
it's supposed to be indented. I'll check with Magnus before landing.

[nisse-webrtc, 2016-05-26 13:55:27]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-05-26 13:55:28]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1988043002/#ps160001 (title:
"Indentation tweak.")

[commit-bot: I haz the power, 2016-05-26 13:55:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988043002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988043002/160001

[commit-bot: I haz the power, 2016-05-26 15:55:57]

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

[commit-bot: I haz the power, 2016-05-26 15:55:58]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-05-27 07:22:56]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-05-27 07:23:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988043002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988043002/160001

[commit-bot: I haz the power, 2016-05-27 07:28:04]

Description was changed from

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.

In addition, arrange so that the SurfaceTextureHelper.stopListening 
method (in java) can be called from any thread.

BUG=
==========

to

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.

In addition, arrange so that the SurfaceTextureHelper.stopListening 
method (in java) can be called from any thread.

BUG=
==========

[commit-bot: I haz the power, 2016-05-27 07:28:05]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-27 07:28:12]

Description was changed from

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.

In addition, arrange so that the SurfaceTextureHelper.stopListening 
method (in java) can be called from any thread.

BUG=
==========

to

==========
Call java SurfaceTextureHelper.dispose from the corresponding C++ destructor.

This makes it clearer that the C++ SurfaceTextureHelper owns its associated java
object it.

In addition, arrange so that the SurfaceTextureHelper.stopListening
method (in java) can be called from any thread.

BUG=

Committed: https://crrev.com/a44e72c44f4dcc2332819ffd089f182385aea08c
Cr-Commit-Position: refs/heads/master@{#12941}
==========

[commit-bot: I haz the power, 2016-05-27 07:28:13]

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