Bug 169389 – Support PeerConnectionStates::BundlePolicy::MaxBundle when setting rtc configuration

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

libwebrtc does not like it currently.
Setting MaxBundle will then remove the possibility to set other fields.

[youenn fablet]

Created attachment 303855 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 303855 [details]
Patch

Clearing flags on attachment: 303855

Committed r213613: <http://trac.webkit.org/changeset/213613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

We should find a way to support this option.

[youenn fablet]

rdar://problem/33006091

[youenn fablet]

Created attachment 313929 [details]
Patch

[Build Bot]

Comment on attachment 313929 [details]
Patch

Attachment 313929 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4007096

New failing tests:
fast/mediastream/RTCSessionDescription.html
imported/w3c/web-platform-tests/webrtc/RTCDataChannel-id.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

[Build Bot]

Created attachment 313932 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313929 [details]
Patch

Attachment 313929 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4007114

New failing tests:
imported/w3c/web-platform-tests/webrtc/RTCDataChannel-id.html
imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-createAnswer.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

[Build Bot]

Created attachment 313934 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[youenn fablet]

Created attachment 314038 [details]
Patch

[Alex Christensen]

Comment on attachment 314038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314038&action=review

> LayoutTests/fast/mediastream/RTCPeerConnection-getConfiguration-expected.txt:56
> -PASS successfullyParsed is true
> +FAIL iceServers.length should be 1. Was 2.

This seems like a test regression.

[youenn fablet]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 314038 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314038&action=review
> 
> > LayoutTests/fast/mediastream/RTCPeerConnection-getConfiguration-expected.txt:56
> > -PASS successfullyParsed is true
> > +FAIL iceServers.length should be 1. Was 2.
> 
> This seems like a test regression.

Right.
Previously, we were trying to set the configuration, failing to do so at libwebrtc backend but reporting that this was done at RTCPeerConnection level.
With the changes done to RTCPeerConnection::setConfiguration, we will report more accurately the actual configuration.

[Alex Christensen]

Comment on attachment 314038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314038&action=review

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:83
> +    else if (configuration.bundlePolicy == RTCBundlePolicy::MaxBundle)

This should probably be a switch with no default so we will see compiler failures if another one is added.

[youenn fablet]

Created attachment 314043 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #15)
> Created attachment 314043 [details]
> Patch

Reduced patch without the setConfiguration behavior change.

[youenn fablet]

(In reply to Alex Christensen from comment #14)
> Comment on attachment 314038 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314038&action=review
> 
> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:83
> > +    else if (configuration.bundlePolicy == RTCBundlePolicy::MaxBundle)
> 
> This should probably be a switch with no default so we will see compiler
> failures if another one is added.

Will do that as a follow-up or at landing time.
I probably also need to update the fixes about the setConfiguration issue with a bugzilla entry.

[youenn fablet]

(In reply to youenn fablet from comment #16)
> (In reply to youenn fablet from comment #15)
> > Created attachment 314043 [details]
> > Patch
> 
> Reduced patch without the setConfiguration behavior change.

Filed bug 173938 for setConfiguration issue

[youenn fablet]

Created attachment 314056 [details]
Correcting style and adding bug link

[WebKit Commit Bot]

Comment on attachment 314056 [details]
Correcting style and adding bug link

Clearing flags on attachment: 314056

Committed r218903: <http://trac.webkit.org/changeset/218903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

This patch makes more tests flaky in the bots.

The only change I see is that we are now really disabling CPU overuse detection with this patch. This might cause issues with the bots.

If that is the case, we might actually want to activate CPU overuse detection, at least on the bots.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 314163 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #24)
> Created attachment 314163 [details]
> Patch

Patch to disable cpu overuse detection.
This will allow verifying whether this is what is causing issues with bots.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 314163 [details]:

workers/bomb.html bug 171985 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 314163 [details]
Patch

Clearing flags on attachment: 314163

Committed r218963: <http://trac.webkit.org/changeset/218963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

I looks like the patch did not fix the issue with the test it is still failing on all WK2 testers. 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fwebrtc%2Fsimplecall.html

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/2628

Talked with Youenn, rolling out both patches.

[Matt Lewis]

Reverted r218963 for reason:

This patch and its fix cause immediate flakiness on all WK2 testers

Committed r218972: <http://trac.webkit.org/changeset/218972>

[Matt Lewis]

Reverted r218903 for reason:

This patch and its fix cause immediate flakiness on all WK2 testers

Committed r218973: <http://trac.webkit.org/changeset/218973>

[youenn fablet]

Created attachment 314265 [details]
Disabling ice candidate pool size

[youenn fablet]

(In reply to youenn fablet from comment #32)
> Created attachment 314265 [details]
> Disabling ice candidate pool size

WK2 testers might have issues with ice candidate pool size

[WebKit Commit Bot]

Comment on attachment 314265 [details]
Disabling ice candidate pool size

Clearing flags on attachment: 314265

Committed r218994: <http://trac.webkit.org/changeset/218994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.