Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1768773002:
New parser for event log. Manually parse the outermost EventStream (Closed)
|
Created:
4 years, 9 months ago by terelius Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, hlundin-webrtc, yujie_mao (webrtc), kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc, the sun, pbos-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs.
Changed rtpdump converter and neteq tool to use new parser, but still aborting if the file is corrupt.
Committed: https://crrev.com/d5c1a0bd5d1faac10db9986117483d25b9c10b1f
Cr-Commit-Position: refs/heads/master@{#12714}
Patch Set 1 #Patch Set 2 : Add parser test #
Total comments: 5
Patch Set 3 : Move parser to its own build target. Fix comments from Henrik. #
Total comments: 29
Patch Set 4 : Comments from ivoc #
Total comments: 30
Patch Set 5 : Rebase #Patch Set 6 : Some comments from stefan #Patch Set 7 : Always use curly braces in if-statements #Patch Set 8 : Rebase #
Total comments: 2
Patch Set 9 : Rebase #
Messages
Total messages: 44 (17 generated)
Description was changed from ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser. ========== to ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser. ==========
terelius@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org, stefan@webrtc.org
Description was changed from ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser. ========== to ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser, but still aborting if the file is corrupt. ==========
Ivo, please review the parser design and the event log Stefan, please review call/ Henrik, please review audio_coding/
On 2016/03/22 10:03:56, terelius wrote: > Ivo, please review the parser design and the event log > Stefan, please review call/ > Henrik, please review audio_coding/ Android failures caused by https://codereview.chromium.org/1818083003/ which has now been reverted. Feel free to ignore those.
https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:52: NULL, &header_length, &packet_length); Nit: NULL -> nullptr Here, and several places below. https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:90: audio_output_index_++; This increment must go outside the if-statement, right?
https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:52: NULL, &header_length, &packet_length); On 2016/03/29 20:34:40, hlundin-webrtc wrote: > Nit: NULL -> nullptr > Here, and several places below. Done. https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:90: audio_output_index_++; On 2016/03/29 20:34:40, hlundin-webrtc wrote: > This increment must go outside the if-statement, right? Done. It actually needs to go both outside and inside (since we return in the if -statement).
webrtc/modules/audio_coding: lgtm https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1768773002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:90: audio_output_index_++; On 2016/03/30 12:41:37, terelius wrote: > On 2016/03/29 20:34:40, hlundin-webrtc wrote: > > This increment must go outside the if-statement, right? > > Done. It actually needs to go both outside and inside (since we return in the if > -statement). Acknowledged.
Looks like a useful change. See some comments below. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log2rtp_dump.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:18: #include "webrtc/call.h" Do we need this include here? https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:128: // TODO(terelius): Maybe add a flag to dump outgoing trafic instead? Sounds like a good feature to add at some point. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:91: *varint |= static_cast<uint64_t>(byte & 0x7F) << (7 * *bytes_read); Would be a good idea to add some comments here to explain this bit of wizardry for the unwary reader. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:96: return false; How about returning bytes_read from this function, and return -1 in error cases? https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:115: expected_tag = (1 << 3) | 2; // (fieldnumber << 3) | wire_type Some explanation for the bit juggling would be nice here as well. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:184: RTC_CHECK(rtp_packet.has_incoming()); I guess these CHECKs can be inside of the ifs, right? https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.h (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. Should be 2016 I guess? https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:35: enum EventType { Would it be possible to reuse the enum from the generated protobuf code? maybe with a typedef? It would be nice if it is updated automatically when new event types are added. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:51: bool ParseFile(const std::string& file_name); I guess it doesn't make a big difference either way (and I think this way is fine), but this could also be done in the constructor. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:57: int64_t GetTimestamp(size_t i) const; I would prefer a more descriptive name instead of 'i', maybe something like event_index (same for functions below)? https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:64: // output parameters. If some value is irrelevant, then that output parameter I think this could be rephrased a bit more clearly, maybe something like "If an output parameter is not needed, it can be set to nullptr.". Same applies to comment below. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest_helper.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:15: #include <cstring> I thought the convention was to use <string.h> style includes, for C libraries.
https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log2rtp_dump.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:18: #include "webrtc/call.h" On 2016/04/01 08:27:59, ivoc wrote: > Do we need this include here? To compile, it has to be included either here or in rtc_event_log_parser.h. Since we are using webrtc::MediaType::AUDIO and the other MediaType identifiers, I think it does belong here because of include-what-you-use. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:91: *varint |= static_cast<uint64_t>(byte & 0x7F) << (7 * *bytes_read); On 2016/04/01 08:27:59, ivoc wrote: > Would be a good idea to add some comments here to explain this bit of wizardry > for the unwary reader. Done. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:96: return false; On 2016/04/01 08:27:59, ivoc wrote: > How about returning bytes_read from this function, and return -1 in error cases? It is slightly more complicated because we also need to know why we failed. Did the file end or was the VarInt invalid? https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:115: expected_tag = (1 << 3) | 2; // (fieldnumber << 3) | wire_type On 2016/04/01 08:27:59, ivoc wrote: > Some explanation for the bit juggling would be nice here as well. I've added a little, but it is hard to give any justification other than "this is how the protobuf format is defined". Please take another look and tell me what you think. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:184: RTC_CHECK(rtp_packet.has_incoming()); On 2016/04/01 08:27:59, ivoc wrote: > I guess these CHECKs can be inside of the ifs, right? They could, but since these fields are specified as required, I think it is better to ensure they exist even if we are not going to read them. If they don't exist, then something has gone seriously wrong with the event, and it would be safer to abort than to continue with potentially faulty data. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.h (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/04/01 08:27:59, ivoc wrote: > Should be 2016 I guess? I created the file in 2015 when I started on the previous version of the parser. So it depends whether we consider this a new implementation, or a refinement of the old one. :) https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:35: enum EventType { On 2016/04/01 08:27:59, ivoc wrote: > Would it be possible to reuse the enum from the generated protobuf code? maybe > with a typedef? It would be nice if it is updated automatically when new event > types are added. It is non-trivial. The enum is actually named Event_EventType and typedef'ed to Event::EventType. This is fine, but the second part of the problem is that the literal names in Event_EventType are Event_EventType_UNKNOWN_EVENT and so on. The Event class in turn declare static constants such as static const EventType UNKNOWN_EVENT = Event_EventType_UNKNOWN_EVENT; So regardless of whether we do typedef Event::EventType EventType or typedef Event_EventType EventType we won't get nice names for the enum literals. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:51: bool ParseFile(const std::string& file_name); On 2016/04/01 08:27:59, ivoc wrote: > I guess it doesn't make a big difference either way (and I think this way is > fine), but this could also be done in the constructor. Yeah, I though about that. I don't have any strong opinion either, but one could conceivably have a program that repeatedly asks the user for a file and analyze it. If parsing is done in the constructor, then one would have to construct new objects on the heap instead of just parsing the next file. On the other hand, passing the file name into the constructor would save one line of code. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:57: int64_t GetTimestamp(size_t i) const; On 2016/04/01 08:27:59, ivoc wrote: > I would prefer a more descriptive name instead of 'i', maybe something like > event_index (same for functions below)? Done. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:64: // output parameters. If some value is irrelevant, then that output parameter On 2016/04/01 08:27:59, ivoc wrote: > I think this could be rephrased a bit more clearly, maybe something like "If an > output parameter is not needed, it can be set to nullptr.". Same applies to > comment below. Done. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest_helper.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:15: #include <cstring> On 2016/04/01 08:27:59, ivoc wrote: > I thought the convention was to use <string.h> style includes, for C libraries. Done.
LGTM https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:184: RTC_CHECK(rtp_packet.has_incoming()); On 2016/04/19 17:01:45, terelius wrote: > On 2016/04/01 08:27:59, ivoc wrote: > > I guess these CHECKs can be inside of the ifs, right? > > They could, but since these fields are specified as required, I think it is > better to ensure they exist even if we are not going to read them. If they don't > exist, then something has gone seriously wrong with the event, and it would be > safer to abort than to continue with potentially faulty data. Acknowledged. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.h (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/04/19 17:01:45, terelius wrote: > On 2016/04/01 08:27:59, ivoc wrote: > > Should be 2016 I guess? > > I created the file in 2015 when I started on the previous version of the parser. > So it depends whether we consider this a new implementation, or a refinement of > the old one. :) Ok, I'll leave it up to you :) https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:35: enum EventType { On 2016/04/19 17:01:45, terelius wrote: > On 2016/04/01 08:27:59, ivoc wrote: > > Would it be possible to reuse the enum from the generated protobuf code? maybe > > with a typedef? It would be nice if it is updated automatically when new event > > types are added. > > It is non-trivial. The enum is actually named Event_EventType and typedef'ed to > Event::EventType. This is fine, but the second part of the problem is that the > literal names in Event_EventType are Event_EventType_UNKNOWN_EVENT and so on. > The Event class in turn declare static constants such as > static const EventType UNKNOWN_EVENT = Event_EventType_UNKNOWN_EVENT; > So regardless of whether we do typedef Event::EventType EventType or typedef > Event_EventType EventType we won't get nice names for the enum literals. Acknowledged. https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:51: bool ParseFile(const std::string& file_name); On 2016/04/19 17:01:45, terelius wrote: > On 2016/04/01 08:27:59, ivoc wrote: > > I guess it doesn't make a big difference either way (and I think this way is > > fine), but this could also be done in the constructor. > > Yeah, I though about that. I don't have any strong opinion either, but one could > conceivably have a program that repeatedly asks the user for a file and analyze > it. If parsing is done in the constructor, then one would have to construct new > objects on the heap instead of just parsing the next file. On the other hand, > passing the file name into the constructor would save one line of code. Ok, sounds reasonable.
https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log2rtp_dump.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:128: // TODO(terelius): Maybe add a flag to dump outgoing trafic instead? "outgoing traffic" https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log2rtp_dump.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:128: // TODO(terelius): Maybe add a flag to dump outgoing trafic instead? traffic https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:90: (*bytes_read)++) { ++*bytes_read https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:120: uint64_t tag, expected_tag = (1 << 3) | 2; Declare one variable per line. expected_tag should be named kExpectedTag https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:135: LOG(LS_WARNING) << "Missing message length after protobuf field tag."; Is it really missing? Can't it also be too large? https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:194: if (media_type != nullptr) { Remove {} to be consistent with line 190. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:207: RTC_CHECK_GE(rtp_packet.header().size(), 12u); Name the constant. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:232: if (media_type != nullptr) { Remove {} for consistency https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:268: config->rtp.remb = receiver_config.remb(); We should probably add transport_cc here too, which is used by send-side BWE. That makes me think if we should have a test which writes a config to an event log and reads it back again and verifies that they are equal? That could have found this issue, at least if we fill the config which we read back into with random values before we write to it (or fill the config with random values before we write it to the log). https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.h (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016? https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:22: #include "external/webrtc/webrtc/call/rtc_event_log.pb.h" I've never seen this before? https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest_helper.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:100: void RtcEventLogTestHelper::VerifyReceiveStreamConfig( So, can we instead do a memcmp() here to avoid having to update this every time we make a change to the config? Seems error prone since we'll forget updating this. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:116: if (config.rtp.rtcp_mode == RtcpMode::kCompound) {} https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:286: for (size_t i = 0; i < header_size; i++) { Feel free to remove {}
https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log2rtp_dump.cc (right): https://codereview.webrtc.org/1768773002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:128: // TODO(terelius): Maybe add a flag to dump outgoing trafic instead? On 2016/04/26 18:39:41, stefan-webrtc (holmer) wrote: > "outgoing traffic" Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > 2016 Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:90: (*bytes_read)++) { On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > ++*bytes_read Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:120: uint64_t tag, expected_tag = (1 << 3) | 2; On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > Declare one variable per line. expected_tag should be named kExpectedTag Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:135: LOG(LS_WARNING) << "Missing message length after protobuf field tag."; On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > Is it really missing? Can't it also be too large? Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:207: RTC_CHECK_GE(rtp_packet.header().size(), 12u); On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > Name the constant. Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:268: config->rtp.remb = receiver_config.remb(); On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > We should probably add transport_cc here too, which is used by send-side BWE. > > That makes me think if we should have a test which writes a config to an event > log and reads it back again and verifies that they are equal? That could have > found this issue, at least if we fill the config which we read back into with > random values before we write to it (or fill the config with random values > before we write it to the log). We do have tests that serialize a config and read it back. However, there is no way to automatically serialize a struct to a protobuf, so each new config field must be added manually to the proto file, the event log and to the parser. Since there is no transport_cc field in the proto, there is no way we can parse it here. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.h (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > 2016? Done. One could argue for either 2015 or 2016. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.h:22: #include "external/webrtc/webrtc/call/rtc_event_log.pb.h" On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > I've never seen this before? It's the same include as in the event log. The protobuf compiler creates the header file rtc_event_log.pb.h somewhere in the out/ directory. Locally, it is out/Debug/gen/protoc_out/webrtc/call/rtc_event_log.pb.h, but apparently the location is different for android builds. I don't know why this is. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest_helper.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:100: void RtcEventLogTestHelper::VerifyReceiveStreamConfig( On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > So, can we instead do a memcmp() here to avoid having to update this every time > we make a change to the config? Seems error prone since we'll forget updating > this. The short answer is that we can't memcmp the serialized event to the expected config struct because that's not how the config is stored in the protobuf. The longer answer is that we can't dump the raw bytes of the config structs. To begin with, the config has a vector of decoders and a map from payload types to RTX configs. If we took a raw dump, we would just store the pointers and not the actual config data. Secondly, the compiler is free to add padding between the members of a struct. The value of such padding bytes is undefined, so doing a memcmp would make the test flaky. The stored event logs would also be incompatible between different compilers and different platforms. Thirdly, even if we could automatically generate code to serialize the config, I'm not sure we would want to do it. Since the log is designed to not store PII, automatically adding more information to the log would be a privacy risk.
https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log2rtp_dump.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log2rtp_dump.cc:128: // TODO(terelius): Maybe add a flag to dump outgoing trafic instead? On 2016/04/26 18:39:41, stefan-webrtc (holmer) wrote: > traffic Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:194: if (media_type != nullptr) { On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > Remove {} to be consistent with line 190. As discussed offline, I'll add braces where they are missing instead. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:232: if (media_type != nullptr) { On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > Remove {} for consistency Acknowledged. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_unittest_helper.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:116: if (config.rtp.rtcp_mode == RtcpMode::kCompound) On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > {} Done. https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_unittest_helper.cc:286: for (size_t i = 0; i < header_size; i++) { On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > Feel free to remove {} Decided on the convention to always have the braces.
https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:268: config->rtp.remb = receiver_config.remb(); On 2016/04/27 14:35:27, terelius wrote: > On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > > We should probably add transport_cc here too, which is used by send-side BWE. > > > > That makes me think if we should have a test which writes a config to an event > > log and reads it back again and verifies that they are equal? That could have > > found this issue, at least if we fill the config which we read back into with > > random values before we write to it (or fill the config with random values > > before we write it to the log). > > We do have tests that serialize a config and read it back. However, there is no > way to automatically serialize a struct to a protobuf, so each new config field > must be added manually to the proto file, the event log and to the parser. Since > there is no > transport_cc field in the proto, there is no way we can parse it here. But if we serialize a struct which isn't fully represented by our protobuf, and then read it back and deserialize it then they shouldn't be equal after except if both contain the same fields, right? That should be a good test for making sure we represent everything. Or am I missing something?
https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_parser.cc (right): https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_parser.cc:268: config->rtp.remb = receiver_config.remb(); On 2016/05/04 12:10:57, stefan-webrtc (holmer) wrote: > On 2016/04/27 14:35:27, terelius wrote: > > On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > > > We should probably add transport_cc here too, which is used by send-side > BWE. > > > > > > That makes me think if we should have a test which writes a config to an > event > > > log and reads it back again and verifies that they are equal? That could > have > > > found this issue, at least if we fill the config which we read back into > with > > > random values before we write to it (or fill the config with random values > > > before we write it to the log). > > > > We do have tests that serialize a config and read it back. However, there is > no > > way to automatically serialize a struct to a protobuf, so each new config > field > > must be added manually to the proto file, the event log and to the parser. > Since > > there is no > > transport_cc field in the proto, there is no way we can parse it here. > > But if we serialize a struct which isn't fully represented by our protobuf, and > then read it back and deserialize it then they shouldn't be equal after except > if both contain the same fields, right? That should be a good test for making > sure we represent everything. Or am I missing something? True, but we don't represent everything and I don't think we want to. There are several parts in the stream configuration that don't make sense to store, such as the transport pointer, render and decode callbacks and so on. This means we would need to create a custom comparator specifically to decide whether two configurations are equal for the purposes of the event log, and if someone updates the config without updating the comparator we would have the same issue we have now. It also wouldn't detect a failure to represent some config member unless we have a function which fills the config struct with random data. (If the field has it's default value in the unit test when we write it, and we don't set the field when we read it back, then the two copies would compare equal.) That randomization function would also have to be updated when a new member is added to the config struct.
lgtm On 2016/05/04 15:34:20, terelius wrote: > https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... > File webrtc/call/rtc_event_log_parser.cc (right): > > https://codereview.webrtc.org/1768773002/diff/60001/webrtc/call/rtc_event_log... > webrtc/call/rtc_event_log_parser.cc:268: config->rtp.remb = > receiver_config.remb(); > On 2016/05/04 12:10:57, stefan-webrtc (holmer) wrote: > > On 2016/04/27 14:35:27, terelius wrote: > > > On 2016/04/26 18:39:42, stefan-webrtc (holmer) wrote: > > > > We should probably add transport_cc here too, which is used by send-side > > BWE. > > > > > > > > That makes me think if we should have a test which writes a config to an > > event > > > > log and reads it back again and verifies that they are equal? That could > > have > > > > found this issue, at least if we fill the config which we read back into > > with > > > > random values before we write to it (or fill the config with random values > > > > before we write it to the log). > > > > > > We do have tests that serialize a config and read it back. However, there is > > no > > > way to automatically serialize a struct to a protobuf, so each new config > > field > > > must be added manually to the proto file, the event log and to the parser. > > Since > > > there is no > > > transport_cc field in the proto, there is no way we can parse it here. > > > > But if we serialize a struct which isn't fully represented by our protobuf, > and > > then read it back and deserialize it then they shouldn't be equal after except > > if both contain the same fields, right? That should be a good test for making > > sure we represent everything. Or am I missing something? > > True, but we don't represent everything and I don't think we want to. There are > several parts in the stream configuration that don't make sense to store, such > as the transport pointer, render and decode callbacks and so on. This means we > would need to create a custom comparator specifically to decide whether two > configurations are equal for the purposes of the event log, and if someone > updates the config without updating the comparator we would have the same issue > we have now. > > It also wouldn't detect a failure to represent some config member unless we have > a function which fills the config struct with random data. (If the field has > it's default value in the unit test when we write it, and we don't set the field > when we read it back, then the two copies would compare equal.) That > randomization function would also have to be updated when a new member is added > to the config struct. I see, then I agree with you.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, ivoc@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1768773002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768773002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768773002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5387)
terelius@webrtc.org changed reviewers: + mflodman@webrtc.org
Magnus, I need owner approval for BUILD.gn. Could you take a look?
terelius@webrtc.org changed reviewers: + kjellander@webrtc.org
terelius@webrtc.org changed reviewers: - mflodman@webrtc.org
I need owner approval for BUILD.gn. Could you take a look, kjellander?
lgtm with a question. https://codereview.webrtc.org/1768773002/diff/140001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/1768773002/diff/140001/webrtc/BUILD.gn#newcode299 webrtc/BUILD.gn:299: if (is_clang && !is_nacl) { Most of our code doesn't have the !is_nacl condition. Is it needed here?
https://codereview.webrtc.org/1768773002/diff/140001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/1768773002/diff/140001/webrtc/BUILD.gn#newcode299 webrtc/BUILD.gn:299: if (is_clang && !is_nacl) { On 2016/05/11 09:27:16, kjellander (webrtc) wrote: > Most of our code doesn't have the !is_nacl condition. Is it needed here? I don't know, but I assume so. It was added by sergeyu in https://codereview.webrtc.org/1327853002 for the target rtc_event_log, which is very similar. More recently it was added by aluebs for the target "webrtc_common".
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768773002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768773002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
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, henrik.lundin@webrtc.org, kjellander@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1768773002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768773002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768773002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768773002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768773002/160001
Message was sent while issue was closed.
Description was changed from ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser, but still aborting if the file is corrupt. ========== to ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser, but still aborting if the file is corrupt. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser, but still aborting if the file is corrupt. ========== to ========== New parser for event log. Manually parse the outermost EventStream to more easily deal with corrupt or partially written logs. Changed rtpdump converter and neteq tool to use new parser, but still aborting if the file is corrupt. Committed: https://crrev.com/d5c1a0bd5d1faac10db9986117483d25b9c10b1f Cr-Commit-Position: refs/heads/master@{#12714} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d5c1a0bd5d1faac10db9986117483d25b9c10b1f Cr-Commit-Position: refs/heads/master@{#12714} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
