Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707133007:
Pick a matching CN codec, rather than the first CN codec. (Closed)
DescriptionPick a matching CN codec, rather than the first CN codec.
It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching.
BUG=webrtc:7282
Review-Url: https://codereview.webrtc.org/2707133007
Cr-Commit-Position: refs/heads/master@{#16979}
Committed: https://chromium.googlesource.com/external/webrtc/+/0c4b849b278de3666e8bb10781f2082e3feb8f74
Patch Set 1 #
Total comments: 7
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Pick a matching CN codec, rather than the first CN codec. BUG= ========== to ========== Pick a matching CN codec, rather than the first CN codec. It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching. BUG= ==========
ossu@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
I'm just gonna let you have a look at this. From my reading the RFCs, and looking at the implementation of AudioEncoderCng, it really seems like we can't be using a clockrate different from the voice codec. Maybe there's some other, internal reason why we might want to set up a CNG codec even when there's no matching voice codec?
lgtm https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2008: case 32000: You will have to check this, but I do think we can accept 48000 Hz as well nowadays.
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
from https://tools.ietf.org/html/rfc3389 it looks like that the clock rate should indeed match. But WebRTC implementation seems to have been more tolerant. What is desired behavior? https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2000: cn_codec.clockrate == codec->clockrate) { I am not familiar with the reason of the way it is implemented. 1. it was really unfortunate to use codec as var name, go ahead change it. 2. my questions are what if there is no CN codec with a matched clock rate. The old code seems to say, well, cng_plfreq can be different from the send codec's plfreq. The new code will then not use any CNG. which is desired?
https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1990: for (const AudioCodec& cn_codec : codecs) { ossu: Add a check here for if external CN is at all allowed with the chosen codec. https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2000: cn_codec.clockrate == codec->clockrate) { On 2017/02/27 08:19:54, minyue-webrtc wrote: > I am not familiar with the reason of the way it is implemented. > > 1. it was really unfortunate to use codec as var name, go ahead change it. > > 2. my questions are what if there is no CN codec with a matched clock rate. The > old code seems to say, well, cng_plfreq can be different from the send codec's > plfreq. The new code will then not use any CNG. which is desired? From refactoring this further in my injectable audio encoders work, I've found that the actual CN encoder lacks any parameters to set clockrate or samplerate, but relies on the values from the codec it was paired with. There's also another check in AudioSendStream that only enables CNG if the sample rate match, so I figure we might not be enabling CNG in general, unless the codec we choose uses a clockrate of 8000, which maybe it almost always does? https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2008: case 32000: On 2017/02/27 08:00:52, hlundin-webrtc wrote: > You will have to check this, but I do think we can accept 48000 Hz as well > nowadays. I think I recall the CN code dealing with that, now that you mention it. Right now it isn't used, because of Opus having its own CN handling. This should be filtered based on the appropriate AudioCodecSpec flag instead.
https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1990: for (const AudioCodec& cn_codec : codecs) { On 2017/03/01 23:24:37, ossu wrote: > ossu: Add a check here for if external CN is at all allowed with the chosen > codec. No, wait, I'm mixing this up with changes I haven't landed yet. :) https://codereview.webrtc.org/2707133007/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2008: case 32000: On 2017/03/01 23:24:38, ossu wrote: > On 2017/02/27 08:00:52, hlundin-webrtc wrote: > > You will have to check this, but I do think we can accept 48000 Hz as well > > nowadays. > > I think I recall the CN code dealing with that, now that you mention it. Right > now it isn't used, because of Opus having its own CN handling. This should be > filtered based on the appropriate AudioCodecSpec flag instead. ... which isn't used in this part of the code yet. I'll leave the 48kHz out of this for now, since it would only apply for Opus and we don't use it with Opus, but will check if 48kHz is supported and, if so, signaling of 48kHz CN in my upcoming injectable encoders changes.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14350)
Description was changed from ========== Pick a matching CN codec, rather than the first CN codec. It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching. BUG= ========== to ========== Pick a matching CN codec, rather than the first CN codec. It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching. BUG=webrtc:7282 ==========
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14386)
ossu@webrtc.org changed reviewers: + solenberg@webrtc.org
+solenberg Need owner approval.
lgtm
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1488481262925820, "parent_rev":
"6ebaa6a283e16579ba68e0443fa8d63ee80317ab", "commit_rev":
"0c4b849b278de3666e8bb10781f2082e3feb8f74"}
Message was sent while issue was closed.
Description was changed from ========== Pick a matching CN codec, rather than the first CN codec. It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching. BUG=webrtc:7282 ========== to ========== Pick a matching CN codec, rather than the first CN codec. It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching. BUG=webrtc:7282 Review-Url: https://codereview.webrtc.org/2707133007 Cr-Commit-Position: refs/heads/master@{#16979} Committed: https://chromium.googlesource.com/external/webrtc/+/0c4b849b278de3666e8bb1078... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/0c4b849b278de3666e8bb1078... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
