Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1316903002:
Update to the neteq_rtpplay utility to support RtcEventLog input files. (Closed)
|
Created:
5 years, 3 months ago by ivoc Modified:
5 years, 3 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. |
DescriptionUpdate to the neteq_rtpplay utility to support RtcEventLog input files.
This CL adds support for simulating neteq using stored RTP packets as well as calls to GetAudio from an RtcEventLog, using the stored timestamps.
The type of the input file is detected automatically.
BUG=webrtc:4741
Committed: https://crrev.com/caa5f4b3d2694c97f010f9a1602d4f15b89ca7c7
Cr-Commit-Position: refs/heads/master@{#9886}
Patch Set 1 : Initial version #
Total comments: 45
Patch Set 2 : Added automatic filetype detection, lots of refactoring. #
Total comments: 12
Patch Set 3 : Replaced filetype detection code, and various small improvements. #
Total comments: 1
Patch Set 4 : Formatting. #Patch Set 5 : Addressed Henrik's comment and improved the formatting. #Patch Set 6 : Rebased. #
Messages
Total messages: 22 (6 generated)
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, minyue@webrtc.org
ivoc@webrtc.org changed required reviewers: + henrik.lundin@webrtc.org, minyue@webrtc.org
Please have a look at this CL to enable neteq_rtpplay to read from RtcEventLog files.
Nice work. See comments. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_tests.gypi (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_tests.gypi:11: ['enable_protobuf==1', { Hmm, does this mean that neteq_rtpplay is only built under this condition? https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:143: "Enables using a stored RtcEventLog as input " No need to break this line. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:564: break; What are you breaking out of, and why do you have to? https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:566: if (replace_payload) { Did you try to run with a replacement audio file? I have a feeling you may have to move next_input_time_ms = INT_MAX; to after this if statement. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:612: break; Same as the break above. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:42: assert(parser_.get()); CHECK instead. This is test/tools code; better to be aggressive about errors. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:49: if (event.has_type() && event.type() == rtclog::Event::RTP_EVENT) { This is a very long harangue of nested if statements. I'm thinking that it may be better to do a helper function like this: const rtclog::RtpPacket* GetRtpPacket(const rtclog::Event& event) { // All the ifs needed to return either a pointer to a valid rtclog::RtpPacket, or nullptr. // Also, this can be implemented in a "break early on failure" manner. if (!event.has_type()) return nullptr; if (event.type() != rtclog::Event::RTP_EVENT) return nullptr; ... } Then you could just do const rtclog::RtpPacket* rtp_packet = GetRtpPacket(event_log_->stream(rtp_packet_index_)); if (rtp_packet) { // Do the stuff... } WDYT? https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:53: rtp_packet.has_incoming() && rtp_packet.incoming() == true && You can omit "== true". https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:59: rtp_packet_index_++; The double increment (in the for statement and here) is awkward. Can you get around it by using a while loop instead? https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:67: if (!packet->valid_header()) { Are we expecting the logger to write invalid headers? Does it verify them before writing, or is it just dumping them verbatim? If we are not expecting invalid headers, simply CHECK(packet->valid_header()). Otherwise, remove the assert, and print a warning. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:73: // This payload type should be filtered out. Continue to the next s/payload type/packet/ https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:89: if (event.has_type() && event.type() == rtclog::Event::DEBUG_EVENT) { Same comment as above about a helper function. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:96: audio_output_index_++; Same awkwardness here. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:97: return event.timestamp_us() / 1000; What is the type of timestamp_us? If it is not int, it would be better to return the original type to avoid implicit casts. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h:48: // value of -1 is returned if the end of the file is reached. Is it end of file that is reached, or are there simply no more audio output events?
I seem to have forgotten to review this, sorry for the delay. Some comments now. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:395: event_log_source = webrtc::test::RtcEventLogSource::Create(argv[1]); Is there a way to detect the format automatically? https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:63: rtc::scoped_ptr<Packet> packet( what is the life time of packet? I'd like the method be void RtcEventLogSource::NextPacket(Packet* packet) and let the caller hold the |packet|, since it is not obvious that the caller has to release the memory. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h:10: #ifndef WEBRTC_MODULES_AUDIO_CODING_NETEQ_TOOLS_RTC_EVENT_LOG_SOURCE_H_ a line break before
a couple of more https://codereview.chromium.org/1316903002/diff/1/webrtc/modules/audio_coding... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.chromium.org/1316903002/diff/1/webrtc/modules/audio_coding... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:429: bool output_event_available = true; I think 428 and 429 are too far, move to 500 if possile https://codereview.chromium.org/1316903002/diff/1/webrtc/modules/audio_coding... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:502: next_output_time_ms = event_log_source->NextAudioOutputEventMs(); why not keeping -1 or let NextAudioOutputEventMs give INT_MAX https://codereview.chromium.org/1316903002/diff/1/webrtc/modules/audio_coding... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.chromium.org/1316903002/diff/1/webrtc/modules/audio_coding... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:61: memcpy(packet_data, rtp_packet.header().data(), Do you need payload data for packet_data, and is rtp_packet.header().data() only a header?
Thanks for the feedback, very useful. I added automatic filetype detection like Minyue suggested, and did a lot of refactoring. Please have another look! https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_tests.gypi (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_tests.gypi:11: ['enable_protobuf==1', { On 2015/08/28 12:06:24, hlundin-webrtc wrote: > Hmm, does this mean that neteq_rtpplay is only built under this condition? Yes, it's either this or sprinkle a bunch of #ifdef/#endifs around the code. I thought that since this is a debug/testing tool it would be okay to only build it when protobufs are enabled. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:143: "Enables using a stored RtcEventLog as input " On 2015/08/28 12:06:24, hlundin-webrtc wrote: > No need to break this line. Fixed. The string was longer before and I forgot to undo the line break :-) https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:395: event_log_source = webrtc::test::RtcEventLogSource::Create(argv[1]); On 2015/08/28 14:50:21, minyue-webrtc wrote: > Is there a way to detect the format automatically? Good idea. I added this feature by checking if the file starts with the magic string "#!rtpplay1.0", which indicates it is an RTPdump file. Otherwise, it is assumed the input file is in RtcEventLog format. I removed the command line flag. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:429: bool output_event_available = true; On 2015/08/28 16:54:17, minyue-webrtc wrote: > I think 428 and 429 are too far, move to 500 if possile Done. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:502: next_output_time_ms = event_log_source->NextAudioOutputEventMs(); On 2015/08/28 16:54:17, minyue-webrtc wrote: > why not keeping -1 or let NextAudioOutputEventMs give INT_MAX Keeping it at -1 would not work because later on the next simulation time is calculated as the minimum of next_output_time_ms and next_input_time_ms, and we don't want the time to go backwards to -1. Good idea on returning INT_MAX from NextAudioOutputEventMs, I changed it. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:564: break; On 2015/08/28 12:06:24, hlundin-webrtc wrote: > What are you breaking out of, and why do you have to? This break is for the while loop on line 512, it is needed to avoid line 576, which would update the value of next_input_time_ms to something other than INT_MAX. I restructured the code a bit to avoid the need for the break, by moving the if (replace_payload) code into the if (temp_packet) block. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:566: if (replace_payload) { On 2015/08/28 12:06:25, hlundin-webrtc wrote: > Did you try to run with a replacement audio file? I have a feeling you may have > to move > next_input_time_ms = INT_MAX; > to after this if statement. It seems like next_input_time_ms is not used at all in this if-statement, so I don't see why it would matter if I do the assignment before or afterwards? I tried using replacement audio, and it seems to work as expected. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:612: break; On 2015/08/28 12:06:24, hlundin-webrtc wrote: > Same as the break above. Here the break is for the while loop on line 580, but it's not strictly necessary here. I will remove it. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:42: assert(parser_.get()); On 2015/08/28 12:06:25, hlundin-webrtc wrote: > CHECK instead. This is test/tools code; better to be aggressive about errors. Done. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:49: if (event.has_type() && event.type() == rtclog::Event::RTP_EVENT) { On 2015/08/28 12:06:25, hlundin-webrtc wrote: > This is a very long harangue of nested if statements. I'm thinking that it may > be better to do a helper function like this: > > const rtclog::RtpPacket* GetRtpPacket(const rtclog::Event& event) { > // All the ifs needed to return either a pointer to a valid rtclog::RtpPacket, > or nullptr. > // Also, this can be implemented in a "break early on failure" manner. > if (!event.has_type()) return nullptr; > if (event.type() != rtclog::Event::RTP_EVENT) return nullptr; > ... > } > > Then you could just do > const rtclog::RtpPacket* rtp_packet = > GetRtpPacket(event_log_->stream(rtp_packet_index_)); > if (rtp_packet) { > // Do the stuff... > } > > WDYT? Great idea, I refactored the code like you suggested and it is much clearer. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:53: rtp_packet.has_incoming() && rtp_packet.incoming() == true && On 2015/08/28 12:06:25, hlundin-webrtc wrote: > You can omit "== true". Done. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:59: rtp_packet_index_++; On 2015/08/28 12:06:25, hlundin-webrtc wrote: > The double increment (in the for statement and here) is awkward. Can you get > around it by using a while loop instead? Done. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:61: memcpy(packet_data, rtp_packet.header().data(), On 2015/08/28 16:54:17, minyue-webrtc wrote: > Do you need payload data for packet_data, and is rtp_packet.header().data() only > a header? Right now the rtp packet message in the protobuf only has a field for the header, there is no place intended for payloads or full packages. So rtp_packet.header().data() should indeed only contain the header. I renamed the variable to make this more clear. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:63: rtc::scoped_ptr<Packet> packet( On 2015/08/28 14:50:21, minyue-webrtc wrote: > what is the life time of packet? > > I'd like the method be > void RtcEventLogSource::NextPacket(Packet* packet) and let the caller hold the > |packet|, since it is not obvious that the caller has to release the memory. I agree that this is not super obvious, but the reason I made it like this is because this method is on the PacketSource interface (which this class and a few others implements). If I want to change it, I should change it everywhere. Do you think I should? https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:67: if (!packet->valid_header()) { On 2015/08/28 12:06:25, hlundin-webrtc wrote: > Are we expecting the logger to write invalid headers? Does it verify them before > writing, or is it just dumping them verbatim? > > If we are not expecting invalid headers, simply CHECK(packet->valid_header()). > Otherwise, remove the assert, and print a warning. The header is currently not checked while writing, they are just dumped exactly as they come in. I made your suggested changes. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:73: // This payload type should be filtered out. Continue to the next On 2015/08/28 12:06:25, hlundin-webrtc wrote: > s/payload type/packet/ I replaced this, but I also restructured this code a bit, so I made some more changes. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:89: if (event.has_type() && event.type() == rtclog::Event::DEBUG_EVENT) { On 2015/08/28 12:06:25, hlundin-webrtc wrote: > Same comment as above about a helper function. I restructured in a similar way. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:96: audio_output_index_++; On 2015/08/28 12:06:25, hlundin-webrtc wrote: > Same awkwardness here. Same fix applied. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:97: return event.timestamp_us() / 1000; On 2015/08/28 12:06:25, hlundin-webrtc wrote: > What is the type of timestamp_us? If it is not int, it would be better to return > the original type to avoid implicit casts. The value is int64_t, so I changed the return type as well as the variables holding timestamps in neteq_rtpplay.cc. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h:10: #ifndef WEBRTC_MODULES_AUDIO_CODING_NETEQ_TOOLS_RTC_EVENT_LOG_SOURCE_H_ On 2015/08/28 14:50:21, minyue-webrtc wrote: > a line break before Done. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h:48: // value of -1 is returned if the end of the file is reached. On 2015/08/28 12:06:25, hlundin-webrtc wrote: > Is it end of file that is reached, or are there simply no more audio output > events? Updated to say that there are no more audio output events.
https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:61: memcpy(packet_data, rtp_packet.header().data(), On 2015/09/01 10:03:50, ivoc wrote: > On 2015/08/28 16:54:17, minyue-webrtc wrote: > > Do you need payload data for packet_data, and is rtp_packet.header().data() > only > > a header? > > Right now the rtp packet message in the protobuf only has a field for the > header, there is no place intended for payloads or full packages. So > rtp_packet.header().data() should indeed only contain the header. I renamed the > variable to make this more clear. Yes, that makes it clearer. Now I see why I had a doubt here. I thought that the Packet must be a full packet and payload should be faked here (maybe advisable, but no action needed in this CL). https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc:63: rtc::scoped_ptr<Packet> packet( On 2015/09/01 10:03:50, ivoc wrote: > On 2015/08/28 14:50:21, minyue-webrtc wrote: > > what is the life time of packet? > > > > I'd like the method be > > void RtcEventLogSource::NextPacket(Packet* packet) and let the caller hold the > > |packet|, since it is not obvious that the caller has to release the memory. > > I agree that this is not super obvious, but the reason I made it like this is > because this method is on the PacketSource interface (which this class and a few > others implements). If I want to change it, I should change it everywhere. Do > you think I should? No bother. maybe no need of scoped_ptr around packet https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:390: // Check if the magic string "#!rtpplay1.0" appears at the beginning of the nice! https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:536: const uint8_t* payload_ptr = packet->payload(); Not very related to this CL, just curious. If packets have no payload and user did not specify replacement_audio_file, what will happen? Is it better to force replacement_audio_file in that situation?
update the CL description also
Solid improvements! Some more comments. https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:566: if (replace_payload) { On 2015/09/01 10:03:50, ivoc wrote: > On 2015/08/28 12:06:25, hlundin-webrtc wrote: > > Did you try to run with a replacement audio file? I have a feeling you may > have > > to move > > next_input_time_ms = INT_MAX; > > to after this if statement. > > It seems like next_input_time_ms is not used at all in this if-statement, so I > don't see why it would matter if I do the assignment before or afterwards? I > tried using replacement audio, and it seems to work as expected. Acknowledged. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:390: // Check if the magic string "#!rtpplay1.0" appears at the beginning of the On 2015/09/01 11:48:35, minyue-webrtc wrote: > nice! Yes, but I suggest you also add RTPencode as a magic string. We used to use that one for the rtpdump files that we created ourselves from scratch (as opposed to converting from wireshark traces). See e.g., webrtc/test/rtp_file_reader.cc, lines 145--. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:393: bool is_rtp_dump = false; This is a lot of extra code to check what is essentially already checked inside RtpFileSource. I suggest you modify RtpFileSource::OpenFile and RtpFileSource::Create to not use FATAL and CHECK for failed files. Instead, Create should return nullptr if the format is wrong. (You may want to do this in a separate CL, and also go through all other uses of RtpFileSource::Create that today blindly assumes it always works, and perhaps CHECK that we got non-nullptr.) Then you would not need all of this. Instead, simply try to open the file with RtpFileSource::Create. If that fails, try with RtcEventLogSource::Create. If that fails too: let slip the dogs of war. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:507: int64_t start_time_ms = packet->time_ms(); packet->time_ms() actually returns a double. Suggest you use rtc::checked_cast<int64_t>(...) https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:536: const uint8_t* payload_ptr = packet->payload(); On 2015/09/01 11:48:35, minyue-webrtc wrote: > Not very related to this CL, just curious. > > If packets have no payload and user did not specify replacement_audio_file, what > will happen? > > Is it better to force replacement_audio_file in that situation? There will be some errors from NetEq, but it is probably useful to issue a warning about it here. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:588: next_input_time_ms = packet->time_ms(); checked_cast here too
Thanks again for the useful comments. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:390: // Check if the magic string "#!rtpplay1.0" appears at the beginning of the On 2015/09/02 08:48:51, hlundin-webrtc wrote: > On 2015/09/01 11:48:35, minyue-webrtc wrote: > > nice! > > Yes, but I suggest you also add RTPencode as a magic string. We used to use that > one for the rtpdump files that we created ourselves from scratch (as opposed to > converting from wireshark traces). See e.g., webrtc/test/rtp_file_reader.cc, > lines 145--. Ah, I did not know that. Thanks. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:393: bool is_rtp_dump = false; On 2015/09/02 08:48:51, hlundin-webrtc wrote: > This is a lot of extra code to check what is essentially already checked inside > RtpFileSource. I suggest you modify RtpFileSource::OpenFile and > RtpFileSource::Create to not use FATAL and CHECK for failed files. Instead, > Create should return nullptr if the format is wrong. (You may want to do this in > a separate CL, and also go through all other uses of RtpFileSource::Create that > today blindly assumes it always works, and perhaps CHECK that we got > non-nullptr.) > > Then you would not need all of this. Instead, simply try to open the file with > RtpFileSource::Create. If that fails, try with RtcEventLogSource::Create. If > that fails too: let slip the dogs of war. You're right, that does seem like a nicer approach. For now I replaced this code with a temporary solution using a RtpFileReader directly to check if the file is valid, and re-reading it if it is. I will make your suggested changes in a follow-up CL. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:507: int64_t start_time_ms = packet->time_ms(); On 2015/09/02 08:48:51, hlundin-webrtc wrote: > packet->time_ms() actually returns a double. Suggest you use > rtc::checked_cast<int64_t>(...) Done. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:536: const uint8_t* payload_ptr = packet->payload(); On 2015/09/02 08:48:51, hlundin-webrtc wrote: > On 2015/09/01 11:48:35, minyue-webrtc wrote: > > Not very related to this CL, just curious. > > > > If packets have no payload and user did not specify replacement_audio_file, > what > > will happen? > > > > Is it better to force replacement_audio_file in that situation? > > There will be some errors from NetEq, but it is probably useful to issue a > warning about it here. I added a check for this higher up in the code, on line 442. https://codereview.webrtc.org/1316903002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:588: next_input_time_ms = packet->time_ms(); On 2015/09/02 08:48:51, hlundin-webrtc wrote: > checked_cast here too Added. I also added one in the call to neteq->InsertPacket on line 551.
One nit, then LGTM. Well done! https://codereview.webrtc.org/1316903002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right): https://codereview.webrtc.org/1316903002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:554: rtc::checked_cast<uint32_t>( I don't think you want the checked_cast here, just use a simple static_cast. The reason is that the arrival timestamp is essentially an RTP timestamp (32 bits) which is expected to wrap every now and then.
lgtm
The CQ bit was checked by ivoc@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/1316903002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316903002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/4537)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1316903002/#ps100001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316903002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316903002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/caa5f4b3d2694c97f010f9a1602d4f15b89ca7c7 Cr-Commit-Position: refs/heads/master@{#9886} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
