Removed double-special-casing of ISAC in libjingle and WebRtcVoE.

Removed double-special-casing of ISAC in libjingle and WebRtcVoE.

webrtcvoiceengine.cc ensured that if the bitrate set for ISAC was 0,
it was changed to -1 so that the codec could manage the bitrate
itself.

webrtcsdp.cc ensured that if the bitrate set for ISAC was 0, it was
explicitly set to default values to avoid the codec's built in bitrate
management.

Eventually, there'll be no codec specific code like this in these
layers. This is one step towards that goal.

BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2642923003
Cr-Commit-Position: refs/heads/master@{#16220}
Committed: https://chromium.googlesource.com/external/webrtc/+/e1405ad0d164230c77c4ac10efd92db3ef2f9030

[ossu, 2017-01-19 16:45:03]

The CQ bit was checked by ossu@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 16:45:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ossu, 2017-01-19 16:46:22]

Description was changed from

==========
Removed double-special-casing of ISAC in libjingle and WebRtcVoE.

webrtcvoiceengine.cc ensured that if the bitrate set for ISAC was 0,
it was changed to -1 so that the codec could manage the bitrate
itself.

webrtcsdp.cc ensured that if the bitrate set for ISAC was 0, it was
explicitly set to default values to avoid the codec's built in bitrate
management.

Eventually, there'll be no codec specific code like this in these
layers. This is one step towards that goal.

BUG=webrtc:5806
==========

to

==========
Removed double-special-casing of ISAC in libjingle and WebRtcVoE.

webrtcvoiceengine.cc ensured that if the bitrate set for ISAC was 0,
it was changed to -1 so that the codec could manage the bitrate
itself.

webrtcsdp.cc ensured that if the bitrate set for ISAC was 0, it was
explicitly set to default values to avoid the codec's built in bitrate
management.

Eventually, there'll be no codec specific code like this in these
layers. This is one step towards that goal.

BUG=webrtc:5806
==========

[ossu, 2017-01-19 16:46:22]

ossu@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, kwiberg@webrtc.org

[ossu, 2017-01-19 16:48:48]

This removes two blocks of codec specific code that seem to be there only to
out-manoeuvre each other. ISAC bitrates will now be picked directly from the
values in acm_common_defs.h, rather than having to replicate them in
webrtcsdp.cc.

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:376: // If ISAC and an explicit bitrate
is not specified,
After removing the rate setting code here and in webrtcsdp.cc, I ran through
*_tests and *_unittests with a CHECK here, that ensured voe_codec.rate was never
zero. It didn't trigger.
I expect zero is not a usable value for ISAC, since we previously had to change
it to -1. (This is, in it self, a bit strange).

[Taylor Brandstetter, 2017-01-19 17:05:09]

lgtm

[commit-bot: I haz the power, 2017-01-19 17:58:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 17:58:23]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-01-23 15:18:11]

https://codereview.webrtc.org/2642923003/diff/1/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (left):

https://codereview.webrtc.org/2642923003/diff/1/webrtc/api/webrtcsdp.cc#oldco...
webrtc/api/webrtcsdp.cc:3106: }
Hmm. I don't quite see how this piece of code didn't have any effect. Doesn't it
set the bitrate to one of two constants, a value which would be passed down to
the encoder without being changed along the way?

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:376: // If ISAC and an explicit bitrate
is not specified,
On 2017/01/19 16:48:48, ossu wrote:
> After removing the rate setting code here and in webrtcsdp.cc, I ran through
> *_tests and *_unittests with a CHECK here, that ensured voe_codec.rate was
never
> zero. It didn't trigger.
> I expect zero is not a usable value for ISAC, since we previously had to
change
> it to -1. (This is, in it self, a bit strange).

So in other words, in.bitrate was always -1 or >=1 here, causing the deleted
code to have no effect?

[ossu, 2017-01-23 15:33:18]

https://codereview.webrtc.org/2642923003/diff/1/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (left):

https://codereview.webrtc.org/2642923003/diff/1/webrtc/api/webrtcsdp.cc#oldco...
webrtc/api/webrtcsdp.cc:3106: }
On 2017/01/23 15:18:11, kwiberg-webrtc wrote:
> Hmm. I don't quite see how this piece of code didn't have any effect. Doesn't
it
> set the bitrate to one of two constants, a value which would be passed down to
> the encoder without being changed along the way?

It did have an effect. It worked around the block of code I removed from
webrtcvoiceengine.cc. These two constants are already set in the ACMCodecDB.

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:376: // If ISAC and an explicit bitrate
is not specified,
On 2017/01/23 15:18:11, kwiberg-webrtc wrote:
> On 2017/01/19 16:48:48, ossu wrote:
> > After removing the rate setting code here and in webrtcsdp.cc, I ran through
> > *_tests and *_unittests with a CHECK here, that ensured voe_codec.rate was
> never
> > zero. It didn't trigger.
> > I expect zero is not a usable value for ISAC, since we previously had to
> change
> > it to -1. (This is, in it self, a bit strange).
> 
> So in other words, in.bitrate was always -1 or >=1 here, causing the deleted
> code to have no effect?

Yes, the block in webrtcsdp.cc ensured that we didn't set bitrate to -1. It set
in.bitrate to match the values that voe_codec.rate would already have been set
to by line 368, above.

[kwiberg-webrtc, 2017-01-23 15:38:41]

OK, I *think* I understand... probably...

lgtm

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:376: // If ISAC and an explicit bitrate
is not specified,
On 2017/01/23 15:33:18, ossu wrote:
> On 2017/01/23 15:18:11, kwiberg-webrtc wrote:
> > On 2017/01/19 16:48:48, ossu wrote:
> > > After removing the rate setting code here and in webrtcsdp.cc, I ran
through
> > > *_tests and *_unittests with a CHECK here, that ensured voe_codec.rate was
> > never
> > > zero. It didn't trigger.
> > > I expect zero is not a usable value for ISAC, since we previously had to
> > change
> > > it to -1. (This is, in it self, a bit strange).
> > 
> > So in other words, in.bitrate was always -1 or >=1 here, causing the deleted
> > code to have no effect?
> 
> Yes, the block in webrtcsdp.cc ensured that we didn't set bitrate to -1. It
set
> in.bitrate to match the values that voe_codec.rate would already have been set
> to by line 368, above.

Acknowledged.

[ossu, 2017-01-23 15:40:35]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2017-01-23 15:40:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-23 15:42:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 15:42:10]

Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16725)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/20722)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/21211)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/21992)

[ossu, 2017-01-23 15:47:35]

On 2017/01/23 15:38:41, kwiberg-webrtc wrote:
> OK, I *think* I understand... probably...
> 
> lgtm
> 
>
https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
> File webrtc/media/engine/webrtcvoiceengine.cc (left):
> 
>
https://codereview.webrtc.org/2642923003/diff/1/webrtc/media/engine/webrtcvoi...
> webrtc/media/engine/webrtcvoiceengine.cc:376: // If ISAC and an explicit
bitrate
> is not specified,
> On 2017/01/23 15:33:18, ossu wrote:
> > On 2017/01/23 15:18:11, kwiberg-webrtc wrote:
> > > On 2017/01/19 16:48:48, ossu wrote:
> > > > After removing the rate setting code here and in webrtcsdp.cc, I ran
> through
> > > > *_tests and *_unittests with a CHECK here, that ensured voe_codec.rate
was
> > > never
> > > > zero. It didn't trigger.
> > > > I expect zero is not a usable value for ISAC, since we previously had to
> > > change
> > > > it to -1. (This is, in it self, a bit strange).
> > > 
> > > So in other words, in.bitrate was always -1 or >=1 here, causing the
deleted
> > > code to have no effect?
> > 
> > Yes, the block in webrtcsdp.cc ensured that we didn't set bitrate to -1. It
> set
> > in.bitrate to match the values that voe_codec.rate would already have been
set
> > to by line 368, above.
> 
> Acknowledged.

Yeah, it's not two separate special cases, but two that essentially cancel out.
I'll walk you through it when you get back. :)

[ossu, 2017-01-23 15:47:44]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2017-01-23 15:47:44]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2642923003/#ps20001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-01-23 15:47:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-23 16:55:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485186464145070,
"parent_rev": "cb893ee6346f27e7e660578c802bbe455fcf9102", "commit_rev":
"e1405ad0d164230c77c4ac10efd92db3ef2f9030"}

[commit-bot: I haz the power, 2017-01-23 16:55:51]

Description was changed from

==========
Removed double-special-casing of ISAC in libjingle and WebRtcVoE.

webrtcvoiceengine.cc ensured that if the bitrate set for ISAC was 0,
it was changed to -1 so that the codec could manage the bitrate
itself.

webrtcsdp.cc ensured that if the bitrate set for ISAC was 0, it was
explicitly set to default values to avoid the codec's built in bitrate
management.

Eventually, there'll be no codec specific code like this in these
layers. This is one step towards that goal.

BUG=webrtc:5806
==========

to

==========
Removed double-special-casing of ISAC in libjingle and WebRtcVoE.

webrtcvoiceengine.cc ensured that if the bitrate set for ISAC was 0,
it was changed to -1 so that the codec could manage the bitrate
itself.

webrtcsdp.cc ensured that if the bitrate set for ISAC was 0, it was
explicitly set to default values to avoid the codec's built in bitrate
management.

Eventually, there'll be no codec specific code like this in these
layers. This is one step towards that goal.

BUG=webrtc:5806

Review-Url: https://codereview.webrtc.org/2642923003
Cr-Commit-Position: refs/heads/master@{#16220}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e1405ad0d164230c77c4ac10e...
==========

[commit-bot: I haz the power, 2017-01-23 16:55:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/e1405ad0d164230c77c4ac10e...