Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763273003:
Changed OLA window for neteq. Old code didnt work well with 48khz (Closed)
|
Created:
3 years, 9 months ago by squashingskak Modified:
3 years, 8 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, AleBzk, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc, yujie_mao (webrtc) Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChanged OLA window for neteq. Old code didnt work well with 48khz
fixing white spaces
updated authors file
Changed OLA window to use Q14 as Q5 dosnt work with 48khz. 1 ms @ 48 khz is > 2^5
BUG=webrtc:1361
Review-Url: https://codereview.webrtc.org/2763273003
Cr-Commit-Position: refs/heads/master@{#17611}
Committed: https://chromium.googlesource.com/external/webrtc/+/9f2c18e237593b5f8cbc7d7a3838ab6596123c53
Patch Set 1 #
Total comments: 24
Patch Set 2 : Updated based on review feeedback #
Total comments: 2
Patch Set 3 : Updated after review comments #
Total comments: 1
Patch Set 4 : Win64 compile fix #Patch Set 5 : Fixing warning #Patch Set 6 : updated hash values for bitexactness tests #Patch Set 7 : updated hash values for Android and win64 #
Messages
Total messages: 49 (21 generated)
I just added a patch. Still learning how the process works
The change breaks the bit exactness tests involving neteq. I am not sure how to handle that. Maybe add new test vectors.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Thanks for the patch! 1. Run "git cl patch" to fix style issues. 2. Tidy up the CL description. The aggregate of commit messages you get when uploading should typically be curated. 3. Add BUG=webrtc:1361, since you are in fact removing some legacy bit-exactness. 4. I can help you with the bitexactness tests once the rest of this CL looks good. See comments inline. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/normal.cc (right): https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:133: int win_length = ola_win_length_ ; int -> size_t https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:135: int16_t win_up_Q14 = win_slope_Q14; Move declaration of win_up_Q14 to after the if statement, when the initial value is known. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:136: assert(channel_ix < output->Channels()); RTC_DCHECK_LT(channel_ix, output->Channels()); https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:137: if(win_length > (int)output->Size()){ Don't cast; both sides should be size_t. Also, when casting, use static_cast instead. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:139: win_slope_Q14 = (1 << 14)/length; Add spaces on both sides of / https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:142: for (int i = 0; i < win_length; i++) { size_t i https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:147: } After this for loop, we expect the value of win_up_Q14 to be 1<<14. I'd usually recommend documenting this by adding a DCHECK. But the problem in this case is that the increment is not exact for 48000 Hz. How far off from 1<<14 will we end up in that case? https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:171: int win_length = ola_win_length_; All the above comments that apply here. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/normal.h (right): https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:43: ola_win_length_ = fs_hz / 1000; Make these two member variables const, and set them in the initializer list of the ctor instead. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:43: ola_win_length_ = fs_hz / 1000; rtc::CheckedDivExact(fs_hz_, 1000) https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:44: ola_win_slope_Q14_ = (1 << 14)/ola_win_length_; Add spaces on both sides of /. I suggest you run "git cl format" to fix all formatting nits. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:62: int fs_hz_; Please, make this const while you are at it. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:66: No blank line, please. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:67: int ola_win_length_; const size_t samples_per_ms_; https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.h:68: int16_t ola_win_slope_Q14_; const int16_t default_win_slope_Q14_;
https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/normal.cc (right): https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:133: int win_length = ola_win_length_ ; On 2017/03/23 11:41:50, hlundin-webrtc wrote: > int -> size_t Acknowledged. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:135: int16_t win_up_Q14 = win_slope_Q14; On 2017/03/23 11:41:51, hlundin-webrtc wrote: > Move declaration of win_up_Q14 to after the if statement, when the initial value > is known. Acknowledged. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:136: assert(channel_ix < output->Channels()); On 2017/03/23 11:41:50, hlundin-webrtc wrote: > RTC_DCHECK_LT(channel_ix, output->Channels()); Acknowledged. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:137: if(win_length > (int)output->Size()){ On 2017/03/23 11:41:51, hlundin-webrtc wrote: > Don't cast; both sides should be size_t. Also, when casting, use static_cast > instead. Acknowledged. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:139: win_slope_Q14 = (1 << 14)/length; On 2017/03/23 11:41:51, hlundin-webrtc wrote: > Add spaces on both sides of / Acknowledged. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:142: for (int i = 0; i < win_length; i++) { On 2017/03/23 11:41:51, hlundin-webrtc wrote: > size_t i Acknowledged. https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:147: } On 2017/03/23 11:41:51, hlundin-webrtc wrote: > After this for loop, we expect the value of win_up_Q14 to be 1<<14. I'd usually > recommend documenting this by adding a DCHECK. But the problem in this case is > that the increment is not exact for 48000 Hz. How far off from 1<<14 will we end > up in that case? 1 << 14 / 48 = 341. 341 * 48 = 16384 = 1<< 14 - 16 https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:171: int win_length = ola_win_length_; On 2017/03/23 11:41:50, hlundin-webrtc wrote: > All the above comments that apply here. Acknowledged.
https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/normal.cc (right): https://codereview.webrtc.org/2763273003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/normal.cc:147: } On 2017/03/27 10:24:59, squashingskak wrote: > On 2017/03/23 11:41:51, hlundin-webrtc wrote: > > After this for loop, we expect the value of win_up_Q14 to be 1<<14. I'd > usually > > recommend documenting this by adding a DCHECK. But the problem in this case is > > that the increment is not exact for 48000 Hz. How far off from 1<<14 will we > end > > up in that case? > > 1 << 14 / 48 = 341. 341 * 48 = 16384 = 1<< 14 - 16 OK. Please, add a DCHECK. if (fs_hz_ == 48000) { RTC_DCHECK_EQ(win_up_Q14, (1 << 14) - 16); } else { RTC_DCHECK_EQ(win_up_Q14, 1 << 14); }
I have adressed the comments with the latest update.
Thanks! Two minor comments inline, and also, please, add BUG=webrtc:1361 to the CL description (edit it in the web UI directly). You'll get presubmit errors otherwise. When you have uploaded the fixes, I can take a look at the bitexactness problems. https://codereview.webrtc.org/2763273003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/normal.h (right): https://codereview.webrtc.org/2763273003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/normal.h:17: #include "webrtc/base/checks.h" (Because of CheckedDivExact.) https://codereview.webrtc.org/2763273003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/normal.h:45: rtc::CheckedDivExact(fs_hz_, 1000); You should be able to do this where you perform and use the actual operation, that is on line 43 above. The CheckedDivExact will emit the result of the division.
Description was changed from ========== Changed OLA window for neteq. Old code didnt work well with 48khz fixing white spaces updated authors file Changed OLA window to use Q14 as Q5 dosnt work with 48khz. 1 ms @ 48 khz is > 2^5 BUG= ========== to ========== Changed OLA window for neteq. Old code didnt work well with 48khz fixing white spaces updated authors file Changed OLA window to use Q14 as Q5 dosnt work with 48khz. 1 ms @ 48 khz is > 2^5 BUG=webrtc:1361 ==========
updated based on recent comments
This looks good. Thanks! Now, I will have to figure out how to handle the bitexactness problems. I'll run some try bots and see what they say. Stay tuned.
https://codereview.webrtc.org/2763273003/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/normal.h (right): https://codereview.webrtc.org/2763273003/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/normal.h:45: default_win_slope_Q14_((1 << 14) / samples_per_ms_) {} You get compilation errors here for Win64. SEe https://build.chromium.org/p/tryserver.webrtc/builders/win_compile_x64_dbg/bu.... You must add an explicit cast to int16_t. Suggest you use rtc::dchecked_cast<int16_t>(...) and also #include "webrtc/base/safe_conversions.h"
Hi. Unfortunatly some other stuff I have done on my machine seems to have broken the build tools now both gn and ninja cannot be found. Is there an easy way to set this up again? Because of some other work I was messing around with ruby setup using rbenv.
Figured it out. Had messsed ou my shell setup script
updated
The CQ bit was checked by henrik.lundin@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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24180)
The CQ bit was checked by henrik.lundin@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: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/24686)
I have tried to fix some more warnings but seems like the test system is still not happy.
On 2017/03/29 13:22:24, squashingskak wrote: > I have tried to fix some more warnings but seems like the test system is still > not happy. I think we are only looking at problems with the bit-exactness tests now. I will have to manually verify that the new output audio is good, but it will take a day or two before I get a chance to do that. Once that is done, I will instruct you on how to update the checksums in this CL. Stay tuned...
On 2017/03/29 15:09:08, hlundin-webrtc wrote: > On 2017/03/29 13:22:24, squashingskak wrote: > > I have tried to fix some more warnings but seems like the test system is still > > not happy. > > I think we are only looking at problems with the bit-exactness tests now. I will > have to manually verify that the new output audio is good, but it will take a > day or two before I get a chance to do that. Once that is done, I will instruct > you on how to update the checksums in this CL. Stay tuned... I analysed the bit-exactness results, with and without this patch, and came to the following results on Linux. - AcmReceiverBitExactnessOldApi_8kHzOutput_output.wav: Only a few (177) single-bit diffs. - AcmReceiverBitExactnessOldApi_16kHzOutput_output.wav: Only a few (386) single- or two-bit diffs. - AcmReceiverBitExactnessOldApi_32kHzOutput_output.wav: Only a few (840) single- or two-bit diffs. - AcmReceiverBitExactnessOldApi_48kHzOutput_output.wav: Only a few (1268, or 1 in 15000) single- or two-bit diffs. Identical for AcmReceiverBitExactnessOldApi_48kHzOutputExternalDecoder_output.wav - AcmSenderBitExactnessOldApi_IsacWb30ms_output.wav: One single-bit diff. - neteq_universal_ref.pcm: A few (1 in 20000) single-bit diffs. - neteq_universal_ref.pcm (opus): Larger but isolated diffs (1 in 2500). These are all very limited and inaudible diffs. I think it is reasonable to assume that the same is true for the other platforms. You can go ahead and update the bitexactness hashes in this CL. (You can see both the expected and the actual hash by inspecting the stdout from the failing bots. Update the code with the new hashes. You will most likely have to find different hashes for win64, arm32, arm64 and "the rest".) Let me know if you need any help updating the hashes. Thanks again!
Updated. Hope I got it right
On 2017/04/07 16:06:24, squashingskak wrote: > Updated. Hope I got it right Thanks! Let's give the try bots another spin.
The CQ bit was checked by henrik.lundin@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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/23160)
Updated more hash values
The CQ bit was checked by henrik.lundin@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: This issue passed the CQ dry run.
On 2017/04/08 15:06:32, squashingskak wrote: > Updated more hash values Awesome! This now passes all bots, and LGTM. Thanks again for your contribution, and for bearing with the cumbersome bitexactness tests.
The CQ bit was checked by henrik.lundin@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491816010056770,
"parent_rev": "c547e84ec5b935151a8fe9797f283665b660dd24", "commit_rev":
"9f2c18e237593b5f8cbc7d7a3838ab6596123c53"}
Message was sent while issue was closed.
Description was changed from ========== Changed OLA window for neteq. Old code didnt work well with 48khz fixing white spaces updated authors file Changed OLA window to use Q14 as Q5 dosnt work with 48khz. 1 ms @ 48 khz is > 2^5 BUG=webrtc:1361 ========== to ========== Changed OLA window for neteq. Old code didnt work well with 48khz fixing white spaces updated authors file Changed OLA window to use Q14 as Q5 dosnt work with 48khz. 1 ms @ 48 khz is > 2^5 BUG=webrtc:1361 Review-Url: https://codereview.webrtc.org/2763273003 Cr-Commit-Position: refs/heads/master@{#17611} Committed: https://chromium.googlesource.com/external/webrtc/+/9f2c18e237593b5f8cbc7d7a3... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/9f2c18e237593b5f8cbc7d7a3...
Message was sent while issue was closed.
Hi. Looking at the final commit I see that wire swiss gmbh is there two times now in the Authors file. This is due to another patch I have made regarding Opus cbr. Can you remove one of them directly ?
Message was sent while issue was closed.
On 2017/04/10 09:33:52, squashingskak wrote: > Hi. Looking at the final commit I see that wire swiss gmbh is there two times > now in the Authors file. This is due to another patch I have made regarding Opus > cbr. Can you remove one of them directly ? Oh, yes, I noticed this when you first uploaded the CLs, but forgot along the way. I'll fix that.
Message was sent while issue was closed.
On 2017/04/10 11:42:35, hlundin-webrtc wrote: > On 2017/04/10 09:33:52, squashingskak wrote: > > Hi. Looking at the final commit I see that wire swiss gmbh is there two times > > now in the Authors file. This is due to another patch I have made regarding > Opus > > cbr. Can you remove one of them directly ? > > Oh, yes, I noticed this when you first uploaded the CLs, but forgot along the > way. I'll fix that. Fixed in https://codereview.webrtc.org/2813553004/.
Message was sent while issue was closed.
Hi. I have recieved an email that indicate that the patch is causing issues. The issue is that the check that was added after the window is sometimes failing. The reason is that in certain situations there isnt 1 ms available in the buffer and the overlap add operation is done with less than 1 ms. In this case the check that checks that the window ends up at 1.0 fails. I wonder if these checks are nessacary. The code is quite simple for me these checks are overkill. Otherwise I can try to calculate the worst case deviation from the window ending up at 1.0. Do I need to create a new issue in order to fix the issue?
Message was sent while issue was closed.
The change will look something like this:
- if (fs_hz_ == 48000) {
- RTC_DCHECK_EQ(win_up_Q14, (1 << 14) - 16);
- } else {
- RTC_DCHECK_EQ(win_up_Q14, 1 << 14);
- }
+ RTC_DCHECK_GT(win_up_Q14, (1 << 14) - 32); // Worst case rouding is a
length of 34
Message was sent while issue was closed.
The issue is this: https://bugs.chromium.org/p/chromium/issues/detail?id=710812 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
