Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1415173005:
Prevent Opus DTX from generating intermittent noise during silence (Closed)
|
Created:
5 years, 1 month ago by minyue-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, peah-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPrevent Opus DTX from generating intermittent noise during silence.
Opus may have an internal error that causes this. Here we make a workaround by adding some small disturbance to the input signals to break a long sequence of zeros.
BUG=webrtc:5127
Committed: https://crrev.com/f475add57eada116bc960fe2935876ec8c077977
Cr-Commit-Position: refs/heads/master@{#10565}
Patch Set 1 #
Total comments: 3
Patch Set 2 : adding various frame sizes to unittest #
Total comments: 2
Patch Set 3 : new memory treatment and a test #
Total comments: 25
Patch Set 4 : refinement #Patch Set 5 : rebase and a small fix #
Total comments: 13
Patch Set 6 : differentiate fixed-point from floating-point #Patch Set 7 : some style changes #Patch Set 8 : rebase #Patch Set 9 : a fix after rebase #
Total comments: 27
Patch Set 10 : on Fredrik's comments #
Messages
Total messages: 49 (18 generated)
Description was changed from ========== fixing BUG= ========== to ========== Prevent Opus DTX from generating intermittent noise during silence. Opus may have an internal error that causes this. Here we make a workaround by adding some small disturbance to the input signals to break a long sequence of zeros. BUG=webrtc:5127 ==========
minyue@webrtc.org changed reviewers: + tina.legrand@webrtc.org
Hi Tina, This is a fix to the DTX problem. The time to add disturbance to break a long silence is trained offline manually. I am also working on an exhaustive test on this fix, to make sure that this solution is sufficient. Meanwhile, you can start review it. Thanks!
A question: is it necessary to check previous frame, to find if there are a certain number of consecutive zeros? What happens if all frames have +/-1 in them, but with a certain space between them? I think we will need tests for larger frames too. https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:137: } I think there is a much simpler way of doing this, that doesn't require the two extra vectors. My thinking it that we simply check if the input is all-zeros or not (per channel, I guess), and if it is, we replace it with a predefined vector. Or do we need to keep track of what happened in the previous frame?
On 2015/10/27 13:51:22, tlegrand-webrtc wrote: > A question: is it necessary to check previous frame, to find if there are a > certain number of consecutive zeros? What happens if all frames have +/-1 in > them, but with a certain space between them? > > I think we will need tests for larger frames too. > > https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... > File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): > > https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... > webrtc/modules/audio_coding/codecs/opus/opus_interface.c:137: } > I think there is a much simpler way of doing this, that doesn't require the two > extra vectors. My thinking it that we simply check if the input is all-zeros or > not (per channel, I guess), and if it is, we replace it with a predefined > vector. Or do we need to keep track of what happened in the previous frame? Yes, definitely need to add other frame sizes to the unittest and will do. BTW, I have tested this solution offline with different frame sizes. I think it is good to prevent any consecutive of a certain number of zeros. It may be no problem if a long sequence of zeros are placed after some non-zeros. But it will make the algorithm and verification much more complicated.
Patchset #2 (id:20001) has been deleted
Hi Tina, It is gonna be an interesting discussion. I wrote some words but let's talk tomorrow. I also updated the test to include different frame sizes. The test was written for 20ms frames, the DTX pattern is quite different for other frame sizes, and therefore, some structural changes are made. https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:137: } On 2015/10/27 13:51:22, tlegrand-webrtc wrote: > I think there is a much simpler way of doing this, that doesn't require the two > extra vectors. My thinking it that we simply check if the input is all-zeros or > not (per channel, I guess), and if it is, we replace it with a predefined > vector. Or do we need to keep track of what happened in the previous frame? There is a problem with checking if whole frame are zeros. Since a frame contains 960 samples (48 Khz * 20 ms). But I found that the longest zero segment that guarantee the problem not to happen is less than 158 (or now I found 157) samples, much smaller than a frame. So both in-frame and cross-frame zeros shall be considered. But of course, there is a trade-off between complexity and quality. Maybe we can still allow some noise pumping as long as it does not last for long period. Then checking all-zeros of frame might be ok. Another point to mention is that the signals are interleaved in stereo case. Therefore the check of zeros in each channel may still need a sample-by-sample check (we cannot use memcmp), then the complexity may be quite the same.
Thanks for adding the new tests, and for the explanation. As we talked about offline, Karl will continue the review, since I'm OOO the rest of the week. https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:137: } On 2015/10/27 18:29:01, minyue-webrtc wrote: > On 2015/10/27 13:51:22, tlegrand-webrtc wrote: > > I think there is a much simpler way of doing this, that doesn't require the > two > > extra vectors. My thinking it that we simply check if the input is all-zeros > or > > not (per channel, I guess), and if it is, we replace it with a predefined > > vector. Or do we need to keep track of what happened in the previous frame? > > There is a problem with checking if whole frame are zeros. Since a frame > contains 960 samples (48 Khz * 20 ms). But I found that the longest zero segment > that guarantee the problem not to happen is less than 158 (or now I found 157) > samples, much smaller than a frame. > > So both in-frame and cross-frame zeros shall be considered. > > But of course, there is a trade-off between complexity and quality. Maybe we can > still allow some noise pumping as long as it does not last for long period. Then > checking all-zeros of frame might be ok. > > Another point to mention is that the signals are interleaved in stereo case. > Therefore the check of zeros in each channel may still need a sample-by-sample > check (we cannot use memcmp), then the complexity may be quite the same. Ah, I see now why you need to check for shorter periods of all zeros. As per our offline discussion, consider changing to stack allocated memory. https://codereview.webrtc.org/1415173005/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:34: kZeroBreakCount = 157, We could change to something lower, just to be on the safe side, but on the other hand, it isn't the end of the world if we enter the "noise pumping" stage for short periods. Your call.
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
I addressed the comment and added another test. I also added Karl to review since Tina is OOO at the moment. https://codereview.webrtc.org/1415173005/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:34: kZeroBreakCount = 157, On 2015/10/28 13:20:37, tlegrand-webrtc wrote: > We could change to something lower, just to be on the safe side, but on the > other hand, it isn't the end of the world if we enter the "noise pumping" stage > for short periods. Your call. I would modify the signal as slightly as possible. This value is quite on the boundary, but it is safe in my tests. More trickily, it is not clear if it will just make it safer to chose any smaller number, since the behavior is not surely linear. I added another test, which can run long time and see if noise pump ever happened. With this value, I did not find any pump.
https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_inst.h (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_inst.h:22: size_t* zero_counts; What is this number more exactly? The number of consecutive zero samples ending with the last sample received? (Also, mention that there's one counter per channel.) https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:40: if (inst == NULL) if (!inst) https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:43: OpusEncInst* state = (OpusEncInst*)calloc(1, sizeof(OpusEncInst)); You don't need to cast. C allows implicit conversion from void* to any pointer type. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:48: state->zero_counts = (size_t*)calloc(channels, sizeof(size_t)); No need to cast. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:51: return -1; Don't try to handle malloc failures. Just assert if you feel you have to do anything. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:73: if (error != OPUS_OK || state->encoder == NULL) { !state->encoder https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:127: } It would've been easier to read this loop if you'd used buffer[i * channels + c] instead of *pointer. Also, you pay the cost of copying whether you need it or not. If long runs of zeros aren't common, this can get expensive. You could copy on demand instead. Before the loop, do const int16_t* input = audio_in; int16_t* writable_input = NULL; Then read from input[i * channels + c], and at the one place where you write, do if (!writable_input) { memcpy(buffer, audio_in, samples * channels * sizeof(int16_t)); input = writable_input = buffer; } writable_input[i * channels + c] = 1; https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:107: EXPECT_LE(*audio, bound); Hmm, does this even compile? These are signed/unsigned comparisons. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:194: // certain frames. ...stops sending for a certain number of frames. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_output_test.cc:111: VoENetwork* network = manager_.NetworkPtr(); You don't need to bind this to a local variable. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_output_test.cc:203: webrtc::SleepMs(kUnmuteTimeMs + random.Gaussian(0, 100)); Replace 100 with e.g. kUnmuteTimeMs/10 here, to make it easier to read? And in principle check that the value is positive, although I guess we'd have to be *really* unlucky...
Thank you for the review! now comes an undate. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_inst.h (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_inst.h:22: size_t* zero_counts; On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > What is this number more exactly? The number of consecutive zero samples ending > with the last sample received? (Also, mention that there's one counter per > channel.) I made some change in wording. I hope it is clearer. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:40: if (inst == NULL) On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > if (!inst) Done. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:43: OpusEncInst* state = (OpusEncInst*)calloc(1, sizeof(OpusEncInst)); On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > You don't need to cast. C allows implicit conversion from void* to any pointer > type. Done. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:48: state->zero_counts = (size_t*)calloc(channels, sizeof(size_t)); On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > No need to cast. Done. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:51: return -1; On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > Don't try to handle malloc failures. Just assert if you feel you have to do > anything. Done. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:65: WebRtcOpus_EncoderFree(state); I found switch() {} can be placed before state gets allocated, to avoid line 65. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:73: if (error != OPUS_OK || state->encoder == NULL) { On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > !state->encoder Done. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:127: } On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > It would've been easier to read this loop if you'd used buffer[i * > channels + c] instead of *pointer. > > Also, you pay the cost of copying whether you need it or not. If long > runs of zeros aren't common, this can get expensive. You could copy on > demand instead. Before the loop, do > > const int16_t* input = audio_in; > int16_t* writable_input = NULL; > > Then read from input[i * channels + c], and at the one place where you > write, do > > if (!writable_input) { > memcpy(buffer, audio_in, samples * channels * sizeof(int16_t)); > input = writable_input = buffer; > } > writable_input[i * channels + c] = 1; Yes, I agree that "input[i * channels + c]" and I have changed it. But will the compiler do "i * channel + c" as efficient as "++"? Your idea of avoiding memcpy is nice and I have made a change. It is slightly different from yours, just for maybe a little bit better readability. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:194: // certain frames. On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > ...stops sending for a certain number of frames. Done. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_output_test.cc:111: VoENetwork* network = manager_.NetworkPtr(); On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > You don't need to bind this to a local variable. you mean "manager_.NetworkPtr()->DeRegisterExternalTransport" in the next line? https://codereview.webrtc.org/1415173005/diff/50001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_output_test.cc:203: webrtc::SleepMs(kUnmuteTimeMs + random.Gaussian(0, 100)); On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > Replace 100 with e.g. kUnmuteTimeMs/10 here, to make it easier to read? > > And in principle check that the value is positive, although I guess we'd have to > be *really* unlucky... To avoid unlucky, we can use a bounded distribution
https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:107: EXPECT_LE(*audio, bound); On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > Hmm, does this even compile? These are signed/unsigned comparisons. I commented on this also, maybe did not save... http://stackoverflow.com/questions/25811220/unsigned-short-and-signed-short-c...
clearly, a rebase is needed, plus that android compiler do not like "for (int i = ...)"
On 2015/10/30 16:53:06, minyue-webrtc wrote: > clearly, a rebase is needed, plus that android compiler do not like "for (int i > = ...)" That's C99, and we don't like that new-fangled fancy-pants stuff here. You need to stick to C89, where variables are declared only immediately following an opening brace, like God intended.
https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:127: } On 2015/10/30 14:06:46, minyue-webrtc wrote: > On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > > It would've been easier to read this loop if you'd used buffer[i * > > channels + c] instead of *pointer. > > > > Also, you pay the cost of copying whether you need it or not. If long > > runs of zeros aren't common, this can get expensive. You could copy on > > demand instead. Before the loop, do > > > > const int16_t* input = audio_in; > > int16_t* writable_input = NULL; > > > > Then read from input[i * channels + c], and at the one place where you > > write, do > > > > if (!writable_input) { > > memcpy(buffer, audio_in, samples * channels * sizeof(int16_t)); > > input = writable_input = buffer; > > } > > writable_input[i * channels + c] = 1; > > Yes, I agree that "input[i * channels + c]" and I have changed it. But will the > compiler do "i * channel + c" as efficient as "++"? Yes. I tried these two: void f1(int a, int b) { for (int i = 0; i < a; ++i) { for (int j = 0; j < b; ++j) { g(i * b + j); h(i * b + j); } } } void f2(int a, int b) { int ij = 0; for (int i = 0; i < a; ++i) { for (int j = 0; j < b; ++j, ++ij) { g(ij); h(ij); } } } clang -O2 didn't generate identical code for them, but close enough as to make no difference. Modern compilers know to make this optimization on their own. https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:107: EXPECT_LE(*audio, bound); On 2015/10/30 14:08:40, minyue-webrtc wrote: > On 2015/10/29 16:35:21, kwiberg-webrtc wrote: > > Hmm, does this even compile? These are signed/unsigned comparisons. > > I commented on this also, maybe did not save... > > http://stackoverflow.com/questions/25811220/unsigned-short-and-signed-short-c... Right, I see. Because both int16_t and uint16_t will fit in int, it'll actually work as intended. Thanks. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:44: int opus_app; You're declaring variables not immediately following a {. That's C99, so some bots will probably be unhappy with it. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:57: } You don't need the braces for each case in the switch. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:134: use_buffer ? buffer : audio_in, The loop looks good now. But you have three blank lines in a row there... https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:58: uint16_t bound); It appears you don't need this declaration. All callers are after the function definition. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:103: uint16_t bound) { audio can be const int16_t* https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:107: EXPECT_LE(*audio, bound); Consider the same treatment here (replacing *audio with audio[i * channels + c]). A more complicated expression, but one less loop variable to keep track of. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/90001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_output_test.cc:125: RTC_DCHECK_EQ(0, base->StartSend(channel_)); Use ASSERT_* or EXPECT_* instead of DCHECK_* in tests. The latter will kill the entire process, taking other tests with it (and fail to trigger at all in release builds). Or do neither ASSERT_* nor EXPECT_* work in constructors and destructors?
To solve the failures on Android bots, I made a change, which offers a different value for zero-breaking and a different threshold for unittest on fixed-point build. This forms patch set 6. To separate this change out, Karl's early suggestions on styles will be addressed in a later patch set. Happy reviewing! https://codereview.webrtc.org/1415173005/diff/90001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/90001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_output_test.cc:125: RTC_DCHECK_EQ(0, base->StartSend(channel_)); On 2015/11/01 02:01:55, kwiberg-webrtc wrote: > Use ASSERT_* or EXPECT_* instead of DCHECK_* in tests. The latter will kill the > entire process, taking other tests with it (and fail to trigger at all in > release builds). > > Or do neither ASSERT_* nor EXPECT_* work in constructors and destructors? Done. and I modified VoEOutputCheckMediaProcess::Process() to let it return quickly.
Now the changes around coding style that Karl commented on are also done. With massive offline testing, I think the fix is quite solid. I would be a good time now for Tina and Karl make a final judgement. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:44: int opus_app; On 2015/11/01 02:01:55, kwiberg-webrtc wrote: > You're declaring variables not immediately following a {. That's C99, so some > bots will probably be unhappy with it. Done. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:57: } On 2015/11/01 02:01:55, kwiberg-webrtc wrote: > You don't need the braces for each case in the switch. I see. this existed before. But I can just change it. https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:134: use_buffer ? buffer : audio_in, On 2015/11/01 02:01:55, kwiberg-webrtc wrote: > The loop looks good now. But you have three blank lines in a row there... :) https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:103: uint16_t bound) { On 2015/11/01 02:01:55, kwiberg-webrtc wrote: > audio can be const int16_t* yes of course https://codereview.webrtc.org/1415173005/diff/90001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:107: EXPECT_LE(*audio, bound); On 2015/11/01 02:01:55, kwiberg-webrtc wrote: > Consider the same treatment here (replacing *audio with audio[i * channels + > c]). A more complicated expression, but one less loop variable to keep track of. Done.
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/1415173005/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415173005/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/5455) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/5390) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/10601) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/9226) mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5112) mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/10329)
Patchset #8 (id:150001) has been deleted
Patchset #8 (id:170001) has been deleted
Patchset #8 (id:190001) has been deleted
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1415173005/#ps210001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415173005/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415173005/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1605)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1415173005/#ps230001 (title: "a fix after rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415173005/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415173005/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1608)
minyue@webrtc.org changed reviewers: + solenberg@webrtc.org
Hi Fredrik, I added a test in VoE, I need you to take a look.
https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_inst.h (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_inst.h:20: // When Opus is in DTX mode, we use |zero_counts| to count consecutive zeros Should you add a TODO to remove this workaround once it has been fixed properly in Opus? https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:14: #include <assert.h> Why is this a .c file? Can we make it a .cc and use RTC_DCHECK() instead of assert? https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:63: assert(state); This should really be an RTC_CHECK() as it is a possible run-time error. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:101: size_t i; declare when these are used; even C99 supports that. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:104: int16_t buffer[2 * 48 * kWebRtcOpusMaxEncodeFrameSizeMs]; what if inst->channels > 2 ? https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:122: memcpy(buffer, audio_in, samples * channels * sizeof(int16_t)); this looks like it will break if the count is reached more than once, i.e.: ... check check reached zerobreakcount for c=0; copy audio_in -> buffer; set zerobreakvalue in buffer ... check check reached zerobreakcount for c=1 (or again for c=0); copy audio_in -> buffer (overwriting); set zerobreakvalue in buffer can you add a unit test for this case? https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:199: const int kRuntimeMs = 2000; super nit: capital T in time https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. Why do we need to test at this level? Shouldn't this be covered in specific tests for the actual codec? VoE shouldn't need to know about codec specifics. It should just be a mediator, forwarding config information to codec instances, and piping data into/out of them. I'm really not fond of adding more tests that use the soon-to-be obsolete VoE interfaces. Also, I'm really not fond of adding 20s of test time for this one particular case (20s, to be added to each try job, on each bot, from hereon until the end of days...) https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:43: void SetOutputBound(int16_t lower_bound, int16_t upper_bound); Make this settable in c-tor instead - the bounds don't change in your test. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:55: void set_lower_bound(int16_t lower_bound) { lower_bound_ = lower_bound; } set in c-tor https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:116: channel_, file_name.c_str(), true, false, kInputFormat, 1.0)); nit: formatting
Hi Fredrik, Thanks for the comments! I did some changes. For some comments, I put my comments. Please just let me know if you want more changes. ~~~~ Karl, About C99 or pre-C99 on opus_interface.c, I do not have a good judgement. Could you comment as well? https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_inst.h (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_inst.h:20: // When Opus is in DTX mode, we use |zero_counts| to count consecutive zeros On 2015/11/09 12:41:00, the sun wrote: > Should you add a TODO to remove this workaround once it has been fixed properly > in Opus? Done. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:14: #include <assert.h> On 2015/11/09 12:41:00, the sun wrote: > Why is this a .c file? Can we make it a .cc and use RTC_DCHECK() instead of > assert? Good idea. but it might be a outreach for this CL. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:63: assert(state); On 2015/11/09 12:41:00, the sun wrote: > This should really be an RTC_CHECK() as it is a possible run-time error. c file made this a limit. may consider in a separate CL I think https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:101: size_t i; On 2015/11/09 12:41:00, the sun wrote: > declare when these are used; even C99 supports that. Karl wanted this be pre-C99 compatible. But, true, ironically, we have define-before-use here and there anyway. I do not mind moving them back to line 116. But Karl, WDYT? https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:104: int16_t buffer[2 * 48 * kWebRtcOpusMaxEncodeFrameSizeMs]; On 2015/11/09 12:41:00, the sun wrote: > what if inst->channels > 2 ? Emm, we really wanted this be statically allocated to by most efficient. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:122: memcpy(buffer, audio_in, samples * channels * sizeof(int16_t)); On 2015/11/09 12:41:00, the sun wrote: > this looks like it will break if the count is reached more than once, i.e.: > > ... > check > check > reached zerobreakcount for c=0; copy audio_in -> buffer; set zerobreakvalue in > buffer > ... > check > check > reached zerobreakcount for c=1 (or again for c=0); copy audio_in -> buffer > (overwriting); set zerobreakvalue in buffer > > can you add a unit test for this case? Doesn't use_buffer prevent multi-entry? Regarding unittest, the current test OpusTest.OpusDtxOn already covers this. The problem you proposed will change the frequency of 1s, and fail that test. I just tried removing line 123, and that failed. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:199: const int kRuntimeMs = 2000; On 2015/11/09 12:41:00, the sun wrote: > super nit: capital T in time fixed :) https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2015/11/09 12:41:01, the sun wrote: > Why do we need to test at this level? Shouldn't this be covered in specific > tests for the actual codec? > > VoE shouldn't need to know about codec specifics. It should just be a mediator, > forwarding config information to codec instances, and piping data into/out of > them. > > I'm really not fond of adding more tests that use the soon-to-be obsolete VoE > interfaces. > > Also, I'm really not fond of adding 20s of test time for this one particular > case (20s, to be added to each try job, on each bot, from hereon until the end > of days...) I see. There are two reasons that I wanted to check this piece of code in 1. It gives a tool to check the output value range of the whole encoding/decoding chain. 2. It makes sure that NetEq etc. does not bring any problems in the DTX particular situation. 3. This is a fuzz test and can be run arbitrary time (20s can be changed). I used it for local testing. But I think others may also use it to debug similar problems. Regarding old VoE APIs, this test relies on VoETestManager to talk to VoE, which is used in other tests. If the plan is to refactor VoETestManager, no particular cares may need to be taken in this test. But if you insist, I can remove the test. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:43: void SetOutputBound(int16_t lower_bound, int16_t upper_bound); On 2015/11/09 12:41:01, the sun wrote: > Make this settable in c-tor instead - the bounds don't change in your test. ok. I just wanted to make this class a bit more generic. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:55: void set_lower_bound(int16_t lower_bound) { lower_bound_ = lower_bound; } On 2015/11/09 12:41:00, the sun wrote: > set in c-tor Done. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:116: channel_, file_name.c_str(), true, false, kInputFormat, 1.0)); On 2015/11/09 12:41:01, the sun wrote: > nit: formatting Done.
lgtm https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:104: int16_t buffer[2 * 48 * kWebRtcOpusMaxEncodeFrameSizeMs]; On 2015/11/09 15:13:00, minyue-webrtc wrote: > On 2015/11/09 12:41:00, the sun wrote: > > what if inst->channels > 2 ? > > Emm, we really wanted this be statically allocated to by most efficient. The problem is if channels > 2 we will get buffer overwrite below. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:122: memcpy(buffer, audio_in, samples * channels * sizeof(int16_t)); On 2015/11/09 15:13:00, minyue-webrtc wrote: > On 2015/11/09 12:41:00, the sun wrote: > > this looks like it will break if the count is reached more than once, i.e.: > > > > ... > > check > > check > > reached zerobreakcount for c=0; copy audio_in -> buffer; set zerobreakvalue in > > buffer > > ... > > check > > check > > reached zerobreakcount for c=1 (or again for c=0); copy audio_in -> buffer > > (overwriting); set zerobreakvalue in buffer > > > > can you add a unit test for this case? > > Doesn't use_buffer prevent multi-entry? Regarding unittest, the current test > OpusTest.OpusDtxOn already covers this. The problem you proposed will change the > frequency of 1s, and fail that test. I just tried removing line 123, and that > failed. Ah, my bad! You're quite right, should work like it is implemented. https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/voe_output_test.cc (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_output_test.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2015/11/09 15:13:00, minyue-webrtc wrote: > On 2015/11/09 12:41:01, the sun wrote: > > Why do we need to test at this level? Shouldn't this be covered in specific > > tests for the actual codec? > > > > VoE shouldn't need to know about codec specifics. It should just be a > mediator, > > forwarding config information to codec instances, and piping data into/out of > > them. > > > > I'm really not fond of adding more tests that use the soon-to-be obsolete VoE > > interfaces. > > > > Also, I'm really not fond of adding 20s of test time for this one particular > > case (20s, to be added to each try job, on each bot, from hereon until the end > > of days...) > > I see. There are two reasons that I wanted to check this piece of code in > 1. It gives a tool to check the output value range of the whole > encoding/decoding chain. > > 2. It makes sure that NetEq etc. does not bring any problems in the DTX > particular situation. > > 3. This is a fuzz test and can be run arbitrary time (20s can be changed). I > used it for local testing. But I think others may also use it to debug similar > problems. > > Regarding old VoE APIs, this test relies on VoETestManager to talk to VoE, which > is used in other tests. If the plan is to refactor VoETestManager, no particular > cares may need to be taken in this test. > > But if you insist, I can remove the test. Ah, I see, sorry I didn't notice it was a fuzzer test. Very well then.
https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:104: int16_t buffer[2 * 48 * kWebRtcOpusMaxEncodeFrameSizeMs]; On 2015/11/09 15:29:56, the sun wrote: > On 2015/11/09 15:13:00, minyue-webrtc wrote: > > On 2015/11/09 12:41:00, the sun wrote: > > > what if inst->channels > 2 ? > > > > Emm, we really wanted this be statically allocated to by most efficient. > > The problem is if channels > 2 we will get buffer overwrite below. Yes, it can in principle happen, but the creation of an encoder should have failed if there are more than 2 channels.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1415173005/#ps250001 (title: "on Fredrik's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415173005/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415173005/250001
Message was sent while issue was closed.
Committed patchset #10 (id:250001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f475add57eada116bc960fe2935876ec8c077977 Cr-Commit-Position: refs/heads/master@{#10565}
Message was sent while issue was closed.
https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1415173005/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:101: size_t i; On 2015/11/09 15:13:00, minyue-webrtc wrote: > On 2015/11/09 12:41:00, the sun wrote: > > declare when these are used; even C99 supports that. > > Karl wanted this be pre-C99 compatible. > > But, true, ironically, we have define-before-use here and there anyway. > > I do not mind moving them back to line 116. But Karl, WDYT? It's not that I *want* C89, it's just that the last time I did nontrivial work in our C code, some bots (I don't recall which, but I'll hazard a guess and say Windows...) were still not accepting C99. If it works now, then great.
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:250001) has been created in https://codereview.webrtc.org/1428613004/ by kjellander@webrtc.org. The reason for reverting is: Breaks voe_auto_test on all three "large tests bots". https://build.chromium.org/p/client.webrtc/builders/Win32%20Release%20%5Blarg... https://build.chromium.org/p/client.webrtc/builders/Mac32%20Release%20%5Blarg... https://build.chromium.org/p/client.webrtc/builders/Linux64%20Release%20%5Bla... Notice these bots are no longer a part of the default trybot set, so they have to be run manually when working with code that affects their tests (they were removed as they queued up all the time in the CQ, and usually don't catch breakages).. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
