Firefox case when screen and camera offer together #2107

boks1971 · 2022-01-31T07:17:24Z

Description

Firefox camera tracks do not fire onTrack when screen share
and camera is negotiated in the same offer.

There were two issues

When updateRemoteDescription runs, only the first
media description of a specific kind was considered.
So, when screen was before camera, screen did not
save RID in extmap. It is in the camera section as
video is using simulcast. All the extra ones in the
camera section were ignored.

Parse all descriptions and collect extensions to fix it.

This one sounds like a Firefox issue. When sending
answer back, if the screen share section has the
RID, Firefox seems to get stuck. It does not send
screen or camera media and after 15 seconds, the
peer connection fails.

To address that, store extensions from SDP in
transceiver and while constructing answer SDP,
cross check against transceiver's negotiated
extensions before adding it to the answer SDP.

The reason I think this is a Firefox issue is
because how it behaves before this change when
camera is negotiated first and screen later in
a separate offer. When screen is negotiated
later, answer does send back RID and it works.
Looks like Firefox has an issue seeing RID
before it expects it.

Testing

Used LiveKit JS SDK sample to negotiate screen and
camera together.

This was not an issue with Chrome as I could not
get Chrome to negotiate screen and camera in the
same offer with screen ahead of camera.

boks1971 · 2022-01-31T07:18:12Z

mediaengine.go

 			m.negotiatedAudio = true
 			typ = RTPCodecTypeAudio
-		case !m.negotiatedVideo && strings.EqualFold(media.MediaName.Media, "video"):
+		case strings.EqualFold(media.MediaName.Media, "video"):


This is for point 1 in the PR commit notes. More than one media description of the same kind was getting ignored.

codecov · 2022-01-31T07:18:48Z

Codecov Report

Patch coverage: 88.37% and project coverage change: -0.07% ⚠️

Comparison is base (94262c1) 77.14% compared to head (395cb28) 77.07%.
Report is 223 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2107      +/-   ##
==========================================
- Coverage   77.14%   77.07%   -0.07%     
==========================================
  Files          87       87              
  Lines        8820     8850      +30     
==========================================
+ Hits         6804     6821      +17     
- Misses       1602     1613      +11     
- Partials      414      416       +2

Flag	Coverage Δ
go	`79.00% <88.37%> (-0.09%)`	⬇️
wasm	`70.02% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
sdp.go	`87.15% <33.33%> (-0.35%)`	⬇️
mediaengine.go	`91.51% <85.71%> (-0.39%)`	⬇️
rtptransceiver.go	`88.43% <94.73%> (+0.77%)`	⬆️
peerconnection.go	`77.77% <100.00%> (+0.06%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mediaengine.go

boks1971 · 2022-01-31T07:19:34Z

peerconnection.go

+					}
+					_ = t.SetHeaderExtensions(headerExtensions)
+				}
+


Storing the extension from SDP media description in the transceiver.

peerconnection.go

boks1971 · 2022-01-31T07:21:28Z

sdp.go

-		extURL, err := url.Parse(rtpExtension.URI)
-		if err != nil {
-			return false, err
+		for _, te := range t.getHeaderExtensions() {


This is for point 2. Cross check with transceiver's extmap and ad only the intersection in the answer.

Firefox camera tracks do not fire onTrack when screen share and camera is negotiated in the same offer. There were two issues 1. When updateRemoteDescription runs, only the first media description of a specific kind was considered. So, when screen was before camera, screen did not $ave RID in extmap. It is in the camera section as video is using simulcast. All the extra ones in the camera section were ignored. Parse all descriptions and collect extensions to fix it. 2. This one sounds like a Firefox issue. When sending answer back, if the screen share section has the RID, Firefox seems to get stuck. It does not send screen or camera media and after 15 seconds, the peer connection fails. To address that, store extensions from SDP in transceiver and while constructing answer SDP, cross check against transceiver's negotiated extensions before adding it to the answer SDP. The reason I think this is a Firefox issue is because how it behaves before this change when camera is negotiated first and screen later in a separate offer. When screen is negotiated later, answer does send back RID and it works. Looks like Firefox has an issue seeing RID before it expects it. Testing -------- Used LiveKit JS SDK sample to negotiate screen and camera together. This was not an issue with Chrome as I could not get Chrome to negotiate screen and camera in the same offer with screen ahead of camera.

…share_and_camera

boks1971 · 2022-01-31T08:37:39Z

rtptransceiver.go

+	// if nothing specific set, accept all
+	if len(t.headerExtensions) == 0 {
+		return true
+	}


Had to add this accept all case to fix a test. But, I think this is fine to keep the old behaviour.

Sean-Der · 2022-01-31T14:53:45Z

rtptransceiver.go

@@ -80,6 +81,33 @@ func (t *RTPTransceiver) getCodecs() []RTPCodecParameters {
 	return filteredCodecs
 }

+// SetHeaderExtensions stores header extension from SDP
+func (t *RTPTransceiver) SetHeaderExtensions(headerExtensions []RTPHeaderExtensionParameter) error {


Would it be ok if we did RTPSender.SetParameters

I don't see this exposed on the receiver side. Still looking

I went with parallel to SetCodecPreferences which is on the transceiver now. I think it could be in the transceiver as sender and receiver on a transceiver should use the same codec/header extensions.

Sean-Der · 2022-01-31T14:55:45Z

@boks1971 This LGTM!

What happens if you have multiple MediaDescriptions with different extension IDs?

boks1971 · 2022-01-31T16:32:38Z

@boks1971 This LGTM!

What happens if you have multiple MediaDescriptions with different extension IDs?

I had the same question @Sean-Der . I do not remember if the spec mandates that the same ID should be used across media sections.

But, I tried to maintain current behaviour as I do not know the history of the changes. Also, there is the Plan B curve ball. In fact, I think we should probably remove the negotiated stuff in media engine (both codecs and header extensions). The info from SDP should be in the corresponding transceiver and when constructing an answer, we should do intersection of what is registered in the media engine and what has been negotiated.

boks1971 · 2022-02-03T11:32:39Z

@Sean-Der @davidzhao Any concerns with this PR?

Sean-Der · 2022-02-03T21:35:44Z

@boks1971 I am happy with everything except SetHeaderExtensions .

I would like to avoid adding SetHeaderExtensions and instead have it match the browser and do RTPSender.SetParameters . This is a long term vision, but would love to have one code base that people could run native + web + mobile clients.

Making this all work is a long way away, but if we don’t match web makes it harder to do. If it is intractable I understand, but would be nice to avoid!

boks1971 · 2022-02-04T17:55:59Z

Thank you @Sean-Der . I will try to spend more time on this next week.

If the SRTP Stream hasn't been opened yet we would attempt to process (and fail) RTP traffic. This change causes us to not even attempt to read. No behavior change from this, but will cause less useless logging.

`AcceptStream` will then be fired again for this SSRC. We depend on the behavior of AcceptStream only returning once for each SSRC.

Generated by renovateBot

Assert that Simulcast probe thread doesn't close the Tracks that have been emitted to the user.

When creating answer, check ICE role while determining DTLS role. ORTC thread on how roles are set w3c/ortc#167 (comment) When remote is ice-lite and there is no answering role set, the answer uses the default which is DTLS client. So 'setup' is set as 'active' and answer sent. But, when DTLS transport is started, it checks the ICE role and sets itself as DTLS server (because remote is lite and hence local agent becomes controlling ICE agent). So, both sides end up being DTLS server and nobody sends client hello.

DTLS should be able to be negotiated over an already established ICETransport Resolves #2113

DTLS v2.1.2 fixes a bug with long delay connections. https://github.com/pion/dtls/releases/tag/v2.1.2

copySessionDescription adds a button to all examples that copies the local Session Description. This makes it easier for users to copy the values. Resolves #2092

Been running these locally only.

Generated by renovateBot

Add SetDTLSRetranmissionInterval setting to SettingEngine. Add test for SetDTLSRetransmissionInterval

Update lint scripts and CI configs.

Only available in the browser, not in wrtc yet. Resolves #2099

…share_and_camera

Sean-Der · 2023-08-06T17:25:54Z

Hey @boks1971 I am going to resolve this, but please re-open if you would like to see this merged.

I haven't seen/heard this impacting anyone else. Did Mozilla update anything on their side to include header extensions in all media sections?

boks1971 · 2023-08-07T03:46:22Z

Thank you @Sean-Der . I have not heard of a fix, but have not heard of more instances of either. Will keep an eye out and re-open if necessary 🙇

davidzhao · 2023-08-07T05:11:13Z

I just verified and looks like this is still an issue. If a FF client offers screen share first, the camera track would not be recognized afterwards.

boks1971 · 2023-08-07T05:19:05Z

Thank you @davidzhao . Will re-visit this some time soon.

Sean-Der · 2023-08-07T13:05:01Z

I can take it from here @boks1971 ! I have some time

boks1971 · 2023-08-07T13:11:59Z

Thank you so much @Sean-Der .

boks1971 commented Jan 31, 2022

View reviewed changes

mediaengine.go Show resolved Hide resolved

boks1971 commented Jan 31, 2022

View reviewed changes

peerconnection.go Show resolved Hide resolved

boks1971 commented Jan 31, 2022

View reviewed changes

boks1971 requested a review from Sean-Der January 31, 2022 07:21

pionbot force-pushed the raja_firefox_screenshare_and_camera branch from 9434296 to 249c12e Compare January 31, 2022 07:50

boks1971 force-pushed the raja_firefox_screenshare_and_camera branch from 249c12e to ea9aa35 Compare January 31, 2022 07:50

pionbot force-pushed the raja_firefox_screenshare_and_camera branch from afad205 to 487f938 Compare January 31, 2022 07:54

boks1971 force-pushed the raja_firefox_screenshare_and_camera branch from b0d3bdd to e5c9056 Compare January 31, 2022 08:30

Merge remote-tracking branch 'origin/master' into raja_firefox_screen…

9498a64

…share_and_camera

boks1971 commented Jan 31, 2022

View reviewed changes

Sean-Der reviewed Jan 31, 2022

View reviewed changes

Sean-Der and others added 10 commits February 9, 2022 19:03

Ignore explicit SSRCes in RID Simulcast processing

55de02b

If the SRTP Stream hasn't been opened yet we would attempt to process (and fail) RTP traffic. This change causes us to not even attempt to read. No behavior change from this, but will cause less useless logging.

Don't close the SRTP Stream in handleIncomingSSRC

ad48d0d

`AcceptStream` will then be fired again for this SSRC. We depend on the behavior of AcceptStream only returning once for each SSRC.

Update golang.org/x/net commit hash to cd36cc0

f400e1a

Generated by renovateBot

Add test for 65b517

687d924

Assert that Simulcast probe thread doesn't close the Tracks that have been emitted to the user.

Remove ICETransportStateNew check in DTLS Start

5e99129

DTLS should be able to be negotiated over an already established ICETransport Resolves #2113

Update DTLS to v2.1.2

9757a0e

DTLS v2.1.2 fixes a bug with long delay connections. https://github.com/pion/dtls/releases/tag/v2.1.2

Update all examples with copySessionDescription

0049913

copySessionDescription adds a button to all examples that copies the local Session Description. This makes it easier for users to copy the values. Resolves #2092

Add StandardJS workflow

1d3ab0f

Been running these locally only.

Update actions/setup-node action to v2

420fc85

Generated by renovateBot

boks1971 and others added 4 commits February 9, 2022 19:03

DTLS retransmission interval setting

e0baf30

Add SetDTLSRetranmissionInterval setting to SettingEngine. Add test for SetDTLSRetransmissionInterval

Update CI configs to v0.6.6

6a63f16

Update lint scripts and CI configs.

Add DTLS, ICE and SCTP Transport to WASM

5a5c1ce

Only available in the browser, not in wrtc yet. Resolves #2099

Merge remote-tracking branch 'origin/master' into raja_firefox_screen…

395cb28

…share_and_camera

boks1971 mentioned this pull request Feb 22, 2023

Enabling camera after enabling screen sharing doesn't work on Firefox livekit/livekit#1238

Open

Sean-Der closed this Aug 6, 2023

Sean-Der reopened this Aug 7, 2023

lukasIO mentioned this pull request Mar 21, 2024

Cannot share camera after screen sharing livekit/components-js#804

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firefox case when screen and camera offer together #2107

Firefox case when screen and camera offer together #2107

boks1971 commented Jan 31, 2022

boks1971 Jan 31, 2022 •

edited

codecov bot commented Jan 31, 2022 •

edited

boks1971 Jan 31, 2022

boks1971 Jan 31, 2022

boks1971 Jan 31, 2022

Sean-Der Jan 31, 2022

boks1971 Jan 31, 2022

Sean-Der commented Jan 31, 2022

boks1971 commented Jan 31, 2022

boks1971 commented Feb 3, 2022

Sean-Der commented Feb 3, 2022 •

edited

boks1971 commented Feb 4, 2022

Sean-Der commented Aug 6, 2023

boks1971 commented Aug 7, 2023

davidzhao commented Aug 7, 2023

boks1971 commented Aug 7, 2023

Sean-Der commented Aug 7, 2023

boks1971 commented Aug 7, 2023

Firefox case when screen and camera offer together #2107

Are you sure you want to change the base?

Firefox case when screen and camera offer together #2107

Conversation

boks1971 commented Jan 31, 2022

Description

Testing

boks1971 Jan 31, 2022 • edited

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2022 • edited

Codecov Report

boks1971 Jan 31, 2022

Choose a reason for hiding this comment

boks1971 Jan 31, 2022

Choose a reason for hiding this comment

boks1971 Jan 31, 2022

Choose a reason for hiding this comment

Sean-Der Jan 31, 2022

Choose a reason for hiding this comment

boks1971 Jan 31, 2022

Choose a reason for hiding this comment

Sean-Der commented Jan 31, 2022

boks1971 commented Jan 31, 2022

boks1971 commented Feb 3, 2022

Sean-Der commented Feb 3, 2022 • edited

boks1971 commented Feb 4, 2022

Sean-Der commented Aug 6, 2023

boks1971 commented Aug 7, 2023

davidzhao commented Aug 7, 2023

boks1971 commented Aug 7, 2023

Sean-Der commented Aug 7, 2023

boks1971 commented Aug 7, 2023

boks1971 Jan 31, 2022 •

edited

codecov bot commented Jan 31, 2022 •

edited

Sean-Der commented Feb 3, 2022 •

edited