Expose webrtc in TheoLiveSource on iOS/Android #707
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes WebRTC configuration options for THEOlive Millicast sources on iOS. The change adds a new webrtc property to the TheoLiveSource interface that allows developers to configure playout delay settings when using THEOlive with WebRTC streaming.
- Added
webrtcproperty to TypeScript interface for THEOlive sources - Implemented iOS Swift code to parse and apply WebRTC playout delay configuration
- Enables control over minimum and maximum playout delay values through the WebRTC options
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/api/theolive/TheoLiveSource.ts | Adds optional webrtc property to TheoLiveSource TypeScript interface for WebRTC configuration |
| ios/theolive/THEOplayerRCTSourceDescriptionBuilder+Theolive.swift | Implements iOS-specific parsing and building of WebRTC options with playout delay settings from JavaScript configuration |
src/api/theolive/TheoLiveSource.ts
Outdated
| /** | ||
| * WebRTC configuration for a THEOlive Millicast source. | ||
| */ | ||
| webrtc?: Record<string, any> |
There was a problem hiding this comment.
Missing semicolon at the end of the webrtc property declaration. All other properties in this interface (e.g., line 23) end with a semicolon, so this should too for consistency.
| webrtc?: Record<string, any> | |
| webrtc?: Record<string, any>; |
src/api/theolive/TheoLiveSource.ts
Outdated
| /** | ||
| * WebRTC configuration for a THEOlive Millicast source. | ||
| */ | ||
| webrtc?: Record<string, any> |
There was a problem hiding this comment.
The type Record<string, any> is too permissive and loses type safety. Based on the iOS implementation which expects a playoutDelayMs object with minimum and maximum properties, consider defining a specific interface for the webrtc configuration. This would provide better type checking and IDE support for developers using this API. For example, you could define:
interface WebrtcPlayoutDelay {
minimum: number;
maximum: number;
}
interface TheoLiveWebrtcOptions {
playoutDelayMs?: WebrtcPlayoutDelay;
}Then use webrtc?: TheoLiveWebrtcOptions instead of Record<string, any>.
There was a problem hiding this comment.
If indeed the record entries are known in advance and we are expecting specific values I'd rather have them explicitly typed.
Also perhaps rename webrtc to something that more suggests it contains a config? i.e. webrtcConfiguration?
There was a problem hiding this comment.
It is named webrtc on other platforms, so I decided to be consistent. Though, I will change its type to a record instead
| profile?: string; | ||
|
|
||
| /** | ||
| * WebRTC configuration for a THEOlive Millicast source. |
There was a problem hiding this comment.
Missing platform annotation in the documentation. Since this webrtc configuration is only implemented for iOS (as indicated by the PR title and the fact that there's no corresponding Android implementation in SourceAdapter.kt), the documentation should include a @platform ios annotation to make it clear to developers which platforms support this feature.
| * WebRTC configuration for a THEOlive Millicast source. | |
| * WebRTC configuration for a THEOlive Millicast source. | |
| * @platform ios |
Yousif-CS
changed the title
| private static func buildWebrtcOptions(_ theoliveData: [String: Any]) -> WebrtcOptions? { | ||
| let webrtc = theoliveData[SD_PROP_WEBRTC] as? [String: Any] | ||
| let playoutDelay = webrtc?[SD_PROP_PLAYOUT_DELAY] as? [String: Int] | ||
| let playoutDelayMin = playoutDelay?[SD_PROP_PLAYOUT_DELAY_MIN] |
There was a problem hiding this comment.
Let's indeed make this explicit in the Typescript typing.
There was a problem hiding this comment.
alright done. @RameshDPrasad could you confirm it still works on android?
|
I've also exposed WebrtcOptions in the Endpoint, as it is public on iOS and Android. |
|
@wvanhaevre please have a look :) |
|
@Yousif-CS, don't forget to add a changelog entry. |
|
|
||
| extension THEOplayerRCTSourceDescriptionBuilder { | ||
|
|
||
| private static func buildWebrtcOptions(_ theoliveData: [String: Any]) -> WebrtcOptions? { |
There was a problem hiding this comment.
WebrtcOptions is a THEOplayerSDK type?
|
@wvanhaevre should we wait to merge until 10.7 release is out? Otherwise develop will be broken (we need |
Yes, we wait for the 10.7 to be released. Currently our CI testing fails because of the missing types. We can finish the PR and once the native SDK's are released we can make a react-native-theoplayer release. |
Yousif-CS
force-pushed
the
OPTI-658-millicast-config-react-native
branch
from
f6ca5f3 to
d0557a9
Compare
4 participants
As the title says. Related ticket: https://opentelly.atlassian.net/browse/OPTI-658