Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686823002:
Add option to print information about configured SSRCs from RTC event logs. (Closed)
DescriptionAdd option to print information about configured SSRCs from RTC event logs.
BUG=webrtc:7118
Review-Url: https://codereview.webrtc.org/2686823002
Cr-Commit-Position: refs/heads/master@{#16500}
Committed: https://chromium.googlesource.com/external/webrtc/+/bb46b95dbea0857651c32883efc02bbac0f90c09
Patch Set 1 #
Total comments: 8
Patch Set 2 : Label SSRCs as ssrc or feedback_ssrc in the output. Remove some debug logging. #
Messages
Total messages: 21 (11 generated)
terelius@webrtc.org changed reviewers: + danilchap@webrtc.org
Description was changed from ========== Add option to print information about configured SSRCs from RTC event logs. BUG=webrtc:7118 ========== to ========== Add option to print information about configured SSRCs from RTC event logs. BUG=webrtc:7118 ==========
terelius@webrtc.org changed reviewers: + kwiberg@webrtc.org
lgtm https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc may be label them remote/local rather than ssrc/rtcp ssrc https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:382: << "\tSSRC=" << config.rtp.remote_ssrc ditto
+kwiberg for build file. https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc On 2017/02/08 14:30:50, danilchap wrote: > may be label them remote/local rather than ssrc/rtcp ssrc Do you find local/remote to be more clear? The local ssrc is only used for RTCP, right?
https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc On 2017/02/08 14:41:05, terelius wrote: > On 2017/02/08 14:30:50, danilchap wrote: > > may be label them remote/local rather than ssrc/rtcp ssrc > > Do you find local/remote to be more clear? The local ssrc is only used for RTCP, > right? Is it? as I understand rtp sender care about local ssrc only rtp receiver care about remote ssrc only rtcp (both sender and receiver) use both.
https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc On 2017/02/08 14:44:44, danilchap wrote: > On 2017/02/08 14:41:05, terelius wrote: > > On 2017/02/08 14:30:50, danilchap wrote: > > > may be label them remote/local rather than ssrc/rtcp ssrc > > > > Do you find local/remote to be more clear? The local ssrc is only used for > RTCP, > > right? > > Is it? as I understand > rtp sender care about local ssrc only > rtp receiver care about remote ssrc only > rtcp (both sender and receiver) use both. I agree, but since this is a receive stream there should not be any RTP sender, right? So that only leaves RTCP using local ssrc.
https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc On 2017/02/08 14:49:11, terelius wrote: > On 2017/02/08 14:44:44, danilchap wrote: > > On 2017/02/08 14:41:05, terelius wrote: > > > On 2017/02/08 14:30:50, danilchap wrote: > > > > may be label them remote/local rather than ssrc/rtcp ssrc > > > > > > Do you find local/remote to be more clear? The local ssrc is only used for > > RTCP, > > > right? > > > > Is it? as I understand > > rtp sender care about local ssrc only > > rtp receiver care about remote ssrc only > > rtcp (both sender and receiver) use both. > > I agree, but since this is a receive stream there should not be any RTP sender, > right? So that only leaves RTCP using local ssrc. Right, I'm still not sure about 'rtcp' label - it is id of the receiver endpoint, but just happen to appear in rtcp messages only. How about label 'Feedback ssrc'?
https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc On 2017/02/08 15:41:45, danilchap wrote: > On 2017/02/08 14:49:11, terelius wrote: > > On 2017/02/08 14:44:44, danilchap wrote: > > > On 2017/02/08 14:41:05, terelius wrote: > > > > On 2017/02/08 14:30:50, danilchap wrote: > > > > > may be label them remote/local rather than ssrc/rtcp ssrc > > > > > > > > Do you find local/remote to be more clear? The local ssrc is only used for > > > RTCP, > > > > right? > > > > > > Is it? as I understand > > > rtp sender care about local ssrc only > > > rtp receiver care about remote ssrc only > > > rtcp (both sender and receiver) use both. > > > > I agree, but since this is a receive stream there should not be any RTP > sender, > > right? So that only leaves RTCP using local ssrc. > > Right, > I'm still not sure about 'rtcp' label - it is id of the receiver endpoint, but > just happen to appear in rtcp messages only. > How about label 'Feedback ssrc'? I'm fine with any label. Do you prefer "ssrc"/"feedback_ssrc" over "local_ssrc"/"remote_ssrc"?
https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2686823002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:359: << "\tSSRC=" << config.rtp.remote_ssrc On 2017/02/08 15:55:42, terelius wrote: > On 2017/02/08 15:41:45, danilchap wrote: > > On 2017/02/08 14:49:11, terelius wrote: > > > On 2017/02/08 14:44:44, danilchap wrote: > > > > On 2017/02/08 14:41:05, terelius wrote: > > > > > On 2017/02/08 14:30:50, danilchap wrote: > > > > > > may be label them remote/local rather than ssrc/rtcp ssrc > > > > > > > > > > Do you find local/remote to be more clear? The local ssrc is only used > for > > > > RTCP, > > > > > right? > > > > > > > > Is it? as I understand > > > > rtp sender care about local ssrc only > > > > rtp receiver care about remote ssrc only > > > > rtcp (both sender and receiver) use both. > > > > > > I agree, but since this is a receive stream there should not be any RTP > > sender, > > > right? So that only leaves RTCP using local ssrc. > > > > Right, > > I'm still not sure about 'rtcp' label - it is id of the receiver endpoint, but > > just happen to appear in rtcp messages only. > > How about label 'Feedback ssrc'? > > I'm fine with any label. Do you prefer "ssrc"/"feedback_ssrc" over > "local_ssrc"/"remote_ssrc"? Yes, I think ssrc/feedback_ssrc is better.
The CQ bit was checked by terelius@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 terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2686823002/#ps20001 (title: "Label SSRCs as ssrc or feedback_ssrc in the output. Remove some debug logging.")
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": 20001, "attempt_start_ts": 1486575273579600,
"parent_rev": "ed1850a71ba0801b05c1374fce1188cc3234ce17", "commit_rev":
"bb46b95dbea0857651c32883efc02bbac0f90c09"}
Message was sent while issue was closed.
Description was changed from ========== Add option to print information about configured SSRCs from RTC event logs. BUG=webrtc:7118 ========== to ========== Add option to print information about configured SSRCs from RTC event logs. BUG=webrtc:7118 Review-Url: https://codereview.webrtc.org/2686823002 Cr-Commit-Position: refs/heads/master@{#16500} Committed: https://chromium.googlesource.com/external/webrtc/+/bb46b95dbea0857651c32883e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/bb46b95dbea0857651c32883e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
