Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1230973005:
Adds logging of configuration information for VideoReceiveStream (Closed)
|
Created:
5 years, 5 months ago by terelius Modified:
5 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRenamed the ACMDump to RtcEventLog and moved it to webrtc/video, since it is not specific to the audio coding module. Updated .gyp and .gn files accordingly.
Placed the protobuf structures in the namespace webrtc::rtclog. Removed the message field from the DebugEvent structure, since it was not used.
Added an interface to set config information for VideoReceiveStream and VideoSendStream in the event log.
Added function to log full RTCP packets and changed RTP-logging to only log headers.
Significantly extended the unit tests for RtcEventLog.
R=ivoc@webrtc.org, minyue@webrtc.org, pbos@webrtc.org, stefan@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/c159b046d7a0086e45ae0f79c00a462f3fafd207
Patch Set 1 #
Total comments: 74
Patch Set 2 : Renamed AcmDump to RtcEventLog and moved it to webrtc/video #Patch Set 3 : Fixed gn build files (I hope) #Patch Set 4 : Refactored and changed some types in accordance with reviewers' comments #Patch Set 5 : Added RTC_NOTREACHED assertions to stop certain compilers from complaining about reaching end of non-void function #Patch Set 6 : Added silly return values in unreachable path. #
Total comments: 18
Patch Set 7 : Addressed Ivo's latest comments #
Total comments: 67
Patch Set 8 : Addressed comments from pbos #
Total comments: 8
Patch Set 9 : Rewrote code to start and stop logging. Fixed some minor reviewer comments. #Patch Set 10 : Removed some commented code #
Total comments: 12
Patch Set 11 : Addressed comments from stefan. #
Messages
Total messages: 35 (4 generated)
terelius@webrtc.org changed reviewers: + ivoc@webrtc.org, stefan@webrtc.org
Looks pretty good! See my comments, most are pretty minor. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:144: // printf("Cannot open file %s\n", file_name.c_str()); Should be removed. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:155: // TODO(terelius): Rvalue references are unapproved. Do we need it? I agree that it's not necessary. It should be auto& event. I can't recall why I used this in the first place :-) https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:175: switch (config.rtp.rtcp_mode) { I think it would be clearer to refactor this into a small function that converts from one enum to the other. See the convertDebugEvent in the anonymous namespace (this was suggested to me during a previous code review). https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:189: for (const auto& r : config.rtp.rtx) { I'm not a fan of single-letter variable names (in most cases). I think r should be renamed to something more descriptive, maybe rtx_pair? https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:196: for (const auto& e : config.rtp.extensions) { e should be renamed to something more descriptive as well. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:202: for (const auto& d : config.decoders) { Same for d. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:210: // after the ReceiveStream is created. I disagree. We can store this ACMDumpEvent as a member of the AcmDump class. That way it is retained permanently until we need it. The best way to do it is to first put it in the recent_log_events_ list (like what your code does now), and then when removing old events (in the AddRecentEvent function), if the type is RECEIVER_CONFIG_EVENT, then copy it to the class member. When starting a log, we should first dump this new class member to the log file before storing the rest of the recent_log_events_. That way we always have the most recent config struct available, while still supporting config changes during the logging process itself. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:225: for (const auto& s : config.rtp.ssrcs) { s should be more descriptive. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:229: for (const auto& e : config.rtp.extensions) { Same here. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:235: for (const auto& r : config.rtp.rtx.ssrcs) { And here. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:249: // after the ReceiveStream is created. See my comment above. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:265: if (media_type == MediaType::VIDEO) I think we should make another small function to convert these enums from one to the other. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:285: rtcp_event.mutable_rtcp_packet()->set_direction( We can reuse the conversion function here. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.h (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:19: #include "webrtc/call.h" // For MediaType definition I think it would be a good idea to move these includes to the .cc file, and use forward declarations for the types that are used on the interface. That should lead to faster compilation. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:43: // documentation? Sounds like a good idea, I probably forgot to change it when changing the return value at some point :-) https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:56: MediaType media_type, media_type should be a const reference. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:63: MediaType media_type, Same here. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:75: const VideoReceiveStream::Config::Rtp::Rtx& rtx = We could consider making this an auto, since it's such a long typename. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:108: EXPECT_TRUE(event.has_timestamp_us()); I think it would be a good idea to refactor the common verification checks (timestamp, event type, etc) into a function that is used in multiple tests. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:244: // since our protobuf file says that the message is optional. Make a decision. I think that's fine, especially since the message field is not really used for anything in the LOG_START event. Right now it contains an empty string, but I guess that might change at some point. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:300: std::vector<size_t> rtp_packet_sizes; I would prefer a vector<vector<uint8_t>> here, the sizes don't have to be stored seperately then (each vector has a size). https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:302: uint8_t* outgoing_rtcp_packet; These should be vector<uint8_t>. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:311: size_t rtp_header_size = 20; Should be const. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:377: int debug_count = 1; // Only LogStart event, These should be const. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:378: int event_count = config_count + debug_count + rtcp_count + rtp_count; This one as well. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:70: RTX = 3; // TODO(terelius): Where does this get set? Nowhere yet :-) We should change that. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:246: // TODO(terelius): Figure out what the requirements are from the audio team. I will try to look into that.
https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:183: // Compiler should warn if anyone adds unhandled new modes. No need for this comment I think. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:265: if (media_type == MediaType::VIDEO) On 2015/07/14 12:13:13, ivoc wrote: > I think we should make another small function to convert these enums from one to > the other. And I would prefer that function to use a switch instead of if statements. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.h (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:56: MediaType media_type, On 2015/07/14 12:13:14, ivoc wrote: > media_type should be a const reference. Why would we do that? Seems like MediaType is a very simple type that wouldn't be harmful to pass by value. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:108: EXPECT_TRUE(event.has_timestamp_us()); On 2015/07/14 12:13:14, ivoc wrote: > I think it would be a good idea to refactor the common verification checks > (timestamp, event type, etc) into a function that is used in multiple tests. Agree, these functions are very long. If possible it would be good to refactor, assuming there's code duplication. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:300: std::vector<size_t> rtp_packet_sizes; On 2015/07/14 12:13:14, ivoc wrote: > I would prefer a vector<vector<uint8_t>> here, the sizes don't have to be stored > seperately then (each vector has a size). And you can also avoid the delete[] https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = 5; Can we simply not add this until we actually need it? https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:98: // RTX is not used for RTCP packets. I don't think this is necessary to comment on.
https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.h (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:56: MediaType media_type, On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > media_type should be a const reference. > > Why would we do that? Seems like MediaType is a very simple type that wouldn't > be harmful to pass by value. Oh, I didn't actually look at the definition of MediaType. If it's small and simple then it makes sense to pass by value. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = 5; On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > Can we simply not add this until we actually need it? +1
https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:144: // printf("Cannot open file %s\n", file_name.c_str()); On 2015/07/14 12:13:13, ivoc wrote: > Should be removed. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:155: // TODO(terelius): Rvalue references are unapproved. Do we need it? On 2015/07/14 12:13:13, ivoc wrote: > I agree that it's not necessary. It should be auto& event. I can't recall why I > used this in the first place :-) Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:175: switch (config.rtp.rtcp_mode) { On 2015/07/14 12:13:13, ivoc wrote: > I think it would be clearer to refactor this into a small function that converts > from one enum to the other. See the convertDebugEvent in the anonymous namespace > (this was suggested to me during a previous code review). Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:183: // Compiler should warn if anyone adds unhandled new modes. On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > No need for this comment I think. I'll put a general warning/explanation of why there are no default cases in the anonymous namespace just before the conversion functions. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:189: for (const auto& r : config.rtp.rtx) { On 2015/07/14 12:13:13, ivoc wrote: > I'm not a fan of single-letter variable names (in most cases). I think r should > be renamed to something more descriptive, maybe rtx_pair? Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:196: for (const auto& e : config.rtp.extensions) { On 2015/07/14 12:13:13, ivoc wrote: > e should be renamed to something more descriptive as well. I'll change this, but I'll explain why I think the current naming is fine. If I wanted to perform this loop in pseudo code I would write for each extension e: do something with e Translating this to C++ in the natural way gives code like for (auto e ; extensions) do something with e In cases like this, I don't think it is a problem to have a short (undescriptive) variable name since the scope is very short. This is similar to using i,j,k for loop indices. While i wouldn't be a good name for a persistent variable, there is little risk for confusion for local variables in a short loop. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:202: for (const auto& d : config.decoders) { On 2015/07/14 12:13:13, ivoc wrote: > Same for d. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:210: // after the ReceiveStream is created. On 2015/07/14 12:13:13, ivoc wrote: > I disagree. We can store this ACMDumpEvent as a member of the AcmDump class. > That way it is retained permanently until we need it. The best way to do it is > to first put it in the recent_log_events_ list (like what your code does now), > and then when removing old events (in the AddRecentEvent function), if the type > is RECEIVER_CONFIG_EVENT, then copy it to the class member. > > When starting a log, we should first dump this new class member to the log file > before storing the rest of the recent_log_events_. > > That way we always have the most recent config struct available, while still > supporting config changes during the logging process itself. Acknowledged. I'll leave the comment as a reminder that we have to fix this in some way. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:225: for (const auto& s : config.rtp.ssrcs) { On 2015/07/14 12:13:13, ivoc wrote: > s should be more descriptive. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:229: for (const auto& e : config.rtp.extensions) { On 2015/07/14 12:13:13, ivoc wrote: > Same here. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:235: for (const auto& r : config.rtp.rtx.ssrcs) { On 2015/07/14 12:13:13, ivoc wrote: > And here. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:249: // after the ReceiveStream is created. On 2015/07/14 12:13:13, ivoc wrote: > See my comment above. Acknowledged. I'll leave the comment as a reminder. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:265: if (media_type == MediaType::VIDEO) On 2015/07/14 13:28:55, stefan-webrtc (holmer) wrote: > On 2015/07/14 12:13:13, ivoc wrote: > > I think we should make another small function to convert these enums from one > to > > the other. > > And I would prefer that function to use a switch instead of if statements. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:285: rtcp_event.mutable_rtcp_packet()->set_direction( On 2015/07/14 12:13:13, ivoc wrote: > We can reuse the conversion function here. We can't reuse it unless we change the protobuf. ACMDumpRtpPacket::VIDEO != ACMDumpRtcpPacket::VIDEO I've made a separate conversion function. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.h (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:19: #include "webrtc/call.h" // For MediaType definition On 2015/07/14 12:13:14, ivoc wrote: > I think it would be a good idea to move these includes to the .cc file, and use > forward declarations for the types that are used on the interface. That should > lead to faster compilation. Good idea, unfortunately it seems difficult to do. We actually use the definition of MediaType from call.h. From the other headers we only need the declarations of VideoReceiveStream::Config and VideoSendStream::Config, but afaik it is not possible to forward declare nested classes outside of the class where they are defined. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:43: // documentation? On 2015/07/14 12:13:14, ivoc wrote: > Sounds like a good idea, I probably forgot to change it when changing the return > value at some point :-) Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:56: MediaType media_type, On 2015/07/14 13:58:03, ivoc wrote: > On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > > On 2015/07/14 12:13:14, ivoc wrote: > > > media_type should be a const reference. > > > > Why would we do that? Seems like MediaType is a very simple type that wouldn't > > be harmful to pass by value. > > Oh, I didn't actually look at the definition of MediaType. If it's small and > simple then it makes sense to pass by value. I'll keep passing by value unless a change of types would motivate using references. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:63: MediaType media_type, On 2015/07/14 12:13:14, ivoc wrote: > Same here. I'll keep it as-is for the time being. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:75: const VideoReceiveStream::Config::Rtp::Rtx& rtx = On 2015/07/14 12:13:14, ivoc wrote: > We could consider making this an auto, since it's such a long typename. While I like using auto to hide obvious boiler-plate code (e.g. iterators, yuck!), I prefer not to use it to hide actual type information. Doing so makes it hard to edit the code without an IDE, imo. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:108: EXPECT_TRUE(event.has_timestamp_us()); On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > I think it would be a good idea to refactor the common verification checks > > (timestamp, event type, etc) into a function that is used in multiple tests. > > Agree, these functions are very long. If possible it would be good to refactor, > assuming there's code duplication. The first 10 lines of each function are similar. I've refactored to eliminate 9 of the lines at the cost of a rather ugly looking helper function. The difficulty is that the gtest ASSERT_TRUE only returns from the current function, so an assertion failure in a subroutine allows the surrounding test to continue. I've instead used custom AssertionFailure()'s to pass information back to the caller, so that the caller can use the ASSERT_TRUE, but still get the detailed error message. Apart from the first 10 lines, the functions are too different to benefit from refactoring imo. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:244: // since our protobuf file says that the message is optional. Make a decision. On 2015/07/14 12:13:14, ivoc wrote: > I think that's fine, especially since the message field is not really used for > anything in the LOG_START event. Right now it contains an empty string, but I > guess that might change at some point. We might want to make LogStart its own event type rather than trying to shoehorn it into DebugEvent. If we don't want to store any data apart from wall clock with the LogStart event we could even skip the event field and just make LOG_START a value of the RelEvent::EventType enum. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:300: std::vector<size_t> rtp_packet_sizes; On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > I would prefer a vector<vector<uint8_t>> here, the sizes don't have to be > stored > > seperately then (each vector has a size). > > And you can also avoid the delete[] Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:302: uint8_t* outgoing_rtcp_packet; On 2015/07/14 12:13:14, ivoc wrote: > These should be vector<uint8_t>. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:311: size_t rtp_header_size = 20; On 2015/07/14 12:13:14, ivoc wrote: > Should be const. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:377: int debug_count = 1; // Only LogStart event, On 2015/07/14 12:13:14, ivoc wrote: > These should be const. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:378: int event_count = config_count + debug_count + rtcp_count + rtp_count; On 2015/07/14 12:13:14, ivoc wrote: > This one as well. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:70: RTX = 3; // TODO(terelius): Where does this get set? On 2015/07/14 12:13:14, ivoc wrote: > Nowhere yet :-) We should change that. I mean that I don't think the RTX flag makes any sense and that it should be removed. That is, the way I am using the PayloadType field is to set it based on MediaType with the valid media types being ANY, AUDIO, VIDEO and DATA. We should be able to decide whether the packet is a retransmission by parsing the header, no? https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = 5; On 2015/07/14 13:58:03, ivoc wrote: > On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > > Can we simply not add this until we actually need it? > > +1 I would *very* strongly advise against this. We should not create privacy invasive code unless we are actually ready use it. See e.g. http://www.wired.com/2012/05/google-wifi-fcc-investigation/ for a previous case where accidental logging of payload data led to a lot of problems and bad press for Google. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:98: // RTX is not used for RTCP packets. On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > I don't think this is necessary to comment on. Done. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:246: // TODO(terelius): Figure out what the requirements are from the audio team. On 2015/07/14 12:13:14, ivoc wrote: > I will try to look into that. Acknowledged.
terelius@webrtc.org changed reviewers: + minyue@webrtc.org, pbos@webrtc.org
Adding minyue as a reviewer (owner in modules/audio_coding). Adding pbos (primarily for reviewing .gyp and .gn files)
On 2015/07/16 15:02:13, terelius wrote: > Adding minyue as a reviewer (owner in modules/audio_coding). > Adding pbos (primarily for reviewing .gyp and .gn files) With video header dumps added, I agree that it should be moved out from audio_coding, but why to a video folder?
On 2015/07/16 15:19:54, minyuel wrote: > On 2015/07/16 15:02:13, terelius wrote: > > Adding minyue as a reviewer (owner in modules/audio_coding). > > Adding pbos (primarily for reviewing .gyp and .gn files) > > With video header dumps added, I agree that it should be moved out from > audio_coding, but why to a video folder? Mostly because there was no better place to put it. In the future, the instantiation of the logging object will be done in Call, and call.cc is located in webrtc/video. Ideally call.cc, rtc_event_log.cc and any other code that ties the audio and video parts together would be put in its own top-level folder, but since no such folder exists we felt the next best thing to do was to keep everything together in the video folder.
Some more comments. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.cc:285: rtcp_event.mutable_rtcp_packet()->set_direction( On 2015/07/16 12:47:02, terelius wrote: > On 2015/07/14 12:13:13, ivoc wrote: > > We can reuse the conversion function here. > > We can't reuse it unless we change the protobuf. > ACMDumpRtpPacket::VIDEO != ACMDumpRtcpPacket::VIDEO > > I've made a separate conversion function. Right, I misread. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump.h (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump.h:19: #include "webrtc/call.h" // For MediaType definition On 2015/07/16 12:47:03, terelius wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > I think it would be a good idea to move these includes to the .cc file, and > use > > forward declarations for the types that are used on the interface. That should > > lead to faster compilation. > > Good idea, unfortunately it seems difficult to do. We actually use the > definition of MediaType from call.h. From the other headers we only need the > declarations of VideoReceiveStream::Config and VideoSendStream::Config, but > afaik it is not possible to forward declare nested classes outside of the class > where they are defined. I think forward declaring MediaType should work at least. Can you try it? https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:75: const VideoReceiveStream::Config::Rtp::Rtx& rtx = On 2015/07/16 12:47:03, terelius wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > We could consider making this an auto, since it's such a long typename. > > While I like using auto to hide obvious boiler-plate code (e.g. iterators, > yuck!), I prefer not to use it to hide actual type information. Doing so makes > it hard to edit the code without an IDE, imo. Okay, that makes sense. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc:244: // since our protobuf file says that the message is optional. Make a decision. On 2015/07/16 12:47:03, terelius wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > I think that's fine, especially since the message field is not really used for > > anything in the LOG_START event. Right now it contains an empty string, but I > > guess that might change at some point. > > We might want to make LogStart its own event type rather than trying to shoehorn > it into DebugEvent. If we don't want to store any data apart from wall clock > with the LogStart event we could even skip the event field and just make > LOG_START a value of the RelEvent::EventType enum. Good idea. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:70: RTX = 3; // TODO(terelius): Where does this get set? On 2015/07/16 12:47:03, terelius wrote: > On 2015/07/14 12:13:14, ivoc wrote: > > Nowhere yet :-) We should change that. > > I mean that I don't think the RTX flag makes any sense and that it should be > removed. That is, the way I am using the PayloadType field is to set it based on > MediaType with the valid media types being ANY, AUDIO, VIDEO and DATA. > > We should be able to decide whether the packet is a retransmission by parsing > the header, no? Okay, makes sense, feel free to remove it in that case. https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = 5; On 2015/07/16 12:47:03, terelius wrote: > On 2015/07/14 13:58:03, ivoc wrote: > > On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > > > Can we simply not add this until we actually need it? > > > > +1 > > I would *very* strongly advise against this. We should not > create privacy invasive code unless we are actually ready > use it. See e.g. > http://www.wired.com/2012/05/google-wifi-fcc-investigation/ > for a previous case where accidental logging of payload data > led to a lot of problems and bad press for Google. I think we were trying to argue that these two lines should be removed from the file, not that we should start logging whole packets. I agree that we need to do a privacy review before doing any sort of logging of whole RTP headers. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/BUILD.gn File webrtc/video/BUILD.gn (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/BUILD.gn#ne... webrtc/video/BUILD.gn:80: "..:rtc_event_log", I think these lists are usually sorted alphabetically. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:127: return RelDebugEvent::UNKNOWN_EVENT; Can you add the RTC_NOTREACHED() macro here as well? https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:138: return RelVideoReceiveConfig::RTCP_COMPOUND; // Silence stupid compilers Although I agree with your sentiment, maybe we can rephrase it to something a little more neutral like "Avoid warning on less clever compilers" or something similar. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:153: return RelRtpPacket::UNKNOWN_TYPE; // Silence stupid compilers Same here https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:168: return RelRtcpPacket::UNKNOWN_TYPE; // Silence stupid compilers And here. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:19: #include "webrtc/call.h" // For MediaType definition I guess these should be in alphabetical order as well. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:19: #include "webrtc/video/rtc_event_log.h" Could you sort these alphabetically? https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:44: << "has" << (event.has_rtp_packet()?"":"no") << "RTP packet"; I wonder how this error message would look. It seems like there should be spaces before & after each argument, e.g. "Event of type " << type. Also I'm guessing the type itself will probably be printed as an int. We could just replace it with the corresponding text in each of the ifs, e.g. "Event of type RTP_EVENT has ". Alternatively we could make a function to convert enums to human-readable strings. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:309: rtp_packets.push_back(std::vector<uint8_t>()); It would be good to call reserve here as well.
On 2015/07/17 07:46:43, terelius wrote: > On 2015/07/16 15:19:54, minyuel wrote: > > On 2015/07/16 15:02:13, terelius wrote: > > > Adding minyue as a reviewer (owner in modules/audio_coding). > > > Adding pbos (primarily for reviewing .gyp and .gn files) > > > > With video header dumps added, I agree that it should be moved out from > > audio_coding, but why to a video folder? > > Mostly because there was no better place to put it. In the future, the > instantiation of the logging object will be done in Call, and call.cc is located > in webrtc/video. Ideally call.cc, rtc_event_log.cc and any other code that ties > the audio and video parts together would be put in its own top-level folder, but > since no such folder exists we felt the next best thing to do was to keep > everything together in the video folder. Ok, thanks for explain, I am acknowledged and I'd like to buy it. I'll wait for Ivo's other comments to be resolved to give approval.
https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/BUILD.gn File webrtc/video/BUILD.gn (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/BUILD.gn#ne... webrtc/video/BUILD.gn:80: "..:rtc_event_log", On 2015/07/17 12:14:28, ivoc wrote: > I think these lists are usually sorted alphabetically. Ok, and ':' comes before '/'? Or is it a special convention that "..:webrtc_common" should appear first in the list? https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:127: return RelDebugEvent::UNKNOWN_EVENT; On 2015/07/17 12:14:29, ivoc wrote: > Can you add the RTC_NOTREACHED() macro here as well? Done. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:138: return RelVideoReceiveConfig::RTCP_COMPOUND; // Silence stupid compilers On 2015/07/17 12:14:29, ivoc wrote: > Although I agree with your sentiment, maybe we can rephrase it to something a > little more neutral like "Avoid warning on less clever compilers" or something > similar. Done. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:153: return RelRtpPacket::UNKNOWN_TYPE; // Silence stupid compilers On 2015/07/17 12:14:28, ivoc wrote: > Same here Done. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:168: return RelRtcpPacket::UNKNOWN_TYPE; // Silence stupid compilers On 2015/07/17 12:14:28, ivoc wrote: > And here. Done. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:19: #include "webrtc/call.h" // For MediaType definition On 2015/07/17 12:14:29, ivoc wrote: > I guess these should be in alphabetical order as well. Done. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:19: #include "webrtc/video/rtc_event_log.h" On 2015/07/17 12:14:29, ivoc wrote: > Could you sort these alphabetically? Done. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:44: << "has" << (event.has_rtp_packet()?"":"no") << "RTP packet"; On 2015/07/17 12:14:29, ivoc wrote: > I wonder how this error message would look. It seems like there should be spaces > before & after each argument, e.g. "Event of type " << type. Also I'm guessing > the type itself will probably be printed as an int. We could just replace it > with the corresponding text in each of the ifs, e.g. "Event of type RTP_EVENT > has ". Alternatively we could make a function to convert enums to human-readable > strings. You're right about the spaces. I'll fix that. We can't replace 'type' with RTP_EVENT because the code will execute both if it is an RTP_EVENT that doesn't have an rtp_packet, but also if it is another event type that *does* have an rtp_packet even though it shouldn't. https://codereview.webrtc.org/1230973005/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:309: rtp_packets.push_back(std::vector<uint8_t>()); On 2015/07/17 12:14:29, ivoc wrote: > It would be good to call reserve here as well. Done. Well spotted! I initially used the constructor to allocate a vector containing all zeroes and then changed the elements instead of appending them with push_back.
lgtm
https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = 5; On 2015/07/17 12:14:28, ivoc wrote: > On 2015/07/16 12:47:03, terelius wrote: > > On 2015/07/14 13:58:03, ivoc wrote: > > > On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > > > > Can we simply not add this until we actually need it? > > > > > > +1 > > > > I would *very* strongly advise against this. We should not > > create privacy invasive code unless we are actually ready > > use it. See e.g. > > http://www.wired.com/2012/05/google-wifi-fcc-investigation/ > > for a previous case where accidental logging of payload data > > led to a lot of problems and bad press for Google. > > I think we were trying to argue that these two lines should be removed from the > file, not that we should start logging whole packets. > > I agree that we need to do a privacy review before doing any sort of logging of > whole RTP headers. I missed this comment in my last reply. Based on the word "until", I interpreted Stefan's suggestion as that we should have a field in the protobuf for full packets, but not use it until later. I think that sounds very dangerous. If you want to remove the field itself, I wouldn't mind but I would still prefer to leave a comment that reminds anyone who might be reading the code that we should not log user data without some serious consideration of privacy issues.
On 2015/07/17 17:48:33, terelius wrote: > https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... > File webrtc/modules/audio_coding/main/acm2/dump.proto (right): > > https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... > webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = > 5; > On 2015/07/17 12:14:28, ivoc wrote: > > On 2015/07/16 12:47:03, terelius wrote: > > > On 2015/07/14 13:58:03, ivoc wrote: > > > > On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > > > > > Can we simply not add this until we actually need it? > > > > > > > > +1 > > > > > > I would *very* strongly advise against this. We should not > > > create privacy invasive code unless we are actually ready > > > use it. See e.g. > > > http://www.wired.com/2012/05/google-wifi-fcc-investigation/ > > > for a previous case where accidental logging of payload data > > > led to a lot of problems and bad press for Google. > > > > I think we were trying to argue that these two lines should be removed from > the > > file, not that we should start logging whole packets. > > > > I agree that we need to do a privacy review before doing any sort of logging > of > > whole RTP headers. > > I missed this comment in my last reply. > > Based on the word "until", I interpreted Stefan's suggestion as that we should > have a field in the protobuf for full packets, but not use it until later. I > think that sounds very dangerous. > > If you want to remove the field itself, I wouldn't mind but I would still prefer > to leave a comment that reminds anyone who might be reading the code that we > should not log user data without some serious consideration of privacy issues. approve changes in acm + a comment on the item under discussion
here is my comment https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1230973005/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:86: // optional bytes payload = 5; On 2015/07/17 17:48:33, terelius wrote: > On 2015/07/17 12:14:28, ivoc wrote: > > On 2015/07/16 12:47:03, terelius wrote: > > > On 2015/07/14 13:58:03, ivoc wrote: > > > > On 2015/07/14 13:28:56, stefan-webrtc (holmer) wrote: > > > > > Can we simply not add this until we actually need it? > > > > > > > > +1 > > > > > > I would *very* strongly advise against this. We should not > > > create privacy invasive code unless we are actually ready > > > use it. See e.g. > > > http://www.wired.com/2012/05/google-wifi-fcc-investigation/ > > > for a previous case where accidental logging of payload data > > > led to a lot of problems and bad press for Google. > > > > I think we were trying to argue that these two lines should be removed from > the > > file, not that we should start logging whole packets. > > > > I agree that we need to do a privacy review before doing any sort of logging > of > > whole RTP headers. > > I missed this comment in my last reply. > > Based on the word "until", I interpreted Stefan's suggestion as that we should > have a field in the protobuf for full packets, but not use it until later. I > think that sounds very dangerous. > > If you want to remove the field itself, I wouldn't mind but I would still prefer > to leave a comment that reminds anyone who might be reading the code that we > should not log user data without some serious consideration of privacy issues. I do not know the privacy policy well enough. But it feels to me that, if we put these two lines, it is like saying "hey, we already considered how to break it." So I prefer to remove them
https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.proto (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:9: message RelEventStream { Is there a need to have Rel infront of all names here? I don't know protobufs well enough, so I have to ask. It can't be avoided by namespaces?
On 2015/07/21 12:41:13, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... > File webrtc/video/rtc_event_log.proto (right): > > https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.proto:9: message RelEventStream { > Is there a need to have Rel infront of all names here? I don't know protobufs > well enough, so I have to ask. It can't be avoided by namespaces? To whoever needs a diff (files moved): pbos@deimos:~/webrtc/src (terelius_patch)$ (git diff master:webrtc/modules/audio_coding/main/acm2/acm_dump.cc webrtc/video/rtc_event_log.cc && git diff master:webrtc/modules/audio_coding/main/acm2/acm_dump.h webrtc/video/rtc_event_log.h && git diff master:webrtc/modules/audio_coding/main/acm2/acm_dump_unittest.cc webrtc/video/rtc_event_log_unittest.cc && git diff master:webrtc/modules/audio_coding/main/acm2/dump.proto webrtc/video/rtc_event_log.proto) > rtc_event_log.patch -> https://pastebin.mozilla.org/8840193
https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:17: #include "webrtc/call.h" // For MediaType definition Remove comment https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:33: // Noop implementation if flag is not set. No-op https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:128: RTC_NOTREACHED(); // Return something only to avoid certain compiler warnings Remove comment, it's very obvious (conveyed by RTC_NOTREACHED();). Here and below. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:144: RelRtpPacket_PayloadType ConvertRtpPayloadType(MediaType media_type) { This is not a payload type. RTP payload types have a special meaning, 101 can be VP8, since this is not the 7-bit payload type, rename it. Why not call it RelRtpPacket_MediaType? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:152: return RelRtpPacket::UNKNOWN_TYPE; Should any "ANY" or "DATA" stamped packets enter here? If not, put a RTC_NOTREACHED(). https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:159: RelRtcpPacket_PayloadType ConvertRtcpPayloadType(MediaType media_type) { Same here, payload type is "kind-of reserved". https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:177: : crit_(webrtc::CriticalSectionWrapper::CreateCriticalSection()), Use a rtc::CriticalSection (this doesn't require additional heap allocation). We're trying to move off webrtc::CriticalSectionWrapper. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:201: for (auto& event : recent_log_events_) { Remove {}s https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:208: const webrtc::VideoReceiveStream::Config& config) { Remove all webrtc::, you're under the webrtc namespace. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:226: for (const auto& config_rtx : config.rtp.rtx) { s/config_rtx/kv, I think. We do this in a bunch of places for maps. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:233: for (const auto& config_extension : config.rtp.extensions) { extension https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:239: for (const auto& config_decoder : config.decoders) { decoder, and switch below DecoderConfig to decoder_config https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:255: RelEvent event; What's the "Rel" in "RelEvent" supposed to mean? Does "LogEvent" or "RtcEvent" make more sense here? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:262: for (const auto& ssrc : config.rtp.ssrcs) { Remove {}s https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:301: incoming ? RelRtpPacket::INCOMING : RelRtpPacket::OUTGOING); Do we need separate direction types for RTP and RTCP? Also, Rel? PacketDirection::INCOMING/OUTGOING? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:302: rtp_event.mutable_rtp_packet()->set_type(ConvertRtpPayloadType(media_type)); Not PayloadType https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:69: const std::string& event_message) = 0; If these are always used as constants, can you make this one a const char* event_message so we don't have to do string construction? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:70: virtual void LogDebugEvent(DebugEvent event_type) = 0; Do you need this one? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:74: static bool ParseRtcEventLog(const std::string& file_name, consider const char* https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.proto (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:26: RECEIVER_CONFIG_EVENT = 4; VIDEO_ https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:27: SENDER_CONFIG_EVENT = 5; VIDEO_ https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:44: // optional - but required if type == RECEIVER_CONFIG_EVENT VIDEO_ https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:47: // optional - but required if type == SENDER_CONFIG_EVENT VIDEO_ https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:66: enum PayloadType { These aren't payload types. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:86: // optional bytes payload = 5; How would this be turned on/off? If this is premature, remove it completely. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:93: enum Direction { Can this be shared? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:138: // or SSRC and port number, but for now we will rely on SSRC only. We'll probably move away from media type as well, since there can be >1 transport per media type (just not in our implementation), meaning that video + ssrc isn't even enough to identify the stream. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:245: // TODO(terelius): Figure out what the requirements are from the audio team. TODO(terelius): Add audio-receive config. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:250: // TODO(terelius): Figure out what the requirements are from the audio team. TODO(terelius): Add audio-send config. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:135: Remove empty lines.
Placed the structures generated from the protobuf in it's own namespace. Renamed several classes and variables based on feedback from pbos. Removed the message field from the DebugEvent structure, since the message was always empty. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:17: #include "webrtc/call.h" // For MediaType definition On 2015/07/22 11:05:27, pbos-webrtc wrote: > Remove comment Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:33: // Noop implementation if flag is not set. On 2015/07/22 11:05:26, pbos-webrtc wrote: > No-op Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:128: RTC_NOTREACHED(); // Return something only to avoid certain compiler warnings On 2015/07/22 11:05:27, pbos-webrtc wrote: > Remove comment, it's very obvious (conveyed by RTC_NOTREACHED();). Here and > below. Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:144: RelRtpPacket_PayloadType ConvertRtpPayloadType(MediaType media_type) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > This is not a payload type. RTP payload types have a special meaning, 101 can be > VP8, since this is not the 7-bit payload type, rename it. Why not call it > RelRtpPacket_MediaType? This is not really related to my CL, and since I wanted to keep it as small and focused as possible, I figured it would be better to just put a comment in the protobuf file that our current usage does not match the fields (or indeed name) very well. I'll change this to MediaType though. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:152: return RelRtpPacket::UNKNOWN_TYPE; On 2015/07/22 11:05:27, pbos-webrtc wrote: > Should any "ANY" or "DATA" stamped packets enter here? If not, put a > RTC_NOTREACHED(). It was unclear what UNKNOWN_TYPE was for, but presumably it would be used for everything that wasn't audio or video. I've now removed the UNKNOWN_TYPE field and added fields for ANY and DATA instead. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:159: RelRtcpPacket_PayloadType ConvertRtcpPayloadType(MediaType media_type) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > Same here, payload type is "kind-of reserved". Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:177: : crit_(webrtc::CriticalSectionWrapper::CreateCriticalSection()), On 2015/07/22 11:05:27, pbos-webrtc wrote: > Use a rtc::CriticalSection (this doesn't require additional heap allocation). > We're trying to move off webrtc::CriticalSectionWrapper. Done. Please review the locking changes extra carefully as I am unfamiliar with both the existing code and webrtc's locks. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:201: for (auto& event : recent_log_events_) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > Remove {}s All other loops and if-statements use braces, even if the body is only a single line. I'd like to keep the braces for consistency. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:208: const webrtc::VideoReceiveStream::Config& config) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > Remove all webrtc::, you're under the webrtc namespace. Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:226: for (const auto& config_rtx : config.rtp.rtx) { On 2015/07/22 11:05:27, pbos-webrtc wrote: > s/config_rtx/kv, I think. We do this in a bunch of places for maps. Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:233: for (const auto& config_extension : config.rtp.extensions) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > extension extension is already used. I'll rename the loop variable to e. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:239: for (const auto& config_decoder : config.decoders) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > decoder, and switch below DecoderConfig to decoder_config decoder is already used. I'll rename the loop variable to d. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:255: RelEvent event; On 2015/07/22 11:05:26, pbos-webrtc wrote: > What's the "Rel" in "RelEvent" supposed to mean? Does "LogEvent" or "RtcEvent" > make more sense here? It originated as an abbreviation of RtcEventLog. Names like RtcEventLogEventStream and RtcEventLogVideoReceiverConfig were cumbersome both to read and to write. We'll see how the code looks after placing the protobuf types in a separate namespace and dropping the Rel prefix. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:262: for (const auto& ssrc : config.rtp.ssrcs) { On 2015/07/22 11:05:26, pbos-webrtc wrote: > Remove {}s All other loops and if-statements use braces, even if the body is only a single line. I'd like to keep the braces for consistency. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:301: incoming ? RelRtpPacket::INCOMING : RelRtpPacket::OUTGOING); On 2015/07/22 11:05:26, pbos-webrtc wrote: > Do we need separate direction types for RTP and RTCP? Also, Rel? > PacketDirection::INCOMING/OUTGOING? I changed to use a bool instead. I have a hard time imagining a situation where we dont know which direction the packets are travelling... https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:302: rtp_event.mutable_rtp_packet()->set_type(ConvertRtpPayloadType(media_type)); On 2015/07/22 11:05:26, pbos-webrtc wrote: > Not PayloadType Changed to ConvertMediaType https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:69: const std::string& event_message) = 0; On 2015/07/22 11:05:27, pbos-webrtc wrote: > If these are always used as constants, can you make this one a const char* > event_message so we don't have to do string construction? Removed message field from protobuf because the message was always an empty string. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:70: virtual void LogDebugEvent(DebugEvent event_type) = 0; On 2015/07/22 11:05:27, pbos-webrtc wrote: > Do you need this one? This is now the only function to log debug events. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:74: static bool ParseRtcEventLog(const std::string& file_name, On 2015/07/22 11:05:27, pbos-webrtc wrote: > consider const char* This is currently used in the unit test where the file name is obtained by concatenating a few std::strings obtained from the gtest framework. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.proto (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:9: message RelEventStream { On 2015/07/21 12:41:13, stefan-webrtc (holmer) wrote: > Is there a need to have Rel infront of all names here? I don't know protobufs > well enough, so I have to ask. It can't be avoided by namespaces? Yes, it is possible to use namespaces. I wanted to avoid it because the names are already very long and I prefer not to have line breaks in simple lines such as SomeType a = b; Is webrtc::rtclog an acceptable namespace? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:26: RECEIVER_CONFIG_EVENT = 4; On 2015/07/22 11:05:27, pbos-webrtc wrote: > VIDEO_ Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:27: SENDER_CONFIG_EVENT = 5; On 2015/07/22 11:05:27, pbos-webrtc wrote: > VIDEO_ Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:44: // optional - but required if type == RECEIVER_CONFIG_EVENT On 2015/07/22 11:05:27, pbos-webrtc wrote: > VIDEO_ Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:47: // optional - but required if type == SENDER_CONFIG_EVENT On 2015/07/22 11:05:27, pbos-webrtc wrote: > VIDEO_ Done. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:66: enum PayloadType { On 2015/07/22 11:05:27, pbos-webrtc wrote: > These aren't payload types. No they are a legacy from an earlier version. We have discussed renaming them, but I guess I can do it now. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:86: // optional bytes payload = 5; On 2015/07/22 11:05:27, pbos-webrtc wrote: > How would this be turned on/off? If this is premature, remove it completely. I'll remove the // optional bytes payload = 5; part, but I will not remove the warning against logging payload data without privacy review. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:93: enum Direction { On 2015/07/22 11:05:27, pbos-webrtc wrote: > Can this be shared? I'll change it to a bool. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:138: // or SSRC and port number, but for now we will rely on SSRC only. On 2015/07/22 11:05:27, pbos-webrtc wrote: > We'll probably move away from media type as well, since there can be >1 > transport per media type (just not in our implementation), meaning that video + > ssrc isn't even enough to identify the stream. Ok, but there is nothing I can do about that unless we already have some better way to identify streams. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:245: // TODO(terelius): Figure out what the requirements are from the audio team. On 2015/07/22 11:05:27, pbos-webrtc wrote: > TODO(terelius): Add audio-receive config. Changed comment. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.proto:250: // TODO(terelius): Figure out what the requirements are from the audio team. On 2015/07/22 11:05:27, pbos-webrtc wrote: > TODO(terelius): Add audio-send config. Changed comment. https://codereview.webrtc.org/1230973005/diff/120001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:135: On 2015/07/22 11:05:27, pbos-webrtc wrote: > Remove empty lines. Done.
https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:152: return RelRtpPacket::UNKNOWN_TYPE; On 2015/07/23 15:54:00, terelius wrote: > On 2015/07/22 11:05:27, pbos-webrtc wrote: > > Should any "ANY" or "DATA" stamped packets enter here? If not, put a > > RTC_NOTREACHED(). > > It was unclear what UNKNOWN_TYPE was for, but presumably it would be used for > everything that wasn't audio or video. I've now removed the UNKNOWN_TYPE field > and added fields for ANY and DATA instead. The reason for UNKNOWN_TYPE is that when you add a field in the future to the enum in the protobuf, all old code will read those new types as UNKNOWN_TYPE (basically whatever happens to be element 0 of the enum). If element 0 is an actual meaningful type (like now), it will cause problems in the old code. Therefore I suggest adding back UNKNOWN_TYPE, and not using it.
lgtm https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:152: return RelRtpPacket::UNKNOWN_TYPE; On 2015/07/24 08:06:04, ivoc wrote: > On 2015/07/23 15:54:00, terelius wrote: > > On 2015/07/22 11:05:27, pbos-webrtc wrote: > > > Should any "ANY" or "DATA" stamped packets enter here? If not, put a > > > RTC_NOTREACHED(). > > > > It was unclear what UNKNOWN_TYPE was for, but presumably it would be used for > > everything that wasn't audio or video. I've now removed the UNKNOWN_TYPE field > > and added fields for ANY and DATA instead. > > The reason for UNKNOWN_TYPE is that when you add a field in the future to the > enum in the protobuf, all old code will read those new types as UNKNOWN_TYPE > (basically whatever happens to be element 0 of the enum). If element 0 is an > actual meaningful type (like now), it will cause problems in the old code. > Therefore I suggest adding back UNKNOWN_TYPE, and not using it. Don't understand this one, are you expecting to parse new protobufs with old code? https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:177: : crit_(webrtc::CriticalSectionWrapper::CreateCriticalSection()), On 2015/07/23 15:54:00, terelius wrote: > On 2015/07/22 11:05:27, pbos-webrtc wrote: > > Use a rtc::CriticalSection (this doesn't require additional heap allocation). > > We're trying to move off webrtc::CriticalSectionWrapper. > > Done. Please review the locking changes extra carefully as I am unfamiliar with > both the existing code and webrtc's locks. The GUARDED_BY macros above are your friend (they make sure you hold the lock while accessing the members). https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:76: void LogDebugEventLocked(DebugEvent event_type) Can this be consolidated into LogDebugEvent as message is removed? https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:97: rtc::scoped_ptr<rtclog::EventStream> stream_ GUARDED_BY(crit_); Why scoped_ptr? Can you use rtclog::EventStream stream_ GUARDED_BY..;? https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:274: // TODO(terelius): It is more convenient and less error prone to parse the Not sure I agree, packet parsing inside here will require knowing which RTP extensions are used for the SSRC (or you'll spam the log with warnings when trying to parse unknown extensions). This'll require a header parser per SSRC with a map internally which I think is less convenient and arguably more error prone (or equally). https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:127: for (int i = 0; i < receiver_config.rtx_map_size(); i++) { Use a foreach loop.
https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:152: return RelRtpPacket::UNKNOWN_TYPE; On 2015/07/24 08:06:04, ivoc wrote: > On 2015/07/23 15:54:00, terelius wrote: > > On 2015/07/22 11:05:27, pbos-webrtc wrote: > > > Should any "ANY" or "DATA" stamped packets enter here? If not, put a > > > RTC_NOTREACHED(). > > > > It was unclear what UNKNOWN_TYPE was for, but presumably it would be used for > > everything that wasn't audio or video. I've now removed the UNKNOWN_TYPE field > > and added fields for ANY and DATA instead. > > The reason for UNKNOWN_TYPE is that when you add a field in the future to the > enum in the protobuf, all old code will read those new types as UNKNOWN_TYPE > (basically whatever happens to be element 0 of the enum). If element 0 is an > actual meaningful type (like now), it will cause problems in the old code. > Therefore I suggest adding back UNKNOWN_TYPE, and not using it. By reading the generated protobuf code I've found that when one attempts to parse a new (ie unknown) enum value, the code will not write anything to the enum. A default constructed enum will thus be left in it's 0 state which would correspond to the UNKNOWN_TYPE. However, the has_bits will also remain zero, so we can detect that no (valid) enum type has been found by checking has_type() before accessing type(). This is the approach the current code is taking.
https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/120001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:152: return RelRtpPacket::UNKNOWN_TYPE; On 2015/07/24 11:43:40, pbos-webrtc wrote: > On 2015/07/24 08:06:04, ivoc wrote: > > On 2015/07/23 15:54:00, terelius wrote: > > > On 2015/07/22 11:05:27, pbos-webrtc wrote: > > > > Should any "ANY" or "DATA" stamped packets enter here? If not, put a > > > > RTC_NOTREACHED(). > > > > > > It was unclear what UNKNOWN_TYPE was for, but presumably it would be used > for > > > everything that wasn't audio or video. I've now removed the UNKNOWN_TYPE > field > > > and added fields for ANY and DATA instead. > > > > The reason for UNKNOWN_TYPE is that when you add a field in the future to the > > enum in the protobuf, all old code will read those new types as UNKNOWN_TYPE > > (basically whatever happens to be element 0 of the enum). If element 0 is an > > actual meaningful type (like now), it will cause problems in the old code. > > Therefore I suggest adding back UNKNOWN_TYPE, and not using it. > > Don't understand this one, are you expecting to parse new protobufs with old > code? Well, not planning on it, but it might happen. It is listed as a best practice on go/protodosdonts to do this. Anyway, after some discussion with terelius@, I changed my mind and agree that in this case it's not particularly useful, mostly due to the mismatch with existing WebRTC data structures when parsing.
https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:76: void LogDebugEventLocked(DebugEvent event_type) On 2015/07/24 11:43:40, pbos-webrtc wrote: > Can this be consolidated into LogDebugEvent as message is removed? Done. This required some refactoring of related functions (start and stop logging), but I think it eliminated some other potential problems. https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:97: rtc::scoped_ptr<rtclog::EventStream> stream_ GUARDED_BY(crit_); On 2015/07/24 11:43:40, pbos-webrtc wrote: > Why scoped_ptr? Can you use rtclog::EventStream stream_ GUARDED_BY..;? Done. Removed scoped_ptr. https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:274: // TODO(terelius): It is more convenient and less error prone to parse the On 2015/07/24 11:43:40, pbos-webrtc wrote: > Not sure I agree, packet parsing inside here will require knowing which RTP > extensions are used for the SSRC (or you'll spam the log with warnings when > trying to parse unknown extensions). This'll require a header parser per SSRC > with a map internally which I think is less convenient and arguably more error > prone (or equally). We don't need to know which extensions are used. The header length is 12 + 4*CC + X*4*(E+1) where CC is bits [4-7], X is bit [3] and E is bits [112+32*CC, 127+32*CC]. The point is that this only has to be written once, whereas someone who tries to use the log might be tempted to hard code a header size of e.g. 20 bytes. Indeed, this is what I did when I tested the functions. Automatically determining the correct header length also prevents some misuse like logging full packets by setting the header length = packet length. https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1230973005/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:127: for (int i = 0; i < receiver_config.rtx_map_size(); i++) { On 2015/07/24 11:43:40, pbos-webrtc wrote: > Use a foreach loop. Done.
LG... https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:37: void StartLogging(const std::string& file_name, int duration_ms) override{}; Space after override. Same below. Also no ; after {} https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:54: #else new line above, possibly https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:77: // Stops logging and clears the stored data and buffers. TODO Owner for TODO https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:154: } // Anonymous namespace. Remove Anonymous (at least that's how I think we typically write). https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:45: virtual void StartLogging(const std::string& file_name, int duration_ms) = 0; It's not entirely clear to me who will be specifying a duration here. Is that something we want to expose in a UI? https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:74: // The result is stored in the given RelEventStream object. EventStream
https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:37: void StartLogging(const std::string& file_name, int duration_ms) override{}; On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > Space after override. Same below. Also no ; after {} Done. https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:54: #else On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > new line above, possibly Done. https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:77: // Stops logging and clears the stored data and buffers. TODO On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > Owner for TODO Done. Removed empty TODO. https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.cc:154: } // Anonymous namespace. On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > Remove Anonymous (at least that's how I think we typically write). Done. https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:45: virtual void StartLogging(const std::string& file_name, int duration_ms) = 0; On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > It's not entirely clear to me who will be specifying a duration here. Is that > something we want to expose in a UI? A user might want to log, e.g., an entire call which can be accomplished by setting the duration to a high value. Otherwise I believe the typical duration would be 10-20 seconds. https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log.h:74: // The result is stored in the given RelEventStream object. On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > EventStream Done.
On 2015/07/28 11:52:09, terelius wrote: > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > File webrtc/video/rtc_event_log.cc (right): > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.cc:37: void StartLogging(const std::string& > file_name, int duration_ms) override{}; > On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > > Space after override. Same below. Also no ; after {} > > Done. > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.cc:54: #else > On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > > new line above, possibly > > Done. > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.cc:77: // Stops logging and clears the stored data > and buffers. TODO > On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > > Owner for TODO > > Done. Removed empty TODO. > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.cc:154: } // Anonymous namespace. > On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > > Remove Anonymous (at least that's how I think we typically write). > > Done. > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > File webrtc/video/rtc_event_log.h (right): > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.h:45: virtual void StartLogging(const std::string& > file_name, int duration_ms) = 0; > On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > > It's not entirely clear to me who will be specifying a duration here. Is that > > something we want to expose in a UI? > > A user might want to log, e.g., an entire call which can be accomplished by > setting the duration to a high value. Otherwise I believe the typical duration > would be 10-20 seconds. > > https://codereview.webrtc.org/1230973005/diff/180001/webrtc/video/rtc_event_l... > webrtc/video/rtc_event_log.h:74: // The result is stored in the given > RelEventStream object. > On 2015/07/28 08:15:34, stefan-webrtc (holmer) wrote: > > EventStream > > Done. LGTM
lgtm
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/patch-status/1230973005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230973005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/271)
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as c159b046d7a0086e45ae0f79c00a462f3fafd207. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
