Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1334303005:
Returning correct duration estimate on Opus DTX packets. (Closed)
|
Created:
5 years, 3 months ago by minyue-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReturning correct duration estimate on Opus DTX packets.
Bug 4985 revealed two flaws
1. Opus duration estimate did not return correct length for DTX packets,
2. NetEq DoCodecInternalCng did not assign enough buffer.
P.S.
Generalizing problem 1, current NetEq decode function checks memory size by calling the duration estimate function. This is not ideal. A better way is to let codec's decode function to receive buffer size and return failure if it is not enough. This can be made in a separate CL.
BUG=webrtc:4985
R=henrik.lundin@webrtc.org
Committed: https://crrev.com/6d92bf59f3f8c0ce8ad445c11aaaf955eae752cc
Cr-Commit-Position: refs/heads/master@{#10031}
Patch Set 1 #
Total comments: 9
Patch Set 2 : small comments fixed #Patch Set 3 : move codec internal cng to decode #Patch Set 4 : adding Neteq test #
Total comments: 14
Patch Set 5 : rebasing on RTC_DCHECK #
Total comments: 2
Patch Set 6 : fixing a corner case #Patch Set 7 : addressing comments and fixing an error #
Total comments: 6
Patch Set 8 : final polish #Patch Set 9 : fixing a silly mistake in unittest #
Messages
Total messages: 30 (10 generated)
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, tina.legrand@webrtc.org
Hi Tina and Henrik, Please take a look at this fix. Henrik, would you suggest how to add a neteq test around DoCodecInternalCng?
Good. Some comments. I'll think some more about testing. https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:363: // WebRtcOpus_Decode calls PLC when payload length is zero. So we returns we returns -> we return https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:386: int plc_samples; Merge lines 386 and 387, and make it const. https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:390: return plc_samples; return (plc_samples <= kWebRtcOpusMaxFrameSizePerChannel) ? plc_samples : kWebRtcOpusMaxFrameSizePerChannel; https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:393: Remove one extra line. https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1657: if (length > 0) { I think you'll have to handle the error differently. If the decoder returns an error, we should probably set the output to zero; use decoder_frame_length_ as default packet size. And then call normal_->Process anyway. But, it is still good to LOG.
I fixed small comments and added a question https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/opus_interface.c (right): https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:386: int plc_samples; On 2015/09/15 13:05:09, hlundin-webrtc wrote: > Merge lines 386 and 387, and make it const. Done. https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:390: return plc_samples; On 2015/09/15 13:05:09, hlundin-webrtc wrote: > return (plc_samples <= kWebRtcOpusMaxFrameSizePerChannel) ? plc_samples : > kWebRtcOpusMaxFrameSizePerChannel; Done. https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/opus_interface.c:393: On 2015/09/15 13:05:09, hlundin-webrtc wrote: > Remove one extra line. Done. https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1657: if (length > 0) { On 2015/09/15 13:05:09, hlundin-webrtc wrote: > I think you'll have to handle the error differently. If the decoder returns an > error, we should probably set the output to zero; use decoder_frame_length_ as > default packet size. And then call normal_->Process anyway. But, it is still > good to LOG. May I suggest yet another solution? When normal decoding fails, it uses expansion, see line 1255. *operation = kExpand; // Do expansion to get data instead. I think DoCodecInternalCng belongs there. I gave myself an action item of moving these lines into Decode() (or DecodeLoop). I did not apply it since I wanted to make a quick fix. Maybe good to do it. WDYT
I did a bigger change, and tested with voe_cmd_test. See if you like it, some tests on Neteq are coming later.
On 2015/09/16 09:06:54, minyue-webrtc wrote: > I did a bigger change, and tested with voe_cmd_test. See if you like it, some > tests on Neteq are coming later. We have test around CodecInternalCng, see neteq_impl_unittest.cc TEST_F(NetEqImplTest, CodecInternalCng) What may be missing is a check of a right behavior when decoder->Decode returns a -1
I added NetEq tests, for which I found another flaw, i.e., if Codec CNG gives less than 10 ms, NetEq would issue an under-run error. This is also fixed. Please take a look.
On 2015/09/17 09:33:06, minyue-webrtc wrote: > I added NetEq tests, for which I found another flaw, i.e., if Codec CNG gives > less than 10 ms, NetEq would issue an under-run error. This is also fixed. > > Please take a look. Check patch set 4 please, 5 is a rebase.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Just a few minor comments, then lgtm. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1279: int NetEqImpl::DecodeCng(AudioDecoder* decoder, int* decoded_length, This never returns anything but 0. Make it return void instead. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1284: decode_length = decoder->Decode( Declare decode_length here instead. You can make it const, too. And, perhaps chose a different name since decode_length is really similar to decoded_length. Maybe just len? https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right): https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1018: // We let decoder gives 5 ms each time, and therefore, 2 packets make 10 ms. gives -> return https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1041: .WillOnce(Return(NetEq::kDecoderErrorCode)); This is the error code returned from the decoder, not from NetEq itself. Suggest you create a local constant kDecoderError = something. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1112: Here you should first verify that neteq_->LastError() returns NetEq::kDecoderErrorCode. Then, you should verify that neteq_->LastDecoderError() return kDecoderError (the local constant suggested above). https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1132: // This test checks the behavior of NetEq when audio decoder fails. More specific: when it fails during internal CNG. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1233: Again, verify output error codes. https://codereview.webrtc.org/1334303005/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1680: assert(normal_.get()); RTC_DCHECK
On 2015/09/17 14:21:30, hlundin-webrtc wrote: > Just a few minor comments, then lgtm. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl.cc:1279: int > NetEqImpl::DecodeCng(AudioDecoder* decoder, int* decoded_length, > This never returns anything but 0. Make it return void instead. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl.cc:1284: decode_length = > decoder->Decode( > Declare decode_length here instead. You can make it const, too. And, perhaps > chose a different name since decode_length is really similar to decoded_length. > Maybe just len? > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right): > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1018: // We let decoder > gives 5 ms each time, and therefore, 2 packets make 10 ms. > gives -> return > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1041: > .WillOnce(Return(NetEq::kDecoderErrorCode)); > This is the error code returned from the decoder, not from NetEq itself. Suggest > you create a local constant kDecoderError = something. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1112: > Here you should first verify that neteq_->LastError() returns > NetEq::kDecoderErrorCode. Then, you should verify that > neteq_->LastDecoderError() return kDecoderError (the local constant suggested > above). > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1132: // This test > checks the behavior of NetEq when audio decoder fails. > More specific: when it fails during internal CNG. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1233: > Again, verify output error codes. > > https://codereview.webrtc.org/1334303005/diff/80001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): > > https://codereview.webrtc.org/1334303005/diff/80001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl.cc:1680: assert(normal_.get()); > RTC_DCHECK It was not really perfect. A corner case was hit, i.e., when switch decoders, the active decoder may be lost and then DecodeCng should fail. See patch set 6.
Patchset #7 (id:150001) has been deleted
Some changes are made. you may take another look. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1279: int NetEqImpl::DecodeCng(AudioDecoder* decoder, int* decoded_length, On 2015/09/17 14:21:29, hlundin-webrtc wrote: > This never returns anything but 0. Make it return void instead. Ok. I'd make this similar to DecodeLoop. I see what is missing. See new patch. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1284: decode_length = decoder->Decode( On 2015/09/17 14:21:29, hlundin-webrtc wrote: > Declare decode_length here instead. You can make it const, too. And, perhaps > chose a different name since decode_length is really similar to decoded_length. > Maybe just len? Done. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right): https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1018: // We let decoder gives 5 ms each time, and therefore, 2 packets make 10 ms. On 2015/09/17 14:21:29, hlundin-webrtc wrote: > gives -> return Done. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1041: .WillOnce(Return(NetEq::kDecoderErrorCode)); On 2015/09/17 14:21:30, hlundin-webrtc wrote: > This is the error code returned from the decoder, not from NetEq itself. Suggest > you create a local constant kDecoderError = something. Done. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1112: On 2015/09/17 14:21:30, hlundin-webrtc wrote: > Here you should first verify that neteq_->LastError() returns > NetEq::kDecoderErrorCode. Then, you should verify that > neteq_->LastDecoderError() return kDecoderError (the local constant suggested > above). Done. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1132: // This test checks the behavior of NetEq when audio decoder fails. On 2015/09/17 14:21:30, hlundin-webrtc wrote: > More specific: when it fails during internal CNG. Done. https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1233: On 2015/09/17 14:21:30, hlundin-webrtc wrote: > Again, verify output error codes. Done. https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1289: (decoded_buffer_length_ - *decoded_length) * sizeof(int16_t), The first version was was wrong here, fixed.
On 2015/09/18 13:41:17, minyue-webrtc wrote: > Some changes are made. you may take another look. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl.cc:1279: int > NetEqImpl::DecodeCng(AudioDecoder* decoder, int* decoded_length, > On 2015/09/17 14:21:29, hlundin-webrtc wrote: > > This never returns anything but 0. Make it return void instead. > > Ok. I'd make this similar to DecodeLoop. I see what is missing. See new patch. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl.cc:1284: decode_length = > decoder->Decode( > On 2015/09/17 14:21:29, hlundin-webrtc wrote: > > Declare decode_length here instead. You can make it const, too. And, perhaps > > chose a different name since decode_length is really similar to > decoded_length. > > Maybe just len? > > Done. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right): > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1018: // We let decoder > gives 5 ms each time, and therefore, 2 packets make 10 ms. > On 2015/09/17 14:21:29, hlundin-webrtc wrote: > > gives -> return > > Done. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1041: > .WillOnce(Return(NetEq::kDecoderErrorCode)); > On 2015/09/17 14:21:30, hlundin-webrtc wrote: > > This is the error code returned from the decoder, not from NetEq itself. > Suggest > > you create a local constant kDecoderError = something. > > Done. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1112: > On 2015/09/17 14:21:30, hlundin-webrtc wrote: > > Here you should first verify that neteq_->LastError() returns > > NetEq::kDecoderErrorCode. Then, you should verify that > > neteq_->LastDecoderError() return kDecoderError (the local constant suggested > > above). > > Done. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1132: // This test > checks the behavior of NetEq when audio decoder fails. > On 2015/09/17 14:21:30, hlundin-webrtc wrote: > > More specific: when it fails during internal CNG. > > Done. > > https://codereview.webrtc.org/1334303005/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:1233: > On 2015/09/17 14:21:30, hlundin-webrtc wrote: > > Again, verify output error codes. > > Done. > > https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): > > https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/neteq/neteq_impl.cc:1289: (decoded_buffer_length_ - > *decoded_length) * sizeof(int16_t), > The first version was was wrong here, fixed. Ping for an attention.
one error found in the first version, see the latest patch set
Still some nits, but still lgtm once those are fixed. https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1281: if (!decoder) { Please, comment under what circumstances this happens. https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1289: (decoded_buffer_length_ - *decoded_length) * sizeof(int16_t), On 2015/09/18 13:41:17, minyue-webrtc wrote: > The first version was was wrong here, fixed. Acknowledged. https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1689: assert(normal_.get()); Still, RTC_DCHECK.
Thanks. Final comments addressed. https://codereview.webrtc.org/1334303005/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1680: assert(normal_.get()); On 2015/09/17 14:21:30, hlundin-webrtc wrote: > RTC_DCHECK Done. https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1281: if (!decoder) { On 2015/09/23 08:55:22, hlundin-webrtc wrote: > Please, comment under what circumstances this happens. Done. https://codereview.webrtc.org/1334303005/diff/170001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/neteq_impl.cc:1689: assert(normal_.get()); On 2015/09/23 08:55:22, hlundin-webrtc wrote: > Still, RTC_DCHECK. oh, yes
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/1334303005/#ps190001 (title: "final polish")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334303005/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334303005/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/9382)
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/1334303005/#ps210001 (title: "fixing a silly mistake in unittest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334303005/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334303005/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Committed patchset #9 (id:210001) manually as 6d92bf59f3f8c0ce8ad445c11aaaf955eae752cc (presubmit successful).
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6d92bf59f3f8c0ce8ad445c11aaaf955eae752cc Cr-Commit-Position: refs/heads/master@{#10031} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
