Java binding for RtpSender/RtpReceiver.

Java binding for RtpSender/RtpReceiver.

The Java PeerConnection maintains a cached list of Java RtpSenders
and RtpReceivers so that the same objects are returned every time
getSenders() or getReceivers() is called. They are disposed of when
PeerConnection.dispose() is called, which will also dispose their
referenced MediaStreamTracks.

Committed: https://crrev.com/4139c0f1c53509ea48c936b58a22a66e63e51fda
Cr-Commit-Position: refs/heads/master@{#10189}

[Taylor Brandstetter, 2015-09-25 19:48:30]

deadbeef@webrtc.org changed reviewers:
+ glaznev@webrtc.org

[Taylor Brandstetter, 2015-09-25 19:48:30]

Hi Alex; here are the Java changes split out of the RtpSender CL.

https://codereview.webrtc.org/1368143002/diff/1/talk/app/webrtc/java/jni/peer...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1368143002/diff/1/talk/app/webrtc/java/jni/peer...
talk/app/webrtc/java/jni/peerconnection_jni.cc:974:
reinterpret_cast<MediaStreamTrackInterface*>(j_p)->Release();
Can't do CHECK_RELEASE any longer, because now RtpSenders/RtpReceivers also hold
references to MediaStreamTracks.

https://codereview.webrtc.org/1368143002/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:270: private void
updateRtpSenders() {
This is necessary because if we just returned new Java RtpSenders every time
getSenders() was called, there would be floating references to RtpSenders (and
thus MediaStreamTracks), so PeerConnection.dispose() would no longer dispose of
MediaStreamTracks.

[pthatcher1, 2015-09-25 21:27:26]

pthatcher@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2015-09-25 21:27:27]

Looking good.  Just style stuff.

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:165: private final
List<RtpReceiver> rtpReceivers;
senders; 
receivers;

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:272: private void
updateRtpSenders() {
updateSenders()

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:319: if
(newReceiver.id() == oldReceiver.id()) {
.equals

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:348: }
It seems like a helper method called hasReceiver could clean this up a bit:

bool hasReceiver(Iterable<RtpReceiver> receivers, String id) {
  for (RtpReceiver receiver: receivers) {
    if (receiver.id().equals(id)) {
      return true;
    }
  }
  return false;
}

private void updateReceivers() {
  for (RtpReceiver oldReceiver: receivers) {
    if (!hasReceiver(newReceivers, oldReceiver.id())) {
      toRemove.add(oldReceiver);
    }
  }

  for (RtpReceiver removed : toRemove) {
    receivers.remove(removed);
    removed.dispose();
  }

  for (RtpReceiver newReceiver : newReceivers) {
    if (hasReceiver(receivers, newReceiver)) {
      newReceiver.dispose();
    } else {
      receivers.add(newReceiver);
    }
  }
}


And if you just copy the old receivers, it's even more readable:

List<RtpReceiver> oldReceivers = getReceivers();
for (RtpReceiver oldReceiver: oldReceivers) {
  if (!hasReceiver(newReceivers, oldReceiver)) {
    receivers.remove(oldReceiver);
  }
}

for (RtpReceiver newReceiver : newReceivers) {
  if (hasReceiver(oldReceivers, newReceiver)) {
    newReceiver.dispose();
  } else {
    receivers.add(newReceiver);
  }
}

[Taylor Brandstetter, 2015-09-25 23:01:08]

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:165: private final
List<RtpReceiver> rtpReceivers;
On 2015/09/25 21:27:27, pthatcher1 wrote:
> senders; 
> receivers;

Done.

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:272: private void
updateRtpSenders() {
On 2015/09/25 21:27:27, pthatcher1 wrote:
> updateSenders()

Done.

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:319: if
(newReceiver.id() == oldReceiver.id()) {
On 2015/09/25 21:27:27, pthatcher1 wrote:
> .equals

Done.

https://codereview.webrtc.org/1368143002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:348: }
On 2015/09/25 21:27:27, pthatcher1 wrote:
> It seems like a helper method called hasReceiver could clean this up a bit:
> 
> bool hasReceiver(Iterable<RtpReceiver> receivers, String id) {
>   for (RtpReceiver receiver: receivers) {
>     if (receiver.id().equals(id)) {
>       return true;
>     }
>   }
>   return false;
> }
> 
> private void updateReceivers() {
>   for (RtpReceiver oldReceiver: receivers) {
>     if (!hasReceiver(newReceivers, oldReceiver.id())) {
>       toRemove.add(oldReceiver);
>     }
>   }
> 
>   for (RtpReceiver removed : toRemove) {
>     receivers.remove(removed);
>     removed.dispose();
>   }
> 
>   for (RtpReceiver newReceiver : newReceivers) {
>     if (hasReceiver(receivers, newReceiver)) {
>       newReceiver.dispose();
>     } else {
>       receivers.add(newReceiver);
>     }
>   }
> }
> 
> 
> And if you just copy the old receivers, it's even more readable:
> 
> List<RtpReceiver> oldReceivers = getReceivers();
> for (RtpReceiver oldReceiver: oldReceivers) {
>   if (!hasReceiver(newReceivers, oldReceiver)) {
>     receivers.remove(oldReceiver);
>   }
> }
> 
> for (RtpReceiver newReceiver : newReceivers) {
>   if (hasReceiver(oldReceivers, newReceiver)) {
>     newReceiver.dispose();
>   } else {
>     receivers.add(newReceiver);
>   }
> }

Done.

[AlexG, 2015-09-25 23:40:00]

https://codereview.webrtc.org/1368143002/diff/60001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:224: updateSenders();
Pass 1: Instead of calling updateSenders() on all these updates I think it is
better to keep Java layer more thin. 

May be instead of calling updateSenders() in a few places you can call it once
only in PeerConnection.getSenders() and convert from native Senders/Receivers to
corresponding Java objects?

[Taylor Brandstetter, 2015-09-26 00:15:49]

https://codereview.webrtc.org/1368143002/diff/60001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:224: updateSenders();
On 2015/09/25 23:40:00, AlexG wrote:
> Pass 1: Instead of calling updateSenders() on all these updates I think it is
> better to keep Java layer more thin. 
> 
> May be instead of calling updateSenders() in a few places you can call it once
> only in PeerConnection.getSenders() and convert from native Senders/Receivers
to
> corresponding Java objects?

I wanted to do this originally, but this would mean that obsolete
senders/receivers (removed as a result of setLocal/RemoteDescription) wouldn't
be "dispose()"d of until getSenders() is called again. Which would also mean
their MediaStreamTracks wouldn't be disposed. I wasn't sure if this would go
against Tachyon's expectations of the PeerConnection interface.

[Taylor Brandstetter, 2015-09-30 17:53:30]

Hi Alex; just a reminder, I was still hoping to get your feedback on this.
Thanks!

[AlexG, 2015-09-30 18:18:46]

On 2015/09/30 17:53:30, Taylor Brandstetter wrote:
> Hi Alex; just a reminder, I was still hoping to get your feedback on this.
> Thanks!

Sorry for delay.
I think originally Peer connection Java API was designed as a thin layer on top
of C++ interface. I am not in favor of idea to now start adding more complicated
logic to this interface. It will probably be ok for now to get Senders/Receivers
only once in PeerConnection.getSenders() call and avoid updating it in multiple
places.

[Taylor Brandstetter, 2015-10-02 18:53:41]

As we discussed, I'm now just retrieving the senders each time getSenders is
called.

[AlexG, 2015-10-02 22:44:36]

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:978:
reinterpret_cast<MediaStreamTrackInterface*>(j_p)->Release();
Can you explain why CHECK_RELEASE is removed?

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1936: JOW(jlong,
RtpSender_nativeTrack)(JNIEnv* jni,
Probably rename to nativeGetandReleaseTrack - if you have Set() you may need
Get() method

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1956: JOW(jlong,
RtpReceiver_nativeTrack)(JNIEnv* jni,
ditto: nativeGetAndReleaseTrack()?

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:198: public void
setLocalDescription(
This change is not needed now - you can revert back to declaring this call as
native

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:203: public void
setRemoteDescription(
ditto: revert back to native

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:236: return new
ArrayList<RtpSender>(senders);
why new here? You can return senders directly without creating extra copy

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:244: return new
ArrayList<RtpReceiver>(receivers);
ditto: return receivers without making extra copy

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java:39: cachedTrack = new
MediaStreamTrack(track);
Why is this logic different from RtpSender? cachedTrack can be null in
RtpSender, but not in RtpReceiver? Can you add comments on it to both classes.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java:56: // Will be released in
dispose().
I think it actually decrements refcount - it calls release() in native?

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/RtpSender.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpSender.java:70: // This should increment
the reference count of the track.
I think it actually decrements refcount - it calls release() in native?

[Taylor Brandstetter, 2015-10-03 01:50:10]

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:978:
reinterpret_cast<MediaStreamTrackInterface*>(j_p)->Release();
On 2015/10/02 22:44:35, AlexG wrote:
> Can you explain why CHECK_RELEASE is removed?

Because now there are two Java MediaStreamTracks that reference the same
underlying MediaStreamTrackInterface: the one the MediaStream owns, and the one
the RtpSender/RtpReceiver creates. I did do some debugging and verified that the
ref count ends up 0 in the end though.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1936: JOW(jlong,
RtpSender_nativeTrack)(JNIEnv* jni,
On 2015/10/02 22:44:35, AlexG wrote:
> Probably rename to nativeGetandReleaseTrack - if you have Set() you may need
> Get() method

Done.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1956: JOW(jlong,
RtpReceiver_nativeTrack)(JNIEnv* jni,
On 2015/10/02 22:44:35, AlexG wrote:
> ditto: nativeGetAndReleaseTrack()?

Done.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:198: public void
setLocalDescription(
On 2015/10/02 22:44:35, AlexG wrote:
> This change is not needed now - you can revert back to declaring this call as
> native

Done.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:203: public void
setRemoteDescription(
On 2015/10/02 22:44:35, AlexG wrote:
> ditto: revert back to native

Done.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:236: return new
ArrayList<RtpSender>(senders);
On 2015/10/02 22:44:35, AlexG wrote:
> why new here? You can return senders directly without creating extra copy

Because if the caller of getSenders() modified the list, that shouldn't affect
whether or not the senders get disposed when getSenders() is next called (or
PeerConnection.dispose() is called).

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:244: return new
ArrayList<RtpReceiver>(receivers);
On 2015/10/02 22:44:35, AlexG wrote:
> ditto: return receivers without making extra copy

See above.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java:39: cachedTrack = new
MediaStreamTrack(track);
On 2015/10/02 22:44:35, AlexG wrote:
> Why is this logic different from RtpSender? cachedTrack can be null in
> RtpSender, but not in RtpReceiver? Can you add comments on it to both classes.

It will be possible in the near future to have an RtpSender without a track (for
the "early warmup without getUserMedia" scenario), but the same is not true for
RtpReceivers.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java:56: // Will be released in
dispose().
On 2015/10/02 22:44:36, AlexG wrote:
> I think it actually decrements refcount - it calls release() in native?

scoped_refptr::release actually means that the pointer object releases the raw
pointer *without* decrementing the ref count. So the ref count will end up
incremented in the end.

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/RtpSender.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpSender.java:70: // This should increment
the reference count of the track.
On 2015/10/02 22:44:36, AlexG wrote:
> I think it actually decrements refcount - it calls release() in native?

See other comment

[AlexG, 2015-10-05 22:44:53]

lgtm

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/80001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:236: return new
ArrayList<RtpSender>(senders);
On 2015/10/03 01:50:10, Taylor Brandstetter wrote:
> On 2015/10/02 22:44:35, AlexG wrote:
> > why new here? You can return senders directly without creating extra copy
> 
> Because if the caller of getSenders() modified the list, that shouldn't affect
> whether or not the senders get disposed when getSenders() is next called (or
> PeerConnection.dispose() is called).

May be you can use Collections.unmodifiableList for this then, because as you
pointed the list you return can become invalid (i.e. contain destroyed elements)
at some point of time.

https://codereview.webrtc.org/1368143002/diff/100001/talk/app/webrtc/java/src...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1368143002/diff/100001/talk/app/webrtc/java/src...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:283: public native void
nativeSetLocalDescription(SdpObserver observer, SessionDescription sdp);
Remove this

https://codereview.webrtc.org/1368143002/diff/100001/talk/app/webrtc/java/src...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:285: public native void
nativeSetRemoteDescription(SdpObserver observer, SessionDescription sdp);
And this as well

[Taylor Brandstetter, 2015-10-06 18:56:22]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-06 18:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1368143002/#ps120001 (title:
"Getting rid of obsolete methods, and using unmodifiableList.")

[commit-bot: I haz the power, 2015-10-06 18:56:45]

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

[commit-bot: I haz the power, 2015-10-06 19:29:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-06 19:29:42]

Patchset 7 (id:??) landed as
https://crrev.com/4139c0f1c53509ea48c936b58a22a66e63e51fda
Cr-Commit-Position: refs/heads/master@{#10189}