Currently, ICE candidates of type 'host' will not be gathered/handed out to the application in case no capture permission has been granted. While this may be a perfect solution for audio/video, it leaves data channel only applications with either srflx/relay only or with the need for an unfitting permission request. The biggest issue with this approach is: How do you explain to the user that you need capture permissions for a direct peer-to-peer connection? That smells fishy and I understand everyone who will decline such a request for this exact reason. So, if the user rejects this permission request, secondary issues are that you a) now always need to provide a STUN server, and b) always need to provide a TURN server (which itself is rather expensive in terms of operating costs and may not be feasible for small open source applications for this reason) because there are plenty of routers who are not capable of doing NAT loopback (yep, even models from 2017). Thus, one cannot rely on server reflexive candidates. My proposal would be that the application can request a permission to "establish a direct connection" (maybe also displaying some hints on what this means towards privacy, etc.) which enables the use of host candidates. (P.S.: You should probably add a WebRTC component to your Bugzilla.)
<rdar://problem/34134281>
Another use case is a viewer in a webinar that only receive audio/video tracks but doesn't transmit. Requesting mic/cam permission might confuse a user that is only intended to be a viewer. In addition to the proposed additional permission request type I suggest that if the website is saved to the home screen in iOS the restrictions will be relaxed.
*** Bug 176921 has been marked as a duplicate of this bug. ***
In general I wouldn't assume that a WebRTC connection is always bi-directional. In my use case I stream multiple live videos from a server to the client. In this scenario I'm using VPN without any NATs. I also would like to avoid using a TURN server. As @Lennart Grahl already mentioned, asking the user for cam/mic permission without even really using it is definitely not a good work around.
As @Ben noted, the important use case of streaming video to a user via WebRTC doesn't fit with the permissions request required. To tell a user that they have to grant access to their camera/mic doesn't make any sense when all they are trying to do is view a video without sending any media back. No user will understand why they have to give permission in this scenario. Basically, getUserMedia shouldn't require camera/mic permissions when the data is not bidirectional. Developers should be able to specify that the data is streaming to the user only, and therefore doesn't require permission. If you decide that it's necessary to request permission, the permission should be fit for the purpose rather than requiring them to grant access to permissions for components that aren't being used.
Created attachment 336643 [details] Patch
(In reply to youenn fablet from comment #6) > Created attachment 336643 [details] > Patch Youenn, using mDNS with UUIDs seems like a clever approach to me and is definitely a step forward which I appreciate. If you plan on suggesting to specify this in ICE (or as an extension to ICE) - great! If not, then this is a proprietary solution limited to Webkit. Cheers Lennart
> using mDNS with UUIDs seems like a clever approach to me and is definitely a > step forward which I appreciate. If you plan on suggesting to specify this > in ICE (or as an extension to ICE) - great! If not, then this is a > proprietary solution limited to Webkit. This is an experiment. If proved to be successful, we hope it could be made interoperable with other browsers.
Created attachment 336667 [details] Patch
Created attachment 336670 [details] Patch
Created attachment 336673 [details] Patch
Comment on attachment 336673 [details] Patch Attachment 336673 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7126070 New failing tests: webrtc/datachannel/mdns-ice-candidates.html
Created attachment 336681 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 336685 [details] Patch
Comment on attachment 336685 [details] Patch Attachment 336685 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7127141 New failing tests: webrtc/datachannel/mdns-ice-candidates.html
Created attachment 336709 [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.6
Created attachment 336716 [details] Patch
Created attachment 336719 [details] Patch
Created attachment 336720 [details] Patch
Putting under review. It seems that on iOS simulator the 250 ms MDNS timeout might be too small.
Comment on attachment 336720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336720&action=review > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:289 > + provider.resolveMDNSName(name, [peerConnection = makeRef(m_peerConnection), this, name, iceCandidate = makeRef(*iceCandidate), promise = WTFMove(promise)] (auto&& result) mutable { Nit: I think "auto" is not a good choice here because the only way for someone that isn't already familiar with the code to figure out "result"'s type is to dig through the libWebRTCProvider header. > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:295 > + // FIXME: Log error. It is probably worth logging the error to the console in this patch, even if that isn't the long term solution. > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:435 > + // FIXME: We might need to clear al pending candidates when setting again local description. Nits: "al" => "all" "when setting again local description" => "when setting local description again" > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:467 > + provider.registerMDNSName(document, ipAddress, [peerConnection = makeRef(m_peerConnection), this, ipAddress] (auto&& result) { Ditto the "auto" comment above. > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:473 > + // FIXME: Log error. Ditto the comment above about logging the error. > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:78 > + virtual void unregisterMDNSNames(Document&) { } > + > + virtual void registerMDNSName(Document& document, const String& ipAddress, CompletionHandler<void(MDNSNameOrError&&)>&& callback) It is a layering violation to use Document here. Can you just use the identifier? > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:80 > + request->connection->send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Why not log the error here, or pass the error value to be logged in the web process? > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:92 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:96 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:104 > + String baseName = createCanonicalUUIDString(); Nit: is the local variable necessary? > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:109 > + if (ip == ( in_addr_t)(-1)) { Nit: unnecessary space after the "(". > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:110 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto the comment above about logging or passing a meaningful error to the web process. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:129 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:154 > + connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:173 > + request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:181 > + request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:193 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:207 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:214 > + m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0); Ditto. > Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:88 > + finishedRegisteringMDNSName(m_pendingRequestsIdentifier, makeUnexpected(MDNSRegisterError::General)); Ditto. > Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:97 > + finishedResolvingMDNSName(m_pendingRequestsIdentifier, makeUnexpected(MDNSRegisterError::General)); Ditto.
Created attachment 337239 [details] Patch for landing
Attachment 337239 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/Network/webrtc/LibWebRTCProvider.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:60: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:95: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 3 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for the review, I took it all. Next step is integrating the MDNS resolution at some lower level so that we do not add this unnecessary delay, ideally at libwebrtc level, otherwise at the WebCore/libwebrtc binding level.
Created attachment 337243 [details] Patch for landing
Attachment 337243 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/Network/webrtc/LibWebRTCProvider.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:60: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 2 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 337243 [details] Patch for landing Clearing flags on attachment: 337243 Committed r230290: <https://trac.webkit.org/changeset/230290>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 337275 [details] Patch
Comment on attachment 337275 [details] Patch Clearing flags on attachment: 337275 Committed r230307: <https://trac.webkit.org/changeset/230307>
So, was the experiment a success? Would you like to pursue this further and incorporate it into the spec?
*** Bug 186302 has been marked as a duplicate of this bug. ***