[FCE-207] Fix error on muted track renegotiation #18
Conversation
kamil-stasiak
requested review from
mironiasty,
graszka22,
czerwiukk,
karkakol and
Rados13
| @@ -164,7 +164,7 @@ const handleNotAllowedError = async (constraints: MediaStreamConstraints): Promi | |||
| return await getMedia({ video: false, audio: false }, { video: PERMISSION_DENIED, audio: PERMISSION_DENIED }); | |||
| }; | |||
|
|
|||
| const getError = (result: GetMedia, type: AudioOrVideoType): DeviceError | null => { | |||
| const getError = (result: GetMedia, type: TrackKind): DeviceError | null => { | |||
There was a problem hiding this comment.
Imo TrackType sounds better than TrackKind
There was a problem hiding this comment.
I changed it because MediaStreamTrack has a kind field
There was a problem hiding this comment.
maybe update the variable name as well to kind: TrackKind
There was a problem hiding this comment.
Agreed with @czerwiukk. lets rename function parameter name to kind.
| midToTrackId: Map<string, string> = new Map(), | ||
| localEndpoint: EndpointWithTrackContext<EndpointMetadata, TrackMetadata>, | ||
| ): Record<string, string> => { | ||
| return [...midToTrackId.entries()] |
There was a problem hiding this comment.
midToTrackId.entries() would not be enough?
There was a problem hiding this comment.
I need it to map the result of entries(), which is an IterableIterator, to a simple array
| const trackContext = Array.from(localTrackIdToTrack.values()).find( | ||
| (trackContext) => trackContext?.track?.id === localTrackId, | ||
| )!; |
| export const getMuted = <EndpointMetadata, TrackMetadata>( | ||
| midToTrackId: Map<string, string> = new Map(), | ||
| localEndpoint: EndpointWithTrackContext<EndpointMetadata, TrackMetadata>, | ||
| ): Record<string, string> => { | ||
| return [...midToTrackId.entries()] | ||
| .filter(([_mid, trackId]) => localEndpoint.tracks.get(trackId)) | ||
| .reduce((acc, [mid, trackId]) => { | ||
| acc[mid] = trackId; | ||
|
|
||
| return acc | ||
| }, {} as Record<string, string>); | ||
| } |
There was a problem hiding this comment.
If I understand correctly this function should return muted tracks, where do we filter out tracks that are not muted here?
There was a problem hiding this comment.
I've changed the names of these functions in this file, and I've added some comments
|
|
||
| const trackContext = this.trackIdToTrack.get(trackId)!; | ||
|
|
||
| trackContext.stream = stream; | ||
| trackContext.track = event.track; | ||
| trackContext.trackKind = event.track.kind; |
There was a problem hiding this comment.
i may have missed something, but why are we copying the event.track.kind information to another field while we can access it through trackCotnext.track.kind?
There was a problem hiding this comment.
The track field is optional; it's possible to mute a track (replace the track with null) and then unmute it in the future. We need the trackKind to assess bandwidth even though the track is muted
| return { ...active, ...muted }; | ||
| }; | ||
|
|
||
| export const getActive = <EndpointMetadata, TrackMetadata>( |
There was a problem hiding this comment.
what is active? isn't the name lacking something?
There was a problem hiding this comment.
looking at the code and comments, i'd go for getActiveTracks/mutedTracks
…ix-error-on-muted-track-renegotiation # Conflicts: # packages/react-client/package.json # packages/ts-client/package.json # packages/ts-client/src/webrtc/bitrate.ts # packages/ts-client/src/webrtc/webRTCEndpoint.ts
…ix-error-on-muted-track-renegotiation # Conflicts: # packages/react-client/package.json # packages/ts-client/package.json # yarn.lock
Description
This PR fixes an error that occurs when a muted track is renegotiated.
Motivation and Context
Steps to reproduce:
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected)
AudioOrVideoTypefromreact-clienthas been replaced byTrackKindfromts-client