Move ownership of PeerConnectionObserver from Java to C++.

New OwnedPeerConnection takes ownership of the observer. This is done
to allow NativePeerConnectionFactory to return a capsulated object.

Bug: webrtc:8662
Change-Id: Ie876f7b9a1a17ebcfbe51537f712a32ab1a7cbfb
Reviewed-on: https://webrtc-review.googlesource.com/35300
Commit-Queue: Sami Kalliomäki <sakal@webrtc.org>
Reviewed-by: Magnus Jedvert <magjed@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21610}

sdk/android/api/org/webrtc/NativePeerConnectionFactory.java

@@ -10,10 +10,10 @@
 
 package org.webrtc;
 
-/** Factory for creating webrtc::PeerConnectionInterface instances. */
+/** Factory for creating webrtc::jni::OwnedPeerConnection instances. */
 public interface NativePeerConnectionFactory {
   /**
-   * Create a new webrtc::PeerConnectionInterface instance and returns a pointer to it.
+   * Create a new webrtc::jni::OwnedPeerConnection instance and returns a pointer to it.
    * The caller takes ownership of the object.
    */
   long createNativePeerConnection();

sdk/android/api/org/webrtc/PeerConnection.java

@@ -548,7 +548,6 @@ public class PeerConnection {
 
   private final List<MediaStream> localStreams = new ArrayList<>();
   private final long nativePeerConnection;
-  private final long nativeObserver;
   private List<RtpSender> senders = new ArrayList<>();
   private List<RtpReceiver> receivers = new ArrayList<>();
 
@@ -557,12 +556,11 @@ public class PeerConnection {
    * their PeerConnection creation in JNI.
    */
   public PeerConnection(NativePeerConnectionFactory factory) {
-    this(factory.createNativePeerConnection(), 0 /* nativeObserver */);
+    this(factory.createNativePeerConnection());
   }
 
-  PeerConnection(long nativePeerConnection, long nativeObserver) {
+  PeerConnection(long nativePeerConnection) {
     this.nativePeerConnection = nativePeerConnection;
-    this.nativeObserver = nativeObserver;
   }
 
   // JsepInterface.
@@ -611,7 +609,7 @@ public class PeerConnection {
   }
 
   public boolean setConfiguration(RTCConfiguration config) {
-    return nativeSetConfiguration(config, nativeObserver);
+    return nativeSetConfiguration(config);
   }
 
   public boolean addIceCandidate(IceCandidate candidate) {
@@ -781,14 +779,16 @@ public class PeerConnection {
       receiver.dispose();
     }
     receivers.clear();
-    JniCommon.nativeReleaseRef(nativePeerConnection);
+    nativeFreeOwnedPeerConnection(nativePeerConnection);
-    if (nativeObserver != 0) {
-      nativeFreePeerConnectionObserver(nativeObserver);
   }
+
+  /** Returns a pointer to the native webrtc::PeerConnectionInterface. */
+  public long getNativePeerConnection() {
+    return nativeGetNativePeerConnection();
   }
 
   @CalledByNative
-  public long getNativePeerConnection() {
+  long getNativeOwnedPeerConnection() {
     return nativePeerConnection;
   }
 
@@ -796,10 +796,7 @@ public class PeerConnection {
     return nativeCreatePeerConnectionObserver(observer);
   }
 
-  public static void freeNativePeerConnectionObserver(long observer) {
+  private native long nativeGetNativePeerConnection();
-    nativeFreePeerConnectionObserver(observer);
-  }
-
   private native SessionDescription nativeGetLocalDescription();
   private native SessionDescription nativeGetRemoteDescription();
   private native DataChannel nativeCreateDataChannel(String label, DataChannel.Init init);
@@ -815,8 +812,8 @@ public class PeerConnection {
   private native IceGatheringState nativeIceGatheringState();
   private native void nativeClose();
   private static native long nativeCreatePeerConnectionObserver(Observer observer);
-  private static native void nativeFreePeerConnectionObserver(long observer);
+  private static native void nativeFreeOwnedPeerConnection(long ownedPeerConnection);
-  private native boolean nativeSetConfiguration(RTCConfiguration config, long nativeObserver);
+  private native boolean nativeSetConfiguration(RTCConfiguration config);
   private native boolean nativeAddIceCandidate(
       String sdpMid, int sdpMLineIndex, String iceCandidateSdp);
   private native boolean nativeRemoveIceCandidates(final IceCandidate[] candidates);

sdk/android/api/org/webrtc/PeerConnectionFactory.java

@@ -232,7 +232,7 @@ public class PeerConnectionFactory {
     if (nativePeerConnection == 0) {
       return null;
     }
-    return new PeerConnection(nativePeerConnection, nativeObserver);
+    return new PeerConnection(nativePeerConnection);
   }
 
   /**

sdk/android/src/jni/pc/peerconnection.cc

@@ -58,8 +58,9 @@ namespace {
 
 PeerConnectionInterface* ExtractNativePC(JNIEnv* jni,
                                          const JavaRef<jobject>& j_pc) {
-  return reinterpret_cast<PeerConnectionInterface*>(
+  return reinterpret_cast<OwnedPeerConnection*>(
-      Java_PeerConnection_getNativePeerConnection(jni, j_pc));
+             Java_PeerConnection_getNativeOwnedPeerConnection(jni, j_pc))
+      ->pc();
 }
 
 PeerConnectionInterface::IceServers JavaToNativeIceServers(
@@ -284,12 +285,6 @@ void PeerConnectionObserverJni::OnAddTrack(
                            NativeToJavaMediaStreamArray(env, streams));
 }
 
-void PeerConnectionObserverJni::SetConstraints(
-    std::unique_ptr<MediaConstraintsInterface> constraints) {
-  RTC_CHECK(!constraints_.get()) << "constraints already set!";
-  constraints_ = std::move(constraints);
-}
-
 // If the NativeToJavaStreamsMap contains the stream, return it.
 // Otherwise, create a new Java MediaStream.
 JavaMediaStream& PeerConnectionObserverJni::GetOrCreateJavaStream(
@@ -318,6 +313,19 @@ PeerConnectionObserverJni::NativeToJavaMediaStreamArray(
       });
 }
 
+OwnedPeerConnection::OwnedPeerConnection(
+    rtc::scoped_refptr<PeerConnectionInterface> peer_connection,
+    std::unique_ptr<PeerConnectionObserver> observer,
+    std::unique_ptr<MediaConstraintsInterface> constraints)
+    : peer_connection_(peer_connection),
+      observer_(std::move(observer)),
+      constraints_(std::move(constraints)) {}
+
+OwnedPeerConnection::~OwnedPeerConnection() {
+  // Ensure that PeerConnection is destroyed before the observer.
+  peer_connection_ = nullptr;
+}
+
 static jlong JNI_PeerConnection_CreatePeerConnectionObserver(
     JNIEnv* jni,
     const JavaParamRef<jclass>&,
@@ -325,12 +333,17 @@ static jlong JNI_PeerConnection_CreatePeerConnectionObserver(
   return jlongFromPointer(new PeerConnectionObserverJni(jni, j_observer));
 }
 
-static void JNI_PeerConnection_FreePeerConnectionObserver(
+static void JNI_PeerConnection_FreeOwnedPeerConnection(
     JNIEnv*,
     const JavaParamRef<jclass>&,
     jlong j_p) {
-  PeerConnectionObserver* p = reinterpret_cast<PeerConnectionObserver*>(j_p);
+  delete reinterpret_cast<OwnedPeerConnection*>(j_p);
-  delete p;
+}
+
+static jlong JNI_PeerConnection_GetNativePeerConnection(
+    JNIEnv* jni,
+    const JavaParamRef<jobject>& j_pc) {
+  return jlongFromPointer(ExtractNativePC(jni, j_pc));
 }
 
 static ScopedJavaLocalRef<jobject> JNI_PeerConnection_GetLocalDescription(
@@ -426,19 +439,18 @@ static void JNI_PeerConnection_SetAudioRecording(
 static jboolean JNI_PeerConnection_SetConfiguration(
     JNIEnv* jni,
     const JavaParamRef<jobject>& j_pc,
-    const JavaParamRef<jobject>& j_rtc_config,
+    const JavaParamRef<jobject>& j_rtc_config) {
-    jlong native_observer) {
   // Need to merge constraints into RTCConfiguration again, which are stored
-  // in the observer object.
+  // in the OwnedPeerConnection object.
-  PeerConnectionObserverJni* observer =
+  OwnedPeerConnection* owned_pc = reinterpret_cast<OwnedPeerConnection*>(
-      reinterpret_cast<PeerConnectionObserverJni*>(native_observer);
+      Java_PeerConnection_getNativeOwnedPeerConnection(jni, j_pc));
   PeerConnectionInterface::RTCConfiguration rtc_config(
       PeerConnectionInterface::RTCConfigurationType::kAggressive);
   JavaToNativeRTCConfiguration(jni, j_rtc_config, &rtc_config);
-  if (observer && observer->constraints()) {
+  if (owned_pc->constraints()) {
-    CopyConstraintsIntoRtcConfiguration(observer->constraints(), &rtc_config);
+    CopyConstraintsIntoRtcConfiguration(owned_pc->constraints(), &rtc_config);
   }
-  return ExtractNativePC(jni, j_pc)->SetConfiguration(rtc_config);
+  return owned_pc->pc()->SetConfiguration(rtc_config);
 }
 
 static jboolean JNI_PeerConnection_AddIceCandidate(

sdk/android/src/jni/pc/peerconnection.h

@@ -61,9 +61,6 @@ class PeerConnectionObserverJni : public PeerConnectionObserver {
                   const std::vector<rtc::scoped_refptr<MediaStreamInterface>>&
                       streams) override;
 
-  void SetConstraints(std::unique_ptr<MediaConstraintsInterface> constraints);
-  const MediaConstraintsInterface* constraints() { return constraints_.get(); }
-
  private:
   typedef std::map<MediaStreamInterface*, JavaMediaStream>
       NativeToJavaStreamsMap;
@@ -86,6 +83,36 @@ class PeerConnectionObserverJni : public PeerConnectionObserver {
   // C++ -> Java remote streams.
   NativeToJavaStreamsMap remote_streams_;
   std::vector<JavaRtpReceiverGlobalOwner> rtp_receivers_;
+};
+
+// PeerConnection doesn't take ownership of the observer. In Java API, we don't
+// want the client to have to manually dispose the observer. To solve this, this
+// wrapper class is used for object ownership.
+//
+// Also stores reference to the deprecated PeerConnection constraints for now.
+class OwnedPeerConnection {
+ public:
+  OwnedPeerConnection(
+      rtc::scoped_refptr<PeerConnectionInterface> peer_connection,
+      std::unique_ptr<PeerConnectionObserver> observer)
+      : OwnedPeerConnection(peer_connection,
+                            std::move(observer),
+                            nullptr /* constraints */) {}
+  // Deprecated. PC constraints are deprecated.
+  OwnedPeerConnection(
+      rtc::scoped_refptr<PeerConnectionInterface> peer_connection,
+      std::unique_ptr<PeerConnectionObserver> observer,
+      std::unique_ptr<MediaConstraintsInterface> constraints);
+  ~OwnedPeerConnection();
+
+  PeerConnectionInterface* pc() const { return peer_connection_.get(); }
+  const MediaConstraintsInterface* constraints() const {
+    return constraints_.get();
+  }
+
+ private:
+  rtc::scoped_refptr<PeerConnectionInterface> peer_connection_;
+  std::unique_ptr<PeerConnectionObserver> observer_;
   std::unique_ptr<MediaConstraintsInterface> constraints_;
 };
 

sdk/android/src/jni/pc/peerconnectionfactory.cc

@@ -417,6 +417,8 @@ static jlong JNI_PeerConnectionFactory_CreatePeerConnection(
   rtc::scoped_refptr<PeerConnectionFactoryInterface> f(
       reinterpret_cast<PeerConnectionFactoryInterface*>(
           factoryFromJava(factory)));
+  std::unique_ptr<PeerConnectionObserver> observer(
+      reinterpret_cast<PeerConnectionObserver*>(observer_p));
 
   PeerConnectionInterface::RTCConfiguration rtc_config(
       PeerConnectionInterface::RTCConfigurationType::kAggressive);
@@ -436,15 +438,15 @@ static jlong JNI_PeerConnectionFactory_CreatePeerConnection(
     rtc_config.certificates.push_back(certificate);
   }
 
-  PeerConnectionObserverJni* observer =
+  std::unique_ptr<MediaConstraintsInterface> constraints;
-      reinterpret_cast<PeerConnectionObserverJni*>(observer_p);
   if (!j_constraints.is_null()) {
-    observer->SetConstraints(JavaToNativeMediaConstraints(jni, j_constraints));
+    constraints = JavaToNativeMediaConstraints(jni, j_constraints);
-    CopyConstraintsIntoRtcConfiguration(observer->constraints(), &rtc_config);
+    CopyConstraintsIntoRtcConfiguration(constraints.get(), &rtc_config);
   }
   rtc::scoped_refptr<PeerConnectionInterface> pc(
-      f->CreatePeerConnection(rtc_config, nullptr, nullptr, observer));
+      f->CreatePeerConnection(rtc_config, nullptr, nullptr, observer.get()));
-  return jlongFromPointer(pc.release());
+  return jlongFromPointer(
+      new OwnedPeerConnection(pc, std::move(observer), std::move(constraints)));
 }
 
 static jlong JNI_PeerConnectionFactory_CreateVideoSource(