Ignore "b=AS:-1" instead of treating as a hard error.

Ignore "b=AS:-1" instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just omitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361
Review-Url: https://codereview.webrtc.org/2995463002
Cr-Commit-Position: refs/heads/master@{#19244}
Committed: https://chromium.googlesource.com/external/webrtc/+/3e8016e1d51e6db21a51b7fcf308b8904aee2f66

[Taylor Brandstetter, 2017-08-03 18:01:01]

deadbeef@webrtc.org changed reviewers:
+ zhihuang@webrtc.org

[Taylor Brandstetter, 2017-08-03 18:01:02]

PTAL; it turns out that negative b=AS values are being used after all.

[Zhi Huang, 2017-08-03 18:12:48]

https://codereview.webrtc.org/2995463002/diff/1/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2995463002/diff/1/webrtc/pc/webrtcsdp.cc#newcod...
webrtc/pc/webrtcsdp.cc:2726: // better to treat this as a hard error instead of
just ignoring it.
Is -1 the only valid negative value? If so, should we treat other negative
values as errors?

[Taylor Brandstetter, 2017-08-03 21:44:33]

Followed your suggestion, take a look again

https://codereview.webrtc.org/2995463002/diff/1/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2995463002/diff/1/webrtc/pc/webrtcsdp.cc#newcod...
webrtc/pc/webrtcsdp.cc:2726: // better to treat this as a hard error instead of
just ignoring it.
On 2017/08/03 18:12:47, Zhi Huang wrote:
> Is -1 the only valid negative value? If so, should we treat other negative
> values as errors?

I guess so. I'll add back the "ParseFailed" here and make two unit tests, one
for -1 and one for values < -1.

[Zhi Huang, 2017-08-03 23:37:40]

lgtm

https://codereview.webrtc.org/2995463002/diff/20001/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2995463002/diff/20001/webrtc/pc/webrtcsdp.cc#ne...
webrtc/pc/webrtcsdp.cc:2733: return ParseFailed(line, "b=AS value can't be
negative.", error);
"b=AS value can't be negative other than -1"?

[Taylor Brandstetter, 2017-08-04 00:10:20]

https://codereview.webrtc.org/2995463002/diff/20001/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2995463002/diff/20001/webrtc/pc/webrtcsdp.cc#ne...
webrtc/pc/webrtcsdp.cc:2733: return ParseFailed(line, "b=AS value can't be
negative.", error);
On 2017/08/03 23:37:40, Zhi Huang wrote:
> "b=AS value can't be negative other than -1"?

Technically, but I don't want to encourage people to do that. :)

[Taylor Brandstetter, 2017-08-04 00:10:31]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-08-04 00:10:41]

CQ is trying da patch.

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

[Taylor Brandstetter, 2017-08-04 00:10:55]

Description was changed from

==========
Ignore negative b=AS values instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just ommitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361
==========

to

==========
Ignore negative b=AS values instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just omitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361
==========

[Taylor Brandstetter, 2017-08-04 00:30:05]

Description was changed from

==========
Ignore negative b=AS values instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just omitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361
==========

to

==========
Ignore "b=AS:-1" instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just omitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361
==========

[commit-bot: I haz the power, 2017-08-04 00:49:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1501805431268860,
"parent_rev": "d89b0bcc8ab9866f3c783492b9748a388a9aaccc", "commit_rev":
"3e8016e1d51e6db21a51b7fcf308b8904aee2f66"}

[commit-bot: I haz the power, 2017-08-04 00:49:53]

Description was changed from

==========
Ignore "b=AS:-1" instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just omitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361
==========

to

==========
Ignore "b=AS:-1" instead of treating as a hard error.

Follow up to https://codereview.webrtc.org/2989243002/.

It turns out that "b=AS:-1" was being used to mean "no bandwidth limit",
even though just omitting "b=AS" completely will do that. So we should
treat this as a soft error for now, and give applications time to
transition to doing the standard thing.

BUG=chromium:675361

Review-Url: https://codereview.webrtc.org/2995463002
Cr-Commit-Position: refs/heads/master@{#19244}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3e8016e1d51e6db21a51b7fcf...
==========

[commit-bot: I haz the power, 2017-08-04 00:49:54]

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