Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

add command line flags for webrtc #504

Merged
merged 4 commits into from Mar 13, 2018
Merged

add command line flags for webrtc #504

merged 4 commits into from Mar 13, 2018

Conversation

kereliuk
Copy link
Collaborator

@kereliuk kereliuk commented Mar 1, 2018

This is intended as a temporary fix to allow the webrtc tests in chrome to call getUserMedia without failing out.

@lukebjerring lukebjerring requested review from mattl and removed request for lukebjerring March 1, 2018 18:55
@lukebjerring
Copy link
Collaborator

Subbing out for @mattl since he'll have more knowledge of flags for wpt run.

@foolip
Copy link
Member

foolip commented Mar 1, 2018

@mattl, let us know if you want to wait to merge this until the code freeze is over, which I believe is March 9. Up to you.

run/run.py Outdated
command.extend(
['--binary-arg=--use-fake-ui-for-media-stream'])
command.extend(
['--binary-arg=--use-fake-device-for-media-stream'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be combined

command.extend([
    '--binary-arg=--use-fake-ui-for-media-stream',
    '--binary-arg=--use-fake-device-for-media-stream'
])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @kereliuk to combine these so we're all set for Monday.

@mattl
Copy link
Contributor

mattl commented Mar 1, 2018

I think we'll wait until the 9th, in the meantime this can easily be run on a feature branch if needed.

@rwaldron's suggestion is a good one.

@foolip
Copy link
Member

foolip commented Mar 1, 2018

@mattl, that sounds good, thanks! @kereliuk, anything we should do to increase confidence that this will in fact work once landed?

@foolip
Copy link
Member

foolip commented Mar 1, 2018

@kereliuk, a concrete idea is to run the webrtc/ directory with and without these flags and list and look at any results that change. If anything goes from passing to not passing, that's weird, and hopefully it's all in the other direction.

@mariestaver
Copy link
Collaborator

@foolip Thank you and yes, we'll pause this one until after 3/9 -- I've added it to our backlog to be sure it gets picked up then.

@foolip
Copy link
Member

foolip commented Mar 2, 2018

@alvestrand FYI, this will make the wpt.fyi results more meaningful for you, as promised

@kereliuk
Copy link
Collaborator Author

kereliuk commented Mar 2, 2018

I ran diffs on the results of with and without these flags.

Got [2, 6] with flags for /webrtc/RTCPeerConnection-ontrack.https.html
Got [1, 6] without flags for /webrtc/RTCPeerConnection-ontrack.https.html

Got [2, 2] with flags for /webrtc/RTCDTMFSender-ontonechange-long.https.html
Got [0, 2] without flags for /webrtc/RTCDTMFSender-ontonechange-long.https.html

Got [8, 12] with flags for /webrtc/RTCDTMFSender-ontonechange.https.html
Got [0, 12] without flags for /webrtc/RTCDTMFSender-ontonechange.https.html

Got [1, 3] with flags for /webrtc/RTCRtpSender-getStats.https.html
Got [0, 3] without flags for /webrtc/RTCRtpSender-getStats.https.html

Got [6, 7] with flags for /webrtc/RTCPeerConnection-setRemoteDescription-replaceTrack.https.html
Got [0, 7] without flags for /webrtc/RTCPeerConnection-setRemoteDescription-replaceTrack.https.html

Got [1, 2] with flags for /webrtc/RTCRtpReceiver-getContributingSources.https.html
Got [0, 2] without flags for /webrtc/RTCRtpReceiver-getContributingSources.https.html

Got [3, 7] with flags for /webrtc/RTCDTMFSender-insertDTMF.https.html
Got [0, 7] without flags for /webrtc/RTCDTMFSender-insertDTMF.https.html

Got [400, 692] with flags for /webrtc/interfaces.https.html
Got [1, 5] without flags for /webrtc/interfaces.https.html

Got [11, 15] with flags for /webrtc/RTCPeerConnection-setRemoteDescription-tracks.https.html
Got [0, 15] without flags for /webrtc/RTCPeerConnection-setRemoteDescription-tracks.https.html

Got [2, 2] with flags for /webrtc/simplecall.https.html
Got [1, 2] without flags for /webrtc/simplecall.https.html

Got [3, 14] with flags for /webrtc/RTCPeerConnection-getStats.https.html
Got [1, 14] without flags for /webrtc/RTCPeerConnection-getStats.https.html

Got [5, 13] with flags for /webrtc/RTCPeerConnection-removeTrack.https.html
Got [0, 13] without flags for /webrtc/RTCPeerConnection-removeTrack.https.html

Got [4, 9] with flags for /webrtc/RTCPeerConnection-addTrack.https.html
Got [0, 9] without flags for /webrtc/RTCPeerConnection-addTrack.https.html

Got [12, 13] with flags for /webrtc/RTCPeerConnection-track-stats.https.html
Got [0, 13] without flags for /webrtc/RTCPeerConnection-track-stats.https.html

Got [1, 2] with flags for /webrtc/RTCRtpReceiver-getSynchronizationSources.https.html
Got [0, 2] without flags for /webrtc/RTCRtpReceiver-getSynchronizationSources.https.html

15 different results

This is just from the summary file, so I'm not 100% sure if anything fails that used to pass on the subtest level of granularity; however, it does increase the pass rate overall as above and the only differences noted only had at most 1 subtest passing originally, so I'm guessing not.

@foolip
Copy link
Member

foolip commented Mar 3, 2018

Sounds solid. Even if some subtest does go from passing to failing, that must be accidental and will just have to be fixed.

@fippo
Copy link

fippo commented Mar 9, 2018

you might want to add https://peter.sh/experiments/chromium-command-line-switches/#mute-audio which (if I recall correctly) was added by the webrtc folks to avoid the beep sounds when running stuff locally

@rwaldron rwaldron merged commit 7206495 into master Mar 13, 2018
@foolip foolip deleted the webrtc-chrome branch March 13, 2018 16:06
jugglinmike added a commit to bocoup/results-collection that referenced this pull request Mar 20, 2018
This change has been merged to the `master` branch since the development
of the Buildbot-powered infrastructure began. See
web-platform-tests#504
jugglinmike added a commit to bocoup/results-collection that referenced this pull request Mar 20, 2018
This change has been merged to the `master` branch since the development
of the Buildbot-powered infrastructure began. See
web-platform-tests#504
jugglinmike added a commit to bocoup/results-collection that referenced this pull request Mar 20, 2018
This change has been merged to the `master` branch since the development
of the Buildbot-powered infrastructure began. See
web-platform-tests#504
jugglinmike added a commit to bocoup/results-collection that referenced this pull request Mar 20, 2018
This change has been merged to the `master` branch since the development
of the Buildbot-powered infrastructure began. See
web-platform-tests#504
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants