Add support for RTCConfiguration.iceCandidatePoolSize.

Add support for RTCConfiguration.iceCandidatePoolSize.

This is a performance optimization that will result in ICE candidates
being gathered before setLocalDescription has been called. Later, when
setLocalDescription *is* called, any pooled candidates will be fired
in an "icecandidate" event immediately.

Besides candidate gathering appearing to occur quicker, this optimization
doesn't have any effects visible to JavaScript.

Note that the pool size can only be changed before setLocalDescription,
as described by JSEP. This pool is only intended to be used for the
first offer/answer, after which an ICE restart must be used to gather
new candidates.

Intent to Implement and Ship thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/dWXRWoi5ueg/nFiVhj5LCAAJ

BUG=chromium:673395
Review-Url: https://codereview.chromium.org/2721163002
Cr-Commit-Position: refs/heads/master@{#456998}
Committed: https://chromium.googlesource.com/chromium/src/+/ff92bdcc0bed8878b0989f14993b3aec98a4769b

[Taylor_Brandstetter, 2017-03-01 00:38:28]

The CQ bit was checked by deadbeef@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 00:39:33]

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

[Taylor_Brandstetter, 2017-03-01 00:53:49]

The CQ bit was unchecked by deadbeef@chromium.org

[Taylor_Brandstetter, 2017-03-01 02:30:00]

The CQ bit was checked by deadbeef@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 02:30:34]

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

[commit-bot: I haz the power, 2017-03-01 04:15:07]

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

[commit-bot: I haz the power, 2017-03-01 04:15:08]

Dry run: This issue passed the CQ dry run.

[Taylor_Brandstetter, 2017-03-01 19:32:26]

deadbeef@chromium.org changed reviewers:
+ foolip@chromium.org, hbos@chromium.org

[Taylor_Brandstetter, 2017-03-01 19:32:26]

foolip@chromium.org: Please review changes in WebRtcConfiguration.h.

hbos@chromium.org: Please review changes everywhere else.

[Taylor_Brandstetter, 2017-03-01 19:32:26]

deadbeef@chromium.org changed reviewers:
+ foolip@chromium.org, hbos@chromium.org

[Taylor_Brandstetter, 2017-03-01 19:32:27]

foolip@chromium.org: Please review changes in WebRtcConfiguration.h.

hbos@chromium.org: Please review changes everywhere else.

[foolip, 2017-03-02 12:38:30]

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html
(right):

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html:48:
shouldNotThrow("new RTCPeerConnection({iceCandidatePoolSize:1});");
Also test 0? That's where an off-by-one bug is most likely I think.

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl
(right):

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl:44:
[EnforceRange] unsigned short iceCandidatePoolSize = 0;
The spec doesn't use [EnforceRage] for this. If it should, can you file a spec
issue?

[Taylor_Brandstetter, 2017-03-02 21:57:01]

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html
(right):

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html:48:
shouldNotThrow("new RTCPeerConnection({iceCandidatePoolSize:1});");
On 2017/03/02 12:38:30, foolip wrote:
> Also test 0? That's where an off-by-one bug is most likely I think.

Done.

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl
(right):

https://codereview.chromium.org/2721163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl:44:
[EnforceRange] unsigned short iceCandidatePoolSize = 0;
On 2017/03/02 12:38:30, foolip wrote:
> The spec doesn't use [EnforceRage] for this. If it should, can you file a spec
> issue?

Done: https://github.com/w3c/webrtc-pc/issues/1048

[hbos_chromium, 2017-03-03 11:56:07]

lgtm if my question about the spec isn't a problem that requires re-review.

https://codereview.chromium.org/2721163002/diff/80001/content/renderer/media/...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/2721163002/diff/80001/content/renderer/media/...
content/renderer/media/peer_connection_tracker.cc:183: << "rtcpMuxPolicy: " <<
SerializeRtcpMuxPolicy(config.rtcp_mux_policy)
I noticed there's no "certificates", even though certificates is one of the
members supported (and translated between) both blink and webrtc.

And the tracker is used to tell the UI process about the PC, which in practice
for this means displaying this string in chrome://webrtc-internals? This can be
addressed separately. Can you add a TODO and assign it to me (unless you want to
do that)?

https://codereview.chromium.org/2721163002/diff/80001/content/test/data/media...
File content/test/data/media/peerconnection-setConfiguration.html (right):

https://codereview.chromium.org/2721163002/diff/80001/content/test/data/media...
content/test/data/media/peerconnection-setConfiguration.html:30:
rtcpMuxPolicy:'require', certificates:[], iceCandidatePoolSize:1});
(Later when getConfiguration is available we should verify the pool size value
after each setConfiguration. Not a blocker.)

https://codereview.chromium.org/2721163002/diff/80001/content/test/data/media...
content/test/data/media/peerconnection-setConfiguration.html:122: // pool size
is not allowed.
Is this spec-compliant? If it isn't but it should be, can you file a bug if
needed?

The latest rawgit spec says: "Set the ICE Agent's prefetched ICE candidate pool
size as defined in [JSEP] (section 3.5.4. and section 4.1.1.) to the value of
configuration.iceCandidatePoolSize. If the new ICE candidate pool size changes
the existing setting, this may result in immediate gathering of new pooled
candidates, or discarding of existing pooled candidates, as defined in [JSEP]
(section 4.1.15.)."

(Latest published spec says: "no action will be taken until the
RTCPeerConnection 's ICE gathering state transitions to gathering. If a script
wants this to happen immediately, it should do an ICE restart.")

I don't see anything about throwing an exception if the value is different after
this event.

[foolip, 2017-03-03 15:06:04]

If there's no particular hurry, I think we should wait for
https://github.com/w3c/webrtc-pc/issues/1048 to be settled. Not necessarily for
the spec change to be merged and published, but to be 90% certain about what the
spec will eventually say.

[Taylor_Brandstetter, 2017-03-03 18:44:46]

Totally agree we should wait for issues to resolve before landing this. There's
no rush since M58 already branched.

https://codereview.chromium.org/2721163002/diff/80001/content/renderer/media/...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/2721163002/diff/80001/content/renderer/media/...
content/renderer/media/peer_connection_tracker.cc:183: << "rtcpMuxPolicy: " <<
SerializeRtcpMuxPolicy(config.rtcp_mux_policy)
On 2017/03/03 11:56:06, hbos_chromium wrote:
> I noticed there's no "certificates", even though certificates is one of the
> members supported (and translated between) both blink and webrtc.
> 
> And the tracker is used to tell the UI process about the PC, which in practice
> for this means displaying this string in chrome://webrtc-internals? This can
be
> addressed separately. Can you add a TODO and assign it to me (unless you want
to
> do that)?

TODO added.

https://codereview.chromium.org/2721163002/diff/80001/content/test/data/media...
File content/test/data/media/peerconnection-setConfiguration.html (right):

https://codereview.chromium.org/2721163002/diff/80001/content/test/data/media...
content/test/data/media/peerconnection-setConfiguration.html:30:
rtcpMuxPolicy:'require', certificates:[], iceCandidatePoolSize:1});
On 2017/03/03 11:56:06, hbos_chromium wrote:
> (Later when getConfiguration is available we should verify the pool size value
> after each setConfiguration. Not a blocker.)

That's done in Zhi's CL: https://codereview.chromium.org/2706563003/

https://codereview.chromium.org/2721163002/diff/80001/content/test/data/media...
content/test/data/media/peerconnection-setConfiguration.html:122: // pool size
is not allowed.
On 2017/03/03 11:56:06, hbos_chromium wrote:
> Is this spec-compliant? If it isn't but it should be, can you file a bug if
> needed?
> 
> The latest rawgit spec says: "Set the ICE Agent's prefetched ICE candidate
pool
> size as defined in [JSEP] (section 3.5.4. and section 4.1.1.) to the value of
> configuration.iceCandidatePoolSize. If the new ICE candidate pool size changes
> the existing setting, this may result in immediate gathering of new pooled
> candidates, or discarding of existing pooled candidates, as defined in [JSEP]
> (section 4.1.15.)."
> 
> (Latest published spec says: "no action will be taken until the
> RTCPeerConnection 's ICE gathering state transitions to gathering. If a script
> wants this to happen immediately, it should do an ICE restart.")
> 
> I don't see anything about throwing an exception if the value is different
after
> this event. 

Yeah, it looks like webrtc-pc is out of sync with JSEP. Filed issue:
https://github.com/w3c/webrtc-pc/issues/1049

[Taylor_Brandstetter, 2017-03-10 23:39:26]

The CQ bit was checked by deadbeef@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 23:39:55]

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

[Taylor_Brandstetter, 2017-03-10 23:40:07]

Spec issues should be addressed now. We ended up making iceCandidatePoolSize an
octet.

[commit-bot: I haz the power, 2017-03-11 01:34:56]

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

[commit-bot: I haz the power, 2017-03-11 01:34:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hbos_chromium, 2017-03-13 09:20:09]

lgtm

[foolip, 2017-03-14 14:49:06]

lgtm. Can you link to
https://groups.google.com/a/chromium.org/d/msg/blink-dev/dWXRWoi5ueg/nFiVhj5L...
in the description?

https://codereview.chromium.org/2721163002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html
(right):

https://codereview.chromium.org/2721163002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html:52:
shouldThrow("new RTCPeerConnection({iceCandidatePoolSize:99999999});");
Can you also try 256?

[Taylor_Brandstetter, 2017-03-14 22:18:35]

Description was changed from

==========
Add support for RTCConfiguration.iceCandidatePoolSize.

This is a performance optimization that will result in ICE candidates
being gathered before setLocalDescription has been called. Later, when
setLocalDescription *is* called, any pooled candidates will be fired
in an "icecandidate" event immediately.

Besides candidate gathering appearing to occur quicker, this optimization
doesn't have any effects visible to JavaScript.

Note that the pool size can only be changed before setLocalDescription,
as described by JSEP. This pool is only intended to be used for the
first offer/answer, after which an ICE restart must be used to gather
new candidates.

BUG=chromium:673395
==========

to

==========
Add support for RTCConfiguration.iceCandidatePoolSize.

This is a performance optimization that will result in ICE candidates
being gathered before setLocalDescription has been called. Later, when
setLocalDescription *is* called, any pooled candidates will be fired
in an "icecandidate" event immediately.

Besides candidate gathering appearing to occur quicker, this optimization
doesn't have any effects visible to JavaScript.

Note that the pool size can only be changed before setLocalDescription,
as described by JSEP. This pool is only intended to be used for the
first offer/answer, after which an ICE restart must be used to gather
new candidates.

Intent to Implement and Ship thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/dWXRWoi5ueg/nFiVhj5L...

BUG=chromium:673395
==========

[Taylor_Brandstetter, 2017-03-14 22:23:11]

https://codereview.chromium.org/2721163002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html
(right):

https://codereview.chromium.org/2721163002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html:52:
shouldThrow("new RTCPeerConnection({iceCandidatePoolSize:99999999});");
On 2017/03/14 14:49:06, foolip wrote:
> Can you also try 256?

Done.

[Taylor_Brandstetter, 2017-03-14 22:23:17]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2017-03-14 22:23:18]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2721163002/#ps140001
(title: "Adding tests for corner cases of candidate pool size.")

[commit-bot: I haz the power, 2017-03-14 22:24:25]

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

[commit-bot: I haz the power, 2017-03-15 01:42:18]

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

[commit-bot: I haz the power, 2017-03-15 01:42:20]

Failed to apply patch for
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:
While running git apply --index -p1;
  <stdin>:21: trailing whitespace.
  PASS new RTCPeerConnection({ iceCandidatePoolSize: toNumberThrows }) 
  error: patch failed:
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:1
  error:
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:
patch does not apply

Patch:      
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
Index:
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
diff --git
a/third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
b/third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
index
3ca3c01c1a8ae98656474682db7f728392b0c617..5c29dab74c45afefd9ab69a578f2fcf82ed0b047
100644
---
a/third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
+++
b/third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
@@ -1,6 +1,6 @@
 CONSOLE WARNING: line 1: The rtcpMuxPolicy option is being considered for
removal and may be removed no earlier than M60, around August 2017. If you
depend on it, please see https://www.chromestatus.com/features/5654810086866944
for more details.
 This is a testharness.js-based test.
-Found 73 tests; 61 PASS, 12 FAIL, 0 TIMEOUT, 0 NOTRUN.
+Found 73 tests; 62 PASS, 11 FAIL, 0 TIMEOUT, 0 NOTRUN.
 PASS RTCPeerConnection.length 
 PASS new RTCPeerConnection() 
 PASS new RTCPeerConnection(null) 
@@ -68,9 +68,7 @@ PASS new RTCPeerConnection({ certificates: undefined })
 PASS new RTCPeerConnection({ certificates: [] }) 
 PASS new RTCPeerConnection({ certificates: [null] }) 
 PASS new RTCPeerConnection({ certificates: [undefined] }) 
-FAIL new RTCPeerConnection({ iceCandidatePoolSize: toNumberThrows })
assert_throws: function "function () {
-        eval(expr);
-      }" did not throw
+PASS new RTCPeerConnection({ iceCandidatePoolSize: toNumberThrows }) 
 PASS new RTCPeerConnection({ certificates: [certificate] }) 
 PASS new RTCPeerConnection({ certificates: [expiredCertificate] }) 
 PASS localDescription initial value

[Taylor_Brandstetter, 2017-03-15 02:56:50]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2017-03-15 02:56:51]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2721163002/#ps160001
(title: "Merge with master")

[commit-bot: I haz the power, 2017-03-15 02:57:23]

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

[commit-bot: I haz the power, 2017-03-15 05:06:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489546610679460,
"parent_rev": "6b6d61a8167f173fbd711dcea247c6905d6c11d9", "commit_rev":
"ff92bdcc0bed8878b0989f14993b3aec98a4769b"}

[commit-bot: I haz the power, 2017-03-15 05:07:56]

Description was changed from

==========
Add support for RTCConfiguration.iceCandidatePoolSize.

This is a performance optimization that will result in ICE candidates
being gathered before setLocalDescription has been called. Later, when
setLocalDescription *is* called, any pooled candidates will be fired
in an "icecandidate" event immediately.

Besides candidate gathering appearing to occur quicker, this optimization
doesn't have any effects visible to JavaScript.

Note that the pool size can only be changed before setLocalDescription,
as described by JSEP. This pool is only intended to be used for the
first offer/answer, after which an ICE restart must be used to gather
new candidates.

Intent to Implement and Ship thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/dWXRWoi5ueg/nFiVhj5L...

BUG=chromium:673395
==========

to

==========
Add support for RTCConfiguration.iceCandidatePoolSize.

This is a performance optimization that will result in ICE candidates
being gathered before setLocalDescription has been called. Later, when
setLocalDescription *is* called, any pooled candidates will be fired
in an "icecandidate" event immediately.

Besides candidate gathering appearing to occur quicker, this optimization
doesn't have any effects visible to JavaScript.

Note that the pool size can only be changed before setLocalDescription,
as described by JSEP. This pool is only intended to be used for the
first offer/answer, after which an ICE restart must be used to gather
new candidates.

Intent to Implement and Ship thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/dWXRWoi5ueg/nFiVhj5L...

BUG=chromium:673395

Review-Url: https://codereview.chromium.org/2721163002
Cr-Commit-Position: refs/heads/master@{#456998}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff92bdcc0bed8878b0989f14993b...
==========

[commit-bot: I haz the power, 2017-03-15 05:07:57]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ff92bdcc0bed8878b0989f14993b...