-
Notifications
You must be signed in to change notification settings - Fork 30
[Fix]Fast reconnection flow due to ICE misconfiguration #741
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
Conversation
SDK Size
|
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.
Looks good 👍 Let's do some testing today and tomorrow before merging it.
| fileName: file, | ||
| lineNumber: line | ||
| ) | ||
| if error is CancellationError { /* No-op */ } |
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.
should we maybe add a debug log here that the task was cancelled? Also the formatting looks weird.
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.
That wouldn't be necessary. Cancelled tasks aren't errors but it means that the operation is invalid (most probably because the initiator of the task has e.g. deallocated). For formatting I just using that kind of formatting across the code base as i won't to make the cancellationError skipping as minimal as possible. Happy to expand it in 3 lines if you want to.
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 meant a debug log, not an error log
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 still don't think it's necessary. This is lowest component for executing the tasks. The only way to get cancelled tasks is to do it explicitely or deallocate the object. In both options the higher level code should log.
Sources/StreamVideo/WebRTC/v2/PeerConnection/Adapters/ICEAdapter.swift
Outdated
Show resolved
Hide resolved
...WebRTC/v2/PeerConnection/MediaAdapters/LocalMediaAdapters/LocalScreenShareMediaAdapter.swift
Show resolved
Hide resolved
Sources/StreamVideo/WebRTC/v2/StateMachine/Stages/WebRTCCoordinator+Joining.swift
Show resolved
Hide resolved
Public Interface public final class SerialActorQueue: Sendable
- public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func cancelAll()
- public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws
+ public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws |
Generated by 🚫 Danger |
b1ffa99 to
17da89a
Compare
Public Interface public final class SerialActorQueue: Sendable
- public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func cancelAll()
- public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws
+ public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws |
17da89a to
0aba5f7
Compare
Public Interface public final class SerialActorQueue: Sendable
- public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func cancelAll()
- public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws
+ public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws |
| /// Handles a JoinResponse event. | ||
| /// | ||
| /// - Parameter event: The JoinResponse event to handle. | ||
| private func handleJoinResponse( |
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.
We remove the handling from the SFUEventAdapter as the processing of the JoinResponse happens in WebRTCCoordinator+Joining. Having it here causes duplicated processing of the event.
Public Interface public final class SerialActorQueue: Sendable
- public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func cancelAll()
- public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws
+ public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws |
Public Interface public final class SerialActorQueue: Sendable
- public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func cancelAll()
- public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws
+ public func async(file: StaticString = #file,functionName: StaticString = #function,line: UInt = #line,_ block: @Sendable @escaping () async throws -> Void)
+ public func sync(_ block: @Sendable @escaping () async throws -> Void)async throws |
|



📝 Summary
FastReconnection seems to have been broken due to ICE candidates not being applied correctly for the subscriber.
🛠 Implementation
We use this revision as an opportunity to align the fast reconnection process to the latest requirements from the SFU. Namely the changes are:
RTCPeerConnection.restartICEmethod but instead sends theiceRestartRPC call to the SFU. The SFU then will send a new subscriberOffer that will trigger the flow to update the ice state on the subscriber connection.🧪 Manual Testing Notes
☑️ Contributor Checklist