Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2352713005:
Posting Opus's set-force-channels functionality to WebRTC. (Closed)
|
Created:
4 years, 3 months ago by minyue-webrtc Modified:
4 years, 2 months ago Reviewers:
tlegrand-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. |
DescriptionPosting Opus's set-force-channels functionality to WebRTC.
BUG=webrtc:6303
Committed: https://crrev.com/c8299f9f8779a541b7e6124646cf9b3a8afe0b61
Cr-Commit-Position: refs/heads/master@{#14394}
Patch Set 1 #Patch Set 2 : reformat #
Total comments: 17
Patch Set 3 : clarify comment #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Posting Opus's set-force-channels functionality to WebRTC. BUG= ========== to ========== Posting Opus's set-force-channels functionality to WebRTC. BUG= ==========
minyue@webrtc.org changed reviewers: + tina.legrand@webrtc.org
Hi Tina, Could you help review this small CL?
I have some questions around how the function is supposed to be used. See comments inline. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:199: * WebRtcOpus_SetForceChannels(...) I don't think this function need to be as general as it is now. How is it intended to be used? Is this how it works now? If encoder init channel = 1 - this function has no power to change things. If encoder init channel = 2 - only force to 1 or 2 will make a change. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:203: * function overrules this decision and forces the encoder to encode in this -> that https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:211: * - num_channels : 0 - Not forced Why would you call a SetForce-function with the input "do not force"? https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:216: * -1 - Error Maybe we shouldn't return -1, but instead return mode. So, in the case the encoder is initiated as mono, this function will always return 1. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:412: TEST_P(OpusTest, OpusForceChannels) { This test will always run either the channels_ == 2 part OR the channels_ = 1. Why not always run both?
Hi Tina, Thanks for the comments! Please find my answers to your questions inline. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:199: * WebRtcOpus_SetForceChannels(...) On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > I don't think this function need to be as general as it is now. How is it > intended to be used? > > Is this how it works now? > > If encoder init channel = 1 - this function has no power to change things. > If encoder init channel = 2 - only force to 1 or 2 will make a change. This function is very much doing the same as Opus native API in forcing number of channels. True that it is effective only when init channel == 2. In that scenario, we can force the channel to 1 or 2. We may also want to stop forcing. Therefore, I introduced "0" so that we don't need to introduce another API, and it also resembles the Opus native API in having three input values: AUTO/1/2. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:203: * function overrules this decision and forces the encoder to encode in On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > this -> that sure https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:211: * - num_channels : 0 - Not forced On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > Why would you call a SetForce-function with the input "do not force"? I do not like that name either, but it resembles the Opus API: https://mf4.xiph.org/jenkins/view/opus/job/opus/ws/doc/html/group__opus__enco... https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:216: * -1 - Error On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > Maybe we shouldn't return -1, but instead return mode. So, in the case the > encoder is initiated as mono, this function will always return 1. A problem can be: if the encoder is in stereo mode, and we set "3", we do not know what action to take. Also, we need to handle the situation that inst is null. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:412: TEST_P(OpusTest, OpusForceChannels) { On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > This test will always run either the channels_ == 2 part OR the channels_ = 1. > Why not always run both? It actually runs both cases. It will be split into multiple (4) cases, due to TEST_P.
I made a proposal for making the comment section clearer, and after that LGTM. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:199: * WebRtcOpus_SetForceChannels(...) On 2016/09/22 15:02:19, minyue-webrtc wrote: > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > I don't think this function need to be as general as it is now. How is it > > intended to be used? > > > > Is this how it works now? > > > > If encoder init channel = 1 - this function has no power to change things. > > If encoder init channel = 2 - only force to 1 or 2 will make a change. > > This function is very much doing the same as Opus native API in forcing number > of channels. > > True that it is effective only when init channel == 2. In that scenario, we can > force the channel to 1 or 2. We may also want to stop forcing. Therefore, I > introduced "0" so that we don't need to introduce another API, and it also > resembles the Opus native API in having three input values: AUTO/1/2. Acknowledged. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:203: * function overrules this decision and forces the encoder to encode in On 2016/09/22 15:02:19, minyue-webrtc wrote: > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > this -> that > > sure Or maybe change the comment to "This function overrules the previous setting, and forces the encoder to encode in..." https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:216: * -1 - Error On 2016/09/22 15:02:19, minyue-webrtc wrote: > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > Maybe we shouldn't return -1, but instead return mode. So, in the case the > > encoder is initiated as mono, this function will always return 1. > > A problem can be: if the encoder is in stereo mode, and we set "3", we do not > know what action to take. > > Also, we need to handle the situation that inst is null. Right! Because this is a C-file, so we can't add a *CHECK you would in C++? https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:412: TEST_P(OpusTest, OpusForceChannels) { On 2016/09/22 15:02:19, minyue-webrtc wrote: > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > This test will always run either the channels_ == 2 part OR the channels_ = 1. > > Why not always run both? > > It actually runs both cases. It will be split into multiple (4) cases, due to > TEST_P. Acknowledged.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:216: * -1 - Error On 2016/09/23 11:37:41, tlegrand-webrtc wrote: > On 2016/09/22 15:02:19, minyue-webrtc wrote: > > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > > Maybe we shouldn't return -1, but instead return mode. So, in the case the > > > encoder is initiated as mono, this function will always return 1. > > > > A problem can be: if the encoder is in stereo mode, and we set "3", we do not > > know what action to take. > > > > Also, we need to handle the situation that inst is null. > > Right! Because this is a C-file, so we can't add a *CHECK you would in C++? Yes, you can! I made variants of the [D]CHECK macros that work in C recently.
Hi, I uploaded a new patch with the wording Tina suggested. On Karl's comment, I suggest that we do a refactor of the whole file in future. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:203: * function overrules this decision and forces the encoder to encode in On 2016/09/23 11:37:41, tlegrand-webrtc wrote: > On 2016/09/22 15:02:19, minyue-webrtc wrote: > > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > > this -> that > > > > sure > > Or maybe change the comment to > "This function overrules the previous setting, and forces the encoder to encode > in..." Done. https://codereview.webrtc.org/2352713005/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:216: * -1 - Error On 2016/09/23 11:40:52, kwiberg-webrtc wrote: > On 2016/09/23 11:37:41, tlegrand-webrtc wrote: > > On 2016/09/22 15:02:19, minyue-webrtc wrote: > > > On 2016/09/22 12:15:42, tlegrand-webrtc wrote: > > > > Maybe we shouldn't return -1, but instead return mode. So, in the case the > > > > encoder is initiated as mono, this function will always return 1. > > > > > > A problem can be: if the encoder is in stereo mode, and we set "3", we do > not > > > know what action to take. > > > > > > Also, we need to handle the situation that inst is null. > > > > Right! Because this is a C-file, so we can't add a *CHECK you would in C++? > > Yes, you can! I made variants of the [D]CHECK macros that work in C recently. That is good news. We may consider doing a refactor on all APIs in this file. I suggest we keep the convention in this new API to be consistent.
kwiberg@webrtc.org changed reviewers: - kwiberg@webrtc.org
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tina.legrand@webrtc.org Link to the patchset: https://codereview.webrtc.org/2352713005/#ps40001 (title: "clarify comment")
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/8707)
Description was changed from ========== Posting Opus's set-force-channels functionality to WebRTC. BUG= ========== to ========== Posting Opus's set-force-channels functionality to WebRTC. BUG=webrtc:6303 ==========
The CQ bit was checked by minyue@webrtc.org
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 ========== Posting Opus's set-force-channels functionality to WebRTC. BUG=webrtc:6303 ========== to ========== Posting Opus's set-force-channels functionality to WebRTC. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Posting Opus's set-force-channels functionality to WebRTC. BUG=webrtc:6303 ========== to ========== Posting Opus's set-force-channels functionality to WebRTC. BUG=webrtc:6303 Committed: https://crrev.com/c8299f9f8779a541b7e6124646cf9b3a8afe0b61 Cr-Commit-Position: refs/heads/master@{#14394} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c8299f9f8779a541b7e6124646cf9b3a8afe0b61 Cr-Commit-Position: refs/heads/master@{#14394} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
