Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531333003:
Create the Java Wrapper of RtpReceiverObserverInterface. (Closed)
DescriptionCreate the Java Wrapper of RtpReceiverObserverInterface.
Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface.
The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received.
BUG=webrtc:6742
Committed: https://crrev.com/c4adabf967e6965cc83de43ec27631085e29663f
Cr-Commit-Position: refs/heads/master@{#15464}
Patch Set 1 : Fix the memory leak. #Patch Set 2 : sync #
Total comments: 16
Patch Set 3 : CR comments. #Patch Set 4 : Fix the failed test. #
Messages
Total messages: 47 (29 generated)
zhihuang@webrtc.org changed reviewers: + glaznev@webrtc.org
Hi Alex, Please take a look. Thanks. https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... File webrtc/api/android/java/src/org/webrtc/RtpReceiver.java (right): https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:18: public void onFirstPacketReceived(MediaStreamTrack.MediaType media_type); Is there a good way to support the default implementation here?
Description was changed from ========== Create the Java Wrapper of RtpReceiverObserverInterface. Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface. The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received. BUG=webrtc:6742 ========== to ========== Create the Java Wrapper of RtpReceiverObserverInterface. Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface. The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received. BUG=webrtc:6742 ==========
glaznev@webrtc.org changed reviewers: + deadbeef@webrtc.org
Aside from a potential memory leak, looks good. https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... File webrtc/api/android/java/src/org/webrtc/RtpReceiver.java (right): https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:17: // Called when first audio and video packet receive. nit: "Called when the first audio or video packet is received." https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:18: public void onFirstPacketReceived(MediaStreamTrack.MediaType media_type); On 2016/11/29 22:32:43, Zhi Huang wrote: > Is there a good way to support the default implementation here? We should figure out if there's anything preventing us from using the default interface feature (see: https://developer.android.com/guide/platform/j8-jack.html). If we can start using it, we definitely should. It would benefit the other CL you're working on too (onAddTrack). https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/jni/peerco... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/jni/peerco... webrtc/api/android/jni/peerconnection_jni.cc:854: : "MEDIA_TYPE_VIDEO"; nit: Could add an "RTC_DCHECK(media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO)" https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/jni/peerco... webrtc/api/android/jni/peerconnection_jni.cc:2468: return jlongFromPointer(rtpReceiverObserver); If setObserver is called twice with a different observer, won't there be a memory leak?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Please take another look. Thanks. https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... File webrtc/api/android/java/src/org/webrtc/RtpReceiver.java (right): https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:17: // Called when first audio and video packet receive. On 2016/12/01 02:12:44, Taylor Brandstetter wrote: > nit: "Called when the first audio or video packet is received." Done. https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:18: public void onFirstPacketReceived(MediaStreamTrack.MediaType media_type); On 2016/12/01 02:12:44, Taylor Brandstetter wrote: > On 2016/11/29 22:32:43, Zhi Huang wrote: > > Is there a good way to support the default implementation here? > > We should figure out if there's anything preventing us from using the default > interface feature (see: > https://developer.android.com/guide/platform/j8-jack.html). If we can start > using it, we definitely should. It would benefit the other CL you're working on > too (onAddTrack). Java8 feature is only supported in Andriod 7.0 (api level 24) while the minimum supported sdk for Duo is level 16. (https://cs.corp.google.com/piper///depot/google3/third_party/tachyon/src/com/...) Alex, any comment on this? https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/jni/peerco... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/jni/peerco... webrtc/api/android/jni/peerconnection_jni.cc:854: : "MEDIA_TYPE_VIDEO"; On 2016/12/01 02:12:44, Taylor Brandstetter wrote: > nit: Could add an "RTC_DCHECK(media_type == cricket::MEDIA_TYPE_AUDIO || > media_type == cricket::MEDIA_TYPE_VIDEO)" Done. https://codereview.webrtc.org/2531333003/diff/1/webrtc/api/android/jni/peerco... webrtc/api/android/jni/peerconnection_jni.cc:2468: return jlongFromPointer(rtpReceiverObserver); On 2016/12/01 02:12:44, Taylor Brandstetter wrote: > If setObserver is called twice with a different observer, won't there be a > memory leak? Ah yes. This is a good point. Thanks for pointing out. I'll unset and delete the previous one before setting a new one.
lgtm By the way, I changed the visibility to "Protected" due to the mention of Duo. It probably doesn't matter in this case, but in the future, just try to speak more generally about internal products. For example, "it would require targeting Android API level 24, which some downstream applications may not be targeting".
On 2016/12/01 20:21:34, Taylor Brandstetter wrote: > lgtm > > By the way, I changed the visibility to "Protected" due to the mention of Duo. > It probably doesn't matter in this case, but in the future, just try to speak > more generally about internal products. For example, "it would require targeting > Android API level 24, which some downstream applications may not be targeting". Oh, I'll be careful next time. Thanks for pointing out. Can I change the visibility to back to Protected after landing this? Because I cannot use Commit checkbox in this mode and I cannot use the "git cl land" either.
lgtm
Patchset #1 (id:1) has been deleted
The CQ bit was checked by zhihuang@google.com
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
Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/13063) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15408) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15267) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19347) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20705)
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 Link to the patchset: https://codereview.webrtc.org/2531333003/#ps80001 (title: "sync")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10981)
zhihuang@webrtc.org changed reviewers: + magjed@webrtc.org - deadbeef@webrtc.org, glaznev@webrtc.org, zhihuang@google.com
Hi, Please take a look. Thanks!
https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/RtpReceiver.java (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:51: nativeUnsetObserver(nativeRtpReceiver, nativeObserver); I would feel safer with if (nativeObserver != 0) { nativeUnsetObserver(nativeRtpReceiver, nativeObserver); nativeObserver = 0; } https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/cl... File webrtc/api/android/jni/classreferenceholder.cc (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/cl... webrtc/api/android/jni/classreferenceholder.cc:72: LoadClass(jni, "org/webrtc/MediaStreamTrack$MediaType"); No need to add it here https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:840: j_observer_class_(jni, GetObjectClass(jni, j_observer)) {} I think it's better if don't store this as a global ref and use GetObjectClass(jni, *j_observer_global_) when needed instead. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:845: jmethodID j_on_first_packet_received_mid = I would prefer if you do JNIEnv*const jni = AttachCurrentThreadIfNeeded(); at the top of this function and remove the jni() function. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:850: FindClass(jni(), "org/webrtc/MediaStreamTrack$MediaType"); do jni()->FindClass("org/webrtc/MediaStreamTrack$MediaType") instead. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:853: media_type == cricket::MEDIA_TYPE_VIDEO); maybe you should print media_type in this dcheck https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:854: std::string media_type_str = media_type == cricket::MEDIA_TYPE_AUDIO use const char* instead
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/...
Hi, Please take another looks. Thanks! https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/RtpReceiver.java (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/RtpReceiver.java:51: nativeUnsetObserver(nativeRtpReceiver, nativeObserver); On 2016/12/06 17:13:55, magjed_webrtc wrote: > I would feel safer with > if (nativeObserver != 0) { > nativeUnsetObserver(nativeRtpReceiver, nativeObserver); > nativeObserver = 0; > } Done. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/cl... File webrtc/api/android/jni/classreferenceholder.cc (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/cl... webrtc/api/android/jni/classreferenceholder.cc:72: LoadClass(jni, "org/webrtc/MediaStreamTrack$MediaType"); On 2016/12/06 17:13:55, magjed_webrtc wrote: > No need to add it here Done. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:840: j_observer_class_(jni, GetObjectClass(jni, j_observer)) {} On 2016/12/06 17:13:56, magjed_webrtc wrote: > I think it's better if don't store this as a global ref and use > GetObjectClass(jni, *j_observer_global_) when needed instead. Done. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:845: jmethodID j_on_first_packet_received_mid = On 2016/12/06 17:13:55, magjed_webrtc wrote: > I would prefer if you do > JNIEnv*const jni = AttachCurrentThreadIfNeeded(); > at the top of this function and remove the jni() function. Done. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:850: FindClass(jni(), "org/webrtc/MediaStreamTrack$MediaType"); On 2016/12/06 17:13:55, magjed_webrtc wrote: > do jni()->FindClass("org/webrtc/MediaStreamTrack$MediaType") instead. Done. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:853: media_type == cricket::MEDIA_TYPE_VIDEO); On 2016/12/06 17:13:55, magjed_webrtc wrote: > maybe you should print media_type in this dcheck Done. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:854: std::string media_type_str = media_type == cricket::MEDIA_TYPE_AUDIO On 2016/12/06 17:13:56, magjed_webrtc wrote: > use const char* instead Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/19275)
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/...
Hi, I have some updates. Please ignore the previous CR request. Thanks. https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:850: FindClass(jni(), "org/webrtc/MediaStreamTrack$MediaType"); On 2016/12/06 17:13:55, magjed_webrtc wrote: > do jni()->FindClass("org/webrtc/MediaStreamTrack$MediaType") instead. Are you suggesting jni->FindClass() ? It cannot find the class in this way. The theory is that it fails because this function is called from a C++ thread.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2531333003/diff/80001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:850: FindClass(jni(), "org/webrtc/MediaStreamTrack$MediaType"); On 2016/12/07 02:04:50, Zhi Huang wrote: > On 2016/12/06 17:13:55, magjed_webrtc wrote: > > do jni()->FindClass("org/webrtc/MediaStreamTrack$MediaType") instead. > > Are you suggesting jni->FindClass() ? > > It cannot find the class in this way. The theory is that it fails because this > function is called from a C++ thread. Ok, that was strange. It's not a big deal, so let's do it this way then.
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 Link to the patchset: https://codereview.webrtc.org/2531333003/#ps120001 (title: "Fix the failed test.")
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": 1481135675011750,
"parent_rev": "a410216cb8021ce481d0a777f674322c109dc829", "commit_rev":
"11a3a5e3befd23a8440f9d386abf34f95479f50e"}
Message was sent while issue was closed.
Description was changed from ========== Create the Java Wrapper of RtpReceiverObserverInterface. Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface. The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received. BUG=webrtc:6742 ========== to ========== Create the Java Wrapper of RtpReceiverObserverInterface. Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface. The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received. BUG=webrtc:6742 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Create the Java Wrapper of RtpReceiverObserverInterface. Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface. The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received. BUG=webrtc:6742 ========== to ========== Create the Java Wrapper of RtpReceiverObserverInterface. Create the RtpReceiver.Observer which is a Java wrapper over the webrtc::RtpReceiverObserverInterface. The callback function onFirstPacketReceived will be called whenever the first audio or video packet it received. BUG=webrtc:6742 Committed: https://crrev.com/c4adabf967e6965cc83de43ec27631085e29663f Cr-Commit-Position: refs/heads/master@{#15464} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c4adabf967e6965cc83de43ec27631085e29663f Cr-Commit-Position: refs/heads/master@{#15464} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
