Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2611983002:
TrackMediaInfoMap added. (Closed)
DescriptionTrackMediaInfoMap added.
This maps, in both directions, [Audio/Video]TrackInterface with
[Voice/Video][Sender/Receiver]Info.
This mapping is necessary for RTCStatsCollector to know the relationship
between RTCMediaStreamTrackStats and RTC[In/Out]boundRTPStreamStats, and
to be able to collect several RTCMediaStreamTrackStats stats.
BUG=webrtc:6757, chromium:659137, chromium:657854, chromium:627816
Review-Url: https://codereview.webrtc.org/2611983002
Cr-Commit-Position: refs/heads/master@{#16090}
Committed: https://chromium.googlesource.com/external/webrtc/+/1f8239ca6f887ecb8c2c8347c00b5ac681e83c60
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed comments #
Total comments: 15
Patch Set 3 : Addressed comments (added comments) #
Total comments: 13
Patch Set 4 : Addressed comments, improved unittests #
Total comments: 18
Patch Set 5 : Addressed comments, less template, renames, refactoring, cleanup #
Messages
Total messages: 46 (31 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== TrackMediaInfoMap added. This maps [Audio/Video]TrackInterface with [Voice/Video][Sender/Receiver]Info. This mapping is necessary for RTCStatsCollector to know the relationship between RTCMediaStreamTrackStats and RTC[In/Out]boundRTPStreamStats, and to be able to collect several RTCMediaStreamTrackStats stats. BUG=webrtc:6757, chromium:659137, chromium:657854, chromium:627816 ========== to ========== TrackMediaInfoMap added. This maps, in both directions, [Audio/Video]TrackInterface with [Voice/Video][Sender/Receiver]Info. This mapping is necessary for RTCStatsCollector to know the relationship between RTCMediaStreamTrackStats and RTC[In/Out]boundRTPStreamStats, and to be able to collect several RTCMediaStreamTrackStats stats. BUG=webrtc:6757, chromium:659137, chromium:657854, chromium:627816 ==========
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by hbos@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/...
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org, pthatcher@webrtc.org
Please take a look, hta, deadbeef and pthatcher.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I didn't finish reviewing this CL; before going further we should decide what RTCMediaStreamTrackStats are expected when you have two RtpSenders sending the same track. If we need to produce two RTCMediaStreamTrackStats in that situation, then methods like "GetVoiceSenderInfos" aren't very useful; we'd want to start using RtpSenders as keys to the map rather than tracks. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:19: std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToTrack( If not bundling, it's theoretically possible to use the same SSRC for an audio and video track. So to be truly robust you'd need to use a combination of "media type" + SSRC as a key... And with unified plan SDP (where it's possible to have multiple m= sections for a given media type) you'd need "mid" (aka "content_name") + SSRC. Though if this is a corner case we don't care about supporting, we may be fine just documenting it with a bug for now. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:28: if (rtp_sender->ssrc() != 0) { Could add a TODO to remove this once RtpEncodingParameters.ssrc is populated in all cases (I'm working on that). https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:30: Add comment explaining that this can only be called once. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:42: Add comment explaining that Initialize must be called before using any of these. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:62: bool is_initialized_; nit: Can initialize this to false here, rather than in the constructor initialization list.
It's hard to evaluate the API without some use cases, but I've made some commentary. Agreed that we need to agree on the meaning of senderinfo before committing this. (for receivers, they're track sources, so a track always has a single RTPReceiver). https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/test/fakertpse... File webrtc/api/test/fakertpsenderreceiver.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/test/fakertpse... webrtc/api/test/fakertpsenderreceiver.cc:20: : media_type_(media_type), rtp_parameters_(parameters) { Despite my earlier comment about not having implementations only in the .h file, this seems like a case for the opposite - there's ~nothing here. Could you get by with only using gmock to construct these classes? Or is there a longer term thought that means a .cc implementation is more reasonable? https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:19: std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToTrack( On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > If not bundling, it's theoretically possible to use the same SSRC for an audio > and video track. So to be truly robust you'd need to use a combination of "media > type" + SSRC as a key... And with unified plan SDP (where it's possible to have > multiple m= sections for a given media type) you'd need "mid" (aka > "content_name") + SSRC. > > Though if this is a corner case we don't care about supporting, we may be fine > just documenting it with a bug for now. The ID for an SSRC (aka RTPMediaStream) should be transport + SSRC. I think that will be robust in all cases, since anything that tries to make that non-unique will be a violation of the RTP spec. (not that that ever stopped anyone....) https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:137: is_initialized_ = true; Here's the part that is not thread-safe (but it may all be a theoretical worry). For sanity, you might want to move the is_initialized to the end of the function. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:34: rtp_receivers); Hm. How do you expect to call this? In particular, is the construtcor and the initializer expected to be called on different threads, or is the assumption that you'd always create and initialize? It would be elegant to have the constructor take the senders & receivers as parameters - then the whole object is verifiably const afer construction, and therefore thread-safe. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap_unittest.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap_unittest.cc:227: video_media_info->senders.push_back(cricket::VideoSenderInfo()); There's a ton of repeated code here. Can you make a helper? Something like AddVideoSender(video_media_info, ssrc)? It would reduce the size of this test file by a large number of lines, and increase readability.
Addressed comments. Please take a look if we decide to to RTCMediaStreamTrackStats (or equivalent) on a per-attachment of track (or per track)-basis. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/test/fakertpse... File webrtc/api/test/fakertpsenderreceiver.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/test/fakertpse... webrtc/api/test/fakertpsenderreceiver.cc:20: : media_type_(media_type), rtp_parameters_(parameters) { On 2017/01/05 09:46:56, hta-webrtc wrote: > Despite my earlier comment about not having implementations only in the .h file, > this seems like a case for the opposite - there's ~nothing here. > > Could you get by with only using gmock to construct these classes? > Or is there a longer term thought that means a .cc implementation is more > reasonable? Done. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:19: std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToTrack( On 2017/01/05 09:46:56, hta-webrtc wrote: > On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > > If not bundling, it's theoretically possible to use the same SSRC for an audio > > and video track. So to be truly robust you'd need to use a combination of > "media > > type" + SSRC as a key... And with unified plan SDP (where it's possible to > have > > multiple m= sections for a given media type) you'd need "mid" (aka > > "content_name") + SSRC. > > > > Though if this is a corner case we don't care about supporting, we may be fine > > just documenting it with a bug for now. > > The ID for an SSRC (aka RTPMediaStream) should be transport + SSRC. I think that > will be robust in all cases, since anything that tries to make that non-unique > will be a violation of the RTP spec. > (not that that ever stopped anyone....) I don't see where I get the transport/mid/content names from. - The Rtp[Sender/Receiver] has: ssrcs, track, kind, id ("Not to be confused with "mid", this is a field we can temporarily use to uniquely identify a receiver until we implement Unified Plan SDP"). - The [Audio/Video]TrackInterface has: id, kind. - The Infos have: ssrcs, and we know what kind it is. But if the concern is an audio and video track can have the same SSRC, I can easily initialize the mapping by distinguishing between kind + ssrc (ssrc_to_audio_track, ssrc_to_video_track). I did this. This should be enough to uniquely and correctly map tracks and infos? https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:28: if (rtp_sender->ssrc() != 0) { On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > Could add a TODO to remove this once RtpEncodingParameters.ssrc is populated in > all cases (I'm working on that). Done. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:137: is_initialized_ = true; On 2017/01/05 09:46:56, hta-webrtc wrote: > Here's the part that is not thread-safe (but it may all be a theoretical worry). > > For sanity, you might want to move the is_initialized to the end of the > function. Merged Initialize with constructor. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:30: On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > Add comment explaining that this can only be called once. Initialize removed. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:34: rtp_receivers); On 2017/01/05 09:46:56, hta-webrtc wrote: > Hm. How do you expect to call this? > > In particular, is the construtcor and the initializer expected to be called on > different threads, or is the assumption that you'd always create and initialize? > > It would be elegant to have the constructor take the senders & receivers as > parameters - then the whole object is verifiably const afer construction, and > therefore thread-safe. > SGTM, merging constructor and Initialize. I only split it up because I think the style guide prefers constructors to be simple. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:42: On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > Add comment explaining that Initialize must be called before using any of these. Initialize removed. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:62: bool is_initialized_; On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > nit: Can initialize this to false here, rather than in the constructor > initialization list. is_initialized_ removed. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap_unittest.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap_unittest.cc:227: video_media_info->senders.push_back(cricket::VideoSenderInfo()); On 2017/01/05 09:46:56, hta-webrtc wrote: > There's a ton of repeated code here. > > Can you make a helper? > Something like > > AddVideoSender(video_media_info, ssrc)? > > It would reduce the size of this test file by a large number of lines, and > increase readability. Done.
The CQ bit was checked by hbos@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.
https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:53: RtpParameters params = rtp_sender->GetParameters(); Something I wanted to point out before I forget: this will invoke on the worker thread currently.
Seems good. Some questions about making code simpler, and one question that Taylor will probably have to answer: if ssrcs is > 1 element, why shouldn't we map all of them to the track? https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:22: template<> C++ ignorance: Why is there value in having the template<> marking when it templates nothing? Why can't these be just overloaded functions? https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:48: if (rtp_sender->ssrc() != 0) { Why isn't it correct to loop over all the ssrcs in rtp_sender->ssrcs() here, mapping them all to the same track? https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:180: cricket::VoiceReceiverInfo*>::const_iterator it = Would "auto" be usable here? Seems that C++ already has all the type information. https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:26: class TrackMediaInfoMap { Class comment: What is this class for, and why does it do it? https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:63: // Tracks to infos. Use complete sentences. "These maps map tracks (identified by a pointer) to their corresponding info object of the correct kind. One track maps to multiple info objects." https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:72: // Infos to tracks. These maps map info objects to their corresponding tracks. They are always the inverse of the maps above. One info object always maps to only one track.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by hbos@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.
PTAL hta and deadbeef. Regardless of the outcome of https://github.com/w3c/webrtc-stats/issues/130, this is used for RTC[In/Out]boundRTPStreamStats.mediaTrackId (https://codereview.webrtc.org/2610843003/). https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:22: template<> On 2017/01/11 12:34:24, hta-webrtc wrote: > C++ ignorance: Why is there value in having the template<> marking when it > templates nothing? Why can't these be just overloaded functions? Without the DCHECK I could remove this function and simply to a static_cast, but the DCHECK needs to either use MediaStreamTrackInterface::kAudioKind or kVideoKind depending on the type T. There's no "T::kKind" so a different function implementation is required depending on T, this is called function template specialization. You can't use normal function overloading because the argument has the same type in both cases, it's the return value that is different depending on T. You declare "CheckedTrackCast<T> is a thing", and then say "for calls to CheckedTrackCast<AudioTrackInterface>, use this implementation" and "for calls to CheckedTrackCast<VideoTrackInterface>, use this implementation". (CheckedTrackCast<Other> is undefined and gives link error.) https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:48: if (rtp_sender->ssrc() != 0) { On 2017/01/11 12:34:23, hta-webrtc wrote: > Why isn't it correct to loop over all the ssrcs in rtp_sender->ssrcs() here, > mapping them all to the same track? If I understand correctly, it's because the first is used as the identifier for that set of SSRCs. Why it is so I don't know. https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:53: RtpParameters params = rtp_sender->GetParameters(); On 2017/01/10 17:29:24, Taylor Brandstetter wrote: > Something I wanted to point out before I forget: this will invoke on the worker > thread currently. Thanks, I didn't realize [1] occurred. Ugh, but I'm guessing the proxy[2] is used, so even if we were on the worker thread we would do W -> S -> W and back. I added a TODO. [1] https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?sq=pack... [2] https://cs.chromium.org/chromium/src/third_party/webrtc/api/rtpsenderinterfac... https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:180: cricket::VoiceReceiverInfo*>::const_iterator it = On 2017/01/11 12:34:23, hta-webrtc wrote: > Would "auto" be usable here? Seems that C++ already has all the type > information. > Done. https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:26: class TrackMediaInfoMap { On 2017/01/11 12:34:24, hta-webrtc wrote: > Class comment: What is this class for, and why does it do it? Done. https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:63: // Tracks to infos. On 2017/01/11 12:34:24, hta-webrtc wrote: > Use complete sentences. "These maps map tracks (identified by a pointer) to > their corresponding info object of the correct kind. One track maps to multiple > info objects." > Done. https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.h:72: // Infos to tracks. On 2017/01/11 12:34:24, hta-webrtc wrote: > These maps map info objects to their corresponding tracks. They are always the > inverse of the maps above. One info object always maps to only one track. Done.
I still have the same question about whether we're gathering stats "per-track" or "per-track-attachement". But ignoring that, this looks good, I just noticed a minor discrepancy in the test. https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/60001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:19: std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToTrack( On 2017/01/05 15:27:56, hbos wrote: > On 2017/01/05 09:46:56, hta-webrtc wrote: > > On 2017/01/05 00:35:42, Taylor Brandstetter wrote: > > > If not bundling, it's theoretically possible to use the same SSRC for an > audio > > > and video track. So to be truly robust you'd need to use a combination of > > "media > > > type" + SSRC as a key... And with unified plan SDP (where it's possible to > > have > > > multiple m= sections for a given media type) you'd need "mid" (aka > > > "content_name") + SSRC. > > > > > > Though if this is a corner case we don't care about supporting, we may be > fine > > > just documenting it with a bug for now. > > > > The ID for an SSRC (aka RTPMediaStream) should be transport + SSRC. I think > that > > will be robust in all cases, since anything that tries to make that non-unique > > will be a violation of the RTP spec. > > (not that that ever stopped anyone....) > > I don't see where I get the transport/mid/content names from. > > - The Rtp[Sender/Receiver] has: ssrcs, track, kind, id ("Not to be confused with > "mid", this is a field we can temporarily use to uniquely identify a receiver > until we implement Unified Plan SDP"). > - The [Audio/Video]TrackInterface has: id, kind. > - The Infos have: ssrcs, and we know what kind it is. > > But if the concern is an audio and video track can have the same SSRC, I can > easily initialize the mapping by distinguishing between kind + ssrc > (ssrc_to_audio_track, ssrc_to_video_track). I did this. This should be enough to > uniquely and correctly map tracks and infos? Yes, this is the best you can do now. Currently "media type + SSRC" is what's used to link an Rtp[Sender/Receiver] to the underlying "[audio/video] [send/receive] stream". When we implement unified plan SDP that won't be enough, but we'll just need to cross that bridge when we come to it; it will need changes in more areas than just stats. https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/80001/webrtc/api/trackmediainfo... webrtc/api/trackmediainfomap.cc:53: RtpParameters params = rtp_sender->GetParameters(); On 2017/01/11 15:55:32, hbos wrote: > On 2017/01/10 17:29:24, Taylor Brandstetter wrote: > > Something I wanted to point out before I forget: this will invoke on the > worker > > thread currently. > > Thanks, I didn't realize [1] occurred. Ugh, but I'm guessing the proxy[2] is > used, so even if we were on the worker thread we would do W -> S -> W and back. > I added a TODO. > > [1] > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?sq=pack... > [2] > https://cs.chromium.org/chromium/src/third_party/webrtc/api/rtpsenderinterfac... Yes, if you call GetSenders you'll get proxies. But a pointer to the inner non-proxy interface is kept around, using this "ProxyWithInternal" thing I added: https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnection.h?... So when we fix this, we can just add a "GetInternalSenders()" method to PeerConnection (and not PeerConnectionInterface). https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:57: RtpParameters params = rtp_sender->GetParameters(); Given that currently, GetParameters will just return the same SSRC as "rtp_sender->ssrc()", you could just skip doing this until it becomes necessary. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.h:53: const AudioTrackInterface& local_audio_track) const; If we're going with "there's an RTCMediaStreamTrackStats for each *attachment*", then should RtpSenders and RtpReceivers be used as the keys to the map instead of tracks? https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap_unittest.cc (right): https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:45: cricket::MediaType media_type, std::initializer_list<uint32_t> ssrcs, I didn't know about std::initializer_list, this is neat. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:108: AddSenderInfos(ssrcs, local_track); This isn't quite right; if an RtpSender uses multiple SSRCs, there will be one VideoSenderInfo with multiple SSRCs. There should be a 1:1 mapping between Rtp[Sender/Receiver] and [Video/Voice][Sender/Receiver]Info, and I'm assuming this will continue to be true in the future. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:133: void AddRtpReceiverOnePerSsrc(std::initializer_list<uint32_t> ssrcs, nit: "AddRtpSendersOnePerSsrc" is plural ("senders"), "AddRtpReceiverOncePerSsrc" isn't https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:347: #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) Is it common to have tests for DCHECKs? I haven't seen that before, but it makes sense. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:358: TEST_F(TrackMediaInfoMapDeathTest, MultipleReceiversWithOneSsrc) { Maybe MultipleReceiversWithSameTrack would be a more fitting name?
The CQ bit was checked by hbos@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/...
PTAL hta, deadbeef. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:57: RtpParameters params = rtp_sender->GetParameters(); On 2017/01/11 22:12:05, Taylor Brandstetter wrote: > Given that currently, GetParameters will just return the same SSRC as > "rtp_sender->ssrc()", you could just skip doing this until it becomes necessary. Done. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.h:53: const AudioTrackInterface& local_audio_track) const; On 2017/01/11 22:12:05, Taylor Brandstetter wrote: > If we're going with "there's an RTCMediaStreamTrackStats for each *attachment*", > then should RtpSenders and RtpReceivers be used as the keys to the map instead > of tracks? No, I still do "for (each stream) { for (each track of that stream) { ... } }" and the sender/receiver is not used for anything we collect. The stats collection is concerned with infos and tracks, sender/receiver is just glue that I'd be happy to hide. This will become "for (each stream) { for (each track of that stream) { for (each info of that track) { ... } } }" (If sender/receiver becomes more important we can always update the map in a future CL.) https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap_unittest.cc (right): https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:108: AddSenderInfos(ssrcs, local_track); On 2017/01/11 22:12:05, Taylor Brandstetter wrote: > This isn't quite right; if an RtpSender uses multiple SSRCs, there will be one > VideoSenderInfo with multiple SSRCs. There should be a 1:1 mapping between > Rtp[Sender/Receiver] and [Video/Voice][Sender/Receiver]Info, and I'm assuming > this will continue to be true in the future. Done. Changed so that: AddRtp[Sender/Receiver]WithSsrcs: 1 sender/receiver with multiple ssrcs, 1 info with multiple ssrcs. AddRtp[Senders/Receivers]OnePerSSrc: replaced by doing one AddRtp[Sender/Receiver]WithSsrcs() call per ssrc at the call site. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:133: void AddRtpReceiverOnePerSsrc(std::initializer_list<uint32_t> ssrcs, On 2017/01/11 22:12:05, Taylor Brandstetter wrote: > nit: "AddRtpSendersOnePerSsrc" is plural ("senders"), > "AddRtpReceiverOncePerSsrc" isn't Function removed. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:347: #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) On 2017/01/11 22:12:05, Taylor Brandstetter wrote: > Is it common to have tests for DCHECKs? I haven't seen that before, but it makes > sense. This exact #if statement is commonly used for death tests. Copied from elsewhere. https://codereview.webrtc.org/2611983002/diff/120001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:358: TEST_F(TrackMediaInfoMapDeathTest, MultipleReceiversWithOneSsrc) { On 2017/01/11 22:12:05, Taylor Brandstetter wrote: > Maybe MultipleReceiversWithSameTrack would be a more fitting name? Need to take into account # senders/receivers and # ssrcs per sender/receiver without it sounding like it's the # ssrcs per track and without a silly long name. New naming convention: [Single/Multiple][One/Multi]Ssrc[Sender(s)/Receiver(s)]PerTrack
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, though it looks like there's a small typo in the test https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap_unittest.cc (right): https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:223: EXPECT_FALSE(map_->GetVoiceSenderInfos(*remote_audio_track_)); Meant to be ReceiverInfos?
Sorry for the late entrance. On most of these, you can push back if you disagree. But I generally prefer more simple code with less templating if it can be avoided. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:50: ssrc_to_track[rtp_sender->ssrc()] = CheckedTrackCast<T>( Do you really need all this templating complexity? I think you could get the same accomplished with something more simple: std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToTrack(...) { std::map<uint32_t, MediaStreamTrackInterface*> ssrc_to_track; // ... build the map // Put this in the right place in the for loop: RTC_DCHECK_EQ(track->kind(), kind); // Get the kind from the media_type passed in. return ssrc_to_track; } std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToAudioTrack(...) { return static_cast<std::map<uint32_t, AudioTrackInterface*>>(GetSsrcToTrack(...)); } std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToVideoTrack(...) { return static_cast<std::map<uint32_t, VideoTrackInterface*>>(GetSsrcToTrack(...)); } https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:80: TrackType* GetAssociatedTrack( You could have just made this FindOrNull and have it work for any map and then pass in the key (use FindOrNull(info.ssrc()). Then you could use it in the 6 places below where you do the same FindOrNull logic. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:83: typename std::map<uint32_t, TrackType*>::const_iterator it = I think an "auto it =" would work well here. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:97: typename RemoteTrackToInfosType> I'd personally prefer just duplicating the logic twice rather than having a method with 6 template arguments. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:114: } I might make sense write a generic "invert map" method. Then build one and invert to get the other. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:233: return it->second; It's curious that you templatized the "return null if it's not in the map" logic for the case when you call it 2 times, but not for the case when you call it 6 times. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.h:76: local_audio_track_to_infos_; Would std::multimap make sense? We use it in call.cc for things like this. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.h:87: rtc::scoped_refptr<AudioTrackInterface>> voice_sender_info_to_track_; I prefer, and usually see in our code y_by_x instead of x_to_y.
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by hbos@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/...
PTAL hta, pthatcher https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.cc (right): https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:50: ssrc_to_track[rtp_sender->ssrc()] = CheckedTrackCast<T>( On 2017/01/13 00:10:29, pthatcher1 wrote: > Do you really need all this templating complexity? I think you could get the > same accomplished with something more simple: > > std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToTrack(...) { > std::map<uint32_t, MediaStreamTrackInterface*> ssrc_to_track; > // ... build the map > // Put this in the right place in the for loop: > RTC_DCHECK_EQ(track->kind(), kind); // Get the kind from the media_type > passed in. > return ssrc_to_track; > } > > std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToAudioTrack(...) { > return static_cast<std::map<uint32_t, > AudioTrackInterface*>>(GetSsrcToTrack(...)); > } > > std::map<uint32_t, MediaStreamTrackInterface*> GetSsrcToVideoTrack(...) { > return static_cast<std::map<uint32_t, > VideoTrackInterface*>>(GetSsrcToTrack(...)); > } > > Done. I also merged it into one function that gets both audio and video and added local variables to hold result of sender/receiver method calls. This in order to reduce the number of calls that do thread jumps until the TODO is addressed about RTP sender/receiver being proxy to signaling doing work on worker thread. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:80: TrackType* GetAssociatedTrack( On 2017/01/13 00:10:29, pthatcher1 wrote: > You could have just made this FindOrNull and have it work for any map and then > pass in the key (use FindOrNull(info.ssrc()). Then you could use it in the 6 > places below where you do the same FindOrNull logic. Done. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:83: typename std::map<uint32_t, TrackType*>::const_iterator it = On 2017/01/13 00:10:29, pthatcher1 wrote: > I think an "auto it =" would work well here. Done. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:97: typename RemoteTrackToInfosType> On 2017/01/13 00:10:29, pthatcher1 wrote: > I'd personally prefer just duplicating the logic twice rather than having a > method with 6 template arguments. Done. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:114: } On 2017/01/13 00:10:29, pthatcher1 wrote: > I might make sense write a generic "invert map" method. Then build one and > invert to get the other. I skipped this one because I think it would increase, not decrease, complexity, for this particular case. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.cc:233: return it->second; On 2017/01/13 00:10:29, pthatcher1 wrote: > It's curious that you templatized the "return null if it's not in the map" logic > for the case when you call it 2 times, but not for the case when you call it 6 > times. Done. Generic FindValueOrNull / FindAddressOrNull are the only template functions now. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap.h (right): https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.h:76: local_audio_track_to_infos_; On 2017/01/13 00:10:29, pthatcher1 wrote: > Would std::multimap make sense? We use it in call.cc for things like this. That works but then I'd have to make Get[Voice/Video]SenderInfos return a pair of iterators, and expose the map's end() iterator or rely on std::distance == 0 or rtc::Optional. std::equal, std::distance, etc I find more messy than working with std::vector. Or if I'm constructing a vector for the return value I might as well map it like I do. I'm keeping std::vector unless there is a good reason to switch. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap.h:87: rtc::scoped_refptr<AudioTrackInterface>> voice_sender_info_to_track_; On 2017/01/13 00:10:29, pthatcher1 wrote: > I prefer, and usually see in our code y_by_x instead of x_to_y. Done. https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... File webrtc/api/trackmediainfomap_unittest.cc (right): https://codereview.webrtc.org/2611983002/diff/140001/webrtc/api/trackmediainf... webrtc/api/trackmediainfomap_unittest.cc:223: EXPECT_FALSE(map_->GetVoiceSenderInfos(*remote_audio_track_)); On 2017/01/12 23:23:47, Taylor Brandstetter wrote: > Meant to be ReceiverInfos? Done. Nice catch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm LGTM Very nice
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2611983002/#ps180001 (title: "Addressed comments, less template, renames, refactoring, cleanup")
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": 180001, "attempt_start_ts": 1484565672903970,
"parent_rev": "be02dcdc4f7a8529d45c0a21666841dfe231a808", "commit_rev":
"1f8239ca6f887ecb8c2c8347c00b5ac681e83c60"}
Message was sent while issue was closed.
Description was changed from ========== TrackMediaInfoMap added. This maps, in both directions, [Audio/Video]TrackInterface with [Voice/Video][Sender/Receiver]Info. This mapping is necessary for RTCStatsCollector to know the relationship between RTCMediaStreamTrackStats and RTC[In/Out]boundRTPStreamStats, and to be able to collect several RTCMediaStreamTrackStats stats. BUG=webrtc:6757, chromium:659137, chromium:657854, chromium:627816 ========== to ========== TrackMediaInfoMap added. This maps, in both directions, [Audio/Video]TrackInterface with [Voice/Video][Sender/Receiver]Info. This mapping is necessary for RTCStatsCollector to know the relationship between RTCMediaStreamTrackStats and RTC[In/Out]boundRTPStreamStats, and to be able to collect several RTCMediaStreamTrackStats stats. BUG=webrtc:6757, chromium:659137, chromium:657854, chromium:627816 Review-Url: https://codereview.webrtc.org/2611983002 Cr-Commit-Position: refs/heads/master@{#16090} Committed: https://chromium.googlesource.com/external/webrtc/+/1f8239ca6f887ecb8c2c8347c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/1f8239ca6f887ecb8c2c8347c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
