Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
3 tasks done
ostap0207 opened this issue Jun 22, 2021 · 5 comments · May be fixed by #1127
Open
3 tasks done

Comments

@ostap0207
Copy link

ostap0207 commented Jun 22, 2021

Please read first!

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

  • I have provided steps to reproduce (e.g. a link to a jsfiddle)
  • I have provided browser name, version and adapter.js version
  • This issue only happens when adapter.js is used

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
Copy link

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
Copy link
Author

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

@ostap0207
Copy link
Author

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

@ostap0207 ostap0207 changed the title removeExtmapAllowMixed shim is not applied in iOS Safari 12.0 Safari <=12.0 media establishing does not work due to removeExtmapAllowMixed shim not being applied Sep 1, 2021
@hthetiot
Copy link

hthetiot commented Sep 1, 2021

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
Copy link
Author

Thanks for the workaround!

@lukasIO lukasIO linked a pull request May 16, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants