Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1765873002:
On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. (Closed)
DescriptionOn WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream.
This CL is a first step to moving codec configuration into AudioSendStream.
BUG=webrtc:4690
TBR=ossu@webrtc.org
Committed: https://crrev.com/72e29d2cbb670b723ac2fa961b15b3b97bf639ef
Cr-Commit-Position: refs/heads/master@{#11911}
Patch Set 1 #
Total comments: 12
Patch Set 2 : zero-init CodecInst's #Patch Set 3 : Can't use braced init #Patch Set 4 : comments #Patch Set 5 : build warning+rebase #
Total comments: 8
Patch Set 6 : Added docs #
Total comments: 6
Patch Set 7 : More comment #Patch Set 8 : another comment #
Total comments: 7
Patch Set 9 : Comment #Patch Set 10 : rebase #
Messages
Total messages: 34 (12 generated)
Description was changed from ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. BUG=webrtc:4690 ========== to ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration in AudioSendStream. BUG=webrtc:4690 ==========
solenberg@webrtc.org changed reviewers: + kwiberg@webrtc.org, ossu@webrtc.org
Description was changed from ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration in AudioSendStream. BUG=webrtc:4690 ========== to ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 ==========
The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765873002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765873002/1
solenberg@webrtc.org changed reviewers: + stefan@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Looks fine in general, but I'll leave approval to someone with better understanding of the innards of WebRTC for now. I've made a few comments, nothing major. https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:411: webrtc::CodecInst voe_codec; Cannot CodecInst be default-initialized to something invalid (like all zeroes). If not, I believe webrtc::CodecInst voe_codec = {0}; should set it to all zeroes without having to resort to memset. https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1558: // when RED is disabled? Maybe it's not possible to set opus (well, sub-encoder) config when it's wrapped with a RED encoder and that NACK doesn't make sense with redundancy? Or maybe it cannot be set on the RED encoder? I'm just stabbing around in the dark here. https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1722: webrtc::PayloadFrequencies cn_freq = webrtc::kFreq8000Hz; This seems ... odd. Why is cn_freq defaulted to 8000 only if the payload frequency is _not_ 8000? I mean, it will either get changed or get to RTC_NOTREACHED(). https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1752: send_codec_spec_.codec_inst.channels != 2) { Probably never going to happen, and not your code originally, but shouldn't it be channels == 1 here? There's some support somewhere for five channel raw PCM16B. Would VAD work with that? https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.h:277: std::memset(&codec_inst, 0, sizeof(codec_inst)); I believe CodecInst could be zero initialized with = {0} here as well (see comments in the .cc file). Some say = {} should work as well. Look it up! :) I know I will.
https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:411: webrtc::CodecInst voe_codec; On 2016/03/04 14:40:22, ossu wrote: > Cannot CodecInst be default-initialized to something invalid (like all zeroes). > If not, I believe webrtc::CodecInst voe_codec = {0}; should set it to all zeroes > without having to resort to memset. To answer my own question: Initializing a struct with too few initializers causes the rest of the members to be initialized as during static initialization (should be all zeroes in this case). So = {} should be fine; {0} doesn't explicitly repeat that zero for every member, it just happens to turn out like that anyways. :)
https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:411: webrtc::CodecInst voe_codec; On 2016/03/04 15:02:02, ossu wrote: > On 2016/03/04 14:40:22, ossu wrote: > > Cannot CodecInst be default-initialized to something invalid (like all > zeroes). > > If not, I believe webrtc::CodecInst voe_codec = {0}; should set it to all > zeroes > > without having to resort to memset. > > To answer my own question: Initializing a struct with too few initializers > causes the rest of the members to be initialized as during static initialization > (should be all zeroes in this case). So = {} should be fine; {0} doesn't > explicitly repeat that zero for every member, it just happens to turn out like > that anyways. :) Done. https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1722: webrtc::PayloadFrequencies cn_freq = webrtc::kFreq8000Hz; On 2016/03/04 14:40:22, ossu wrote: > This seems ... odd. Why is cn_freq defaulted to 8000 only if the payload > frequency is _not_ 8000? I mean, it will either get changed or get to > RTC_NOTREACHED(). Done. https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1752: send_codec_spec_.codec_inst.channels != 2) { On 2016/03/04 14:40:22, ossu wrote: > Probably never going to happen, and not your code originally, but shouldn't it > be channels == 1 here? There's some support somewhere for five channel raw > PCM16B. Would VAD work with that? I would assume not. :) https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.h:277: std::memset(&codec_inst, 0, sizeof(codec_inst)); On 2016/03/04 14:40:22, ossu wrote: > I believe CodecInst could be zero initialized with = {0} here as well (see > comments in the .cc file). Some say = {} should work as well. Look it up! :) I > know I will. Yes, unfortunately direct braced init of the field won't work though.
lgtm, but consider adding doc comments. https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:412: if (!ToCodecInst(*found_codec, &voe_codec)) { Zeroing the output argument before calling ToCodecInst in a bunch of places, while laudable, is entirely unrelated to the rest of this CL, right? (Would it be possible to have it return an Optional<CodecInst> instead?) https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:571: // Temporarily turn logging level up for the Init call. Fixing spelling errors in lines that your CL is not otherwise touching makes reviewing harder. https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1521: These methods don't have any docs that explain what they do, which would've been particularly helpful for the two methods that are both named SetSendCodecs yet do very different things. https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1605: continue; Hmm. Not changed by this CL, but don't we support 48k as well?
https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:412: if (!ToCodecInst(*found_codec, &voe_codec)) { On 2016/03/05 02:29:45, kwiberg-webrtc wrote: > Zeroing the output argument before calling ToCodecInst in a bunch of places, > while laudable, is entirely unrelated to the rest of this CL, right? Well, I actually changed that in response to one of ossu@'s comments. So yes, in principle unrelated, but I don't see a problem keeping it here. > (Would it be possible to have it return an Optional<CodecInst> instead?) That would be a bigger change (touching tests too) and I'd rather do it separately. https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:571: // Temporarily turn logging level up for the Init call. On 2016/03/05 02:29:45, kwiberg-webrtc wrote: > Fixing spelling errors in lines that your CL is not otherwise touching makes > reviewing harder. Acknowledged. Should have kept my fingers in check but won't revert the change. https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1605: continue; On 2016/03/05 02:29:45, kwiberg-webrtc wrote: > Hmm. Not changed by this CL, but don't we support 48k as well? It's not in the list of preferred codecs. Do we support it in the lower layers?
https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1521: On 2016/03/05 02:29:45, kwiberg-webrtc wrote: > These methods don't have any docs that explain what they do, which would've been > particularly helpful for the two methods that are both named SetSendCodecs yet > do very different things. Done.
solenberg@webrtc.org changed reviewers: + minyue@webrtc.org
On 2016/03/07 09:02:35, the sun wrote: > https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/1765873002/diff/80001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine.cc:1521: > On 2016/03/05 02:29:45, kwiberg-webrtc wrote: > > These methods don't have any docs that explain what they do, which would've > been > > particularly helpful for the two methods that are both named SetSendCodecs yet > > do very different things. > > Done. Added holmer@ to verify changes to transport cc set up. Added minyue@ to verify changes to Opus configuration. PTAL
Transport cc looks correct, so lgtm on that. I had a few other questions that you may want to consider. https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1651: cn_freq = webrtc::kFreq8000Hz; Have we removed support for this? https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1584: // Loop through the codecs list again to find the CN codec. It would be nice to break the code below out to a function similar to GetPreferredCodec() https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1621: if (!SetSendCodecs(ch.second->channel())) { Should this method have a different name? It doesn't sound like it caches codecs
https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1651: cn_freq = webrtc::kFreq8000Hz; On 2016/03/07 09:27:50, stefan-webrtc (holmer) wrote: > Have we removed support for this? I hope not! It should just be the order of the conditions that has changed. https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1584: // Loop through the codecs list again to find the CN codec. On 2016/03/07 09:27:50, stefan-webrtc (holmer) wrote: > It would be nice to break the code below out to a function similar to > GetPreferredCodec() Yes. This code is due to go through a lot of changes in the near term, but I'd like to do it piecewise. Adding a TODO. https://codereview.webrtc.org/1765873002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1621: if (!SetSendCodecs(ch.second->channel())) { On 2016/03/07 09:27:50, stefan-webrtc (holmer) wrote: > Should this method have a different name? It doesn't sound like it caches codecs Well, it is the name that has been used so far and, more importantly, its functionality will be gradually folded into AudioSendStream from now on, so I hope that Pretty Soon (tm) the function will be pining for the fjords. So I will leave it as is for now.
lgtm
On 2016/03/07 14:23:21, kwiberg-webrtc wrote: > lgtm ossu@, minyue@. PTAL
https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1558: // when RED is disabled? On 2016/03/04 14:40:22, ossu wrote: > Maybe it's not possible to set opus (well, sub-encoder) config when it's wrapped > with a RED encoder and that NACK doesn't make sense with redundancy? Or maybe it > cannot be set on the RED encoder? I'm just stabbing around in the dark here. I think it was because Opus cannot have RED and FEC meanwhile. But, other parameters, dtx/maxplaybackrate should not be affected by RED. I can later investigate this. https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:416: *out = voe_codec; what is the benefit of copying? https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1585: // TODO(solenberg): Break out into a separate function? may just do it in this CL https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.h:289: webrtc::CodecInst codec_inst; maybe webrtc::CodecInst codec_inst = {0} and remove 227 & 228
https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1558: // when RED is disabled? On 2016/03/08 10:19:47, minyue-webrtc wrote: > On 2016/03/04 14:40:22, ossu wrote: > > Maybe it's not possible to set opus (well, sub-encoder) config when it's > wrapped > > with a RED encoder and that NACK doesn't make sense with redundancy? Or maybe > it > > cannot be set on the RED encoder? I'm just stabbing around in the dark here. > > I think it was because Opus cannot have RED and FEC meanwhile. But, other > parameters, dtx/maxplaybackrate should not be affected by RED. I can later > investigate this. Thanks for clarifying. Added a comment. I think we can leave the logic as is until we have changed the code to use AudioEncoder objects and AudioSendStream. https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:416: *out = voe_codec; On 2016/03/08 10:19:47, minyue-webrtc wrote: > what is the benefit of copying? The benefit is that we don't risk mutating the caller's state until we really have a result available. https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1585: // TODO(solenberg): Break out into a separate function? On 2016/03/08 10:19:47, minyue-webrtc wrote: > may just do it in this CL No, I'm not sure it is the right thing to do. https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.h:289: webrtc::CodecInst codec_inst; On 2016/03/08 10:19:47, minyue-webrtc wrote: > maybe webrtc::CodecInst codec_inst = {0} and remove 227 & 228 I'd like to but the Windows compilers won't fall for that. IIUC the expression in this context is equivalent to using list-initialization, which is also not permitted by the style guide.
thanks for the explanation, lgtm
Description was changed from ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 ========== to ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 TBR=ossu@webrtc.org ==========
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, kwiberg@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1765873002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765873002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765873002/180001
https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.h:277: webrtc::CodecInst empty_inst = {0}; Apologies if this resolves exactly to what minyue asked below, but why a separate empty_inst? Wouldn't codec_inst = {0} work directly? I mean, in the end it probably ends up the same when compiled. It just looked a bit odd to me.
On 2016/03/08 13:52:53, ossu wrote: > https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... > File webrtc/media/engine/webrtcvoiceengine.h (right): > > https://codereview.webrtc.org/1765873002/diff/140001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine.h:277: webrtc::CodecInst empty_inst = {0}; > Apologies if this resolves exactly to what minyue asked below, but why a > separate empty_inst? > Wouldn't codec_inst = {0} work directly? I mean, in the end it probably ends up > the same when compiled. It just looked a bit odd to me. IIUC codec_inst = {0} is equivalent to codec_inst{0}, which is a syntax banned per the Chromium style guide (http://chromium-cpp.appspot.com/). I actually tried a version of that in the first patch set and had the Windows bots yell at me, but in all fairness I notice what I tried was codec_inst = {-1, {0}, 0, 0, 0, 0}. If ={0} is in fact evaluated as initializing a static type it may be possible to use. I'll give it a try in a follow-up (this CL is already well through the CQ).
Message was sent while issue was closed.
Description was changed from ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 TBR=ossu@webrtc.org ========== to ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 TBR=ossu@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 TBR=ossu@webrtc.org ========== to ========== On WVoMC::SetSendParameters(), figure out send codec settings ONCE, not for each send stream. This CL is a first step to moving codec configuration into AudioSendStream. BUG=webrtc:4690 TBR=ossu@webrtc.org Committed: https://crrev.com/72e29d2cbb670b723ac2fa961b15b3b97bf639ef Cr-Commit-Position: refs/heads/master@{#11911} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/72e29d2cbb670b723ac2fa961b15b3b97bf639ef Cr-Commit-Position: refs/heads/master@{#11911} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
