Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2378403004:
Resurrected test_api_audio.cc (Closed)
DescriptionResurrected test_api_audio.cc
I'll be doing some changes to code it tests (rtp_receiver_audio,
specifically) and want to make sure there are tests in place before I
touch anything.
Fixed test_api_audio not properly checking payload data. Required a
fix to LoopBackTransport in test_api to as to act like the regular
audio and video parts of WebRTC and separate payload from header data.
Also added a test for CNG and cleaned up constants.
BUG=webrtc:5805
Committed: https://crrev.com/b2d1e0d1daf41a158cde8a4519eb81d44603d78c
Cr-Commit-Position: refs/heads/master@{#14529}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed comments. #
Total comments: 19
Patch Set 3 : More test improvements #
Total comments: 14
Patch Set 4 : Reinstated RTPCallback use. Fixed spelling of payloadType. Updated comment. Made default-initialize… #Patch Set 5 : Value initialization of CodecInsts. #Patch Set 6 : Reorganized RtpRtcpAudioTest members. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 28 (10 generated)
ossu@webrtc.org changed reviewers: + danilchap@webrtc.org, henrik.lundin@webrtc.org
PTAL. This is the first of two CLs to change rtp_receiver_audio. I've put you both as reviewers since they deal with the RtpRtcp module and how it interacts with the ACM. Please delegate if I've picked wrong. :)
https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... File webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc (right): https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:56: static_cast<const uint8_t*>(data) + header.headerLength; static_cast is not needed here (it casts to same type data already is). https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:57: assert(len >= header.headerLength); RTC_DCHECK_GE(len, header.headerLength) https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:58: const size_t payload_length = len - header.headerLength; may be just "size_t payload_length": function tail is short, payload_length used just once there, probably no point to stress it is not changed next line. https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:37: case 96: may be define these constants too. https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:76: may be fix tests style by terminating unnamed namespace here (fixture and tests themselves should be in webrtc namespace) https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:278: TEST_F(RtpRtcpAudioTest, CNG) { Can you move all unimportant setup into helper method and use several smaller tests instead of loops? btw, what CNG is? what this test actually tests? https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:351: EXPECT_EQ(true, module1->SendOutgoingData( EXPECT_TRUE(...) instead of EXPECT_EQ(true,...) looks cleaner.
https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... File webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc (right): https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:56: static_cast<const uint8_t*>(data) + header.headerLength; On 2016/10/03 13:51:19, danilchap wrote: > static_cast is not needed here (it casts to same type data already is). Acknowledged. https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:57: assert(len >= header.headerLength); On 2016/10/03 13:51:19, danilchap wrote: > RTC_DCHECK_GE(len, header.headerLength) You are right on both counts! I just borrowed this code and didn't clean it up. I'll get on it! https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:58: const size_t payload_length = len - header.headerLength; On 2016/10/03 13:51:19, danilchap wrote: > may be just "size_t payload_length": > function tail is short, payload_length used just once there, probably no point > to stress it is not changed next line. Granted, it's not strictly necessary. However, I'm trying to put const wherever a variable doesn't change, for the few benefits that can be gotten in the general case (mostly getting slapped if accidentally changing the value). https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:37: case 96: On 2016/10/03 13:51:19, danilchap wrote: > may be define these constants too. Hmm... perhaps I could extract the payload types and their proper parameters rather than folding it all in here... https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:76: On 2016/10/03 13:51:19, danilchap wrote: > may be fix tests style by terminating unnamed namespace here (fixture and tests > themselves should be in webrtc namespace) Acknowledged. https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:278: TEST_F(RtpRtcpAudioTest, CNG) { On 2016/10/03 13:51:19, danilchap wrote: > Can you move all unimportant setup into helper method and use several smaller > tests instead of loops? > btw, what CNG is? what this test actually tests? CNG is short for ComfortNoiseGenerator. In this case I want to register several comfort noise payload types at the same time and make sure they are resolved correctly, so that one comfort noise payload type doesn't get mapped to the wrong parameters. I should probably make sure the frequency is correct as well. Will look into expanding the test a bit. https://codereview.webrtc.org/2378403004/diff/1/webrtc/modules/rtp_rtcp/test/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:351: EXPECT_EQ(true, module1->SendOutgoingData( On 2016/10/03 13:51:19, danilchap wrote: > EXPECT_TRUE(...) instead of EXPECT_EQ(true,...) looks cleaner. Acknowledged.
I've addressed your comments. PTAL.
https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:16: add #include "webrtc/base/checks.h" https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:37: CngCodecSpec kCngCodecs[] = {{13, 8000}, add const (or constexpr), to be safe. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:48: const uint8_t payloadType = rtpHeader->header.payloadType; payload_type = ... https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:57: auto is_cng_codec = [] (uint8_t payloadType) { may be put this helper function near kCngCodecs defined instead of creating lambda: bool IsCngCodec(int payload_type) { ... }
https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:56: RTC_DCHECK_GE(len, header.headerLength); This is test code, so you might just as well RTC_CHECK_GE. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:37: CngCodecSpec kCngCodecs[] = {{13, 8000}, On 2016/10/04 13:00:32, danilchap wrote: > add const (or constexpr), to be safe. +1 https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:53: const size_t min_size = std::min(sizeof(kTestPayload), payloadSize); Shouldn't it be an error if payloadSize != sizeof(kTestPayload)? If so, I suggest you replace this line with ASSERT_EQ(sizeof(kTestPayload), payloadSize); and do the memcmp over sizeof(kTestPayload) unconditionally. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:311: (voice_codec.rate < 0) ? 0 : voice_codec.rate)); voice_codec.rate should be deterministically known at this point, right? So why the conditional? https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:319: (voice_codec.rate < 0) ? 0 : voice_codec.rate)); Same here.
https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:16: On 2016/10/04 13:00:32, danilchap wrote: > add #include "webrtc/base/checks.h" Done. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc:56: RTC_DCHECK_GE(len, header.headerLength); On 2016/10/04 13:45:02, hlundin-webrtc wrote: > This is test code, so you might just as well RTC_CHECK_GE. Alright. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:37: CngCodecSpec kCngCodecs[] = {{13, 8000}, On 2016/10/04 13:45:03, hlundin-webrtc wrote: > On 2016/10/04 13:00:32, danilchap wrote: > > add const (or constexpr), to be safe. > > +1 Sure thing! https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:48: const uint8_t payloadType = rtpHeader->header.payloadType; On 2016/10/04 13:00:32, danilchap wrote: > payload_type = ... Done. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:53: const size_t min_size = std::min(sizeof(kTestPayload), payloadSize); On 2016/10/04 13:45:03, hlundin-webrtc wrote: > Shouldn't it be an error if payloadSize != sizeof(kTestPayload)? If so, I > suggest you replace this line with > ASSERT_EQ(sizeof(kTestPayload), payloadSize); > and do the memcmp over sizeof(kTestPayload) unconditionally. Unfortunately, I can't use ASSERT because the function doesn't return void. The previous test code exploded here when memcpy:ing unconditionally (due to payloadSize including the size of the header as well). However, that never happened because the payload types were wrong so the test never ran. :) I could skip the rest if the payload size is off, but I believe it can be useful for debugging to also compare the data. Maybe the size is off but the data is correct. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:57: auto is_cng_codec = [] (uint8_t payloadType) { On 2016/10/04 13:00:32, danilchap wrote: > may be put this helper function near kCngCodecs defined instead of creating > lambda: > bool IsCngCodec(int payload_type) { > ... > } Done. https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:311: (voice_codec.rate < 0) ? 0 : voice_codec.rate)); On 2016/10/04 13:45:02, hlundin-webrtc wrote: > voice_codec.rate should be deterministically known at this point, right? So why > the conditional? I don't know. It was like this when I got here. :) https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:319: (voice_codec.rate < 0) ? 0 : voice_codec.rate)); On 2016/10/04 13:45:02, hlundin-webrtc wrote: > Same here. Well, alright, it was like this in the test I stole it from. I don't think rate will ever be < 0 in any of the tests. I'll clean it up.
https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:53: const size_t min_size = std::min(sizeof(kTestPayload), payloadSize); On 2016/10/04 14:55:30, ossu wrote: > On 2016/10/04 13:45:03, hlundin-webrtc wrote: > > Shouldn't it be an error if payloadSize != sizeof(kTestPayload)? If so, I > > suggest you replace this line with > > ASSERT_EQ(sizeof(kTestPayload), payloadSize); > > and do the memcmp over sizeof(kTestPayload) unconditionally. > > Unfortunately, I can't use ASSERT because the function doesn't return void. The > previous test code exploded here when memcpy:ing unconditionally (due to > payloadSize including the size of the header as well). However, that never > happened because the payload types were wrong so the test never ran. :) > > I could skip the rest if the payload size is off, but I believe it can be useful > for debugging to also compare the data. Maybe the size is off but the data is > correct. Another solution is include gmock and use matchers from it: EXPECT_THAT(::testing::make_tuple(payloadData, payloadSize), ::testing::ElementsAreArray(kTestPayload)); that test size and data together. (just a suggestion. personally I'm fine both with current way and with removing min_size and comparing with sizeof(kTestPayload) unconditionally. Worst case failing test will crash, but still will catch a problem). https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:42: bool IsComfortNoisePayload(uint8_t payloadType) { s/payloadType/payload_type/ https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:60: // All our test vectors for payload type 96 and 97 even the stereo is on may be update the comment with improvements to code: All our test vectors for Pcmu and Dtmf payloads equal to |kTestPayload| https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:141: void RegisterPayload(const CodecInst& codec) { nice helper. makes test below obviously clearer. https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:158: std::unique_ptr<RtpRtcp> module1; likely be better to put modules last so that they will be destroyed first. Otherwise for a short while they will hold invalid pointers that might make some future tests flaky. https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:166: std::unique_ptr<VerifyingAudioReceiver> data_receiver1; might also turn this members from unique pointers to just members: VerifyingAudioReceiver data_receiver1_; (since they are not created by a factory and do not need to live longer than the fixture. If you prefer pointers for consistency with other members, live them as pointers. https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:275: CodecInst voice_codec = {0}; Probably better to use older, primitive memset way of reseting the structure, because it is clearer. Or write factory helper that return a CodecInst with all members reset. It seems this line would initialize 1st member (pltype) of CodecInst struct to zero and use default values (i.e. initialized to indeterminate values) for other struct members. (http://en.cppreference.com/w/cpp/language/aggregate_initialization)
https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:141: void RegisterPayload(const CodecInst& codec) { On 2016/10/05 08:12:57, danilchap wrote: > nice helper. makes test below obviously clearer. \o/ https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:158: std::unique_ptr<RtpRtcp> module1; On 2016/10/05 08:12:57, danilchap wrote: > likely be better to put modules last so that they will be destroyed first. > Otherwise for a short while they will hold invalid pointers that might make some > future tests flaky. Done. https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:166: std::unique_ptr<VerifyingAudioReceiver> data_receiver1; On 2016/10/05 08:12:57, danilchap wrote: > might also turn this members from unique pointers to just members: > VerifyingAudioReceiver data_receiver1_; (since they are not created by a factory > and do not need to live longer than the fixture. > > If you prefer pointers for consistency with other members, live them as > pointers. Done. Turned out rtp_callback was never used, so I re-instated it. I expect there was some point to creating it in the first place, though this whole tests seems like it was only half finished way back when. https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:275: CodecInst voice_codec = {0}; On 2016/10/05 08:12:57, danilchap wrote: > Probably better to use older, primitive memset way of reseting the structure, > because it is clearer. > Or write factory helper that return a CodecInst with all members reset. > > It seems this line would initialize 1st member (pltype) of CodecInst struct to > zero and use default values (i.e. initialized to indeterminate values) for > other struct members. > (http://en.cppreference.com/w/cpp/language/aggregate_initialization) > I believe it will value initialize everything, just like if static, i.e. primitives to zero. {0} is in fact the same as just writing {} but perhaps a bit clearer (though actually slightly misleading since the 0 only initializes the first value - the rest only happen to get initialized to zero as well, i.e. putting a 1 there won't get everything initialized to ones). See: https://akrzemi1.wordpress.com/2013/09/10/value-initialization-with-c/ I find memset on structs/classes an abomination in C++. :/
https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:275: CodecInst voice_codec = {0}; On 2016/10/05 09:42:29, ossu wrote: > On 2016/10/05 08:12:57, danilchap wrote: > > Probably better to use older, primitive memset way of reseting the structure, > > because it is clearer. > > Or write factory helper that return a CodecInst with all members reset. > > > > It seems this line would initialize 1st member (pltype) of CodecInst struct to > > zero and use default values (i.e. initialized to indeterminate values) for > > other struct members. > > (http://en.cppreference.com/w/cpp/language/aggregate_initialization) > > > > I believe it will value initialize everything, just like if static, i.e. > primitives to zero. {0} is in fact the same as just writing {} but perhaps a bit > clearer (though actually slightly misleading since the 0 only initializes the > first value - the rest only happen to get initialized to zero as well, i.e. > putting a 1 there won't get everything initialized to ones). > > See: https://akrzemi1.wordpress.com/2013/09/10/value-initialization-with-c/ > > I find memset on structs/classes an abomination in C++. :/ According to the link I posted since C++11 the remaining fields use default initialization, not value initialization. If you do not like memset (agree it looks odd in c++ code) may be has default values in CodecInst directly or, if you prefer this CL to be local, add a factory above: CodecInst CreateCodec() { CodecInst result; result.pltype = 0; result.plname[0] = 0; result.pltype = 0; ... return result; } (this might not be efficient in production code, but should be fine for test)
https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:275: CodecInst voice_codec = {0}; On 2016/10/05 09:54:22, danilchap wrote: > On 2016/10/05 09:42:29, ossu wrote: > > On 2016/10/05 08:12:57, danilchap wrote: > > > Probably better to use older, primitive memset way of reseting the > structure, > > > because it is clearer. > > > Or write factory helper that return a CodecInst with all members reset. > > > > > > It seems this line would initialize 1st member (pltype) of CodecInst struct > to > > > zero and use default values (i.e. initialized to indeterminate values) for > > > other struct members. > > > (http://en.cppreference.com/w/cpp/language/aggregate_initialization) > > > > > > > I believe it will value initialize everything, just like if static, i.e. > > primitives to zero. {0} is in fact the same as just writing {} but perhaps a > bit > > clearer (though actually slightly misleading since the 0 only initializes the > > first value - the rest only happen to get initialized to zero as well, i.e. > > putting a 1 there won't get everything initialized to ones). > > > > See: https://akrzemi1.wordpress.com/2013/09/10/value-initialization-with-c/ > > > > I find memset on structs/classes an abomination in C++. :/ > > According to the link I posted since C++11 the remaining fields use default > initialization, not value initialization. > > If you do not like memset (agree it looks odd in c++ code) may be has default > values in CodecInst directly or, if you prefer this CL to be local, add a > factory above: > CodecInst CreateCodec() { > CodecInst result; > result.pltype = 0; > result.plname[0] = 0; > result.pltype = 0; > ... > return result; > } > (this might not be efficient in production code, but should be fine for test) No, it doesn't. I quote (removing the C++17 specific blocks that don't apply to us, for clarity): "If the number of initializer clauses is less than the number of members or initializer list is completely empty, the remaining members are initialized by empty lists, in accordance with the usual list-initialization rules (which performs value-initialization for non-class types and non-aggregate classes with default constructors, and aggregate initialization for aggregates)." So each of the members are either value-initialized or aggregate-initialized themselves.
lgtm https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:275: CodecInst voice_codec = {0}; On 2016/10/05 10:00:57, ossu wrote: > On 2016/10/05 09:54:22, danilchap wrote: > > On 2016/10/05 09:42:29, ossu wrote: > > > On 2016/10/05 08:12:57, danilchap wrote: > > > > Probably better to use older, primitive memset way of reseting the > > structure, > > > > because it is clearer. > > > > Or write factory helper that return a CodecInst with all members reset. > > > > > > > > It seems this line would initialize 1st member (pltype) of CodecInst > struct > > to > > > > zero and use default values (i.e. initialized to indeterminate values) > for > > > > other struct members. > > > > (http://en.cppreference.com/w/cpp/language/aggregate_initialization) > > > > > > > > > > I believe it will value initialize everything, just like if static, i.e. > > > primitives to zero. {0} is in fact the same as just writing {} but perhaps a > > bit > > > clearer (though actually slightly misleading since the 0 only initializes > the > > > first value - the rest only happen to get initialized to zero as well, i.e. > > > putting a 1 there won't get everything initialized to ones). > > > > > > See: https://akrzemi1.wordpress.com/2013/09/10/value-initialization-with-c/ > > > > > > I find memset on structs/classes an abomination in C++. :/ > > > > According to the link I posted since C++11 the remaining fields use default > > initialization, not value initialization. > > > > If you do not like memset (agree it looks odd in c++ code) may be has default > > values in CodecInst directly or, if you prefer this CL to be local, add a > > factory above: > > CodecInst CreateCodec() { > > CodecInst result; > > result.pltype = 0; > > result.plname[0] = 0; > > result.pltype = 0; > > ... > > return result; > > } > > (this might not be efficient in production code, but should be fine for test) > > No, it doesn't. I quote (removing the C++17 specific blocks that don't apply to > us, for clarity): > "If the number of initializer clauses is less than the number of members or > initializer list is completely empty, the remaining members are initialized by > empty lists, in accordance with the usual list-initialization rules (which > performs value-initialization for non-class types and non-aggregate classes with > default constructors, and aggregate initialization for aggregates)." > > So each of the members are either value-initialized or aggregate-initialized > themselves. Yes, you right, I didn't read that part careful enough and missed conditions 'since c++14' and 'for class types' May be at least remove 0 and initialize with empty {} since I doubt misleading initialization can be clearer.
https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:275: CodecInst voice_codec = {0}; On 2016/10/05 10:54:09, danilchap wrote: > On 2016/10/05 10:00:57, ossu wrote: > > On 2016/10/05 09:54:22, danilchap wrote: > > > On 2016/10/05 09:42:29, ossu wrote: > > > > On 2016/10/05 08:12:57, danilchap wrote: > > > > > Probably better to use older, primitive memset way of reseting the > > > structure, > > > > > because it is clearer. > > > > > Or write factory helper that return a CodecInst with all members reset. > > > > > > > > > > It seems this line would initialize 1st member (pltype) of CodecInst > > struct > > > to > > > > > zero and use default values (i.e. initialized to indeterminate values) > > for > > > > > other struct members. > > > > > (http://en.cppreference.com/w/cpp/language/aggregate_initialization) > > > > > > > > > > > > > I believe it will value initialize everything, just like if static, i.e. > > > > primitives to zero. {0} is in fact the same as just writing {} but perhaps > a > > > bit > > > > clearer (though actually slightly misleading since the 0 only initializes > > the > > > > first value - the rest only happen to get initialized to zero as well, > i.e. > > > > putting a 1 there won't get everything initialized to ones). > > > > > > > > See: > https://akrzemi1.wordpress.com/2013/09/10/value-initialization-with-c/ > > > > > > > > I find memset on structs/classes an abomination in C++. :/ > > > > > > According to the link I posted since C++11 the remaining fields use default > > > initialization, not value initialization. > > > > > > If you do not like memset (agree it looks odd in c++ code) may be has > default > > > values in CodecInst directly or, if you prefer this CL to be local, add a > > > factory above: > > > CodecInst CreateCodec() { > > > CodecInst result; > > > result.pltype = 0; > > > result.plname[0] = 0; > > > result.pltype = 0; > > > ... > > > return result; > > > } > > > (this might not be efficient in production code, but should be fine for > test) > > > > No, it doesn't. I quote (removing the C++17 specific blocks that don't apply > to > > us, for clarity): > > "If the number of initializer clauses is less than the number of members or > > initializer list is completely empty, the remaining members are initialized by > > empty lists, in accordance with the usual list-initialization rules (which > > performs value-initialization for non-class types and non-aggregate classes > with > > default constructors, and aggregate initialization for aggregates)." > > > > So each of the members are either value-initialized or aggregate-initialized > > themselves. > Yes, you right, I didn't read that part careful enough and missed conditions > 'since c++14' and 'for class types' > > May be at least remove 0 and initialize with empty {} since I doubt misleading > initialization can be clearer. Agreed!
https://codereview.webrtc.org/2378403004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:164: LoopBackTransport transport1; Tried to put these in order of initialization but moduleX and transportX depend on each other. Regardless, it seems to work.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8979)
lgtm https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc (right): https://codereview.webrtc.org/2378403004/diff/20001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc:53: const size_t min_size = std::min(sizeof(kTestPayload), payloadSize); On 2016/10/05 08:12:57, danilchap wrote: > On 2016/10/04 14:55:30, ossu wrote: > > On 2016/10/04 13:45:03, hlundin-webrtc wrote: > > > Shouldn't it be an error if payloadSize != sizeof(kTestPayload)? If so, I > > > suggest you replace this line with > > > ASSERT_EQ(sizeof(kTestPayload), payloadSize); > > > and do the memcmp over sizeof(kTestPayload) unconditionally. > > > > Unfortunately, I can't use ASSERT because the function doesn't return void. > The > > previous test code exploded here when memcpy:ing unconditionally (due to > > payloadSize including the size of the header as well). However, that never > > happened because the payload types were wrong so the test never ran. :) > > > > I could skip the rest if the payload size is off, but I believe it can be > useful > > for debugging to also compare the data. Maybe the size is off but the data is > > correct. > > Another solution is include gmock and use matchers from it: > EXPECT_THAT(::testing::make_tuple(payloadData, payloadSize), > ::testing::ElementsAreArray(kTestPayload)); > that test size and data together. > > (just a suggestion. personally I'm fine both with current way and with removing > min_size and comparing with sizeof(kTestPayload) unconditionally. Worst case > failing test will crash, but still will catch a problem). Right, I didn't think of the non-void return type. Keep it as it is.
Description was changed from ========== Resurrected test_api_audio.cc I'll be doing some changes to code it tests (rtp_receiver_audio, specifically) and want to make sure there are tests in place before I touch anything. Fixed test_api_audio not properly checking payload data. Required a fix to LoopBackTransport in test_api to as to act like the regular audio and video parts of WebRTC and separate payload from header data. Also added a test for CNG and cleaned up constants. BUG= ========== to ========== Resurrected test_api_audio.cc I'll be doing some changes to code it tests (rtp_receiver_audio, specifically) and want to make sure there are tests in place before I touch anything. Fixed test_api_audio not properly checking payload data. Required a fix to LoopBackTransport in test_api to as to act like the regular audio and video parts of WebRTC and separate payload from header data. Also added a test for CNG and cleaned up constants. BUG=webrtc:5805 ==========
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2378403004/#ps100001 (title: "Reorganized RtpRtcpAudioTest members.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Resurrected test_api_audio.cc I'll be doing some changes to code it tests (rtp_receiver_audio, specifically) and want to make sure there are tests in place before I touch anything. Fixed test_api_audio not properly checking payload data. Required a fix to LoopBackTransport in test_api to as to act like the regular audio and video parts of WebRTC and separate payload from header data. Also added a test for CNG and cleaned up constants. BUG=webrtc:5805 ========== to ========== Resurrected test_api_audio.cc I'll be doing some changes to code it tests (rtp_receiver_audio, specifically) and want to make sure there are tests in place before I touch anything. Fixed test_api_audio not properly checking payload data. Required a fix to LoopBackTransport in test_api to as to act like the regular audio and video parts of WebRTC and separate payload from header data. Also added a test for CNG and cleaned up constants. BUG=webrtc:5805 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Resurrected test_api_audio.cc I'll be doing some changes to code it tests (rtp_receiver_audio, specifically) and want to make sure there are tests in place before I touch anything. Fixed test_api_audio not properly checking payload data. Required a fix to LoopBackTransport in test_api to as to act like the regular audio and video parts of WebRTC and separate payload from header data. Also added a test for CNG and cleaned up constants. BUG=webrtc:5805 ========== to ========== Resurrected test_api_audio.cc I'll be doing some changes to code it tests (rtp_receiver_audio, specifically) and want to make sure there are tests in place before I touch anything. Fixed test_api_audio not properly checking payload data. Required a fix to LoopBackTransport in test_api to as to act like the regular audio and video parts of WebRTC and separate payload from header data. Also added a test for CNG and cleaned up constants. BUG=webrtc:5805 Committed: https://crrev.com/b2d1e0d1daf41a158cde8a4519eb81d44603d78c Cr-Commit-Position: refs/heads/master@{#14529} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b2d1e0d1daf41a158cde8a4519eb81d44603d78c Cr-Commit-Position: refs/heads/master@{#14529} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
