Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513723002:
Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer (Closed)
DescriptionCreated a java wrapper for the callback OnAddTrack to PeerConnection.Observer
Created a java wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API
The callback function is called when a track is signaled by remote side and a new RtpReceiver is created.
The application can tell when tracks are added to the streams by listening to this callback.
BUG=webrtc:6112
Review-Url: https://codereview.webrtc.org/2513723002
Cr-Commit-Position: refs/heads/master@{#15745}
Committed: https://chromium.googlesource.com/external/webrtc/+/dcccda7e7c5e0e68cc0e8b6b401d5b1a119893c2
Patch Set 1 : Create java wrapper for callback. #
Total comments: 14
Patch Set 2 : Address the CR comments. #
Total comments: 15
Patch Set 3 : CR comments. Modified the function signature. #Patch Set 4 : Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer #
Messages
Total messages: 30 (17 generated)
Description was changed from ========== Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer A callback OnAddTrack has been added to native C++ API and a java wrapper is created for it in this CL. The callback function is called when a track is added and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ========== to ========== Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer Created a java wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is added and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
The only real problem I see is that onAddTrack creates a new Java RtpReceiver wrapper, which adds a reference to the track underneath... So unless the application remembers to call "dispose", the track will be leaked, even after "dispose"-ing the PeerConnection. To address this, I think we should do something similar to "NativeToJavaStreamsMap remote_streams_". Each signaled receiver is added to a map, and the object is disposed in PCOJava's destructor. "getReceivers" could also use this data structure. Right now, every time it's called it destroys every Java-land RtpReceiver and recreates it, which is not ideal. But that doesn't need to happen in this CL. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/PeerConnection.java:84: /** Triggered when a track is added to streams. */ nit: I think a more descriptive comment would be "Triggered when a new track is signaled by the remote peer, as a result of setRemoteDescription." Or something similar. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (left): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:1916: GetMethodID(jni, j_rtp_receiver_class, "<init>", "(J)V"); Can now use j_rtp_receiver_class_ and j_rtp_receiver_ctor_ here. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:385: CHECK_EXCEPTION(jni()) << "error during NewObject"; Need to call receiver->AddRef() after this, like in nativeGetReceivers. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:455: CHECK_EXCEPTION(jni) << "error during NewObject"; I think this CHECK_EXCEPTION needs to be right after "NewObjectArray". And the message should be "error during NewObjectArray". https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:457: } There should be an "else" with an RTC_DCHECK. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:885: // So the onAddTrack is expected to be called once. nit: "the onAddTrack callback" https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:890: // Finally, remove the audio track as well, which should completely remove This comment should be updated.
Please take another look. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/PeerConnection.java:84: /** Triggered when a track is added to streams. */ On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > nit: I think a more descriptive comment would be "Triggered when a new track is > signaled by the remote peer, as a result of setRemoteDescription." Or something > similar. Done. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (left): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:1916: GetMethodID(jni, j_rtp_receiver_class, "<init>", "(J)V"); On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > Can now use j_rtp_receiver_class_ and j_rtp_receiver_ctor_ here. They are not in the same scope https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:385: CHECK_EXCEPTION(jni()) << "error during NewObject"; On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > Need to call receiver->AddRef() after this, like in nativeGetReceivers. Good catch! This could be a potential problem. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:455: CHECK_EXCEPTION(jni) << "error during NewObject"; On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > I think this CHECK_EXCEPTION needs to be right after "NewObjectArray". And the > message should be "error during NewObjectArray". Done. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:457: } On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > There should be an "else" with an RTC_DCHECK. What about always creating new stream if we cannot find one in the map? https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:885: // So the onAddTrack is expected to be called once. On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > nit: "the onAddTrack callback" Done. https://codereview.webrtc.org/2513723002/diff/40001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:890: // Finally, remove the audio track as well, which should completely remove On 2016/11/17 23:02:53, Taylor Brandstetter wrote: > This comment should be updated. Done.
lgtm
zhihuang@webrtc.org changed reviewers: + glaznev@webrtc.org
Hi Alex, Please take a look. Thanks.
Description was changed from ========== Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer Created a java wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is added and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ========== to ========== Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer Created a java wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ==========
lgtm
skvlad@webrtc.org changed reviewers: + skvlad@webrtc.org
https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:420: GetMethodID(jni(), *j_rtp_receiver_class_, "dispose", "()V")); Calling dispose() from Java will call Release() on the RefCountInterface each time, potentially bringing it down to zero while it is still in use. Either make dispose() private (so that only JNI code can call it, not Java clients), or add a check to prevent it from being called more than once. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:422: DeleteGlobalRef(jni(), j_rtp_receiver); You could just store ScopedGlobalRefs in the map and avoid explicit creation and destruction of global refs. Either way is fine though.
https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:420: GetMethodID(jni(), *j_rtp_receiver_class_, "dispose", "()V")); On 2016/11/28 21:35:42, skvlad wrote: > Calling dispose() from Java will call Release() on the RefCountInterface each > time, potentially bringing it down to zero while it is still in use. Either make > dispose() private (so that only JNI code can call it, not Java clients), or add > a check to prevent it from being called more than once. That's possible before this CL, though. I agree it's bad, but it's a fundamental problem with how "dispose" works in our Java API, and I think it needs to be addressed separately.
zhihuang@webrtc.org changed reviewers: + magjed@webrtc.org - deadbeef@webrtc.org, glaznev@webrtc.org, skvlad@webrtc.org
Hi, I have some changes on the PeerConnectionTest.java Please take a look. Thanks.
https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/PeerConnection.java:84: /** Triggered when a new track is signaled by the remote peer, as a result of nit: multiline Javadoc blocks should be formatted like: /** * Multiple lines of Javadoc text are written here, * wrapped normally... */ https://google.github.io/styleguide/javaguide.html#s7-javadoc https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:378: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) override { this should be const-ref. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:381: *j_rtp_receiver_class_, j_rtp_receiver_ctor_, (jlong)receiver.get()); It's dangerous to cast the pointer to a jlong that way, you need to use the jlongFromPointer() function. Same for the other places in this CL where a cast to jlong is needed. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:457: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) { const-ref https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:462: for (rtc::scoped_refptr<MediaStreamInterface> stream : streams) { I think it would be cleaner with: for (size_t i = 0; i < streams.size(); ++i) { jobject j_stream = GetOrCreateJavaStream(streams[i]); jni->SetObjectArrayElement(java_streams, i, j_stream); } https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:472: rtc::scoped_refptr<MediaStreamInterface> stream) { Use const-ref, or raw MediaStreamInterface*, I'm not sure what's best. This is a very unusual case where we do manual AddRef().
Patchset #3 (id:80001) has been deleted
Hello, Please take another look. Thanks! https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/PeerConnection.java:84: /** Triggered when a new track is signaled by the remote peer, as a result of On 2016/12/02 12:19:46, magjed_webrtc wrote: > nit: multiline Javadoc blocks should be formatted like: > /** > * Multiple lines of Javadoc text are written here, > * wrapped normally... > */ > https://google.github.io/styleguide/javaguide.html#s7-javadoc Done. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:378: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) override { On 2016/12/02 12:19:46, magjed_webrtc wrote: > this should be const-ref. Oh this is my fault. Hopefully it's not too late to change the C++ interface. Thanks for pointing out. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:381: *j_rtp_receiver_class_, j_rtp_receiver_ctor_, (jlong)receiver.get()); On 2016/12/02 12:19:46, magjed_webrtc wrote: > It's dangerous to cast the pointer to a jlong that way, you need to use the > jlongFromPointer() function. Same for the other places in this CL where a cast > to jlong is needed. Done. There are many places, besides this CL, using (jlong) for casting. Maybe it makes sense to make a separate CL to replacing all the dangerous usage. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:457: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) { On 2016/12/02 12:19:46, magjed_webrtc wrote: > const-ref Done. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:462: for (rtc::scoped_refptr<MediaStreamInterface> stream : streams) { On 2016/12/02 12:19:47, magjed_webrtc wrote: > I think it would be cleaner with: > for (size_t i = 0; i < streams.size(); ++i) { > jobject j_stream = GetOrCreateJavaStream(streams[i]); > jni->SetObjectArrayElement(java_streams, i, j_stream); > } Done. https://codereview.webrtc.org/2513723002/diff/60001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:472: rtc::scoped_refptr<MediaStreamInterface> stream) { On 2016/12/02 12:19:46, magjed_webrtc wrote: > Use const-ref, or raw MediaStreamInterface*, I'm not sure what's best. This is a > very unusual case where we do manual AddRef(). Done.
lgtm
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org, deadbeef@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2513723002/#ps120001 (title: "Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482357666474240,
"parent_rev": "b820f9c92e833d585429c62418a212f752c2d009", "commit_rev":
"dcccda7e7c5e0e68cc0e8b6b401d5b1a119893c2"}
Message was sent while issue was closed.
Description was changed from ========== Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer Created a java wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 ========== to ========== Created a java wrapper for the callback OnAddTrack to PeerConnection.Observer Created a java wrapper for the callback OnAddTrack in this CL since it has been added to native C++ API The callback function is called when a track is signaled by remote side and a new RtpReceiver is created. The application can tell when tracks are added to the streams by listening to this callback. BUG=webrtc:6112 Review-Url: https://codereview.webrtc.org/2513723002 Cr-Commit-Position: refs/heads/master@{#15745} Committed: https://chromium.googlesource.com/external/webrtc/+/dcccda7e7c5e0e68cc0e8b6b4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/dcccda7e7c5e0e68cc0e8b6b4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
