-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Firefox case when screen and camera offer together #2107
Conversation
m.negotiatedAudio = true | ||
typ = RTPCodecTypeAudio | ||
case !m.negotiatedVideo && strings.EqualFold(media.MediaName.Media, "video"): | ||
case strings.EqualFold(media.MediaName.Media, "video"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for point 1 in the PR commit notes. More than one media description of the same kind was getting ignored.
Codecov ReportPatch coverage:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
} | ||
_ = t.SetHeaderExtensions(headerExtensions) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing the extension from SDP media description in the transceiver.
sdp.go
Outdated
extURL, err := url.Parse(rtpExtension.URI) | ||
if err != nil { | ||
return false, err | ||
for _, te := range t.getHeaderExtensions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for point 2. Cross check with transceiver's extmap and ad only the intersection in the answer.
9434296
to
249c12e
Compare
249c12e
to
ea9aa35
Compare
afad205
to
487f938
Compare
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.
b0d3bdd
to
e5c9056
Compare
// if nothing specific set, accept all | ||
if len(t.headerExtensions) == 0 { | ||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add this accept all case to fix a test. But, I think this is fine to keep the old behaviour.
@@ -80,6 +81,33 @@ func (t *RTPTransceiver) getCodecs() []RTPCodecParameters { | |||
return filteredCodecs | |||
} | |||
|
|||
// SetHeaderExtensions stores header extension from SDP | |||
func (t *RTPTransceiver) SetHeaderExtensions(headerExtensions []RTPHeaderExtensionParameter) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be ok if we did RTPSender.SetParameters
I don't see this exposed on the receiver side. Still looking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
@Sean-Der @davidzhao Any concerns with this PR? |
@boks1971 I am happy with everything except 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! |
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
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? |
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 🙇 |
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. |
Thank you @davidzhao . Will re-visit this some time soon. |
I can take it from here @boks1971 ! I have some time |
Thank you so much @Sean-Der . |
Closing this as a different PR address the underlying issue - #2444 |
Description
Firefox camera tracks do not fire onTrack when screen share
and camera is negotiated in the same offer.
There were two issues
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.
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.