Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1677013002:
Switch to using new ACM methods for encoder management (Closed)
|
Created:
4 years, 10 months ago by kwiberg-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@acm-13 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSwitch to using new ACM methods for encoder management
BUG=webrtc:5028
Committed: https://crrev.com/c8d071e4e074934ef8346fc91ad4f24ae333afee
Cr-Commit-Position: refs/heads/master@{#12267}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : review fixes #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : Solar review comments #
Total comments: 7
Patch Set 5 : DCHECKs #
Messages
Total messages: 44 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
kwiberg@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
LG, but I have two questions. https://codereview.webrtc.org/1677013002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/codec_manager.h (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/codec_manager.h:74: return true; If we did not get a stack, we still return true. Does that not count as an error? https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (left): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1261: assert(!(disableDTX && enableVAD)); // disableDTX mode is deprecated. Why did the assert have to go? The last parameter is still deprecated, and now also unused. Maybe document it in the interface file (if you cannot remove the parameter now). https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1356: if (audio_coding_->RegisterReceiveCodec( One could wonder if there is any valid case for registering a decoder with the same RTP payload type as an already registered one that warrants this try-retry construct. I could only come up with an historical reason, where we used to allow registering CNG at different sample rates to one and the same payload type. But that case should be no more.
https://codereview.webrtc.org/1677013002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/codec_manager.h (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/codec_manager.h:74: return true; On 2016/03/29 20:23:32, hlundin-webrtc wrote: > If we did not get a stack, we still return true. Does that not count as an > error? No, because callers may call this method before a speech encoder has been set—e.g. if CN and RED are registered first. If the logic seems unduly complex, that’s because it is. What we have currently (and still have as of this CL) is VoE/libjingle sending configuration data to the ACM in pieces (such as single CodecInst objects, requests to enable VAD, etc.), and the ACM must rebuild the encoder stack to the best of its ability upon receiving each piece. What we hope to get eventually is a complete specification presented all at once. https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (left): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1261: assert(!(disableDTX && enableVAD)); // disableDTX mode is deprecated. On 2016/03/29 20:23:32, hlundin-webrtc wrote: > Why did the assert have to go? The last parameter is still deprecated, and now > also unused. Maybe document it in the interface file (if you cannot remove the > parameter now). Good question. I remember removing it for a reason, but I can’t recall what reason... I’ll restore it (as a DCHECK). https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1356: if (audio_coding_->RegisterReceiveCodec( On 2016/03/29 20:23:32, hlundin-webrtc wrote: > One could wonder if there is any valid case for registering a decoder with the > same RTP payload type as an already registered one that warrants this try-retry > construct. I could only come up with an historical reason, where we used to > allow registering CNG at different sample rates to one and the same payload > type. But that case should be no more. I don’t know of a valid case either.
lgtm https://codereview.webrtc.org/1677013002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/codec_manager.h (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/codec_manager.h:74: return true; On 2016/03/29 23:25:32, kwiberg-webrtc wrote: > On 2016/03/29 20:23:32, hlundin-webrtc wrote: > > If we did not get a stack, we still return true. Does that not count as an > > error? > > No, because callers may call this method before a speech encoder has been > set—e.g. if CN and RED are registered first. > > If the logic seems unduly complex, that’s because it is. What we have currently > (and still have as of this CL) is VoE/libjingle sending configuration data to > the ACM in pieces (such as single CodecInst objects, requests to enable VAD, > etc.), and the ACM must rebuild the encoder stack to the best of its ability > upon receiving each piece. What we hope to get eventually is a complete > specification presented all at once. Acknowledged. https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (left): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1261: assert(!(disableDTX && enableVAD)); // disableDTX mode is deprecated. On 2016/03/29 23:25:32, kwiberg-webrtc wrote: > On 2016/03/29 20:23:32, hlundin-webrtc wrote: > > Why did the assert have to go? The last parameter is still deprecated, and now > > also unused. Maybe document it in the interface file (if you cannot remove the > > parameter now). > > Good question. I remember removing it for a reason, but I can’t recall what > reason... I’ll restore it (as a DCHECK). Good. https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1356: if (audio_coding_->RegisterReceiveCodec( On 2016/03/29 23:25:32, kwiberg-webrtc wrote: > On 2016/03/29 20:23:32, hlundin-webrtc wrote: > > One could wonder if there is any valid case for registering a decoder with the > > same RTP payload type as an already registered one that warrants this > try-retry > > construct. I could only come up with an historical reason, where we used to > > allow registering CNG at different sample rates to one and the same payload > > type. But that case should be no more. > > I don’t know of a valid case either. Right. You may want to add a TODO that this is essentially the same that is done in AcmReceiver::AddCodec. It's up to you.
https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (left): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1261: assert(!(disableDTX && enableVAD)); // disableDTX mode is deprecated. On 2016/03/30 08:36:10, hlundin-webrtc wrote: > On 2016/03/29 23:25:32, kwiberg-webrtc wrote: > > On 2016/03/29 20:23:32, hlundin-webrtc wrote: > > > Why did the assert have to go? The last parameter is still deprecated, and > now > > > also unused. Maybe document it in the interface file (if you cannot remove > the > > > parameter now). > > > > Good question. I remember removing it for a reason, but I can’t recall what > > reason... I’ll restore it (as a DCHECK). > > Good. Done. https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1356: if (audio_coding_->RegisterReceiveCodec( On 2016/03/30 08:36:10, hlundin-webrtc wrote: > On 2016/03/29 23:25:32, kwiberg-webrtc wrote: > > On 2016/03/29 20:23:32, hlundin-webrtc wrote: > > > One could wonder if there is any valid case for registering a decoder with > the > > > same RTP payload type as an already registered one that warrants this > > try-retry > > > construct. I could only come up with an historical reason, where we used to > > > allow registering CNG at different sample rates to one and the same payload > > > type. But that case should be no more. > > > > I don’t know of a valid case either. > > Right. You may want to add a TODO that this is essentially the same that is done > in AcmReceiver::AddCodec. It's up to you. Done.
The CQ bit was checked by kwiberg@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/1677013002/#ps80001 (title: "review fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677013002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4470)
kwiberg@webrtc.org changed reviewers: + asapersson@webrtc.org, solenberg@webrtc.org
Forgot to loop in some OWNERS: +asapersson for webrtc/modules/utility/ +solenberg for webrtc/voice_engine/
Can you explain why we need to do this change? We want to start using the new ACM functionality in the WVoMC and AudioNnStream layers, but voe::Channel is a dead end, so I'd rather not we touch it unless it is blocking other work.
On 2016/03/31 12:06:02, the sun wrote: > Can you explain why we need to do this change? > > We want to start using the new ACM functionality in the WVoMC and AudioNnStream > layers, but voe::Channel is a dead end, so I'd rather not we touch it unless it > is blocking other work. Well, the immediate reason is that if I don't update all callers, I can't deprecate the ACM methods that require the ACM to be able to create encoders. voe::Channel just happens to be the piece of code in Voice Engine that actually calls the ACM at the moment.
On 2016/03/31 12:43:23, kwiberg-webrtc wrote: > On 2016/03/31 12:06:02, the sun wrote: > > Can you explain why we need to do this change? > > > > We want to start using the new ACM functionality in the WVoMC and > AudioNnStream > > layers, but voe::Channel is a dead end, so I'd rather not we touch it unless > it > > is blocking other work. > > Well, the immediate reason is that if I don't update all callers, I can't > deprecate the ACM methods that require the ACM to be able to create encoders. > voe::Channel just happens to be the piece of code in Voice Engine that actually > calls the ACM at the moment. Ok, I need to take a closer look. The lambda code looks pretty complicated at first glance and I'm wondering if that is because I'm not that used to reading code using lambdas, or if it can be done simpler using old-fashioned return codes (perhaps using the new-fashioned rtc::Optional...) :)
On 2016/03/31 12:54:58, the sun wrote: > On 2016/03/31 12:43:23, kwiberg-webrtc wrote: > > On 2016/03/31 12:06:02, the sun wrote: > > > Can you explain why we need to do this change? > > > > > > We want to start using the new ACM functionality in the WVoMC and > > AudioNnStream > > > layers, but voe::Channel is a dead end, so I'd rather not we touch it unless > > it > > > is blocking other work. > > > > Well, the immediate reason is that if I don't update all callers, I can't > > deprecate the ACM methods that require the ACM to be able to create encoders. > > voe::Channel just happens to be the piece of code in Voice Engine that > actually > > calls the ACM at the moment. > > Ok, I need to take a closer look. The lambda code looks pretty complicated at > first glance and I'm wondering if that is because I'm not that used to reading > code using lambdas, or if it can be done simpler using old-fashioned return > codes (perhaps using the new-fashioned rtc::Optional...) :) I've got to agree with The Sun here. The MakeEncoder calls look much more complicated than they need to be. Lambdas are nice and all but in this case, I think they convolute program flow unnecessarily. rtc::Optional seems like a better choice to me, however it might be an issue that it default-constructs an instance of what it's holding that causes it to be unusable for this right now. Still, turning it into a tagged union-type thing where it's contents are only sometimes initialized shouldn't be too difficult.
On 2016/04/04 09:28:24, ossu wrote:
> On 2016/03/31 12:54:58, the sun wrote:
> > On 2016/03/31 12:43:23, kwiberg-webrtc wrote:
> > > On 2016/03/31 12:06:02, the sun wrote:
> > > > Can you explain why we need to do this change?
> > > >
> > > > We want to start using the new ACM functionality in the WVoMC and
> > > AudioNnStream
> > > > layers, but voe::Channel is a dead end, so I'd rather not we touch it
> unless
> > > it
> > > > is blocking other work.
> > >
> > > Well, the immediate reason is that if I don't update all callers, I can't
> > > deprecate the ACM methods that require the ACM to be able to create
> encoders.
> > > voe::Channel just happens to be the piece of code in Voice Engine that
> > actually
> > > calls the ACM at the moment.
> >
> > Ok, I need to take a closer look. The lambda code looks pretty complicated
at
> > first glance and I'm wondering if that is because I'm not that used to
reading
> > code using lambdas, or if it can be done simpler using old-fashioned return
> > codes (perhaps using the new-fashioned rtc::Optional...) :)
>
> I've got to agree with The Sun here. The MakeEncoder calls look much more
> complicated than they need to be. Lambdas are nice and all but in this case, I
> think they convolute program flow unnecessarily. rtc::Optional seems like a
> better choice to me, however it might be an issue that it default-constructs
an
> instance of what it's holding that causes it to be unusable for this right
now.
> Still, turning it into a tagged union-type thing where it's contents are only
> sometimes initialized shouldn't be too difficult.
We don't need rtc::Optional here; the new encoder is already wrapped
in std::unique_ptr, and a null value there will do nicely to indicate
that there was no new encoder.
Right now, calls look like this:
if (!codec_manager_.MakeEncoder(
&rent_a_codec_,
[&](std::unique_ptr<AudioEncoder> enc) {
do_something_with_the_encoder(std::move(enc));
})) {
handle_error();
}
If we continued to return a bool indicating success or failure, and
returned the new encoder via an output argument, we'd get something
like this:
std::unique_ptr<AudioEncoder> new_enc;
if (!codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc)) {
handle_error();
}
if (new_enc) {
do_something_with_the_encoder(std::move(new_enc));
}
Returning a struct with a bool and an encoder would look like this:
auto new_enc = codec_manager_.MakeEncoder(&rent_a_codec_);
if (!new_enc.success) {
handle_error();
} else if (new_enc.encoder) {
do_something_with_the_encoder(std::move(new_enc.encoder));
}
There would be some complications with calls like this one:
if (!STR_CASE_CMP(codec.plname, "CN")) {
if (!codec_manager_.RegisterEncoder(codec) ||
!codec_manager_.MakeEncoder(&rent_a_codec_,
[&](std::unique_ptr<AudioEncoder> enc) {
audio_coding_->SetEncoder(
std::move(enc));
}) ||
audio_coding_->RegisterReceiveCodec(
codec, [&] { return rent_a_codec_.RentIsacDecoder(); }) == -1 ||
_rtpRtcpModule->RegisterSendPayload(codec) == -1) {
WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
"Channel::Init() failed to register CN (%d/%d) "
"correctly - 1",
codec.pltype, codec.plfreq);
}
}
Because currently audio_coding_->SetEncoder is being called before
audio_coding_->RegisterReceiveCodec. But we probably don't have to
preserve the exact order of the calls.
On 2016/04/04 09:57:45, kwiberg-webrtc wrote:
> On 2016/04/04 09:28:24, ossu wrote:
> > On 2016/03/31 12:54:58, the sun wrote:
> > > On 2016/03/31 12:43:23, kwiberg-webrtc wrote:
> > > > On 2016/03/31 12:06:02, the sun wrote:
> > > > > Can you explain why we need to do this change?
> > > > >
> > > > > We want to start using the new ACM functionality in the WVoMC and
> > > > AudioNnStream
> > > > > layers, but voe::Channel is a dead end, so I'd rather not we touch it
> > unless
> > > > it
> > > > > is blocking other work.
> > > >
> > > > Well, the immediate reason is that if I don't update all callers, I
can't
> > > > deprecate the ACM methods that require the ACM to be able to create
> > encoders.
> > > > voe::Channel just happens to be the piece of code in Voice Engine that
> > > actually
> > > > calls the ACM at the moment.
> > >
> > > Ok, I need to take a closer look. The lambda code looks pretty complicated
> at
> > > first glance and I'm wondering if that is because I'm not that used to
> reading
> > > code using lambdas, or if it can be done simpler using old-fashioned
return
> > > codes (perhaps using the new-fashioned rtc::Optional...) :)
> >
> > I've got to agree with The Sun here. The MakeEncoder calls look much more
> > complicated than they need to be. Lambdas are nice and all but in this case,
I
> > think they convolute program flow unnecessarily. rtc::Optional seems like a
> > better choice to me, however it might be an issue that it default-constructs
> an
> > instance of what it's holding that causes it to be unusable for this right
> now.
> > Still, turning it into a tagged union-type thing where it's contents are
only
> > sometimes initialized shouldn't be too difficult.
>
> We don't need rtc::Optional here; the new encoder is already wrapped
> in std::unique_ptr, and a null value there will do nicely to indicate
> that there was no new encoder.
>
> Right now, calls look like this:
>
> if (!codec_manager_.MakeEncoder(
> &rent_a_codec_,
> [&](std::unique_ptr<AudioEncoder> enc) {
> do_something_with_the_encoder(std::move(enc));
> })) {
> handle_error();
> }
>
> If we continued to return a bool indicating success or failure, and
> returned the new encoder via an output argument, we'd get something
> like this:
>
> std::unique_ptr<AudioEncoder> new_enc;
> if (!codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc)) {
> handle_error();
> }
> if (new_enc) {
> do_something_with_the_encoder(std::move(new_enc));
> }
>
> Returning a struct with a bool and an encoder would look like this:
>
> auto new_enc = codec_manager_.MakeEncoder(&rent_a_codec_);
> if (!new_enc.success) {
> handle_error();
> } else if (new_enc.encoder) {
> do_something_with_the_encoder(std::move(new_enc.encoder));
> }
>
> There would be some complications with calls like this one:
>
> if (!STR_CASE_CMP(codec.plname, "CN")) {
> if (!codec_manager_.RegisterEncoder(codec) ||
> !codec_manager_.MakeEncoder(&rent_a_codec_,
> [&](std::unique_ptr<AudioEncoder> enc) {
> audio_coding_->SetEncoder(
> std::move(enc));
> }) ||
> audio_coding_->RegisterReceiveCodec(
> codec, [&] { return rent_a_codec_.RentIsacDecoder(); }) == -1 ||
> _rtpRtcpModule->RegisterSendPayload(codec) == -1) {
> WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
> "Channel::Init() failed to register CN (%d/%d) "
> "correctly - 1",
> codec.pltype, codec.plfreq);
> }
> }
>
> Because currently audio_coding_->SetEncoder is being called before
> audio_coding_->RegisterReceiveCodec. But we probably don't have to
> preserve the exact order of the calls.
(Continuing.) If we return the new encoder in an output arguenmt (and
allow the SetEncoder call to happen somewhat later than in the
original code, but that should be fine), we'd get something like this:
if (!STR_CASE_CMP(codec.plname, "CN")) {
std::unique_ptr<AudioEncoder> new_enc;
if (!codec_manager_.RegisterEncoder(codec) ||
!codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc) ||
audio_coding_->RegisterReceiveCodec(
codec, [&] { return rent_a_codec_.RentIsacDecoder(); }) == -1 ||
_rtpRtcpModule->RegisterSendPayload(codec) == -1) {
WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
"Channel::Init() failed to register CN (%d/%d) "
"correctly - 1",
codec.pltype, codec.plfreq);
}
if (new_enc) {
audio_coding_->SetEncoder(std::move(new_enc));
}
}
If we return a struct with a success flag and the new encoder, this
example becomes very convoluted, since we have to split it into
several separate statements. We also have to duplicate the failure
handling code, or put it in a subroutine.
Having thought this through in more detail, I can't say I regret going
for the lambda-based solution. It seems more complicated than the
others because C++ programmers still aren't familiar enough with
lambdas, but I'd argue that it's in fact less complicated because it's
easier to use correctly. I'm not opposed to going with the output
argument variant, though.
Wishful thingking: What I'd really like to use here is exceptions.
Then we'd get something like this:
if (!STR_CASE_CMP(codec.plname, "CN")) {
try {
codec_manager_.RegisterEncoder(codec);
auto new_enc = codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc);
if (new_enc) {
audio_coding_->SetEncoder(std::move(new_enc));
}
audio_coding_->RegisterReceiveCodec(
codec, [&] { return rent_a_codec_.RentIsacDecoder(); });
_rtpRtcpModule->RegisterSendPayload(codec);
} catch (const SmurfError& e) {
WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
"Channel::Init() failed to register CN (%d/%d) "
"correctly - 1",
codec.pltype, codec.plfreq);
}
}
On 2016/04/04 09:57:45, kwiberg-webrtc wrote:
> On 2016/04/04 09:28:24, ossu wrote:
> > On 2016/03/31 12:54:58, the sun wrote:
> > > On 2016/03/31 12:43:23, kwiberg-webrtc wrote:
> > > > On 2016/03/31 12:06:02, the sun wrote:
> > > > > Can you explain why we need to do this change?
> > > > >
> > > > > We want to start using the new ACM functionality in the WVoMC and
> > > > AudioNnStream
> > > > > layers, but voe::Channel is a dead end, so I'd rather not we touch it
> > unless
> > > > it
> > > > > is blocking other work.
> > > >
> > > > Well, the immediate reason is that if I don't update all callers, I
can't
> > > > deprecate the ACM methods that require the ACM to be able to create
> > encoders.
> > > > voe::Channel just happens to be the piece of code in Voice Engine that
> > > actually
> > > > calls the ACM at the moment.
> > >
> > > Ok, I need to take a closer look. The lambda code looks pretty complicated
> at
> > > first glance and I'm wondering if that is because I'm not that used to
> reading
> > > code using lambdas, or if it can be done simpler using old-fashioned
return
> > > codes (perhaps using the new-fashioned rtc::Optional...) :)
> >
> > I've got to agree with The Sun here. The MakeEncoder calls look much more
> > complicated than they need to be. Lambdas are nice and all but in this case,
I
> > think they convolute program flow unnecessarily. rtc::Optional seems like a
> > better choice to me, however it might be an issue that it default-constructs
> an
> > instance of what it's holding that causes it to be unusable for this right
> now.
> > Still, turning it into a tagged union-type thing where it's contents are
only
> > sometimes initialized shouldn't be too difficult.
>
> We don't need rtc::Optional here; the new encoder is already wrapped
> in std::unique_ptr, and a null value there will do nicely to indicate
> that there was no new encoder.
>
> Right now, calls look like this:
>
> if (!codec_manager_.MakeEncoder(
> &rent_a_codec_,
> [&](std::unique_ptr<AudioEncoder> enc) {
> do_something_with_the_encoder(std::move(enc));
> })) {
> handle_error();
> }
>
> If we continued to return a bool indicating success or failure, and
> returned the new encoder via an output argument, we'd get something
> like this:
>
> std::unique_ptr<AudioEncoder> new_enc;
> if (!codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc)) {
> handle_error();
> }
> if (new_enc) {
> do_something_with_the_encoder(std::move(new_enc));
> }
>
> Returning a struct with a bool and an encoder would look like this:
>
> auto new_enc = codec_manager_.MakeEncoder(&rent_a_codec_);
> if (!new_enc.success) {
> handle_error();
> } else if (new_enc.encoder) {
> do_something_with_the_encoder(std::move(new_enc.encoder));
> }
Sorry, I said "return codes" when I mean "return values".
How about:
std::unique_ptr<AudioEncoder> new_enc =
codec_manager_.MakeEncoder(&rent_a_codec_);
if (!new_enc) {
handle_error();
} else {
do_something_with_the_encoder(std::move(new_enc));
}
wdyt?
> There would be some complications with calls like this one:
>
> if (!STR_CASE_CMP(codec.plname, "CN")) {
> if (!codec_manager_.RegisterEncoder(codec) ||
> !codec_manager_.MakeEncoder(&rent_a_codec_,
> [&](std::unique_ptr<AudioEncoder> enc) {
> audio_coding_->SetEncoder(
> std::move(enc));
> }) ||
> audio_coding_->RegisterReceiveCodec(
> codec, [&] { return rent_a_codec_.RentIsacDecoder(); }) == -1 ||
> _rtpRtcpModule->RegisterSendPayload(codec) == -1) {
> WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
> "Channel::Init() failed to register CN (%d/%d) "
> "correctly - 1",
> codec.pltype, codec.plfreq);
> }
> }
>
> Because currently audio_coding_->SetEncoder is being called before
> audio_coding_->RegisterReceiveCodec. But we probably don't have to
> preserve the exact order of the calls.
I think that code is very convoluted. To start with it shouldn't be written as
one long conditional statement which makes it hard to read and debug (try
setting a breakpoint).
Now, as for the lambdas, I can see the use f the second one, in
RegisterReceiveCodec(), but there' s already an ACMImpl::RegisterReceiveCodec()
which doesn't take a callback parameter. Is that going away?
On 2016/04/04 11:33:13, the sun wrote:
> On 2016/04/04 09:57:45, kwiberg-webrtc wrote:
> > On 2016/04/04 09:28:24, ossu wrote:
> > > On 2016/03/31 12:54:58, the sun wrote:
> > > > On 2016/03/31 12:43:23, kwiberg-webrtc wrote:
> > > > > On 2016/03/31 12:06:02, the sun wrote:
> > > > > > Can you explain why we need to do this change?
> > > > > >
> > > > > > We want to start using the new ACM functionality in the WVoMC and
> > > > > AudioNnStream
> > > > > > layers, but voe::Channel is a dead end, so I'd rather not we touch
it
> > > unless
> > > > > it
> > > > > > is blocking other work.
> > > > >
> > > > > Well, the immediate reason is that if I don't update all callers, I
> can't
> > > > > deprecate the ACM methods that require the ACM to be able to create
> > > encoders.
> > > > > voe::Channel just happens to be the piece of code in Voice Engine that
> > > > actually
> > > > > calls the ACM at the moment.
> > > >
> > > > Ok, I need to take a closer look. The lambda code looks pretty
complicated
> > at
> > > > first glance and I'm wondering if that is because I'm not that used to
> > reading
> > > > code using lambdas, or if it can be done simpler using old-fashioned
> return
> > > > codes (perhaps using the new-fashioned rtc::Optional...) :)
> > >
> > > I've got to agree with The Sun here. The MakeEncoder calls look much more
> > > complicated than they need to be. Lambdas are nice and all but in this
case,
> I
> > > think they convolute program flow unnecessarily. rtc::Optional seems like
a
> > > better choice to me, however it might be an issue that it
default-constructs
> > an
> > > instance of what it's holding that causes it to be unusable for this right
> > now.
> > > Still, turning it into a tagged union-type thing where it's contents are
> only
> > > sometimes initialized shouldn't be too difficult.
> >
> > We don't need rtc::Optional here; the new encoder is already wrapped
> > in std::unique_ptr, and a null value there will do nicely to indicate
> > that there was no new encoder.
> >
> > Right now, calls look like this:
> >
> > if (!codec_manager_.MakeEncoder(
> > &rent_a_codec_,
> > [&](std::unique_ptr<AudioEncoder> enc) {
> > do_something_with_the_encoder(std::move(enc));
> > })) {
> > handle_error();
> > }
> >
> > If we continued to return a bool indicating success or failure, and
> > returned the new encoder via an output argument, we'd get something
> > like this:
> >
> > std::unique_ptr<AudioEncoder> new_enc;
> > if (!codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc)) {
> > handle_error();
> > }
> > if (new_enc) {
> > do_something_with_the_encoder(std::move(new_enc));
> > }
> >
> > Returning a struct with a bool and an encoder would look like this:
> >
> > auto new_enc = codec_manager_.MakeEncoder(&rent_a_codec_);
> > if (!new_enc.success) {
> > handle_error();
> > } else if (new_enc.encoder) {
> > do_something_with_the_encoder(std::move(new_enc.encoder));
> > }
>
> Sorry, I said "return codes" when I mean "return values".
>
> How about:
>
> std::unique_ptr<AudioEncoder> new_enc =
> codec_manager_.MakeEncoder(&rent_a_codec_);
> if (!new_enc) {
> handle_error();
> } else {
> do_something_with_the_encoder(std::move(new_enc));
> }
>
> wdyt?
>
> > There would be some complications with calls like this one:
> >
> > if (!STR_CASE_CMP(codec.plname, "CN")) {
> > if (!codec_manager_.RegisterEncoder(codec) ||
> > !codec_manager_.MakeEncoder(&rent_a_codec_,
> > [&](std::unique_ptr<AudioEncoder> enc) {
> > audio_coding_->SetEncoder(
> > std::move(enc));
> > }) ||
> > audio_coding_->RegisterReceiveCodec(
> > codec, [&] { return rent_a_codec_.RentIsacDecoder(); }) == -1 ||
> > _rtpRtcpModule->RegisterSendPayload(codec) == -1) {
> > WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId,
_channelId),
> > "Channel::Init() failed to register CN (%d/%d) "
> > "correctly - 1",
> > codec.pltype, codec.plfreq);
> > }
> > }
> >
> > Because currently audio_coding_->SetEncoder is being called before
> > audio_coding_->RegisterReceiveCodec. But we probably don't have to
> > preserve the exact order of the calls.
>
> I think that code is very convoluted. To start with it shouldn't be written as
> one long conditional statement which makes it hard to read and debug (try
> setting a breakpoint).
>
> Now, as for the lambdas, I can see the use f the second one, in
> RegisterReceiveCodec(), but there' s already an
ACMImpl::RegisterReceiveCodec()
> which doesn't take a callback parameter. Is that going away?
Oh, fail to update before submit... :)
On 2016/04/04 11:33:13, the sun wrote:
> On 2016/04/04 09:57:45, kwiberg-webrtc wrote:
> > On 2016/04/04 09:28:24, ossu wrote:
> > > On 2016/03/31 12:54:58, the sun wrote:
> > > > On 2016/03/31 12:43:23, kwiberg-webrtc wrote:
> > > > > On 2016/03/31 12:06:02, the sun wrote:
> > > > > > Can you explain why we need to do this change?
> > > > > >
> > > > > > We want to start using the new ACM functionality in the WVoMC and
> > > > > AudioNnStream
> > > > > > layers, but voe::Channel is a dead end, so I'd rather not we touch
it
> > > unless
> > > > > it
> > > > > > is blocking other work.
> > > > >
> > > > > Well, the immediate reason is that if I don't update all callers, I
> can't
> > > > > deprecate the ACM methods that require the ACM to be able to create
> > > encoders.
> > > > > voe::Channel just happens to be the piece of code in Voice Engine that
> > > > actually
> > > > > calls the ACM at the moment.
> > > >
> > > > Ok, I need to take a closer look. The lambda code looks pretty
complicated
> > at
> > > > first glance and I'm wondering if that is because I'm not that used to
> > reading
> > > > code using lambdas, or if it can be done simpler using old-fashioned
> return
> > > > codes (perhaps using the new-fashioned rtc::Optional...) :)
> > >
> > > I've got to agree with The Sun here. The MakeEncoder calls look much more
> > > complicated than they need to be. Lambdas are nice and all but in this
case,
> I
> > > think they convolute program flow unnecessarily. rtc::Optional seems like
a
> > > better choice to me, however it might be an issue that it
default-constructs
> > an
> > > instance of what it's holding that causes it to be unusable for this right
> > now.
> > > Still, turning it into a tagged union-type thing where it's contents are
> only
> > > sometimes initialized shouldn't be too difficult.
> >
> > We don't need rtc::Optional here; the new encoder is already wrapped
> > in std::unique_ptr, and a null value there will do nicely to indicate
> > that there was no new encoder.
> >
> > Right now, calls look like this:
> >
> > if (!codec_manager_.MakeEncoder(
> > &rent_a_codec_,
> > [&](std::unique_ptr<AudioEncoder> enc) {
> > do_something_with_the_encoder(std::move(enc));
> > })) {
> > handle_error();
> > }
> >
> > If we continued to return a bool indicating success or failure, and
> > returned the new encoder via an output argument, we'd get something
> > like this:
> >
> > std::unique_ptr<AudioEncoder> new_enc;
> > if (!codec_manager_.MakeEncoder(&rent_a_codec_, &new_enc)) {
> > handle_error();
> > }
> > if (new_enc) {
> > do_something_with_the_encoder(std::move(new_enc));
> > }
> >
> > Returning a struct with a bool and an encoder would look like this:
> >
> > auto new_enc = codec_manager_.MakeEncoder(&rent_a_codec_);
> > if (!new_enc.success) {
> > handle_error();
> > } else if (new_enc.encoder) {
> > do_something_with_the_encoder(std::move(new_enc.encoder));
> > }
>
> Sorry, I said "return codes" when I mean "return values".
>
> How about:
>
> std::unique_ptr<AudioEncoder> new_enc =
> codec_manager_.MakeEncoder(&rent_a_codec_);
> if (!new_enc) {
> handle_error();
> } else {
> do_something_with_the_encoder(std::move(new_enc));
> }
>
> wdyt?
You can fail to get a new encoder even if there is no error. This happens if we
haven't yet registered enough stuff to be able to construct a full encoder
stack, e.g. if we register CNG before speech encoder. Generalizing your
suggestion to this yields something like my "return struct" suggestion.
I like that approach in theory, but it's going to be a nuisance to shoe-horn it
into all the existing call sites. And the whole CodecManager class isn't
something that's going to be around in the long term---its job is to translate a
sequence of CodecInsts coming from VoiceEngine to new encoders, and we want
VoiceEngine to set the encoder directly instead.
> > There would be some complications with calls like this one:
> >
> > if (!STR_CASE_CMP(codec.plname, "CN")) {
> > if (!codec_manager_.RegisterEncoder(codec) ||
> > !codec_manager_.MakeEncoder(&rent_a_codec_,
> > [&](std::unique_ptr<AudioEncoder> enc) {
> > audio_coding_->SetEncoder(
> > std::move(enc));
> > }) ||
> > audio_coding_->RegisterReceiveCodec(
> > codec, [&] { return rent_a_codec_.RentIsacDecoder(); }) == -1 ||
> > _rtpRtcpModule->RegisterSendPayload(codec) == -1) {
> > WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId,
_channelId),
> > "Channel::Init() failed to register CN (%d/%d) "
> > "correctly - 1",
> > codec.pltype, codec.plfreq);
> > }
> > }
> >
> > Because currently audio_coding_->SetEncoder is being called before
> > audio_coding_->RegisterReceiveCodec. But we probably don't have to
> > preserve the exact order of the calls.
>
> I think that code is very convoluted. To start with it shouldn't be written as
> one long conditional statement which makes it hard to read and debug (try
> setting a breakpoint).
On the one hand, I agree. On the other hand, writing it like you suggest is much
more verbose, especially because we end up having to duplicate the error
handling code (or put it into a subroutine).
> Now, as for the lambdas, I can see the use f the second one, in
> RegisterReceiveCodec(),
Yes, that one is just a factory. I wasn't considering getting rid of it.
> but there' s already an ACMImpl::RegisterReceiveCodec()
> which doesn't take a callback parameter. Is that going away?
Yes. At the end of my queue is a CL that deprecates a bunch of ACM methods that
will have become unused, including that one.
Thanks for explaining. I have another suggestion that I'd like you to consider. https://codereview.webrtc.org/1677013002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:394: if (audio_coding_->RegisterReceiveCodec(receiveCodec, [&] { Make this a local utility. I appreciate that separating RAC from ACM will be good, but don't think we need to do this much typing. https://codereview.webrtc.org/1677013002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:983: !codec_manager_.MakeEncoder(&rent_a_codec_, One thing that struct me with this method is that it takes a RAC*. In such cases, when a function belongs to several classes, it is often a good idea to put it in *neither* - pull it out into a separate utility function instead, which sort of pulls the classes together. Could we do that in this case? Incidentally we'd have the opportunity to make the code less verbose at the same time - the utility could take a CodecManager*, RAC* and ACM*. Do we really need MakeEncoder to live in CodecManager? Could we have a local function here instead?
https://codereview.webrtc.org/1677013002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:394: if (audio_coding_->RegisterReceiveCodec(receiveCodec, [&] { On 2016/04/04 12:21:35, the sun wrote: > Make this a local utility. I appreciate that separating RAC from ACM will be > good, but don't think we need to do this much typing. Do you mean a three-line method to call audio_coding_->RegisterReceiveCodec with a given receive codec? I chose not to do that because the number of saved lines (~5 call sites @ ~2 lines apiece) isn't large compared to the cognitive overhead an extra level of indirection imposes. But if you think it would improve readability I could give it a try. https://codereview.webrtc.org/1677013002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:983: !codec_manager_.MakeEncoder(&rent_a_codec_, On 2016/04/04 12:21:35, the sun wrote: > One thing that struct me :-) > with this method is that it takes a RAC*. In such cases, when a > function belongs to several classes, it is often a good idea to put > it in *neither* - pull it out into a separate utility function > instead, which sort of pulls the classes together. Could we do that > in this case? Yes, MakeEncoder could be a separate function. I don't see exactly what we'd gain by doing that, though. > Incidentally we'd have the opportunity to make the code less verbose > at the same time - the utility could take a CodecManager*, RAC* and > ACM*. Aha! That's a splendid idea! I just checked, and all MakeEncoder callers do exactly the same thing in their lambda function: call SetEncoder on an ACM.
Patchset #3 (id:100001) has been deleted
Fredrik: I've tried to implement your suggestions. Åsa: Please review webrtc/modules/utility/.
Much nicer, thank you! LGTM, but consider the comments. https://codereview.webrtc.org/1677013002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/codec_manager.h (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/codec_manager.h:62: bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm) { In a big fan of DCHECKing pointers before I use them. In this case it looks like you should do it at the beginning of this function. https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2870: if (!codec_manager_.SetCopyRed(enable) || Nit: should this be RED, not Red?
https://codereview.webrtc.org/1677013002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/codec_manager.h (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/codec_manager.h:62: bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm) { On 2016/04/06 11:58:25, the sun wrote: > In a big fan of DCHECKing pointers before I use them. In this case it looks like > you should do it at the beginning of this function. I'm on the other side in this debate: since dereferencing a null pointer and trying to use the result is guaranteed to cause a nice and clean crash, I feel it isn't worth cluttering the code with [D]CHECKs for pointer nullness most of the time. If a function has a lot of callers or if the place where dereferencing would cause a crash is far away it might still be worth it, but neither of those is the case here. Ideally, we'd use mutable references here to make it difficult for callers to pass null pointers, but style guide. https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2870: if (!codec_manager_.SetCopyRed(enable) || On 2016/04/06 11:58:25, the sun wrote: > Nit: should this be RED, not Red? Style guide says: "Prefer to capitalize acronyms as single words (i.e. StartRpc(), not StartRPC())." https://google.github.io/styleguide/cppguide.html#Function_Names
still lgtm https://codereview.webrtc.org/1677013002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/codec_manager.h (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/codec_manager.h:62: bool MakeEncoder(RentACodec* rac, AudioCodingModule* acm) { On 2016/04/06 12:15:29, kwiberg-webrtc wrote: > On 2016/04/06 11:58:25, the sun wrote: > > In a big fan of DCHECKing pointers before I use them. In this case it looks > like > > you should do it at the beginning of this function. > > I'm on the other side in this debate: since dereferencing a null pointer and > trying to use the result is guaranteed to cause a nice and clean crash, I feel > it isn't worth cluttering the code with [D]CHECKs for pointer nullness most of > the time. If a function has a lot of callers or if the place where dereferencing > would cause a crash is far away it might still be worth it, but neither of those > is the case here. The real reason for the DCHECKs is to document expectations. If you don't do that, and there's a crash at a later point, the developer fixing the issue may look at the code, going "oh, someone forgot to check for the pointer being null, I'll add an early out in that case", instead of "oh, this function expects the pointer argument to always be valid, I'll look at what the caller is doing wrong". > Ideally, we'd use mutable references here to make it difficult for callers to > pass null pointers, but style guide. https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2870: if (!codec_manager_.SetCopyRed(enable) || On 2016/04/06 12:15:29, kwiberg-webrtc wrote: > On 2016/04/06 11:58:25, the sun wrote: > > Nit: should this be RED, not Red? > > Style guide says: "Prefer to capitalize acronyms as single words (i.e. > StartRpc(), not StartRPC())." > > https://google.github.io/styleguide/cppguide.html#Function_Names Thanks for looking that up for me! :) Ok, then it is the other functions that should be renamed instead...
lgtm
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, solenberg@webrtc.org, asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/1677013002/#ps160001 (title: "DCHECKs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677013002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677013002/160001
https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1677013002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2870: if (!codec_manager_.SetCopyRed(enable) || On 2016/04/06 13:05:59, the sun wrote: > On 2016/04/06 12:15:29, kwiberg-webrtc wrote: > > On 2016/04/06 11:58:25, the sun wrote: > > > Nit: should this be RED, not Red? > > > > Style guide says: "Prefer to capitalize acronyms as single words (i.e. > > StartRpc(), not StartRPC())." > > > > https://google.github.io/styleguide/cppguide.html#Function_Names > > Thanks for looking that up for me! :) > > Ok, then it is the other functions that should be renamed instead... It just says you should prefer it, not that you must always get what you want. :-) Seriously though, renaming them shouldn't be done in this CL, and since the entire class is scheduled for a grisly end, I'd rather postpone the rename until after that.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677013002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677013002/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Switch to using new ACM methods for encoder management BUG=webrtc:5028 ========== to ========== Switch to using new ACM methods for encoder management BUG=webrtc:5028 Committed: https://crrev.com/c8d071e4e074934ef8346fc91ad4f24ae333afee Cr-Commit-Position: refs/heads/master@{#12267} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c8d071e4e074934ef8346fc91ad4f24ae333afee Cr-Commit-Position: refs/heads/master@{#12267} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
