Don't recreate the speech encoder if we don't have to

Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647
Committed: https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b
Cr-Commit-Position: refs/heads/master@{#13273}

[kwiberg-webrtc, 2016-06-22 13:46:11]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, ossu@webrtc.org

[kwiberg-webrtc, 2016-06-22 13:46:11]

kwiberg@webrtc.org changed required reviewers:
+ henrik.lundin@webrtc.org

[kwiberg-webrtc, 2016-06-22 13:46:12]

This CL fixes bug 6020. Reaching in and taking back the speech encoder from the
bottom of the encoder stack isn't pretty, though.

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:159:
ReclaimContainedEncoders();
This method is something of a hack. Hopefully, we'll be able to get rid of it
when we implement the prophesied encoder factory (since we should then set all
encoder properties at creation, and thus will lose no config state by recreating
encoders, which we should not end up doing anyway because we don't set a bunch
of properties after the fact).

[minyue-webrtc, 2016-06-22 15:47:34]

On 2016/06/22 13:46:12, kwiberg-webrtc wrote:
> This CL fixes bug 6020. Reaching in and taking back the speech encoder from
the
> bottom of the encoder stack isn't pretty, though.
> 
>
https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/c...
> File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):
> 
>
https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/c...
> webrtc/modules/audio_coding/codecs/audio_encoder.h:159:
> ReclaimContainedEncoders();
> This method is something of a hack. Hopefully, we'll be able to get rid of it
> when we implement the prophesied encoder factory (since we should then set all
> encoder properties at creation, and thus will lose no config state by
recreating
> encoders, which we should not end up doing anyway because we don't set a bunch
> of properties after the fact).

Great job! Have you tested it on Chromium?

[kwiberg-webrtc, 2016-06-22 15:53:16]

On 2016/06/22 15:47:34, minyue-webrtc wrote:
> On 2016/06/22 13:46:12, kwiberg-webrtc wrote:
> > This CL fixes bug 6020. Reaching in and taking back the speech encoder from
> the
> > bottom of the encoder stack isn't pretty, though.
> > 
> >
>
https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/c...
> > File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):
> > 
> >
>
https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/c...
> > webrtc/modules/audio_coding/codecs/audio_encoder.h:159:
> > ReclaimContainedEncoders();
> > This method is something of a hack. Hopefully, we'll be able to get rid of
it
> > when we implement the prophesied encoder factory (since we should then set
all
> > encoder properties at creation, and thus will lose no config state by
> recreating
> > encoders, which we should not end up doing anyway because we don't set a
bunch
> > of properties after the fact).
> 
> Great job! Have you tested it on Chromium?

Yes.

[hlundin-webrtc, 2016-06-23 07:46:37]

LG. One question, though, repeated twice.

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:242: return true;
Should recreate_encoder_ be modified here?

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:248: return false;
.. and here?

[ossu, 2016-06-23 08:32:07]

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:197: if (!recreate_encoder_) {
The flow of this whole function is pretty difficult to follow.
I realize this is a "temporary" bug fix until, but I'm a bit afraid it will keep
for longer than we expect. Would it be possible to hold the speech encoder
separate from the rest of the stack (through, say, a scoped_refptr in the ACM
and then in whatever encoder stack that uses it) so that we could just reuse it
directly, rather than having to peel back the layers of the encoder stack onion
to get to it?

[kwiberg-webrtc, 2016-06-23 10:31:51]

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:197: if (!recreate_encoder_) {
On 2016/06/23 08:32:07, ossu wrote:
> The flow of this whole function is pretty difficult to follow.
> I realize this is a "temporary" bug fix until, but I'm a bit afraid it will
keep
> for longer than we expect. Would it be possible to hold the speech encoder
> separate from the rest of the stack (through, say, a scoped_refptr in the ACM
> and then in whatever encoder stack that uses it) so that we could just reuse
it
> directly, rather than having to peel back the layers of the encoder stack
onion
> to get to it?

Hmm. Yes, that's possible. We change the cng and red encoders to hold either a
raw (non-owning) pointer to the speech encoder, or some sort of shared pointer,
and have the ACM (co-)own the speech encoder; we can then easily make an
accessor for pulling it out again. That's arguably less bad than the solution I
have right now.

However, this means nontrivial modifications to the cng and red encoders, and to
this code. That isn't compatible with getting this bug fixed quickly enough.

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:242: return true;
On 2016/06/23 07:46:36, hlundin-webrtc wrote:
> Should recreate_encoder_ be modified here?

No. It's currently true, and should be true because we should recreate the
encoder as soon as possible. (Alternatively, it doesn't matter what value it
has, because we never set send_codec_inst_ without simultaneously setting
recreate_encoder_ to true.)

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:248: return false;
On 2016/06/23 07:46:36, hlundin-webrtc wrote:
> .. and here?

Same reasoning as above. We should only set it on line 251, which is the first
point where we know that we successfully created an encoder and injected it into
the ACM.

[ossu, 2016-06-23 10:34:50]

> Hmm. Yes, that's possible. We change the cng and red encoders to hold either a
> raw (non-owning) pointer to the speech encoder, or some sort of shared
pointer,
> and have the ACM (co-)own the speech encoder; we can then easily make an
> accessor for pulling it out again. That's arguably less bad than the solution
I
> have right now.
> 
> However, this means nontrivial modifications to the cng and red encoders, and
to
> this code. That isn't compatible with getting this bug fixed quickly enough.

Yeah, let's get this out the door for now. Just promise it works as intended,
ok! :)

[hlundin-webrtc, 2016-06-23 10:38:47]

lgtm

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/codec_manager.cc (right):

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:197: if (!recreate_encoder_) {
On 2016/06/23 10:31:51, kwiberg-webrtc wrote:
> On 2016/06/23 08:32:07, ossu wrote:
> > The flow of this whole function is pretty difficult to follow.
> > I realize this is a "temporary" bug fix until, but I'm a bit afraid it will
> keep
> > for longer than we expect. Would it be possible to hold the speech encoder
> > separate from the rest of the stack (through, say, a scoped_refptr in the
ACM
> > and then in whatever encoder stack that uses it) so that we could just reuse
> it
> > directly, rather than having to peel back the layers of the encoder stack
> onion
> > to get to it?
> 
> Hmm. Yes, that's possible. We change the cng and red encoders to hold either a
> raw (non-owning) pointer to the speech encoder, or some sort of shared
pointer,
> and have the ACM (co-)own the speech encoder; we can then easily make an
> accessor for pulling it out again. That's arguably less bad than the solution
I
> have right now.
> 
> However, this means nontrivial modifications to the cng and red encoders, and
to
> this code. That isn't compatible with getting this bug fixed quickly enough.

Speed is of importance here.

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:242: return true;
On 2016/06/23 10:31:51, kwiberg-webrtc wrote:
> On 2016/06/23 07:46:36, hlundin-webrtc wrote:
> > Should recreate_encoder_ be modified here?
> 
> No. It's currently true, and should be true because we should recreate the
> encoder as soon as possible. (Alternatively, it doesn't matter what value it
> has, because we never set send_codec_inst_ without simultaneously setting
> recreate_encoder_ to true.)

Acknowledged.

https://codereview.webrtc.org/2089183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/codec_manager.cc:248: return false;
On 2016/06/23 10:31:51, kwiberg-webrtc wrote:
> On 2016/06/23 07:46:36, hlundin-webrtc wrote:
> > .. and here?
> 
> Same reasoning as above. We should only set it on line 251, which is the first
> point where we know that we successfully created an encoder and injected it
into
> the ACM.

Acknowledged.

[kwiberg-webrtc, 2016-06-23 10:56:39]

Description was changed from

==========
Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020
==========

to

==========
Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647
==========

[kwiberg-webrtc, 2016-06-23 10:56:59]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-06-23 10:57:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089183002/1

[commit-bot: I haz the power, 2016-06-23 10:58:39]

Description was changed from

==========
Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647
==========

to

==========
Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647
==========

[commit-bot: I haz the power, 2016-06-23 10:58:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-23 10:58:48]

Description was changed from

==========
Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647
==========

to

==========
Don't recreate the speech encoder if we don't have to

If the specification for the speech encoder hasn't changed, we should
reuse it instead of recreating it. Otherwise, we lose its state. (This
problem was originally discovered because AudioEncoderOpus instances
would forget that they were supposed to be using DTX.)

BUG=webrtc:6020, chromium:622647

Committed: https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b
Cr-Commit-Position: refs/heads/master@{#13273}
==========

[commit-bot: I haz the power, 2016-06-23 10:58:49]

Patchset 1 (id:??) landed as
https://crrev.com/3f81fcd2e845505c2873c7ec2edade910213596b
Cr-Commit-Position: refs/heads/master@{#13273}