Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918103002:
Make rtc_event_log2text output header extensions (Closed)
DescriptionMake rtc_event_log2text output header extensions
BUG=webrtc:none
Review-Url: https://codereview.webrtc.org/2918103002
Cr-Commit-Position: refs/heads/master@{#18530}
Committed: https://chromium.googlesource.com/external/webrtc/+/a8e781aedf16102e1b6f68a1b2bea371cd2aed2d
Patch Set 1 #
Total comments: 5
Patch Set 2 : Implement Terelius@ comments #Patch Set 3 : Move extension maps map to rtc_event_log_parser #Patch Set 4 : Add comment about colliding video and audio ssrcs and extension maps #
Total comments: 4
Messages
Total messages: 28 (8 generated)
ilnik@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:393: webrtc::RtpHeaderExtensionMap(config.rtp_extensions); Maybe this should be parsed from audio configs too even though it's not yet written to the log? WDYT? https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << "\tAbsSendTime=" Is this the format that perkj suggested?
ilnik@webrtc.org changed reviewers: + perkj@chromium.org
https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:393: webrtc::RtpHeaderExtensionMap(config.rtp_extensions); On 2017/06/02 10:55:03, terelius wrote: > Maybe this should be parsed from audio configs too even though it's not yet > written to the log? WDYT? Done. https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << "\tAbsSendTime=" On 2017/06/02 10:55:03, terelius wrote: > Is this the format that perkj suggested? No. It was something quick I used for myself. Do we even need special format for this text tool? +perkj, WDYT about this reporting format?
https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << "\tAbsSendTime=" On 2017/06/02 10:55:03, terelius wrote: > Is this the format that perkj suggested? We had an offline discussion with perkj@. He doesn't have any opinion on report format, but he asked me to refactor things a little, so extension map is extracted and returned by ParsedRtcEventLog. That way there's no need to copy here the code from tools/event_log_visualizer/analyzer.cc. I am working on it.
On 2017/06/02 11:43:35, ilnik wrote: > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << > "\tAbsSendTime=" > On 2017/06/02 10:55:03, terelius wrote: > > Is this the format that perkj suggested? > > We had an offline discussion with perkj@. He doesn't have any opinion on report > format, but he asked me to refactor things a little, so extension map is > extracted and returned by ParsedRtcEventLog. That way there's no need to copy > here the code from tools/event_log_visualizer/analyzer.cc. I am working on it. I think there should be a function that returns a fully parsed RtpHeader, including the extensions, in ParsedRtcEventLog.
On 2017/06/02 11:47:52, perkj_webrtc wrote: > On 2017/06/02 11:43:35, ilnik wrote: > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << > > "\tAbsSendTime=" > > On 2017/06/02 10:55:03, terelius wrote: > > > Is this the format that perkj suggested? > > > > We had an offline discussion with perkj@. He doesn't have any opinion on > report > > format, but he asked me to refactor things a little, so extension map is > > extracted and returned by ParsedRtcEventLog. That way there's no need to copy > > here the code from tools/event_log_visualizer/analyzer.cc. I am working on it. > > I think there should be a function that returns a fully parsed RtpHeader, > including the extensions, in ParsedRtcEventLog. Long term, I think ParsedRtcEventLog should contain a std::vector<LoggedRtpPacket> and a std::map<StreamID, std::vector<LoggedRtpPacket*>> that lets you easily iterate over the packets based on stream. I don't think this has to be done now though.
On 2017/06/02 12:53:39, terelius wrote: > On 2017/06/02 11:47:52, perkj_webrtc wrote: > > On 2017/06/02 11:43:35, ilnik wrote: > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << > > > "\tAbsSendTime=" > > > On 2017/06/02 10:55:03, terelius wrote: > > > > Is this the format that perkj suggested? > > > > > > We had an offline discussion with perkj@. He doesn't have any opinion on > > report > > > format, but he asked me to refactor things a little, so extension map is > > > extracted and returned by ParsedRtcEventLog. That way there's no need to > copy > > > here the code from tools/event_log_visualizer/analyzer.cc. I am working on > it. > > > > I think there should be a function that returns a fully parsed RtpHeader, > > including the extensions, in ParsedRtcEventLog. > > Long term, I think ParsedRtcEventLog should contain a > std::vector<LoggedRtpPacket> and a std::map<StreamID, > std::vector<LoggedRtpPacket*>> that lets you easily iterate over the packets > based on stream. I don't think this has to be done now though. I've moved identical code from analyzer.cc and newly added rtc_event_log2text to rtc_event_log_parser.
Description was changed from ========== Make rtc_event_log2text output header extensions BUG=webrtc:none ========== to ========== Make rtc_event_log2text output header extensions BUG=webrtc:none ==========
ilnik@webrtc.org changed reviewers: + perkj@webrtc.org - perkj@chromium.org
On 2017/06/02 13:50:30, ilnik wrote: > On 2017/06/02 12:53:39, terelius wrote: > > On 2017/06/02 11:47:52, perkj_webrtc wrote: > > > On 2017/06/02 11:43:35, ilnik wrote: > > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << > > > > "\tAbsSendTime=" > > > > On 2017/06/02 10:55:03, terelius wrote: > > > > > Is this the format that perkj suggested? > > > > > > > > We had an offline discussion with perkj@. He doesn't have any opinion on > > > report > > > > format, but he asked me to refactor things a little, so extension map is > > > > extracted and returned by ParsedRtcEventLog. That way there's no need to > > copy > > > > here the code from tools/event_log_visualizer/analyzer.cc. I am working on > > it. > > > > > > I think there should be a function that returns a fully parsed RtpHeader, > > > including the extensions, in ParsedRtcEventLog. > > > > Long term, I think ParsedRtcEventLog should contain a > > std::vector<LoggedRtpPacket> and a std::map<StreamID, > > std::vector<LoggedRtpPacket*>> that lets you easily iterate over the packets > > based on stream. I don't think this has to be done now though. > > I've moved identical code from analyzer.cc and newly added rtc_event_log2text to > rtc_event_log_parser. There is a small issue here. Suppose a stream/SSRC is reconfigured from video (with video rotation extension) to audio (with audio level header extension). If we first parse all configs and then go back to interpret the packets, we're going to interpret all packets as audio with audio level extensions. The proper way would be to interpret packets before the reconfiguration as video and remaining ones as audio. This is of course unlikely to happen in practice so maybe it shouldn't block this CL, but it is the reason why I wanted to parse all configs and packets in a single pass.
On 2017/06/02 14:12:53, terelius wrote: > On 2017/06/02 13:50:30, ilnik wrote: > > On 2017/06/02 12:53:39, terelius wrote: > > > On 2017/06/02 11:47:52, perkj_webrtc wrote: > > > > On 2017/06/02 11:43:35, ilnik wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > > > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > > > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << > > > > > "\tAbsSendTime=" > > > > > On 2017/06/02 10:55:03, terelius wrote: > > > > > > Is this the format that perkj suggested? > > > > > > > > > > We had an offline discussion with perkj@. He doesn't have any opinion on > > > > report > > > > > format, but he asked me to refactor things a little, so extension map is > > > > > extracted and returned by ParsedRtcEventLog. That way there's no need to > > > copy > > > > > here the code from tools/event_log_visualizer/analyzer.cc. I am working > on > > > it. > > > > > > > > I think there should be a function that returns a fully parsed RtpHeader, > > > > including the extensions, in ParsedRtcEventLog. > > > > > > Long term, I think ParsedRtcEventLog should contain a > > > std::vector<LoggedRtpPacket> and a std::map<StreamID, > > > std::vector<LoggedRtpPacket*>> that lets you easily iterate over the packets > > > based on stream. I don't think this has to be done now though. > > > > I've moved identical code from analyzer.cc and newly added rtc_event_log2text > to > > rtc_event_log_parser. > > There is a small issue here. Suppose a stream/SSRC is reconfigured from video > (with video rotation extension) to audio (with audio level header extension). If > we first parse all configs and then go back to interpret the packets, we're > going to interpret all packets as audio with audio level extensions. The proper > way would be to interpret packets before the reconfiguration as video and > remaining ones as audio. > > This is of course unlikely to happen in practice so maybe it shouldn't block > this CL, but it is the reason why I wanted to parse all configs and packets in a > single pass. We are not doing it anywhere. Of course, someone trying to destroy our rtc_event_log tools may produce such example, but the worst case here is that extensions would not be shown in that specific log. I think, the amount of work to fix that greatly outweights benefit here. I am adding a comment about that for future generations.
Patchset #5 (id:80001) has been deleted
On 2017/06/05 07:54:35, ilnik wrote: > On 2017/06/02 14:12:53, terelius wrote: > > On 2017/06/02 13:50:30, ilnik wrote: > > > On 2017/06/02 12:53:39, terelius wrote: > > > > On 2017/06/02 11:47:52, perkj_webrtc wrote: > > > > > On 2017/06/02 11:43:35, ilnik wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > > > > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2918103002/diff/1/webrtc/logging/rtc_event_log/... > > > > > > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:481: std::cout << > > > > > > "\tAbsSendTime=" > > > > > > On 2017/06/02 10:55:03, terelius wrote: > > > > > > > Is this the format that perkj suggested? > > > > > > > > > > > > We had an offline discussion with perkj@. He doesn't have any opinion > on > > > > > report > > > > > > format, but he asked me to refactor things a little, so extension map > is > > > > > > extracted and returned by ParsedRtcEventLog. That way there's no need > to > > > > copy > > > > > > here the code from tools/event_log_visualizer/analyzer.cc. I am > working > > on > > > > it. > > > > > > > > > > I think there should be a function that returns a fully parsed > RtpHeader, > > > > > including the extensions, in ParsedRtcEventLog. > > > > > > > > Long term, I think ParsedRtcEventLog should contain a > > > > std::vector<LoggedRtpPacket> and a std::map<StreamID, > > > > std::vector<LoggedRtpPacket*>> that lets you easily iterate over the > packets > > > > based on stream. I don't think this has to be done now though. > > > > > > I've moved identical code from analyzer.cc and newly added > rtc_event_log2text > > to > > > rtc_event_log_parser. > > > > There is a small issue here. Suppose a stream/SSRC is reconfigured from video > > (with video rotation extension) to audio (with audio level header extension). > If > > we first parse all configs and then go back to interpret the packets, we're > > going to interpret all packets as audio with audio level extensions. The > proper > > way would be to interpret packets before the reconfiguration as video and > > remaining ones as audio. > > > > This is of course unlikely to happen in practice so maybe it shouldn't block > > this CL, but it is the reason why I wanted to parse all configs and packets in > a > > single pass. > > We are not doing it anywhere. Of course, someone trying to destroy our > rtc_event_log tools may produce such example, but the worst case here is that > extensions would not be shown in that specific log. I think, the amount of work > to fix that greatly outweights benefit here. I am adding a comment about that > for future generations. So, should I return to how it was done before (patchset 2) and land that, if there are no other comments? Or is this version fine?
lgtm I still think the parsing of all configs and header extensions should be done in a single pass, but I can do that in a follow-up CL.
On 2017/06/07 15:43:12, terelius wrote: > lgtm > > I still think the parsing of all configs and header extensions should be done in > a single pass, but I can do that in a follow-up CL. No need to do that. Look at patchset #2. I that no problems could be caused by reconfiguration of streams, us config events are parsed in the same pass as packets. There is kind-of double pass, though: ParsedRtcEventLog::ParseFile() and then code in log2text.cc:main(). There's also some copypaste from event_log_visualizer/analyzer.cc, although. Or do you want to implement parsing of extensions also inside ParsedRtcEventLog::ParseFile?
https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:444: webrtc::RtpHeaderExtensionMap* extension_map = parsed_stream.GetRtpHeader( Why can't this return a webrtc::RtpHeader directly?
https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:444: webrtc::RtpHeaderExtensionMap* extension_map = parsed_stream.GetRtpHeader( On 2017/06/07 16:49:57, perkj_webrtc wrote: > Why can't this return a webrtc::RtpHeader directly? I don't know why it was done like that. Maybe designers of ParsedRtcEventLog thought to make it more flexible like that. What if some user would want to print the bytes of a header itself to check a corrupted header?
https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:444: webrtc::RtpHeaderExtensionMap* extension_map = parsed_stream.GetRtpHeader( On 2017/06/08 08:43:43, ilnik wrote: > On 2017/06/07 16:49:57, perkj_webrtc wrote: > > Why can't this return a webrtc::RtpHeader directly? > I don't know why it was done like that. Maybe designers of ParsedRtcEventLog > thought to make it more flexible like that. What if some user would want to > print the bytes of a header itself to check a corrupted header? RTPHeader doesn't contain direction, media_type or length fields. And, as Ilya pointed out, it can't parse corrupt headers. In retrospect, we almost never care about corrupt headers, so it would indeed be more convenient to get a fully parsed header. Eventually I think that the parser should contain std::vector<LoggedRtpPacket> incoming_packets_; std::vector<LoggedRtpPacket> outgoing_packets_; std::map<StreamId, std::vector<LoggedRtpPacket*>> incoming_packets_by_stream_; std::map<StreamId, std::vector<LoggedRtpPacket*>> outgoing_packets_by_stream_; These would be populated as soon as the file is read. The analysis tools can then access the packets via getters const std::vector<LoggedRtpPacket>& incoming_packets(); All other event types (RTCP, BWE, Audio playouts, etc) would be handled in the same manner. This is slightly less flexible, but it matches the way we actually use the events in the analysis tools.
On 2017/06/07 16:11:42, ilnik wrote: > On 2017/06/07 15:43:12, terelius wrote: > > lgtm > > > > I still think the parsing of all configs and header extensions should be done > in > > a single pass, but I can do that in a follow-up CL. > > No need to do that. Look at patchset #2. I that no problems could be caused by > reconfiguration of streams, us config events are parsed in the same pass as > packets. There is kind-of double pass, though: ParsedRtcEventLog::ParseFile() > and then code in log2text.cc:main(). There's also some copypaste from > event_log_visualizer/analyzer.cc, although. > > Or do you want to implement parsing of extensions also inside > ParsedRtcEventLog::ParseFile? Yes, I mean parsing everything that will ever need to be parsed inside ParseFile.
https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log2text.cc:444: webrtc::RtpHeaderExtensionMap* extension_map = parsed_stream.GetRtpHeader( On 2017/06/08 09:40:28, terelius wrote: > On 2017/06/08 08:43:43, ilnik wrote: > > On 2017/06/07 16:49:57, perkj_webrtc wrote: > > > Why can't this return a webrtc::RtpHeader directly? > > I don't know why it was done like that. Maybe designers of ParsedRtcEventLog > > thought to make it more flexible like that. What if some user would want to > > print the bytes of a header itself to check a corrupted header? > > RTPHeader doesn't contain direction, media_type or length fields. And, as Ilya > pointed out, it can't parse corrupt headers. In retrospect, we almost never care > about corrupt headers, so it would indeed be more convenient to get a fully > parsed header. > > Eventually I think that the parser should contain std::vector<LoggedRtpPacket> > incoming_packets_; > std::vector<LoggedRtpPacket> outgoing_packets_; > std::map<StreamId, std::vector<LoggedRtpPacket*>> incoming_packets_by_stream_; > std::map<StreamId, std::vector<LoggedRtpPacket*>> outgoing_packets_by_stream_; > These would be populated as soon as the file is read. The analysis tools can > then access the packets via getters > const std::vector<LoggedRtpPacket>& incoming_packets(); > All other event types (RTCP, BWE, Audio playouts, etc) would be handled in the > same manner. This is slightly less flexible, but it matches the way we actually > use the events in the analysis tools. Looks like there'll be a massive overhaul of rtp_log_parser in the future. But it should be in a separate CL. Perkj@, do you approve of this CL? May I land it?
On 2017/06/08 09:44:16, ilnik wrote: > https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right): > > https://codereview.webrtc.org/2918103002/diff/60001/webrtc/logging/rtc_event_... > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:444: > webrtc::RtpHeaderExtensionMap* extension_map = parsed_stream.GetRtpHeader( > On 2017/06/08 09:40:28, terelius wrote: > > On 2017/06/08 08:43:43, ilnik wrote: > > > On 2017/06/07 16:49:57, perkj_webrtc wrote: > > > > Why can't this return a webrtc::RtpHeader directly? > > > I don't know why it was done like that. Maybe designers of > ParsedRtcEventLog > > > thought to make it more flexible like that. What if some user would want to > > > print the bytes of a header itself to check a corrupted header? > > > > RTPHeader doesn't contain direction, media_type or length fields. And, as Ilya > > pointed out, it can't parse corrupt headers. In retrospect, we almost never > care > > about corrupt headers, so it would indeed be more convenient to get a fully > > parsed header. > > > > Eventually I think that the parser should contain std::vector<LoggedRtpPacket> > > incoming_packets_; > > std::vector<LoggedRtpPacket> outgoing_packets_; > > std::map<StreamId, std::vector<LoggedRtpPacket*>> incoming_packets_by_stream_; > > std::map<StreamId, std::vector<LoggedRtpPacket*>> outgoing_packets_by_stream_; > > These would be populated as soon as the file is read. The analysis tools can > > then access the packets via getters > > const std::vector<LoggedRtpPacket>& incoming_packets(); > > All other event types (RTCP, BWE, Audio playouts, etc) would be handled in the > > same manner. This is slightly less flexible, but it matches the way we > actually > > use the events in the analysis tools. > > Looks like there'll be a massive overhaul of rtp_log_parser in the future. But > it should be in a separate CL. > > Perkj@, do you approve of this CL? May I land it? You don't need my approval. Terelius decide. But I still don't undertand why you dont let the parser parse the header for you and one way or the other return a parsed header instead of doing it in several steps. webrtc::RTPHeader parsed_header; parsed_stream.GetRtpHeader(i, &direction, header, &header_length, &parsed_header); or maybe webrtc::RTPHeader parsed_header; CHECK(parsed_stream.GetRtpHeader(i, &direction, &parsed_header)); if you want the parser to return a bool if it can't parse the packet.
The CQ bit was checked by ilnik@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": 1497253211375110,
"parent_rev": "3fae6280944d71f0f34d926304e8dd9b22873f6b", "commit_rev":
"a8e781aedf16102e1b6f68a1b2bea371cd2aed2d"}
Message was sent while issue was closed.
Description was changed from ========== Make rtc_event_log2text output header extensions BUG=webrtc:none ========== to ========== Make rtc_event_log2text output header extensions BUG=webrtc:none Review-Url: https://codereview.webrtc.org/2918103002 Cr-Commit-Position: refs/heads/master@{#18530} Committed: https://chromium.googlesource.com/external/webrtc/+/a8e781aedf16102e1b6f68a1b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/a8e781aedf16102e1b6f68a1b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
