Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673403002:
New tool for printing basic packet information from an RTC event log to stdout. (Closed)
DescriptionNew tool for printing basic packet information from an RTC event log to stdout.
BUG=webrtc:7118
Review-Url: https://codereview.webrtc.org/2673403002
Cr-Commit-Position: refs/heads/master@{#16488}
Committed: https://chromium.googlesource.com/external/webrtc/+/d4ed7f59e4866d812cffa89a10feefabe1733bb2
Patch Set 1 #
Total comments: 13
Patch Set 2 : Nits #
Total comments: 3
Patch Set 3 : Clarify when packet type is unknown #Patch Set 4 : Parse RTCP correctly #
Messages
Total messages: 24 (12 generated)
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/...
terelius@webrtc.org changed reviewers: + ivoc@webrtc.org, stefan@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks like a useful tool. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:12: // #include <memory> Please remove if it's not needed. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:26: DEFINE_bool(nooutgoing, false, "Excludes incoming packets."); I assume this should be "Excludes outgoing packets". https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:27: // TODO(terelius): Note that the media type don't work with outgoing packets. doesn't https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:97: return "()"; I think it would be good to add an RTC_NOTREACHED here. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:102: // This utility will convert a stored event log to the rtpdump format. Please update comment :-) https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:136: // RTP dumps based on broken event logs. Please update comment.
Sorry about the partially updated comments. :) https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:12: // #include <memory> On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > Please remove if it's not needed. Done. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:26: DEFINE_bool(nooutgoing, false, "Excludes incoming packets."); On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > I assume this should be "Excludes outgoing packets". Done. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:27: // TODO(terelius): Note that the media type don't work with outgoing packets. On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > doesn't Done. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:97: return "()"; On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > I think it would be good to add an RTC_NOTREACHED here. It isn't unreachable; both direction and media_type can be ANY. In fact it is reached in practice for outgoing RTCP packets. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:102: // This utility will convert a stored event log to the rtpdump format. On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > Please update comment :-) Done. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:136: // RTP dumps based on broken event logs. On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > Please update comment. Done.
LGTM. https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2673403002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:97: return "()"; On 2017/02/06 17:09:43, terelius wrote: > On 2017/02/06 16:38:11, ivoc-OOO until feb 6 wrote: > > I think it would be good to add an RTC_NOTREACHED here. > > It isn't unreachable; both direction and media_type can be ANY. In fact it is > reached in practice for outgoing RTCP packets. Acknowledged.
rs-lgtm
danilchap@webrtc.org changed reviewers: + danilchap@webrtc.org
thank you for the tool! https://codereview.webrtc.org/2673403002/diff/20001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2673403002/diff/20001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:21: #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/common_header.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h" (see comment below) https://codereview.webrtc.org/2673403002/diff/20001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:87: } may be add return "(out)" to at least log direction when media_type is unsure; or add a case media_type == webrtc::MediaType::ALL https://codereview.webrtc.org/2673403002/diff/20001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:167: // Parse header to get SSRC and RTP time. rtcp are not rtp packets. As a start might be enough to parse sender reports: // Parse rtcp to get SSRC and RTP time. webrtc::rtcp::CommonHeader rtcp_header; webrtc::rtcp::SenderReport sr; if (!rtcp_header.Parse(packet, length)) continue; if (rtcp_header.type() != webrtc::rtcp::SenderReport::kPacketType) continue; if (!sr.Parse(rtcp_header)) continue; if (ExcludePacket(direction, media_type, sr.sender_ssrc(), filtered_ssrc)) continue; sr.sender_ssrc(); sr.rtp_timestamp();
Thanks Danil. I have made a bunch of changes. Do you have more comments?
lgtm
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2673403002/#ps60001 (title: "Parse RTCP correctly")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17242) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17068) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22505)
The CQ bit was checked by terelius@webrtc.org
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": 60001, "attempt_start_ts": 1486554646824890,
"parent_rev": "abcef5d32eeee7a5050873e6bec5a668addca405", "commit_rev":
"d4ed7f59e4866d812cffa89a10feefabe1733bb2"}
Message was sent while issue was closed.
Description was changed from ========== New tool for printing basic packet information from an RTC event log to stdout. BUG=webrtc:7118 ========== to ========== New tool for printing basic packet information from an RTC event log to stdout. BUG=webrtc:7118 Review-Url: https://codereview.webrtc.org/2673403002 Cr-Commit-Position: refs/heads/master@{#16488} Committed: https://chromium.googlesource.com/external/webrtc/+/d4ed7f59e4866d812cffa89a1... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/d4ed7f59e4866d812cffa89a1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
