[webrtc] Use procedurally-generated media streams #10746
Conversation
wpt-pr-bot
requested review from
agouaillard,
alvestrand,
dontcallmedom,
guidou,
henbos,
phoglund,
rwaldron and
snuggs
|
Pinging all reviewers! |
|
@youennf Would you mind taking a look? |
|
the Chrome failures because of "NotSupported" indicate that the tests are being run in a wrong environment. Since these are .https.html tests, they should be running in a secure environment (https), and therefore the objects should be supported. |
|
Given that we are still cleaning up after the last attempt to avoid getUserMedia repeating that pattern seems like a bad idea. Also Edge does not support captureStream. I would not be surprised if the flakyness comes from having too many streams open due to lack of cleaning up. |
What was the approach that time, and why didn't it work?
Any WebDriver solution we come up with would initially not be supported by any browser at all, so that As for what I think: RTCPeerConnection may well be testable with captureStream, but getUserMedia itself is not. https://w3c.github.io/mediacapture-main/ is pretty complicated and to test something like https://w3c.github.io/mediacapture-main/#dfn-selectsettings it's necessary to mock the existence of devices. |
using addTransceiver. The recording of https://www.w3.org/2018/04/26-webrtc-minutes has some details, the tl;dr is that nobody implemented addTransceiver at that point (and it yields a track which is not active in any way) |
|
Not implemented by anyone and already being implemented by 3 engines is quite an important difference in these approaches. Unless there is something fundamental/special about getUserMedia that nothing other than getUserMedia would get us? |
|
@alvestrand that seems to be a mistake in the way the test was authored, not an environment error. The diff doesn't have enough context to show this, but the 6 sub-test failures in Chromium related to secure origins all come from The fix for that would be to simply re-name the test. However, that change isn't necessary with this patch applied. Renaming the file would actually be undesirable if we merged this patch because requiring HTTPS would make the test more specific than necessary. |
|
@jugglinmike That raises a good point. AFAIK nothing in the WebRTC spec says RTCPeerConnection may be limited to HTTPS only, so we need to test it in HTTP as well. |
@alvestrand is this an oversight? Does no spec describe how RTCPeerConnection is limited to secure contexts in Chromium? |
|
see also w3ctag/design-reviews#228 |
It isn't, as that fiddle shows both |
|
Ah, I guess it's just |
FWIW, Firefox does implement transceivers now, so using them for fake tracks works there. |
Yes, though even there, Mediacapture allows limited (non-persistent) access to getUserMedia in http, so Firefox (and last I tried, Edge) are compliant. We are considering removing support in http however. |
|
We've had a month to discuss this patch, and we haven't identified any reason not to move forward with it. Because I authored the changeset, I'd prefer if one of my reviewers merged on my behalf. @jan-ivar think it's ready? |
jugglinmike
force-pushed
the
webrtc-media
branch
from
3a5cd7d
to
afb2b57
Compare
|
the test failure might be due to https://bugs.chromium.org/p/chromium/issues/detail?id=848747 -- @henbos can you take a look if the unstable version used includes your fix? |
Not sure how to check, can we fix the conflict and rerun the test? Fingers crossed |
|
No problem. I've resolved the new conflicts introduced by today's earlier commit to master. |
|
@henbos 69.0.3452.0-1 -- you probably know omahaproxy better than I do to find out if this simply doesn't have the fix for the issue yet. Still flaky in I'd say merge and if this is still an issue its on the two of us, not @jugglinmike |
|
@eric-carlson @youennf you indicated yesterday that this won't actually work in Safari. Can you elaborate? |
@foolip |
There was a problem hiding this comment.
IIRC as discussed in the F2F Stockholm meeting, getNoiseStream() need to feature detect on the captureStream() and MediaStreamAudioDestinationNode. If the functions are not available on the running browser, getNoiseStream() should revert back to using getUserMedia() to generate the stream.
|
Sounds right to me, @soareschen! I've resolved the new conflicts, updated the new tests, and implemented the feature detection. Would you mind merging this if it looks good to you? |
webrtc/RTCPeerConnection-helper.js
Outdated
| var tracks = []; | ||
|
|
||
| if (!HTMLCanvasElement.prototype.captureStream || | ||
| typeof MediaStreamAudioDestinationNode === 'undefined') { |
There was a problem hiding this comment.
With this condition, the noise streams wouldn't be used if either of the feature is missing. Would it be better if the condition be checked depending on individual capability, so that tests that request only one of the supported capabilities still get to bypass getUserMedia?
There was a problem hiding this comment.
Sure, at the cost of some complexity in the helper. I've done this somewhat verbosely to help readers understand what is going on.
webrtc/RTCPeerConnection-helper.js
Outdated
| // @param {boolean} [caps.video] - flag indicating whether the generated stream | ||
| // should include a video track | ||
| function getNoiseStream(caps) { | ||
| var tracks = []; |
There was a problem hiding this comment.
Maybe const would be more precise
webrtc/RTCPeerConnection-helper.js
Outdated
| // should include an audio track | ||
| // @param {boolean} [caps.video] - flag indicating whether the generated stream | ||
| // should include a video track | ||
| function getNoiseStream(caps) { |
There was a problem hiding this comment.
Nit: we could use async function here, and use default args.
async function getNoiseStream(caps = {}) {for benefits shown below
webrtc/RTCPeerConnection-helper.js
Outdated
| function getNoiseStream(caps) { | ||
| var tracks = []; | ||
|
|
||
| if (caps && caps.audio) { |
There was a problem hiding this comment.
...then we wouldn't need to test caps here.
webrtc/RTCPeerConnection-helper.js
Outdated
| tracks.push(trackFactories.video()); | ||
| } | ||
|
|
||
| return Promise.resolve(new MediaStream(tracks)); |
There was a problem hiding this comment.
...and we could just
return new MediaStream(tracks);
webrtc/RTCPeerConnection-helper.js
Outdated
|
|
||
| return Promise.resolve(new MediaStream(tracks)); | ||
| } | ||
|
|
||
| // Obtain a MediaStreamTrack of kind using getUserMedia. |
|
Thanks, @soareschen! @jan-ivar it looks like you're the final stakeholder we need to approve this. If it looks good to you, would you mind merging it? |
|
Sure thing, @jan-ivar. @soareschen would you do the honors? |
This is not exactly correct, although it is true that it was not discussed deeply.
Some differences between the two approaches:
Btw, the current way of testing whether getUserMedia should be used or not will not work for WebKit. window.MediaStreamAudioDestinationNode for instance is defined but cannot currently be used with RTCPeerConnection. Ideally, there should be an easy way to decide whether to switch between the two approaches. |
Makes sense once we have WebDriver-controlled getUserMedia. I think ideally we'd like to test all the things—especially web-exposed features—so we have a bit of a false choice atm. RTCPeerConnection is source-agnostic, even though camera and microphone are obviously the most important. Firefox's regular
I think in those cases, captureStream and webaudio can make sense, as they naturally give tests better control over the output. Mocks in contrast would be limited to fixed output, which might suffice, if we can get vendors to agree on what the output should be. Happy to do our part there. Short-term, is there an opportunity here to have
Is there a way to feature-detect that they don't work with RTCPeerConnection? |
When scripted to rely on
getUserMedia, tests for WebRTC require humaninteraction and physical capture devices. Some browsers offer features
to circumvent these requirements, but such functionality is non-standard
and subverts code paths used in real-world settings.
Create media streams algorithmically in order to eliminate the
dependency on the
getUserMediaAPI and better facilitate automation.This patch is based on a suggestion from @jan-ivar in gh-7424, who wrote:
It does not rely on any out-of-band configuration, implementation-specific code, or standardized test APIs (e.g. new WebDriver commands). In other words, the tests are more portable, more realistic, and require less effort to execute. For these reasons, it seems preferable to the
getUserMedia-based approach implemented inmastertoday.It may be that I'm missing a detail of WebRTC that makes this approach inappropriate. Anecdotally, it improves results for both Chromium and Firefox when run in automation. Here's how this patch affects test results in Chromium and Firefox Nightly (additional details about testing procedure are included below):
This data does not communicate changes in the subtest failures themselves. In that regard, the patch may improve things for Chromium because the error messages more clearly relate to the feature under test. I've included a comparison of the "WPT report" for both Chromium and Firefox below. (This is the data structure generated by the WPT CLI via the
--log-wptreportoptions.)It is more difficult for me to validate the change in WebKit/Safari, though there may be a problem using this approach there. In gh-7424, @youennf wrote:
Naively, I expected such filtering would interfere with a
getUserMedia-based solution, as well (since it concerns the candidate's IP address, not the way the media stream). @youennf: could you elaborate?Testing environment
masterbranch:--binary-arg=--use-fake-ui-for-media-stream --binary-arg=--use-fake-device-for-media-streammasterbranch:--setpref 'media.navigator.streams.fake=true' --setpref 'media.navigator.permission.disabled=true'All WPT CLI processes were run via
xvfb-run --auto-servernumto simulate a "headless" testing environment. The browser-specific configuration was informed by a recent discussion on collecting WebRTC results for https://wpt.fyi: web-platform-tests/results-collection#125Comparison of Chromium reports
Comparison of Firefox reports