iSAC: Functions for importing and exporting bandwidth est. info

iSAC: Functions for importing and exporting bandwidth est. info

They make it possible to send bandwidth estimation info from decoder
to encoder even if they are separate objects (which we want them to be
because multithreading).

R=henrik.lundin@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/2224294c52fc0bb95eedb8187a62bedf1b5eb853

[kwiberg-webrtc, 2015-06-25 14:47:41]

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

[kwiberg-webrtc, 2015-06-25 14:52:44]

To be followed by another CL that uses the new functions to turn the iSAC
AudioEncoder and AudioDecoder into separate objects.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:146: 1, send_time,
receive_time));
If I've understood correctly, this means that each packet is delivered 6 ms late
(and not, as I first assumed, that the packets spent 6 ms in flight), causing
the bw estimator to successively lower the bitrate until it reaches the lowest
allowed value.

[hlundin-webrtc, 2015-06-26 10:35:15]

Nice. Mostly LG, but please see comments.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/bandwidth_info.h (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/bandwidth_info.h:1: /*
Interesting base revision you got for this one... :)

(You probably already know, but you can tweak the --similarity parameter when
doing git cl upload to avoid these weird connections.)

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/fix/interface/isacfix.h (left):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/fix/interface/isacfix.h:15: * Define the
fixpoint numeric formats
Good riddance!

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/fix/source/bandwidth_estimator.c
(right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/fix/source/bandwidth_estimator.c:808: if
(val <= min)
Consider
return val <= min ? min : (val >= max ? max : val);
Optional....

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.c
(right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.c:763:
if (val <= min)
Same here: consider one-liner.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:26: std::vector<int16_t>
LoadSpeechData() {
Consider using webrtc::test::InputAudioFile instead. (It is under
audio_coding/neteq/tools and should probably be moved elsewhere, but that is for
another time.)

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:51: std::string Str(const
IsacBandwidthInfo& bi) {
The function name is a bit too compact. Consider ToString or BwInfoToString.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:93: int encoded_bytes =
T::Encode(inst, speech_data, output.data());
What if encoded_bytes is too large for output?

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:98: CHECK_EQ(0,
encoded_bytes);
CHECK is quite aggressive here. Consider using EXPECT instead.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:106: EXPECT_EQ(0,
T::Create(&encdec));
All the create and init calls should be ASSERT_EQ instead of EXPECT_EQ. It makes
little sense to continue with the test if they fail.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:146: 1, send_time,
receive_time));
On 2015/06/25 14:52:43, kwiberg1 wrote:
> If I've understood correctly, this means that each packet is delivered 6 ms
late
> (and not, as I first assumed, that the packets spent 6 ms in flight), causing
> the bw estimator to successively lower the bitrate until it reaches the lowest
> allowed value.

I'm not so sure. The receive time should just be the local wall-clock, but with
the same frequency as the RTP timestamps (i.e., the send time). What you have is
just a constant offset between send_time and receive_time, which will likely not
trigger any BW actions. You will probably have to _increase_ the
offset for each packet in the loop to get some action. E.g.,

receive_time = last_receive_time + kSamplesPerPacket * 1.1;

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:150:
EXPECT_EQ(CountRange(send_bw_avg, std::numeric_limits<int>::min(), 10000), 0);
EXPECT_EQ should have swapped parameters; expected value first. Four instances
in total.

[kwiberg-webrtc, 2015-06-27 23:43:44]

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:146: 1, send_time,
receive_time));
On 2015/06/26 10:35:15, hlundin-webrtc wrote:
> On 2015/06/25 14:52:43, kwiberg1 wrote:
> > If I've understood correctly, this means that each packet is delivered 6 ms
> late
> > (and not, as I first assumed, that the packets spent 6 ms in flight),
causing
> > the bw estimator to successively lower the bitrate until it reaches the
lowest
> > allowed value.
> 
> I'm not so sure. The receive time should just be the local wall-clock, but
with
> the same frequency as the RTP timestamps (i.e., the send time). What you have
is
> just a constant offset between send_time and receive_time, which will likely
not
> trigger any BW actions. You will probably have to _increase_ the
> offset for each packet in the loop to get some action. E.g.,
> 
> receive_time = last_receive_time + kSamplesPerPacket * 1.1;

But I do get some action. First it keeps the bitrate at 20k for a dozen packets,
then it appears to decide corrective action is needed and gradually lowers it
(over a period of about 200 packets) until it reaches 10k (the minimum allowed
value), and then it stays there. My guess that it's seeing the 6 ms
send-to-receive difference as a delay that should be corrected is based on this
behavior.

[kwiberg-webrtc, 2015-06-28 03:17:42]

The changes discussed below are patch set #2.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/fix/source/bandwidth_estimator.c
(right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/fix/source/bandwidth_estimator.c:808: if
(val <= min)
On 2015/06/26 10:35:14, hlundin-webrtc wrote:
> Consider
> return val <= min ? min : (val >= max ? max : val);
> Optional....

Done.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.c
(right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.c:763:
if (val <= min)
On 2015/06/26 10:35:14, hlundin-webrtc wrote:
> Same here: consider one-liner.

Done.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:26: std::vector<int16_t>
LoadSpeechData() {
On 2015/06/26 10:35:15, hlundin-webrtc wrote:
> Consider using webrtc::test::InputAudioFile instead. (It is under
> audio_coding/neteq/tools and should probably be moved elsewhere, but that is
for
> another time.)

Done.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:51: std::string Str(const
IsacBandwidthInfo& bi) {
On 2015/06/26 10:35:14, hlundin-webrtc wrote:
> The function name is a bit too compact. Consider ToString or BwInfoToString.

Done.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:93: int encoded_bytes =
T::Encode(inst, speech_data, output.data());
On 2015/06/26 10:35:15, hlundin-webrtc wrote:
> What if encoded_bytes is too large for output?

It shouldn't be. And some sanitizer bot would complain. But I'll CHECK it.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:98: CHECK_EQ(0,
encoded_bytes);
On 2015/06/26 10:35:14, hlundin-webrtc wrote:
> CHECK is quite aggressive here. Consider using EXPECT instead.

OK. Although I sort of doubt the rest of the run will be useful if we fail here.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:106: EXPECT_EQ(0,
T::Create(&encdec));
On 2015/06/26 10:35:15, hlundin-webrtc wrote:
> All the create and init calls should be ASSERT_EQ instead of EXPECT_EQ. It
makes
> little sense to continue with the test if they fail.

Done.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:150:
EXPECT_EQ(CountRange(send_bw_avg, std::numeric_limits<int>::min(), 10000), 0);
On 2015/06/26 10:35:15, hlundin-webrtc wrote:
> EXPECT_EQ should have swapped parameters; expected value first. Four instances
> in total.

Done. For symmetry, I also swapped the EXPECT_GEs.

[MegaMyFunLiFe, 2015-06-28 05:41:30]

megamyfunlife@gmail.com changed reviewers:
+ MegaMyFunLiFe@gmail.com

[MegaMyFunLiFe, 2015-06-28 05:41:30]

Fully redundant, fault-tolerant, reliable name servers
Easy-to-use interface
Instant updates
Low TTLs, for when your details change often
Wildcard domain names
No setup fees, contracts, or commitments
Any DNS record type
Dynamic DNS
DNS API
Round robin DNS?
No need to transfer your domain name registrar
Integrates with Pingability's DNS fail over
Payment options
Vanity name servers
Fully redundant, fault-tolerant, reliable name servers
Many domain owners run their own DNS, on the server their DNS points to.  If
that server goes down, it then becomes very time consuming to change your DNS to
point anywhere else.  So you should always run your DNS on a different server
than the one to which it points to.  The Zonomi name servers are spread around
the globe (London, Dallas, New York and Auckland). Making it tolerant to network
or power failures in any one particular data center.
Easy-to-use interface
Used by thousands of users on over 30 thousand domains.
Instant updates
Our name servers are powered by PowerDNS, a very popular and powerful DNS
product. Rather than 'flat files', it can use a database back end.  Making it
very efficient, capable of handling a huge number of domains and responding
instantly to DNS lookup queries.  We use database replication to propagate DNS
record changes to all our name servers instantly.
Low TTLs, for when your details change often
Time-to-live (TTL) settings control how long a client will cache your DNS
records.  Lower TTL values will enable you to change your IP addresses more
quickly.  This is essential if you are using techniques like DNS failover.  Or
if you are using a dynamic IP (e.g.  a IP dynamically assigned to you by your
ISP) to host public service (like a website)with a domain name.
Wildcard domain names
You can add in 'wildcard' records.  Like *.example.com so that any sub-domain in
your domain zone can have an IP address.
No setup fees, contracts, or commitments
Sign up.  Use the service.  No obligation.  Get free DNS hosting for a few
domains.  Only provide credit card details or pay any money if you need one of
our higher end professional DNS hosting plans.
Any DNS record type
Enter whatever records types you need: A, AAAA, MX, CNAME, PTR, SRV, TXT (e.g. 
SPF). You name it.
Dynamic DNS
Use a basic HTTP GET request to update your domain's IP address.
DNS API
Running a hosting company? Or need to automate the creation of new zones or
updating of DNS records? We provide an easy to use, powerful API to automate API
updates.
Round robin DNS?
No problem.  We support that.  You can add multiple IP addresses for any domain
name.
No need to transfer your domain name registrar
Keep your current domain name registrar.  Just tell your registrar to set
Zonomi's name servers for your domains.
Integrates with Pingability's DNS fail over
Pingability's server monitoring service integrates with Zonomi so if a server
fails it can automatically change your DNS to point to a different server.
Payment options
We accept credit card payments.  A minimum 5.00 USD payment applies.
We accept Paypal payments as well.
We accept wire transfer payments for amounts over $300 (contact us for details).
Some credit card issuers may apply an optional issuer surcharge/fee to
transactions where the merchant is located outside the country where the
customer's credit card was issued.  The surcharge is typically 1-2% of the
billed amount.  Zonomi has no control over this.
Vanity name servers
Zonomi lets you use custom (or vanity) name servers. e.g. so you can use
ns1.yourdomain.com instead of, say, ns1.zonomi.com.
Sign up and get:

1 DNS zone (domain name).
Up to 10 DNS records.
Up to 1 million DN

[hlundin-webrtc, 2015-06-29 08:42:17]

henrik.lundin@webrtc.org changed reviewers:
+ megamyfunlife@gmail.com
- MegaMyFunLiFe@gmail.com

[hlundin-webrtc, 2015-06-29 08:42:18]

I'd still like to make sure the channel model is sane. Granted, the test
exercises the BWE code in some way, since it does change the values, but
anyway... See inline.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:98: CHECK_EQ(0,
encoded_bytes);
On 2015/06/28 03:17:41, kwiberg1 wrote:
> On 2015/06/26 10:35:14, hlundin-webrtc wrote:
> > CHECK is quite aggressive here. Consider using EXPECT instead.
> 
> OK. Although I sort of doubt the rest of the run will be useful if we fail
here.

Right. But a CHECK will kill the test binary, in this case all of
modules_unittests, so no more tests from that will be run (unless they are run
in a parallel-process fashion). I still think that ASSERT_* is better in tests.

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:146: 1, send_time,
receive_time));
On 2015/06/27 23:43:43, kwiberg1 wrote:
> On 2015/06/26 10:35:15, hlundin-webrtc wrote:
> > On 2015/06/25 14:52:43, kwiberg1 wrote:
> > > If I've understood correctly, this means that each packet is delivered 6
ms
> > late
> > > (and not, as I first assumed, that the packets spent 6 ms in flight),
> causing
> > > the bw estimator to successively lower the bitrate until it reaches the
> lowest
> > > allowed value.
> > 
> > I'm not so sure. The receive time should just be the local wall-clock, but
> with
> > the same frequency as the RTP timestamps (i.e., the send time). What you
have
> is
> > just a constant offset between send_time and receive_time, which will likely
> not
> > trigger any BW actions. You will probably have to _increase_ the
> > offset for each packet in the loop to get some action. E.g.,
> > 
> > receive_time = last_receive_time + kSamplesPerPacket * 1.1;
> 
> But I do get some action. First it keeps the bitrate at 20k for a dozen
packets,
> then it appears to decide corrective action is needed and gradually lowers it
> (over a period of about 200 packets) until it reaches 10k (the minimum allowed
> value), and then it stays there. My guess that it's seeing the 6 ms
> send-to-receive difference as a delay that should be corrected is based on
this
> behavior.

Hmm. I still think that the BWE should ignore this constant offset between send
and receive timestamps. Please, try with another (wildly different) offset and
see if you get another behavior.

[kwiberg-webrtc, 2015-06-29 11:53:48]

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:98: CHECK_EQ(0,
encoded_bytes);
On 2015/06/29 08:42:18, hlundin-webrtc wrote:
> On 2015/06/28 03:17:41, kwiberg1 wrote:
> > On 2015/06/26 10:35:14, hlundin-webrtc wrote:
> > > CHECK is quite aggressive here. Consider using EXPECT instead.
> > 
> > OK. Although I sort of doubt the rest of the run will be useful if we fail
> here.
> 
> Right. But a CHECK will kill the test binary, in this case all of
> modules_unittests, so no more tests from that will be run (unless they are run
> in a parallel-process fashion). I still think that ASSERT_* is better in
tests.

Mmmm, I didn't think things through enough to realize that's what a failing
CHECK would do---but given that, you're right of course. I'll replace all the
CHECKs, but turning them into ASSERTs would involve making subroutines such as
this one return void, which is too much of a mess, so I'll use EXPECTs.

[kwiberg-webrtc, 2015-06-30 08:38:25]

kwiberg@webrtc.org changed reviewers:
- megamyfunlife@gmail.com

[kwiberg-webrtc, 2015-07-02 13:35:22]

I think this is ready to land now. The most important change is that I've
managed to get identical output from the conjoined encoder+decoder pair and the
separate encoder, which makes the unit test much simpler.

Please review.

[hlundin-webrtc, 2015-07-02 14:04:36]

lgtm with two nits and a question.

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:65:
BoundedCapacityChannel(int rate_bps)
Suggest you call the parameter rate_bits_per_second, since bytes per sample
share the same acronym.

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:70: // the
Re-flow the line breaks.

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:183: double avg_rate_bps =
8.0 * avg_size / 60e-3;
Shouldn't 60e-3 be 30e-3 in the non-adaptive case?

[kwiberg-webrtc, 2015-07-03 00:23:06]

Patchset #8 (id:130001) has been deleted

[kwiberg-webrtc, 2015-07-03 00:27:54]

Patch set #8 fixes review comments and some minor issues caught by the trybots.

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:65:
BoundedCapacityChannel(int rate_bps)
On 2015/07/02 14:04:36, hlundin-webrtc wrote:
> Suggest you call the parameter rate_bits_per_second, since bytes per sample
> share the same acronym.

Done, for the whole file.

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:70: // the
On 2015/07/02 14:04:36, hlundin-webrtc wrote:
> Re-flow the line breaks.

Done.

https://codereview.webrtc.org/1208923002/diff/100001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:183: double avg_rate_bps =
8.0 * avg_size / 60e-3;
On 2015/07/02 14:04:36, hlundin-webrtc wrote:
> Shouldn't 60e-3 be 30e-3 in the non-adaptive case?

Yes. Done.

https://codereview.webrtc.org/1208923002/diff/150001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/isac/unittest.cc (right):

https://codereview.webrtc.org/1208923002/diff/150001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:195:
EXPECT_GT(avg_rate_bits_per_second / expected_rate_bits_per_second, 0.95);
I had to change this from 0.99 to 0.95 because of iSACfix on arm.

https://codereview.webrtc.org/1208923002/diff/150001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/isac/unittest.cc:201: double size_range =
max_size - min_size;
minmax_element is C++11

[kwiberg-webrtc, 2015-07-03 01:58:55]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2015-07-03 01:58:56]

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/1208923002/#ps150001 (title:
"review comments")

[commit-bot: I haz the power, 2015-07-03 01:59:08]

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

[commit-bot: I haz the power, 2015-07-03 02:00:39]

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

[commit-bot: I haz the power, 2015-07-03 02:00:40]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/92)

[kwiberg-webrtc, 2015-07-03 02:04:54]

Committed patchset #8 (id:150001) manually as
2224294c52fc0bb95eedb8187a62bedf1b5eb853 (presubmit successful).