webrtcHacks / adapter

Shim to insulate apps from spec changes and prefix differences. Latest adapter.js release:
https://webrtcHacks.github.io/adapter/adapter-latest.js
BSD 3-Clause "New" or "Revised" License
3.63k stars 846 forks source link

Safari <=12.0 media establishing does not work due to `removeExtmapAllowMixed` shim not being applied #1082

Open ostap0207 opened 3 years ago

ostap0207 commented 3 years ago

Please read first!

Please use discuss-webrtc for general technical discussions and questions.

Note: If the checkboxes above are not checked (which you do after the issue is posted), the issue will be closed.

Versions affected

Safari 12.0

adapter.js 7.7.0, 7.7.1, 8.0

Description

removeExtmapAllowMixed shim is not applied to iOS Safari 12. Based on this PR's comments the shim should be applied to Safari < 13.1, however it is not applied in iOS Safari 12.

I think the culprit is this check https://github.com/webrtcHacks/adapter/blob/master/src/js/common_shim.js#L327

  if (browserDetails.browser === 'safari' && browserDetails.version >= 605) {
    return;
  }

browserDetails.version returns AppleWebKit version and this version is 605 in both iOS Safari 12 and iOS Safari 13. For example here is UserAgent from iOS Safari 12: Mozilla/5.0 (iPhone; CPU iPhone OS 12_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1 here is UserAgent from iOS Safari 13: Mozilla/5.0 (iPhone; CPU iPhone OS 13_2_3 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.3 Mobile/15E148 Safari/604.1

Based on local testing this shim is only needed in Safari 12.0 and below, as all works correctly in Safari 12.1+

Steps to reproduce

  1. Go to https://appr.tc/ in Chrome 91 and create a room
  2. Go to that room from iOS Safari 12, allow camera, in console paste the latest adapter release https://github.com/webrtcHacks/adapter/blob/master/release/adapter.js. This is because https://appr.tc/ does not use the newest version at the moment.
  3. In iOS Safari press Join

Expected results

2-way video is established

Actual results

Error: setRemoteDescription: OperationError: Expects at least 2 fields.

Screenshot 2021-06-22 at 09 23 04
hthetiot commented 3 years ago

How about using feature detection instead of user agent sniffing. All what is needed is try setRemoteDescription with extmap in a test RTCconnection, that much more reliable than user agent sniffing.

ostap0207 commented 3 years ago

Wouldn't this require to always override setRemoteDescription, while we probably should override it only when needed?

ostap0207 commented 3 years ago

Any updates on this? This breaks media establishing with Safari 12.0 for us.

hthetiot commented 3 years ago

you can always do that in the meantime on your signaling layer.

        // Remove extmap-allow-mixed sdp header
    if (desc && desc.sdp && desc.sdp.indexOf('\na=extmap-allow-mixed') !== -1) {
        desc = new RTCSessionDescription({
            type: desc.type,
            sdp: desc.sdp
                .split('\n')
                .filter((line) => {
                    return line.trim() !== 'a=extmap-allow-mixed';
                })
                .join('\n')
        });
    }

via: https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/js/RTCPeerConnection.js#L316

ostap0207 commented 3 years ago

Thanks for the workaround!