Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2307493002:
NetEq: Flush and reset if the speech and cng sample rates mismatch (Closed)
|
Created:
4 years, 3 months ago by hlundin-webrtc Modified:
4 years, 3 months ago Reviewers:
minyue-webrtc CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNetEq: Flush and reset if the speech and cng sample rates mismatch
If a CNG packet is received first, followed by a speech packet with
another sample rate, NetEq should treat this as a change of codec, flush
out the CNG packet and reset the sample rate to that of the speech
packet.
BUG=webrtc:5447
NOTRY=True
Committed: https://crrev.com/067d855291f85a83ee914f4432cca311aaa816e2
Cr-Commit-Position: refs/heads/master@{#14032}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updates after review #
Total comments: 6
Patch Set 3 : Changing names of constants #Patch Set 4 : Changing names again #
Messages
Total messages: 19 (9 generated)
henrik.lundin@webrtc.org changed reviewers: + minyue@webrtc.org
Minyue, please take a look. Thanks!
Thanks! This looks generally good % comment. BTW, would it be better to add a unit test in packet_buffer_unittest. https://codereview.webrtc.org/2307493002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right): https://codereview.webrtc.org/2307493002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/packet_buffer.cc:41: // Returns true if either payload types is empty; or if both payload types are "Returns true if either payload types is empty" can be moved out of the function to match the function name better. and can avoid "rtc::Optional<uint8_t>(packet->header.payloadType)" as the first argument for the use of this function
Updated and added a unit test. PTAL again. https://codereview.webrtc.org/2307493002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right): https://codereview.webrtc.org/2307493002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/packet_buffer.cc:41: // Returns true if either payload types is empty; or if both payload types are On 2016/09/01 09:35:32, minyue-webrtc wrote: > "Returns true if either payload types is empty" can be moved out of the function > to match the function name better. > > and can avoid "rtc::Optional<uint8_t>(packet->header.payloadType)" as the first > argument for the use of this function Done.
lgtm https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:400: const DecoderDatabase::DecoderInfo infoCng(NetEqDecoder::kDecoderCNGnb, "", info_cng https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:407: .WillRepeatedly(Return(&infoSpeech)); info_speech
https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:400: const DecoderDatabase::DecoderInfo infoCng(NetEqDecoder::kDecoderCNGnb, "", On 2016/09/01 15:59:45, minyue-webrtc wrote: > info_cng kInfoCng, because it is a compile-time constant. https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:407: .WillRepeatedly(Return(&infoSpeech)); On 2016/09/01 15:59:45, minyue-webrtc wrote: > info_speech kInfoSpeech, because it is a compile-time constant.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2307493002/#ps40001 (title: "Changing names of constants")
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 henrik.lundin@webrtc.org
I was wrong. Changing again. https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:400: const DecoderDatabase::DecoderInfo infoCng(NetEqDecoder::kDecoderCNGnb, "", On 2016/09/01 16:11:15, hlundin-webrtc wrote: > On 2016/09/01 15:59:45, minyue-webrtc wrote: > > info_cng > > kInfoCng, because it is a compile-time constant. No, you are right. Because of "factory", this is not a compile-time constant. https://codereview.webrtc.org/2307493002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:407: .WillRepeatedly(Return(&infoSpeech)); On 2016/09/01 16:11:15, hlundin-webrtc wrote: > On 2016/09/01 15:59:45, minyue-webrtc wrote: > > info_speech > > kInfoSpeech, because it is a compile-time constant. No, you are right. Because of "factory", this is not a compile-time constant.
Description was changed from ========== NetEq: Flush and reset if the speech and cng sample rates mismatch If a CNG packet is received first, followed by a speech packet with another sample rate, NetEq should treat this as a change of codec, flush out the CNG packet and reset the sample rate to that of the speech packet. BUG=webrtc:5447 ========== to ========== NetEq: Flush and reset if the speech and cng sample rates mismatch If a CNG packet is received first, followed by a speech packet with another sample rate, NetEq should treat this as a change of codec, flush out the CNG packet and reset the sample rate to that of the speech packet. BUG=webrtc:5447 NOTRY=True ==========
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2307493002/#ps60001 (title: "Changing names again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== NetEq: Flush and reset if the speech and cng sample rates mismatch If a CNG packet is received first, followed by a speech packet with another sample rate, NetEq should treat this as a change of codec, flush out the CNG packet and reset the sample rate to that of the speech packet. BUG=webrtc:5447 NOTRY=True ========== to ========== NetEq: Flush and reset if the speech and cng sample rates mismatch If a CNG packet is received first, followed by a speech packet with another sample rate, NetEq should treat this as a change of codec, flush out the CNG packet and reset the sample rate to that of the speech packet. BUG=webrtc:5447 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== NetEq: Flush and reset if the speech and cng sample rates mismatch If a CNG packet is received first, followed by a speech packet with another sample rate, NetEq should treat this as a change of codec, flush out the CNG packet and reset the sample rate to that of the speech packet. BUG=webrtc:5447 NOTRY=True ========== to ========== NetEq: Flush and reset if the speech and cng sample rates mismatch If a CNG packet is received first, followed by a speech packet with another sample rate, NetEq should treat this as a change of codec, flush out the CNG packet and reset the sample rate to that of the speech packet. BUG=webrtc:5447 NOTRY=True Committed: https://crrev.com/067d855291f85a83ee914f4432cca311aaa816e2 Cr-Commit-Position: refs/heads/master@{#14032} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/067d855291f85a83ee914f4432cca311aaa816e2 Cr-Commit-Position: refs/heads/master@{#14032} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
