Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522103002:
Update NetEq network statistics in neteq_unittest. (Closed)
|
Created:
5 years ago by minyue-webrtc Modified:
5 years ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate NetEq network statistics in neteq_unittest.
NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics.
New neteq4_network_stats.dat generated by running TestBitExactness with flag "genref"
BUG=
Committed: https://crrev.com/5f026d03af3a4310db85e132d68b4b770653acee
Cr-Commit-Position: refs/heads/master@{#11052}
Patch Set 1 #Patch Set 2 : replacing old file instead of adding new #Patch Set 3 : using protobuf #
Total comments: 15
Patch Set 4 : better handling of macros #
Total comments: 10
Patch Set 5 : fixing grammar #
Messages
Total messages: 29 (15 generated)
Description was changed from ========== new network file BUG= ========== to ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. BUG= ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Hi Henrik, Could you review this small change?
Description was changed from ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. BUG= ========== to ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. New neteq4_network_stats.dat generated by running TestBitExactness with flag "genref" BUG= ==========
On 2015/12/14 16:25:36, minyue-webrtc wrote: > Hi Henrik, > > Could you review this small change? LG, but I don't think you should upload a new file with a new name; simply update the old file.
Thanks! updated.
lgtm
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522103002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522103002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/11347)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Hi Henrik, The old test was not platform independent. I modified it by using protobuf. The testing structure remains untouched. Would you review another time?
Patchset #3 (id:80001) has been deleted
Very nice. Thanks a lot for cleaning up! I only have a few comments on the use of #ifdefs. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right): https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:63: static void Convert(const NetEqNetworkStatistics& stats_raw, Please, enclose these methods in an unnamed namespace. And then I don't think the static qualifier gives you any benefits. (Bonus points if you also include IsAllZero and IsAllNonZero in the unnamed namespace and drop the static on those too.) https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:64: neteq_unittest::NetEqNetworkStatistics* stats) { Should you perhaps name the protobuf representation NetworkStatistics isntead, since the proto namespace already states that it is neteq. WDYT? https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:111: Close the #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT here. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:189: void RefFiles::WriteToFile(const NetEqNetworkStatistics& stats_raw) { Provide dummy implementations of WriteToFile and ReadFromFileAndCompare when WEBRTC_NETEQ_UNITTEST_BITEXACT is not defined. void RefFiles::WriteToFile(const NetEqNetworkStatistics& stats_raw) { FAIL(); } https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:275: #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT Remove this #ifdef. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:421: #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT Remove this #ifdef. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:507: #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT The preferred way of disabling tests is to do the MAYBE_TestName thing. These tests don't follow that, and instead use the DIABLED_ON_ANDROID/IOS macros. However, those are deprecated and error prone, and should be deleted; see https://bugs.chromium.org/p/webrtc/issues/detail?id=5333. I suggest the following: #if defined(WEBRTC_NETEQ_UNITTEST_BITEXACT) && !defined(WEBRTC_ANDROID) && !defined(WEBRTC_IOS) #define MAYBE_TestBitExactness TestBitExactness #else #define MAYBE_TestBitExactness DISABLED_TestBitExactness #endif Then the test will always compile, but will not run. And, if someone runs it anyway (e.g., --gtest_also_run_disabled_tests), then the dummy implementations suggested above will FAIL().
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
thanks for the comment! updated https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right): https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:63: static void Convert(const NetEqNetworkStatistics& stats_raw, On 2015/12/16 10:04:51, hlundin-webrtc wrote: > Please, enclose these methods in an unnamed namespace. And then I don't think > the static qualifier gives you any benefits. > (Bonus points if you also include IsAllZero and IsAllNonZero in the unnamed > namespace and drop the static on those too.) Done. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:64: neteq_unittest::NetEqNetworkStatistics* stats) { On 2015/12/16 10:04:51, hlundin-webrtc wrote: > Should you perhaps name the protobuf representation NetworkStatistics isntead, > since the proto namespace already states that it is neteq. WDYT? I actually used NetworkStatistics from beginning and then renamed to NetEqNetworkStatistics. The reason for it is that the protobuf type NetEqNetworkStatistics is intended to store (cross-platform) the geniune NetEqNetworkStatistics. The same name makes that intention clearest. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:111: On 2015/12/16 10:04:51, hlundin-webrtc wrote: > Close the #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT here. Done. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:189: void RefFiles::WriteToFile(const NetEqNetworkStatistics& stats_raw) { On 2015/12/16 10:04:51, hlundin-webrtc wrote: > Provide dummy implementations of WriteToFile and ReadFromFileAndCompare when > WEBRTC_NETEQ_UNITTEST_BITEXACT is not defined. > > void RefFiles::WriteToFile(const NetEqNetworkStatistics& stats_raw) { > FAIL(); > } Done. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:275: #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT On 2015/12/16 10:04:51, hlundin-webrtc wrote: > Remove this #ifdef. Done. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:421: #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT On 2015/12/16 10:04:51, hlundin-webrtc wrote: > Remove this #ifdef. Done. https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:507: #ifdef WEBRTC_NETEQ_UNITTEST_BITEXACT On 2015/12/16 10:04:51, hlundin-webrtc wrote: > The preferred way of disabling tests is to do the MAYBE_TestName thing. These > tests don't follow that, and instead use the DIABLED_ON_ANDROID/IOS macros. > However, those are deprecated and error prone, and should be deleted; see > https://bugs.chromium.org/p/webrtc/issues/detail?id=5333. > > I suggest the following: > #if defined(WEBRTC_NETEQ_UNITTEST_BITEXACT) && !defined(WEBRTC_ANDROID) && > !defined(WEBRTC_IOS) > #define MAYBE_TestBitExactness TestBitExactness > #else > #define MAYBE_TestBitExactness DISABLED_TestBitExactness > #endif > > Then the test will always compile, but will not run. And, if someone runs it > anyway (e.g., --gtest_also_run_disabled_tests), then the dummy implementations > suggested above will FAIL(). Done.
Very nice! I only have a couple of language nits and a small request for the #ifdef. Thanks for cleaning up! LGTM https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right): https://codereview.webrtc.org/1522103002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:64: neteq_unittest::NetEqNetworkStatistics* stats) { On 2015/12/16 13:51:27, minyue-webrtc wrote: > On 2015/12/16 10:04:51, hlundin-webrtc wrote: > > Should you perhaps name the protobuf representation NetworkStatistics isntead, > > since the proto namespace already states that it is neteq. WDYT? > > I actually used NetworkStatistics from beginning and then renamed to > NetEqNetworkStatistics. The reason for it is that the protobuf type > NetEqNetworkStatistics is intended to store (cross-platform) the geniune > NetEqNetworkStatistics. The same name makes that intention clearest. Acknowledged. https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right): https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:205: FAIL() << "Write reference file needs Proto Buffer."; "Writing to reference file requires Proto Buffer." https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:235: FAIL() << "Read reference file needs Proto Buffer."; "Reading from reference file requires Proto Buffer." https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:250: FAIL() << "Write reference file needs Proto Buffer."; "Writing to reference file requires Proto Buffer." https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:270: FAIL() << "Read reference file needs Proto Buffer."; "Reading from reference file requires Proto Buffer." https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:514: WEBRTC_NETEQ_UNITTEST_BITEXACT) && \ For completeness, I think you should also write defined(WEBRTC_NETEQ_UNITTEST_BITEXACT).
Patchset #5 (id:180001) has been deleted
thanks. now fixed. https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right): https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:205: FAIL() << "Write reference file needs Proto Buffer."; On 2015/12/16 14:10:50, hlundin-webrtc wrote: > "Writing to reference file requires Proto Buffer." yes of course https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:235: FAIL() << "Read reference file needs Proto Buffer."; On 2015/12/16 14:10:50, hlundin-webrtc wrote: > "Reading from reference file requires Proto Buffer." Done. https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:250: FAIL() << "Write reference file needs Proto Buffer."; On 2015/12/16 14:10:50, hlundin-webrtc wrote: > "Writing to reference file requires Proto Buffer." Done. https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:270: FAIL() << "Read reference file needs Proto Buffer."; On 2015/12/16 14:10:50, hlundin-webrtc wrote: > "Reading from reference file requires Proto Buffer." Done. https://codereview.webrtc.org/1522103002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_unittest.cc:514: WEBRTC_NETEQ_UNITTEST_BITEXACT) && \ On 2015/12/16 14:10:50, hlundin-webrtc wrote: > For completeness, I think you should also write > defined(WEBRTC_NETEQ_UNITTEST_BITEXACT). Done.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1522103002/#ps200001 (title: "fixing grammar")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522103002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522103002/200001
Message was sent while issue was closed.
Description was changed from ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. New neteq4_network_stats.dat generated by running TestBitExactness with flag "genref" BUG= ========== to ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. New neteq4_network_stats.dat generated by running TestBitExactness with flag "genref" BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. New neteq4_network_stats.dat generated by running TestBitExactness with flag "genref" BUG= ========== to ========== Update NetEq network statistics in neteq_unittest. NetEqNetworkStatistics has been updated some time ago. A bit exactness test in neteq unittests is still using the old NetEqNetworkStatistics. New neteq4_network_stats.dat generated by running TestBitExactness with flag "genref" BUG= Committed: https://crrev.com/5f026d03af3a4310db85e132d68b4b770653acee Cr-Commit-Position: refs/heads/master@{#11052} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5f026d03af3a4310db85e132d68b4b770653acee Cr-Commit-Position: refs/heads/master@{#11052} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
