Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505173002:
Added a callback function OnAddTrack to PeerConnectionObserver (Closed)
DescriptionAdded a callback function OnAddTrack to PeerConnectionObserver
Added the callback in 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
Committed: https://crrev.com/81c3a0300486f49ca5dae5c0a7f50ae858dd78fa
Cr-Commit-Position: refs/heads/master@{#15142}
Patch Set 1 : Add a callback function to PeerConnectionObserver. #Patch Set 2 : Simple change on java and obj-c. #
Total comments: 11
Patch Set 3 : CR comments. Remove unrelated file changes. #
Total comments: 8
Patch Set 4 : CR comments. Remove unrelated file changes. #
Messages
Total messages: 31 (21 generated)
Description was changed from ========== Added a callback function OnAddTrack to PeerConnectionObserver Added the callback in native c++ API. The callback function is called when a track is added and a new RtpReceiver is created. The application can tell when tracked are added to the streams by listening to this callback. BUG=webrtc:6112 ========== to ========== Added a callback function OnAddTrack to PeerConnectionObserver Added the callback in 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 ==========
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/...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12566)
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
Looks great. I just noticed a couple minor things; mainly that the new method can't be pure virtual, or we'll break downstream dependencies. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... File webrtc/api/peerconnectionfactory_unittest.cc (right): https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectionfactory_unittest.cc:90: rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver, Good idea putting just the receiver in the event, since you can get the track from the receiver. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:599: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) = 0; Since Chromium has PeerConnectionObserver subclasses, this will need to have a default implementation too in order to not break Chromium. Just like the above two methods with the "TODO(honghaiz)" comment. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:580: int num_added_track_ = 0; nit: I'd change this to "num_added_tracks_", plural https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:2672: // This tests the PeerConnectionObserver::OnAddTrack are correctly called. nit: "the" -> "that", "are" -> "is" https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:2679: observer_.last_added_track_label_ = ""; These don't need to be initialized here since they're initialized in the constructor. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:2691: observer_.last_added_track_label_ = ""; This isn't needed, since each test case completely reconstructs the test object. P.S.: This does make you wonder, "then what is the SetUp method for?" The answer is "for things you can't do in a constructor"; see https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul.... This surprised me when I first learned about it.
Patchset #3 (id:60001) has been deleted
honghaiz@webrtc.org changed reviewers: + honghaiz@webrtc.org
Thanks for figuring this out!
https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1430: streams.push_back(rtc::scoped_refptr<MediaStreamInterface>(stream)); Is there any reason to use a vector of streams here? https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:421: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) override {} You don't need this if the base class has implemented it as an empty method. https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:599: virtual void OnAddTrack( Should we also Add OnRemoveTrack? If users expect to be notified when tracks are added, they may also expect to be notified when tracks are removed.
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/...
Patchset #4 (id:90001) has been deleted
Dry run: Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19488) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20209)
Patchset #4 (id:110001) has been deleted
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/...
I removed all the unnecessary code changes since we have a default implementation. Please take another look. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:599: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) = 0; On 2016/11/17 00:34:23, Taylor Brandstetter wrote: > Since Chromium has PeerConnectionObserver subclasses, this will need to have a > default implementation too in order to not break Chromium. Just like the above > two methods with the "TODO(honghaiz)" comment. Ah, I see. I thought that "TODO" is something we can do now. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:580: int num_added_track_ = 0; On 2016/11/17 00:34:23, Taylor Brandstetter wrote: > nit: I'd change this to "num_added_tracks_", plural Done. I also removed the initialization of last_added_track_label which is not necessary. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:2672: // This tests the PeerConnectionObserver::OnAddTrack are correctly called. On 2016/11/17 00:34:23, Taylor Brandstetter wrote: > nit: "the" -> "that", "are" -> "is" Done. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:2679: observer_.last_added_track_label_ = ""; On 2016/11/17 00:34:23, Taylor Brandstetter wrote: > These don't need to be initialized here since they're initialized in the > constructor. Done. https://codereview.webrtc.org/2505173002/diff/40001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:2691: observer_.last_added_track_label_ = ""; On 2016/11/17 00:34:23, Taylor Brandstetter wrote: > This isn't needed, since each test case completely reconstructs the test object. > > P.S.: This does make you wonder, "then what is the SetUp method for?" The answer > is "for things you can't do in a constructor"; see > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul.... > This surprised me when I first learned about it. Good to know. Thanks for the link! https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1430: streams.push_back(rtc::scoped_refptr<MediaStreamInterface>(stream)); On 2016/11/17 01:19:02, honghaiz3 wrote: > Is there any reason to use a vector of streams here? According to the spec: https://www.w3.org/TR/webrtc/#dom-rtctrackevent the event has a sequence of MediaStream. I think this means a track can be attached to multiple streams. We may call this OnAddTrack() function once we encounter that case. https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:421: std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams) override {} On 2016/11/17 01:19:02, honghaiz3 wrote: > You don't need this if the base class has implemented it as an empty method. Done. https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:599: virtual void OnAddTrack( On 2016/11/17 01:19:02, honghaiz3 wrote: > Should we also Add OnRemoveTrack? > If users expect to be notified when tracks are added, they may also expect to be > notified when tracks are removed. According to spec https://www.w3.org/TR/webrtc/#event-track, the event is fired when "A new incoming MediaStreamTrack has been created, and an associated RTCRtpReceiver has been added to the set of receivers.". I agree with you that it would make sense to have another callback. But I think that can comes later in a separate CL, we can land this first so that this won't be a blocker for both iOS and android.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16307) win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16198)
https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1431: observer_->OnAddTrack(receiver, streams); Should we just get the track from the receiver and use that as the argument? Something like observer_->OnAddTrack(receiver->track(), stream)?
On 2016/11/17 01:52:31, honghaiz3 wrote: > https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection.cc > File webrtc/api/peerconnection.cc (right): > > https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... > webrtc/api/peerconnection.cc:1431: observer_->OnAddTrack(receiver, streams); > Should we just get the track from the receiver and use that as the argument? > Something like observer_->OnAddTrack(receiver->track(), stream)? Ignore my last comment. We need the rtp_receiver based on w3c.
lgtm https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2505173002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1430: streams.push_back(rtc::scoped_refptr<MediaStreamInterface>(stream)); On 2016/11/17 01:42:06, Zhi Huang wrote: > On 2016/11/17 01:19:02, honghaiz3 wrote: > > Is there any reason to use a vector of streams here? > > According to the spec: https://www.w3.org/TR/webrtc/#dom-rtctrackevent > the event has a sequence of MediaStream. I think this means a track can be > attached to multiple streams. We may call this OnAddTrack() function once we > encounter that case. I agree about this. Even if we take a long time to support multiple streams for one track (or never support it), we might as well be forward thinking so we don't need to change the callback later.
The CQ bit was checked by zhihuang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Added a callback function OnAddTrack to PeerConnectionObserver Added the callback in 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 ========== Added a callback function OnAddTrack to PeerConnectionObserver Added the callback in 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Added a callback function OnAddTrack to PeerConnectionObserver Added the callback in 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 ========== Added a callback function OnAddTrack to PeerConnectionObserver Added the callback in 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 Committed: https://crrev.com/81c3a0300486f49ca5dae5c0a7f50ae858dd78fa Cr-Commit-Position: refs/heads/master@{#15142} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/81c3a0300486f49ca5dae5c0a7f50ae858dd78fa Cr-Commit-Position: refs/heads/master@{#15142} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
