Bug 169389 - Support PeerConnectionStates::BundlePolicy::MaxBundle when setting rtc configuration
Summary: Support PeerConnectionStates::BundlePolicy::MaxBundle when setting rtc config...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-08 15:48 PST by youenn fablet
Modified: 2017-06-30 08:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.30 KB, patch)
2017-03-08 15:50 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.50 KB, patch)
2017-06-27 11:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.43 MB, application/zip)
2017-06-27 12:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.03 MB, application/zip)
2017-06-27 12:42 PDT, Build Bot
no flags Details
Patch (27.34 KB, patch)
2017-06-28 10:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (23.22 KB, patch)
2017-06-28 11:16 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Correcting style and adding bug link (24.21 KB, patch)
2017-06-28 14:28 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2017-06-29 14:16 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Disabling ice candidate pool size (24.61 KB, patch)
2017-06-30 07:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-08 15:48:43 PST
libwebrtc does not like it currently.
Setting MaxBundle will then remove the possibility to set other fields.
Comment 1 youenn fablet 2017-03-08 15:50:50 PST
Created attachment 303855 [details]
Patch
Comment 2 WebKit Commit Bot 2017-03-08 17:35:41 PST
Comment on attachment 303855 [details]
Patch

Clearing flags on attachment: 303855

Committed r213613: <http://trac.webkit.org/changeset/213613>
Comment 3 WebKit Commit Bot 2017-03-08 17:35:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 youenn fablet 2017-03-08 20:09:57 PST
We should find a way to support this option.
Comment 5 youenn fablet 2017-06-27 10:55:10 PDT
rdar://problem/33006091
Comment 6 youenn fablet 2017-06-27 11:00:10 PDT
Created attachment 313929 [details]
Patch
Comment 7 Build Bot 2017-06-27 12:19:00 PDT
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
Comment 8 Build Bot 2017-06-27 12:19:01 PDT
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
Comment 9 Build Bot 2017-06-27 12:42:53 PDT
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
Comment 10 Build Bot 2017-06-27 12:42:55 PDT
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
Comment 11 youenn fablet 2017-06-28 10:33:06 PDT
Created attachment 314038 [details]
Patch
Comment 12 Alex Christensen 2017-06-28 10:35:41 PDT
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.
Comment 13 youenn fablet 2017-06-28 11:04:02 PDT
(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.
Comment 14 Alex Christensen 2017-06-28 11:10:53 PDT
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.
Comment 15 youenn fablet 2017-06-28 11:16:52 PDT
Created attachment 314043 [details]
Patch
Comment 16 youenn fablet 2017-06-28 11:17:28 PDT
(In reply to youenn fablet from comment #15)
> Created attachment 314043 [details]
> Patch

Reduced patch without the setConfiguration behavior change.
Comment 17 youenn fablet 2017-06-28 11:18:06 PDT
(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.
Comment 18 youenn fablet 2017-06-28 14:27:16 PDT
(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
Comment 19 youenn fablet 2017-06-28 14:28:09 PDT
Created attachment 314056 [details]
Correcting style and adding bug link
Comment 20 WebKit Commit Bot 2017-06-28 16:24:45 PDT
Comment on attachment 314056 [details]
Correcting style and adding bug link

Clearing flags on attachment: 314056

Committed r218903: <http://trac.webkit.org/changeset/218903>
Comment 21 WebKit Commit Bot 2017-06-28 16:24:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 youenn fablet 2017-06-29 13:31:27 PDT
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.
Comment 23 youenn fablet 2017-06-29 14:16:26 PDT
Reopening to attach new patch.
Comment 24 youenn fablet 2017-06-29 14:16:27 PDT
Created attachment 314163 [details]
Patch
Comment 25 youenn fablet 2017-06-29 14:17:18 PDT
(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.
Comment 26 WebKit Commit Bot 2017-06-29 15:04:45 PDT
The commit-queue encountered the following flaky tests while processing attachment 314163 [details]:

workers/bomb.html bug 171985 (author: fpizlo@apple.com)
The commit-queue is continuing to process your patch.
Comment 27 WebKit Commit Bot 2017-06-29 15:05:13 PDT
Comment on attachment 314163 [details]
Patch

Clearing flags on attachment: 314163

Committed r218963: <http://trac.webkit.org/changeset/218963>
Comment 28 WebKit Commit Bot 2017-06-29 15:05:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Matt Lewis 2017-06-29 16:58:22 PDT
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.
Comment 30 Matt Lewis 2017-06-29 17:14:59 PDT
Reverted r218963 for reason:

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

Committed r218972: <http://trac.webkit.org/changeset/218972>
Comment 31 Matt Lewis 2017-06-29 17:16:01 PDT
Reverted r218903 for reason:

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

Committed r218973: <http://trac.webkit.org/changeset/218973>
Comment 32 youenn fablet 2017-06-30 07:22:50 PDT
Created attachment 314265 [details]
Disabling ice candidate pool size
Comment 33 youenn fablet 2017-06-30 07:23:36 PDT
(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
Comment 34 WebKit Commit Bot 2017-06-30 08:00:46 PDT
Comment on attachment 314265 [details]
Disabling ice candidate pool size

Clearing flags on attachment: 314265

Committed r218994: <http://trac.webkit.org/changeset/218994>
Comment 35 WebKit Commit Bot 2017-06-30 08:00:48 PDT
All reviewed patches have been landed.  Closing bug.