Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1257163003:
Changed LogRtpHeader to read the header length from the packet (Closed)
|
Created:
5 years, 4 months ago by terelius Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, andresp, perkj_webrtc, mflodman, kwiberg-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChanged LogRtpHeader to read the header length from the packet instead of requiring an extra argument.
BUG=
Committed: https://crrev.com/2f9fd5ddb9097054b71fce20ba24bc34b2f084c0
Cr-Commit-Position: refs/heads/master@{#9856}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Manual control of header extensions in unit test, and minor reviewer comments. #
Total comments: 30
Patch Set 3 : Small refactoring of code to select header extensions #Patch Set 4 : Changed integer literals to unsigned #
Total comments: 8
Patch Set 5 : Renaming constants and fixing some other reviewer comments #Patch Set 6 : Represent packets using rtc::Buffer #
Total comments: 4
Patch Set 7 : Nits #Patch Set 8 : Fixed upload of wrong nits patch #
Total comments: 2
Patch Set 9 : Avoid implicit conversions int -> bool #
Messages
Total messages: 26 (4 generated)
terelius@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org, stefan@webrtc.org
Please have a look at this patch which moves the parsing the header length into the logging function instead of letting the caller pass the header length as an extra argument. The rationale for the change is to prevent the caller from passing in an incorrect value. For example, passing the packet length instead of header length would cause the RtcEventLog to log full packets instead of headers.
See comments inline. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:294: const uint8_t X = (header[0] & 0x10) == 0 ? 0 : 1; const bool x = (header[0] & 0x10) != 0; https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:295: const uint8_t CC = header[0] & 0x0f; cc https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:296: size_t header_length = 12u + CC*4u; The style guide says it is ok to omit the spaces around factors, but I think we tend to include them in the WebRTC code base. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:302: size_t XLen = ByteReader<uint16_t>::ReadBigEndian(header + 14 + CC*4); x_len https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:303: header_length += (XLen+1)*4; Spaces around binary operators, please. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.h#... webrtc/video/rtc_event_log.h:57: // Logs the header of an incoming or outgoing RTP packet. Please, explain that packet_length is the total length of the packet, including the payload. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:261: RTPSender rtp_sender(rand(), // int32_t id Use 0 as id instead. This test is not for testing the RTPSender. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:286: if (rand()%2) { I don't like this randomized test configuration. Construct the test such that you can iterate through different configs, e.g., no extensions, one extension, all extensions... https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:475: for (size_t i = 0; i < rtp_count; i++) { for (auto packet : rtp_packets) { delete[] packet; }
https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:299: if (packet_length < 12u + CC*4u + 4u) { Why not make this 16u + CC*4u? Another option would be header_length + 4u. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> rtp_header_sizes; I don't see the advantage of this collection of datastructures over the vector of vectors that used here previously. I would prefer the vector of vectors because it eliminates any chance at memory leaks.
https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:294: const uint8_t X = (header[0] & 0x10) == 0 ? 0 : 1; On 2015/08/04 09:43:50, hlundin-webrtc wrote: > const bool x = (header[0] & 0x10) != 0; Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:295: const uint8_t CC = header[0] & 0x0f; On 2015/08/04 09:43:50, hlundin-webrtc wrote: > cc Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:296: size_t header_length = 12u + CC*4u; On 2015/08/04 09:43:50, hlundin-webrtc wrote: > The style guide says it is ok to omit the spaces around factors, but I think we > tend to include them in the WebRTC code base. Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:299: if (packet_length < 12u + CC*4u + 4u) { On 2015/08/06 12:29:44, ivoc wrote: > Why not make this 16u + CC*4u? Another option would be header_length + 4u. I wrote it this way to more clearly match the RTP specification. The fixed header is 12 bytes, then follows CC 32-bit words and then (if there are header extensions) 4 bytes containing the number of extensions. All optimizing compilers will compute it as 16+CC*4, so there is no performance penalty. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:302: size_t XLen = ByteReader<uint16_t>::ReadBigEndian(header + 14 + CC*4); On 2015/08/04 09:43:50, hlundin-webrtc wrote: > x_len Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:303: header_length += (XLen+1)*4; On 2015/08/04 09:43:50, hlundin-webrtc wrote: > Spaces around binary operators, please. Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.h File webrtc/video/rtc_event_log.h (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.h#... webrtc/video/rtc_event_log.h:57: // Logs the header of an incoming or outgoing RTP packet. On 2015/08/04 09:43:50, hlundin-webrtc wrote: > Please, explain that packet_length is the total length of the packet, including > the payload. Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:261: RTPSender rtp_sender(rand(), // int32_t id On 2015/08/04 09:43:50, hlundin-webrtc wrote: > Use 0 as id instead. This test is not for testing the RTPSender. Done. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:286: if (rand()%2) { On 2015/08/04 09:43:50, hlundin-webrtc wrote: > I don't like this randomized test configuration. Construct the test such that > you can iterate through different configs, e.g., no extensions, one extension, > all extensions... I've rewritten to the tests to allow manual control of which extensions are used. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> rtp_header_sizes; On 2015/08/06 12:29:44, ivoc wrote: > I don't see the advantage of this collection of datastructures over the vector > of vectors that used here previously. I would prefer the vector of vectors > because it eliminates any chance at memory leaks. I agree that memory leaks is a concern, but all other code that manipulate packets represent them as a uint8_t* with a size_t length. Thus, interfacing with other code becomes easier this way, while most memory errors should be caught by the asan/msan trybots if not by local testing. It is certainly possible to use a vector, but I think it will lead to code like ================== vector<uint8_t> packet; packet.assign(0, IP_SIZE); size_t length = GenerateSomething(packet.data()); packet.resize(length); DoSomething(packet.data(), packet.size()); ================== which I think is less clear than ================== uint8_t* packet = new uint8_t[IP_SIZE]; size_t length = GenerateSomething(packet); DoSomething(packet, length); ================== https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:475: for (size_t i = 0; i < rtp_count; i++) { On 2015/08/04 09:43:50, hlundin-webrtc wrote: > for (auto packet : rtp_packets) { > delete[] packet; > } Done.
A few more comments. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:299: if (packet_length < 12u + CC*4u + 4u) { On 2015/08/12 13:12:40, terelius wrote: > On 2015/08/06 12:29:44, ivoc wrote: > > Why not make this 16u + CC*4u? Another option would be header_length + 4u. > > I wrote it this way to more clearly match the RTP specification. The fixed > header is 12 bytes, then follows CC 32-bit words and then (if there are header > extensions) 4 bytes containing the number of extensions. All optimizing > compilers will compute it as 16+CC*4, so there is no performance penalty. I like it this way. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:38: MediaType GetRuntimeMediaType(rtclog::MediaType media_type) { Collect all the file-scope declarations and functions in an unnamed namespace. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:251: ASSERT_TRUE(IsValidBasicEvent(event)); If any of the ASSERT_*s trigger, only this subfunction will exit, and the rest of the current test will be executed anyway. This may have some unforeseen consequences. You might have already thought about this, but I wanted to point it out anyway. See link below for discussions and ways to avoid it. https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:267: * LSB of extension_bitvector indicates presence of TransmissionTimeOffset, Please, add constant bitmasks representing each bit: static const uint32_t kTransmissionTimeOffsetBit = 1; static const uint32_t kAudioLevelBit = 2; ... and so on. Then use these when you construct your test cases. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:298: if (extensions_bitvector & 1) Use the bitmasks here as well. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:361: if (extensions_bitvector & 1) { ... and here. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:362: extension_name = RtpExtension::kTOffset; Can't you use RtpExtension::kTOffset directly in the call to RtpExtension? Then you can delete extension_name, and save 25% of the lines here. Same thing happens again later in the code. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:397: if (extensions_bitvector & 1) { ... and bitmasks here, too. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:427: assert(rtcp_count <= rtp_count); Use ASSERT_LE instead, to avoid crashing the entire test binary. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:548: LogSessionAndReadBack(5, 2, 0, 0, 0, 321); Use the bitmask constants here too. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:552: for (unsigned extensions = 0; extensions < 32; extensions++) { size_t, since extensions is used as input argument to LogSessionAndReadBack(size_t ...) https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:553: for (unsigned csrcs_count = 0; csrcs_count < 3; csrcs_count++) { size_t
https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc File webrtc/video/rtc_event_log.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log.cc... webrtc/video/rtc_event_log.cc:299: if (packet_length < 12u + CC*4u + 4u) { On 2015/08/13 14:09:51, hlundin-webrtc wrote: > On 2015/08/12 13:12:40, terelius wrote: > > On 2015/08/06 12:29:44, ivoc wrote: > > > Why not make this 16u + CC*4u? Another option would be header_length + 4u. > > > > I wrote it this way to more clearly match the RTP specification. The fixed > > header is 12 bytes, then follows CC 32-bit words and then (if there are header > > extensions) 4 bytes containing the number of extensions. All optimizing > > compilers will compute it as 16+CC*4, so there is no performance penalty. > > I like it this way. Acknowledged. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> rtp_header_sizes; On 2015/08/12 13:12:40, terelius wrote: > On 2015/08/06 12:29:44, ivoc wrote: > > I don't see the advantage of this collection of datastructures over the vector > > of vectors that used here previously. I would prefer the vector of vectors > > because it eliminates any chance at memory leaks. > > I agree that memory leaks is a concern, but all other code that manipulate > packets represent them as a uint8_t* with a size_t length. Thus, interfacing > with other code becomes easier this way, while most memory errors should be > caught by the asan/msan trybots if not by local testing. > > It is certainly possible to use a vector, but I think it will lead to code like > ================== > vector<uint8_t> packet; > packet.assign(0, IP_SIZE); > size_t length = GenerateSomething(packet.data()); > packet.resize(length); > DoSomething(packet.data(), packet.size()); > ================== > which I think is less clear than > ================== > uint8_t* packet = new uint8_t[IP_SIZE]; > size_t length = GenerateSomething(packet); > DoSomething(packet, length); > ================== I think in your example the vector code could look like this: ==================== vector<uint8_t> packet(IP_SIZE); size_t length = GenerateSomething(packet.data()); DoSomething(packet.data(), length); ==================== Which is fairly similar to the non-vector case, with the difference that a delete is no longer needed. More specific to this function you could do something like this: ==================== vector<vector<uint8_t>> rtp_packets(rtp_count); ... for (auto &p: rtp_packets) { size_t packet_size = 1000 + rand() % 30; p.resize(packet_size); ... } ==================== Which can be compared to the following current code: ==================== std::vector<uint8_t*> rtp_packets; std::vector<size_t> rtp_packet_sizes; ... for (size_t i = 0; i < rtp_count; i++) { size_t packet_size = 1000 + rand() % 30; rtp_packet_sizes.push_back(packet_size); rtp_packets.push_back(new uint8_t[packet_size]); ... } ... for (size_t i = 0; i < rtp_count; i++) { delete[] rtp_packets[i]; } ==================== I honestly don't see any downsides of the vector approach, to me it looks shorter, easier to read and safer.
https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:38: MediaType GetRuntimeMediaType(rtclog::MediaType media_type) { On 2015/08/13 14:09:52, hlundin-webrtc wrote: > Collect all the file-scope declarations and functions in an unnamed namespace. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces This will be moved to the RtcEventLogParser class by https://codereview.webrtc.org/1295753003/ https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:251: ASSERT_TRUE(IsValidBasicEvent(event)); On 2015/08/13 14:09:52, hlundin-webrtc wrote: > If any of the ASSERT_*s trigger, only this subfunction will exit, and the rest > of the current test will be executed anyway. This may have some unforeseen > consequences. > > You might have already thought about this, but I wanted to point it out anyway. > See link below for discussions and ways to avoid it. > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... Yes, it is intentional. Even if we fail to read one event in the event stream, we might still be able to read the rest. This makes it easier to distinguish whether it is a particular event type that we fail to parse, or whether it is something that affects all events. (The downside is that we might flood the terminal with error messages, so there is no obvious solution here.) Thanks for the link though. I'll save it for future reference. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:267: * LSB of extension_bitvector indicates presence of TransmissionTimeOffset, On 2015/08/13 14:09:52, hlundin-webrtc wrote: > Please, add constant bitmasks representing each bit: > static const uint32_t kTransmissionTimeOffsetBit = 1; > static const uint32_t kAudioLevelBit = 2; > ... and so on. > Then use these when you construct your test cases. I changed the approach to avoid repeating almost the same code for each of the 5 possible extensions. There is now an array of extension names and a corresponding array of extension types. The bitmask for element i in these vectors is 1<<i, but as it is only needed in very few places I have not added names for each bitmask. Do you still think that having named bitmasks would be useful? https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:298: if (extensions_bitvector & 1) On 2015/08/13 14:09:52, hlundin-webrtc wrote: > Use the bitmasks here as well. Rewritten. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:361: if (extensions_bitvector & 1) { On 2015/08/13 14:09:52, hlundin-webrtc wrote: > ... and here. Rewritten. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:362: extension_name = RtpExtension::kTOffset; On 2015/08/13 14:09:52, hlundin-webrtc wrote: > Can't you use RtpExtension::kTOffset directly in the call to RtpExtension? Then > you can delete extension_name, and save 25% of the lines here. Same thing > happens again later in the code. I could, but that would make the lines too long. The automatic formatting would then undo the 25% reduction of code we achieved by removing extension_name. The new code is much shorter anyway. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:397: if (extensions_bitvector & 1) { On 2015/08/13 14:09:52, hlundin-webrtc wrote: > ... and bitmasks here, too. Rewritten. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:427: assert(rtcp_count <= rtp_count); On 2015/08/13 14:09:52, hlundin-webrtc wrote: > Use ASSERT_LE instead, to avoid crashing the entire test binary. Done. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:548: LogSessionAndReadBack(5, 2, 0, 0, 0, 321); On 2015/08/13 14:09:52, hlundin-webrtc wrote: > Use the bitmask constants here too. This is the one place where named bitmasks might be more convenient. Advice? https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:552: for (unsigned extensions = 0; extensions < 32; extensions++) { On 2015/08/13 14:09:52, hlundin-webrtc wrote: > size_t, since extensions is used as input argument to > LogSessionAndReadBack(size_t ...) Sorry, I don't understand. Can you elaborate? https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:553: for (unsigned csrcs_count = 0; csrcs_count < 3; csrcs_count++) { On 2015/08/13 14:09:52, hlundin-webrtc wrote: > size_t I don't understand. Can you elaborate?
Henrik, do you want to make the final vote on the representation of packets in the unit test? I prefer using a byte-pointer and a separate length to conform to same standard as the existing code and to have an easier time interfacing with it. Ivo prefers using a vector to avoid memory leaks, and to conform to the C++ way of managing memory. https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> rtp_header_sizes; On 2015/08/14 11:21:53, ivoc wrote: > On 2015/08/12 13:12:40, terelius wrote: > > On 2015/08/06 12:29:44, ivoc wrote: > > > I don't see the advantage of this collection of datastructures over the > vector > > > of vectors that used here previously. I would prefer the vector of vectors > > > because it eliminates any chance at memory leaks. > > > > I agree that memory leaks is a concern, but all other code that manipulate > > packets represent them as a uint8_t* with a size_t length. Thus, interfacing > > with other code becomes easier this way, while most memory errors should be > > caught by the asan/msan trybots if not by local testing. > > > > It is certainly possible to use a vector, but I think it will lead to code > like > > ================== > > vector<uint8_t> packet; > > packet.assign(0, IP_SIZE); > > size_t length = GenerateSomething(packet.data()); > > packet.resize(length); > > DoSomething(packet.data(), packet.size()); > > ================== > > which I think is less clear than > > ================== > > uint8_t* packet = new uint8_t[IP_SIZE]; > > size_t length = GenerateSomething(packet); > > DoSomething(packet, length); > > ================== > > I think in your example the vector code could look like this: > ==================== > vector<uint8_t> packet(IP_SIZE); > size_t length = GenerateSomething(packet.data()); > DoSomething(packet.data(), length); > ==================== > Which is fairly similar to the non-vector case, with the difference that a > delete is no longer needed. > More specific to this function you could do something like this: > ==================== > vector<vector<uint8_t>> rtp_packets(rtp_count); > ... > for (auto &p: rtp_packets) { > size_t packet_size = 1000 + rand() % 30; > p.resize(packet_size); > ... > } > ==================== > Which can be compared to the following current code: > ==================== > std::vector<uint8_t*> rtp_packets; > std::vector<size_t> rtp_packet_sizes; > ... > for (size_t i = 0; i < rtp_count; i++) { > size_t packet_size = 1000 + rand() % 30; > rtp_packet_sizes.push_back(packet_size); > rtp_packets.push_back(new uint8_t[packet_size]); > ... > } > ... > for (size_t i = 0; i < rtp_count; i++) { > delete[] rtp_packets[i]; > } > ==================== > I honestly don't see any downsides of the vector approach, to me it looks > shorter, easier to read and safer. Some functions like BuildRTPHeader writes some bytes and returns the number of bytes written. If we are going to use the .size() to keep track of the packet size, then we need to call resize twice. Once before building the header to allocate enough space and once after to set the actual size. Your first proposal which does not use resize at all, implicitly requires storing the actual packet lengths in a second vector. I think this solution is a bit odd since we would have a bunch of byte vectors each with it's own size() function and then we would have a second vector for storing the actual size of each packet.
https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> rtp_header_sizes; On 2015/08/14 18:17:45, terelius wrote: > On 2015/08/14 11:21:53, ivoc wrote: > > On 2015/08/12 13:12:40, terelius wrote: > > > On 2015/08/06 12:29:44, ivoc wrote: > > > > I don't see the advantage of this collection of datastructures over the > > vector > > > > of vectors that used here previously. I would prefer the vector of vectors > > > > because it eliminates any chance at memory leaks. > > > > > > I agree that memory leaks is a concern, but all other code that manipulate > > > packets represent them as a uint8_t* with a size_t length. Thus, interfacing > > > with other code becomes easier this way, while most memory errors should be > > > caught by the asan/msan trybots if not by local testing. > > > > > > It is certainly possible to use a vector, but I think it will lead to code > > like > > > ================== > > > vector<uint8_t> packet; > > > packet.assign(0, IP_SIZE); > > > size_t length = GenerateSomething(packet.data()); > > > packet.resize(length); > > > DoSomething(packet.data(), packet.size()); > > > ================== > > > which I think is less clear than > > > ================== > > > uint8_t* packet = new uint8_t[IP_SIZE]; > > > size_t length = GenerateSomething(packet); > > > DoSomething(packet, length); > > > ================== > > > > I think in your example the vector code could look like this: > > ==================== > > vector<uint8_t> packet(IP_SIZE); > > size_t length = GenerateSomething(packet.data()); > > DoSomething(packet.data(), length); > > ==================== > > Which is fairly similar to the non-vector case, with the difference that a > > delete is no longer needed. > > More specific to this function you could do something like this: > > ==================== > > vector<vector<uint8_t>> rtp_packets(rtp_count); > > ... > > for (auto &p: rtp_packets) { > > size_t packet_size = 1000 + rand() % 30; > > p.resize(packet_size); > > ... > > } > > ==================== > > Which can be compared to the following current code: > > ==================== > > std::vector<uint8_t*> rtp_packets; > > std::vector<size_t> rtp_packet_sizes; > > ... > > for (size_t i = 0; i < rtp_count; i++) { > > size_t packet_size = 1000 + rand() % 30; > > rtp_packet_sizes.push_back(packet_size); > > rtp_packets.push_back(new uint8_t[packet_size]); > > ... > > } > > ... > > for (size_t i = 0; i < rtp_count; i++) { > > delete[] rtp_packets[i]; > > } > > ==================== > > I honestly don't see any downsides of the vector approach, to me it looks > > shorter, easier to read and safer. > > Some functions like BuildRTPHeader writes some bytes and returns the number of > bytes written. If we are going to use the .size() to keep track of the packet > size, then we need to call resize twice. Once before building the header to > allocate enough space and once after to set the actual size. > > Your first proposal which does not use resize at all, implicitly requires > storing the actual packet lengths in a second vector. I think this solution is a > bit odd since we would have a bunch of byte vectors each with it's own size() > function and then we would have a second vector for storing the actual size of > each packet. My random thoughts: 1. Consider using rtc::Buffer instead of std::vector. 2. You are caught between new and old paradigms. No solution is perfect. 3. If you are going for the raw pointers solution, use scoped_ptr to avoid leaks. Now, a concrete proposal: Rewrite GeneateRtpPacket to size_t GenerateRtpPacket(rtc::Buffer* packet) { // Desired packet length is determined by packet->size(); ... const size_t header_size = rtp_sender.BuildRTPheader(packet->data(), ...); ASSERT_LE(header_size, packet->size()); // Randomize the payload as before. return header_size; } Use: std::vector<rtc::Buffer> rtp_packets; std::vector<size_t> rtp_header_sizes; But skip: std::vector<size_t> rtp_packet_sizes; Use rtc::Buffer in the validation method(s) as well. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:38: MediaType GetRuntimeMediaType(rtclog::MediaType media_type) { On 2015/08/14 17:48:26, terelius wrote: > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > Collect all the file-scope declarations and functions in an unnamed namespace. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > This will be moved to the RtcEventLogParser class by > https://codereview.webrtc.org/1295753003/ > Acknowledged. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:251: ASSERT_TRUE(IsValidBasicEvent(event)); On 2015/08/14 17:48:26, terelius wrote: > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > If any of the ASSERT_*s trigger, only this subfunction will exit, and the rest > > of the current test will be executed anyway. This may have some unforeseen > > consequences. > > > > You might have already thought about this, but I wanted to point it out > anyway. > > See link below for discussions and ways to avoid it. > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > > Yes, it is intentional. Even if we fail to read one event in the event stream, > we might still be able to read the rest. This makes it easier to distinguish > whether it is a particular event type that we fail to parse, or whether it is > something that affects all events. (The downside is that we might flood the > terminal with error messages, so there is no obvious solution here.) > > Thanks for the link though. I'll save it for future reference. Acknowledged. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:548: LogSessionAndReadBack(5, 2, 0, 0, 0, 321); On 2015/08/14 17:48:26, terelius wrote: > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > Use the bitmask constants here too. > > This is the one place where named bitmasks might be more convenient. Advice? I suggest you introduce an array of bitmasks, the same size as the other const arrays you already added at the top of the file. Then you can index that with the same index as the other arrays, and use it both when constructing and parsing the combined bit-patterns. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:552: for (unsigned extensions = 0; extensions < 32; extensions++) { On 2015/08/14 17:48:26, terelius wrote: > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > size_t, since extensions is used as input argument to > > LogSessionAndReadBack(size_t ...) > > Sorry, I don't understand. Can you elaborate? I was nagging about you using unsigned as the type. Then you are passing it as argument to LogSessionAndReadBack(..., uint32_t extensions_bitvector, ...). Better to decide on using one type. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:553: for (unsigned csrcs_count = 0; csrcs_count < 3; csrcs_count++) { On 2015/08/14 17:48:27, terelius wrote: > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > size_t > > I don't understand. Can you elaborate? Same as above. https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:39: RTPExtensionType extension_types[] = { kExtensionTypes https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:39: RTPExtensionType extension_types[] = { const https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:45: const char* extension_names[] = {RtpExtension::kTOffset, kExtensionNames https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:50: unsigned n_extensions = 5; const size_t kNumExtensions
On 2015/08/17 13:47:07, hlundin-webrtc wrote: > https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... > File webrtc/video/rtc_event_log_unittest.cc (right): > > https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... > webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> > rtp_header_sizes; > On 2015/08/14 18:17:45, terelius wrote: > > On 2015/08/14 11:21:53, ivoc wrote: > > > On 2015/08/12 13:12:40, terelius wrote: > > > > On 2015/08/06 12:29:44, ivoc wrote: > > > > > I don't see the advantage of this collection of datastructures over the > > > vector > > > > > of vectors that used here previously. I would prefer the vector of > vectors > > > > > because it eliminates any chance at memory leaks. > > > > > > > > I agree that memory leaks is a concern, but all other code that manipulate > > > > packets represent them as a uint8_t* with a size_t length. Thus, > interfacing > > > > with other code becomes easier this way, while most memory errors should > be > > > > caught by the asan/msan trybots if not by local testing. > > > > > > > > It is certainly possible to use a vector, but I think it will lead to code > > > like > > > > ================== > > > > vector<uint8_t> packet; > > > > packet.assign(0, IP_SIZE); > > > > size_t length = GenerateSomething(packet.data()); > > > > packet.resize(length); > > > > DoSomething(packet.data(), packet.size()); > > > > ================== > > > > which I think is less clear than > > > > ================== > > > > uint8_t* packet = new uint8_t[IP_SIZE]; > > > > size_t length = GenerateSomething(packet); > > > > DoSomething(packet, length); > > > > ================== > > > > > > I think in your example the vector code could look like this: > > > ==================== > > > vector<uint8_t> packet(IP_SIZE); > > > size_t length = GenerateSomething(packet.data()); > > > DoSomething(packet.data(), length); > > > ==================== > > > Which is fairly similar to the non-vector case, with the difference that a > > > delete is no longer needed. > > > More specific to this function you could do something like this: > > > ==================== > > > vector<vector<uint8_t>> rtp_packets(rtp_count); > > > ... > > > for (auto &p: rtp_packets) { > > > size_t packet_size = 1000 + rand() % 30; > > > p.resize(packet_size); > > > ... > > > } > > > ==================== > > > Which can be compared to the following current code: > > > ==================== > > > std::vector<uint8_t*> rtp_packets; > > > std::vector<size_t> rtp_packet_sizes; > > > ... > > > for (size_t i = 0; i < rtp_count; i++) { > > > size_t packet_size = 1000 + rand() % 30; > > > rtp_packet_sizes.push_back(packet_size); > > > rtp_packets.push_back(new uint8_t[packet_size]); > > > ... > > > } > > > ... > > > for (size_t i = 0; i < rtp_count; i++) { > > > delete[] rtp_packets[i]; > > > } > > > ==================== > > > I honestly don't see any downsides of the vector approach, to me it looks > > > shorter, easier to read and safer. > > > > Some functions like BuildRTPHeader writes some bytes and returns the number of > > bytes written. If we are going to use the .size() to keep track of the packet > > size, then we need to call resize twice. Once before building the header to > > allocate enough space and once after to set the actual size. > > > > Your first proposal which does not use resize at all, implicitly requires > > storing the actual packet lengths in a second vector. I think this solution is > a > > bit odd since we would have a bunch of byte vectors each with it's own size() > > function and then we would have a second vector for storing the actual size of > > each packet. > > My random thoughts: > > 1. Consider using rtc::Buffer instead of std::vector. > 2. You are caught between new and old paradigms. No solution is perfect. > 3. If you are going for the raw pointers solution, use scoped_ptr to avoid > leaks. > > > Now, a concrete proposal: > > Rewrite GeneateRtpPacket to > > size_t GenerateRtpPacket(rtc::Buffer* packet) { > // Desired packet length is determined by packet->size(); > ... > const size_t header_size = rtp_sender.BuildRTPheader(packet->data(), > > ...); > ASSERT_LE(header_size, packet->size()); > // Randomize the payload as before. > return header_size; > } > > Use: > std::vector<rtc::Buffer> rtp_packets; > std::vector<size_t> rtp_header_sizes; > > But skip: > std::vector<size_t> rtp_packet_sizes; > > Use rtc::Buffer in the validation method(s) as well. +kwiberg to optionally weigh in on the above. > > https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... > File webrtc/video/rtc_event_log_unittest.cc (right): > > https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:38: MediaType > GetRuntimeMediaType(rtclog::MediaType media_type) { > On 2015/08/14 17:48:26, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > Collect all the file-scope declarations and functions in an unnamed > namespace. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > > > This will be moved to the RtcEventLogParser class by > > https://codereview.webrtc.org/1295753003/ > > > > Acknowledged. > > https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:251: > ASSERT_TRUE(IsValidBasicEvent(event)); > On 2015/08/14 17:48:26, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > If any of the ASSERT_*s trigger, only this subfunction will exit, and the > rest > > > of the current test will be executed anyway. This may have some unforeseen > > > consequences. > > > > > > You might have already thought about this, but I wanted to point it out > > anyway. > > > See link below for discussions and ways to avoid it. > > > > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > > > > Yes, it is intentional. Even if we fail to read one event in the event stream, > > we might still be able to read the rest. This makes it easier to distinguish > > whether it is a particular event type that we fail to parse, or whether it is > > something that affects all events. (The downside is that we might flood the > > terminal with error messages, so there is no obvious solution here.) > > > > Thanks for the link though. I'll save it for future reference. > > Acknowledged. > > https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:548: LogSessionAndReadBack(5, 2, 0, 0, 0, > 321); > On 2015/08/14 17:48:26, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > Use the bitmask constants here too. > > > > This is the one place where named bitmasks might be more convenient. Advice? > > I suggest you introduce an array of bitmasks, the same size as the other const > arrays you already added at the top of the file. Then you can index that with > the same index as the other arrays, and use it both when constructing and > parsing the combined bit-patterns. > > https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:552: for (unsigned extensions = 0; > extensions < 32; extensions++) { > On 2015/08/14 17:48:26, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > size_t, since extensions is used as input argument to > > > LogSessionAndReadBack(size_t ...) > > > > Sorry, I don't understand. Can you elaborate? > > I was nagging about you using unsigned as the type. Then you are passing it as > argument to LogSessionAndReadBack(..., uint32_t extensions_bitvector, ...). > Better to decide on using one type. > > https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:553: for (unsigned csrcs_count = 0; > csrcs_count < 3; csrcs_count++) { > On 2015/08/14 17:48:27, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > size_t > > > > I don't understand. Can you elaborate? > > Same as above. > > https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... > File webrtc/video/rtc_event_log_unittest.cc (right): > > https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:39: RTPExtensionType extension_types[] = > { > kExtensionTypes > > https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:39: RTPExtensionType extension_types[] = > { > const > > https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:45: const char* extension_names[] = > {RtpExtension::kTOffset, > kExtensionNames > > https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... > webrtc/video/rtc_event_log_unittest.cc:50: unsigned n_extensions = 5; > const size_t kNumExtensions
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/1/webrtc/video/rtc_event_log_un... webrtc/video/rtc_event_log_unittest.cc:367: std::vector<size_t> rtp_header_sizes; On 2015/08/17 13:47:07, hlundin-webrtc wrote: > On 2015/08/14 18:17:45, terelius wrote: > > On 2015/08/14 11:21:53, ivoc wrote: > > > On 2015/08/12 13:12:40, terelius wrote: > > > > On 2015/08/06 12:29:44, ivoc wrote: > > > > > I don't see the advantage of this collection of datastructures over the > > > vector > > > > > of vectors that used here previously. I would prefer the vector of > vectors > > > > > because it eliminates any chance at memory leaks. > > > > > > > > I agree that memory leaks is a concern, but all other code that manipulate > > > > packets represent them as a uint8_t* with a size_t length. Thus, > interfacing > > > > with other code becomes easier this way, while most memory errors should > be > > > > caught by the asan/msan trybots if not by local testing. > > > > > > > > It is certainly possible to use a vector, but I think it will lead to code > > > like > > > > ================== > > > > vector<uint8_t> packet; > > > > packet.assign(0, IP_SIZE); > > > > size_t length = GenerateSomething(packet.data()); > > > > packet.resize(length); > > > > DoSomething(packet.data(), packet.size()); > > > > ================== > > > > which I think is less clear than > > > > ================== > > > > uint8_t* packet = new uint8_t[IP_SIZE]; > > > > size_t length = GenerateSomething(packet); > > > > DoSomething(packet, length); > > > > ================== > > > > > > I think in your example the vector code could look like this: > > > ==================== > > > vector<uint8_t> packet(IP_SIZE); > > > size_t length = GenerateSomething(packet.data()); > > > DoSomething(packet.data(), length); > > > ==================== > > > Which is fairly similar to the non-vector case, with the difference that a > > > delete is no longer needed. > > > More specific to this function you could do something like this: > > > ==================== > > > vector<vector<uint8_t>> rtp_packets(rtp_count); > > > ... > > > for (auto &p: rtp_packets) { > > > size_t packet_size = 1000 + rand() % 30; > > > p.resize(packet_size); > > > ... > > > } > > > ==================== > > > Which can be compared to the following current code: > > > ==================== > > > std::vector<uint8_t*> rtp_packets; > > > std::vector<size_t> rtp_packet_sizes; > > > ... > > > for (size_t i = 0; i < rtp_count; i++) { > > > size_t packet_size = 1000 + rand() % 30; > > > rtp_packet_sizes.push_back(packet_size); > > > rtp_packets.push_back(new uint8_t[packet_size]); > > > ... > > > } > > > ... > > > for (size_t i = 0; i < rtp_count; i++) { > > > delete[] rtp_packets[i]; > > > } > > > ==================== > > > I honestly don't see any downsides of the vector approach, to me it looks > > > shorter, easier to read and safer. > > > > Some functions like BuildRTPHeader writes some bytes and returns the number of > > bytes written. If we are going to use the .size() to keep track of the packet > > size, then we need to call resize twice. Once before building the header to > > allocate enough space and once after to set the actual size. > > > > Your first proposal which does not use resize at all, implicitly requires > > storing the actual packet lengths in a second vector. I think this solution is > a > > bit odd since we would have a bunch of byte vectors each with it's own size() > > function and then we would have a second vector for storing the actual size of > > each packet. > > My random thoughts: > > 1. Consider using rtc::Buffer instead of std::vector. > 2. You are caught between new and old paradigms. No solution is perfect. > 3. If you are going for the raw pointers solution, use scoped_ptr to avoid > leaks. > > > Now, a concrete proposal: > > Rewrite GeneateRtpPacket to > > size_t GenerateRtpPacket(rtc::Buffer* packet) { > // Desired packet length is determined by packet->size(); > ... > const size_t header_size = rtp_sender.BuildRTPheader(packet->data(), > > ...); > ASSERT_LE(header_size, packet->size()); > // Randomize the payload as before. > return header_size; > } > > Use: > std::vector<rtc::Buffer> rtp_packets; > std::vector<size_t> rtp_header_sizes; > > But skip: > std::vector<size_t> rtp_packet_sizes; > > Use rtc::Buffer in the validation method(s) as well. +1
I have not done the rtc::Buffer change yet. I'll look into it. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:548: LogSessionAndReadBack(5, 2, 0, 0, 0, 321); On 2015/08/17 13:47:07, hlundin-webrtc wrote: > On 2015/08/14 17:48:26, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > Use the bitmask constants here too. > > > > This is the one place where named bitmasks might be more convenient. Advice? > > I suggest you introduce an array of bitmasks, the same size as the other const > arrays you already added at the top of the file. Then you can index that with > the same index as the other arrays, and use it both when constructing and > parsing the combined bit-patterns. That would not simplify the code here. The issue is that I want to select two specific extensions, for which I know the names but not the indices. Even if I added an array of bitmasks, I would still have to iterate over the array to find the correct mask. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:552: for (unsigned extensions = 0; extensions < 32; extensions++) { On 2015/08/17 13:47:07, hlundin-webrtc wrote: > On 2015/08/14 17:48:26, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > size_t, since extensions is used as input argument to > > > LogSessionAndReadBack(size_t ...) > > > > Sorry, I don't understand. Can you elaborate? > > I was nagging about you using unsigned as the type. Then you are passing it as > argument to LogSessionAndReadBack(..., uint32_t extensions_bitvector, ...). > Better to decide on using one type. Oh, so you meant unsigned vs uint32_t. I got confused by the comment about size_t, which isn't used at either place. Changed to uint32_t. https://codereview.webrtc.org/1257163003/diff/20001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:553: for (unsigned csrcs_count = 0; csrcs_count < 3; csrcs_count++) { On 2015/08/17 13:47:07, hlundin-webrtc wrote: > On 2015/08/14 17:48:27, terelius wrote: > > On 2015/08/13 14:09:52, hlundin-webrtc wrote: > > > size_t > > > > I don't understand. Can you elaborate? > > Same as above. Same as above. https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:39: RTPExtensionType extension_types[] = { On 2015/08/17 13:47:07, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:39: RTPExtensionType extension_types[] = { On 2015/08/17 13:47:07, hlundin-webrtc wrote: > kExtensionTypes Done. https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:45: const char* extension_names[] = {RtpExtension::kTOffset, On 2015/08/17 13:47:07, hlundin-webrtc wrote: > kExtensionNames Done. https://codereview.webrtc.org/1257163003/diff/60001/webrtc/video/rtc_event_lo... webrtc/video/rtc_event_log_unittest.cc:50: unsigned n_extensions = 5; On 2015/08/17 13:47:07, hlundin-webrtc wrote: > const size_t kNumExtensions Done.
Looks good. Eagerly awaiting the rtc::Buffer change. :)
Changed the representation to rtc::Buffer, but I did not change the verification functions. The reason is that once https://codereview.webrtc.org/1295753003/ lands, we will also parse the events and compare the results to what we originally write. Since the parse functions have to use pointer+length representation, it'll be more symmetric if we use the same format for the verification functions. Does this sound reasonable?
LGTM
LGTM with two nits. https://codereview.webrtc.org/1257163003/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:293: // CHECK(packet->size() == packet_size); Don't keep code that is commented away. https://codereview.webrtc.org/1257163003/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:294: CHECK(packet_size >= 16 + 4 * csrcs_count + 4 * kNumExtensions); CHECK_GE; gives better printouts on failure.
Stefan, could you prioritize this CL? https://codereview.webrtc.org/1257163003/diff/100001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:293: // CHECK(packet->size() == packet_size); On 2015/08/26 09:42:22, hlundin-webrtc wrote: > Don't keep code that is commented away. Done. https://codereview.webrtc.org/1257163003/diff/100001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:294: CHECK(packet_size >= 16 + 4 * csrcs_count + 4 * kNumExtensions); On 2015/08/26 09:42:22, hlundin-webrtc wrote: > CHECK_GE; gives better printouts on failure. Done.
lgtm https://codereview.webrtc.org/1257163003/diff/140001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:324: bool marker_bit = rand() & 0x01; I think you may have to cast this to bool to avoid build errors on some bots. For instance by doing == 1. Same below. Feel free to ignore if you think this is incorrect. :)
https://codereview.webrtc.org/1257163003/diff/140001/webrtc/video/rtc_event_l... File webrtc/video/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1257163003/diff/140001/webrtc/video/rtc_event_l... webrtc/video/rtc_event_log_unittest.cc:324: bool marker_bit = rand() & 0x01; On 2015/09/03 09:38:34, stefan-webrtc (holmer) wrote: > I think you may have to cast this to bool to avoid build errors on some bots. > For instance by doing == 1. Same below. > > Feel free to ignore if you think this is incorrect. :) The C++ standard and all our trybots allow implicit type conversions from int to bool. Reading up on this, it seems like MVS can give a warning (possibly depending on version or settings), but they also give the same warning about static_cast<bool>(some_int). Is it a project goal to support compilers behaving in this way? I'll change to (some_int == 1) here and in a few other places to avoid this issue. I'll also change bitwise-and to a reduction modulo 2, as this is more consistent with other uses in the file. The compiler will do the optimization (in release builds) anyway.
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, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1257163003/#ps160001 (title: "Avoid implicit conversions int -> bool")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1257163003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1257163003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2f9fd5ddb9097054b71fce20ba24bc34b2f084c0 Cr-Commit-Position: refs/heads/master@{#9856} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
