Skip to content
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

[FCE-207] Fix error on muted track renegotiation #18

Merged
merged 11 commits into from
Jul 4, 2024
1 change: 0 additions & 1 deletion examples/ts-client/simple-app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"vite-plugin-checker": "^0.6.4"
},
"dependencies": {
"@fishjam-dev/browser-media-utils": "https://github.com/fishjam-dev/browser-media-utils#1.0.0",
"@fishjam-dev/ts-client": "*"
}
}
25 changes: 12 additions & 13 deletions packages/react-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,21 @@
"module": "./dist/index.js",
"types": "./dist/index.d.ts",
"files": [
"dist/src"
"dist/**"
],
"bugs": {
"url": "https://github.com/fishjam-dev/react-client-sdk/issues"
},
"homepage": "https://github.com/fishjam-dev/react-client-sdk#readme",
"repository": "github:fishjam-cloud/web-client-sdk",
"homepage": "https://github.com/fishjam-cloud/web-client-sdk#readme",
"bugs": "https://github.com/fishjam-cloud/web-client-sdk/issues",
"keywords": [
"webrtc",
"membrane",
"fishjam"
],
"repository": {
"type": "git",
"url": "git://github.com/fishjam-dev/react-client-sdk.git"
},
"exports": {
".": "./dist/index.js"
".": {
"import": "./dist/index.js",
"types": "./dist/index.d.ts"
}
},
"typesVersions": {
"*": {
Expand All @@ -41,7 +39,7 @@
"format:check": "prettier --check . --ignore-path ./.eslintignore",
"lint:fix": "eslint . --ext .ts,.tsx --fix",
"lint:check": "eslint . --ext .ts,.tsx",
"prepare": "tsc"
"prepack": "yarn workspace @fishjam-dev/ts-client build && yarn build"
},
"devDependencies": {
"@playwright/test": "^1.42.1",
Expand All @@ -64,11 +62,12 @@
"typescript": "^5.4.0"
},
"dependencies": {
"@fishjam-dev/ts-client": "*",
"@fishjam-dev/ts-client": "workspace:*",
"events": "3.3.0",
"lodash.isequal": "4.5.0"
},
"directories": {
"example": "examples"
}
},
"packageManager": "[email protected]"
}
10 changes: 5 additions & 5 deletions packages/react-client/src/DeviceManager.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type {
AudioOrVideoType,
CurrentDevices,
DeviceError,
DeviceManagerConfig,
Expand All @@ -25,6 +24,7 @@ import {
import EventEmitter from "events";
import type TypedEmitter from "typed-emitter";
import type { TrackType } from "./ScreenShareManager";
import { TrackKind } from "@fishjam-dev/ts-client";

const removeExact = (
trackConstraints: boolean | MediaTrackConstraints | undefined,
Expand Down Expand Up @@ -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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo TrackType sounds better than TrackKind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it because MediaStreamTrack has a kind field

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe update the variable name as well to kind: TrackKind

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @czerwiukk. lets rename function parameter name to kind.

if (result.type === "OK") {
return result.previousErrors[type] || null;
}
Expand Down Expand Up @@ -477,7 +477,7 @@ export class DeviceManager extends (EventEmitter as new () => TypedEmitter<Devic
}
}

private onTrackEnded = async (type: AudioOrVideoType, trackId: string) => {
private onTrackEnded = async (type: TrackKind, trackId: string) => {
if (trackId === this?.[type].media?.track?.id) {
await this.stop(type);
}
Expand Down Expand Up @@ -637,14 +637,14 @@ export class DeviceManager extends (EventEmitter as new () => TypedEmitter<Devic
}
}

public async stop(type: AudioOrVideoType) {
public async stop(type: TrackKind) {
this[type].media?.track?.stop();
this[type].media = null;

this.emit("deviceStopped", { trackType: type }, { audio: this.audio, video: this.video });
}

public setEnable(type: AudioOrVideoType, value: boolean) {
public setEnable(type: TrackKind, value: boolean) {
if (!this[type].media || !this[type].media?.track) {
return;
}
Expand Down
7 changes: 4 additions & 3 deletions packages/react-client/src/ScreenShareManager.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import EventEmitter from "events";
import type TypedEmitter from "typed-emitter";
import type { AudioOrVideoType, DeviceError, DevicesStatus } from "./types";
import type { DeviceError, DevicesStatus } from "./types";
import { parseError } from "./types";
import type { TrackKind } from "@fishjam-dev/ts-client";

export type TrackType = "audio" | "video" | "audiovideo";
export type TrackType = TrackKind | "audiovideo";
export type MediaDeviceType = "displayMedia" | "userMedia";

export type DisplayMediaManagerEvents = {
Expand Down Expand Up @@ -129,7 +130,7 @@ export class ScreenShareManager extends (EventEmitter as new () => TypedEmitter<
}
}

private onTrackEnded = async (type: AudioOrVideoType, trackId: string) => {
private onTrackEnded = async (type: TrackKind, trackId: string) => {
const mediaType = type === "video" ? "videoMedia" : "audioMedia";
if (trackId === this?.data[mediaType]?.track?.id) {
await this.stop("audiovideo");
Expand Down
2 changes: 0 additions & 2 deletions packages/react-client/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import type { PeerStatus, Selector, State, Track, TrackId, TrackWithOrigin, UseR
import type { JSX, ReactNode } from "react";
import type { Client } from "./Client";

export type AudioOrVideoType = "audio" | "video";

export type DevicesStatus = "OK" | "Error" | "Not requested" | "Requesting";
export type MediaStatus = "OK" | "Error" | "Not requested" | "Requesting";

Expand Down
27 changes: 20 additions & 7 deletions packages/ts-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
"description": "Typescript client library for Fishjam",
"license": "Apache-2.0",
"author": "Software Mansion (https://swmansion.com)",
"repository": "github:fishjam-dev/ts-client-sdk",
"homepage": "https://github.com/fishjam-dev/ts-client-sdk#readme",
"bugs": "https://github.com/fishjam-dev/ts-client-sdk/issues",
"repository": "github:fishjam-cloud/web-client-sdk",
"homepage": "https://github.com/fishjam-cloud/web-client-sdk#readme",
"bugs": "https://github.com/fishjam-cloud/web-client-sdk/issues",
"keywords": [
"webrtc",
"membrane",
Expand All @@ -15,10 +15,21 @@
"main": "./dist/src/index.js",
"types": "./dist/src/index.d.ts",
"files": [
"dist/src"
"dist/src/**"
],
"exports": {
".": "./dist/src/index.js"
".": {
"import": "./dist/src/index.js",
"types": "./dist/src/index.d.ts"
},
"./protos": {
"import": "./dist/src/protos/index.js",
"types": "./dist/src/protos/index.d.ts"
},
"./webrtc": {
"import": "./dist/src/webrtc/index.js",
"types": "./dist/src/webrtc/index.d.ts"
}
},
"scripts": {
"build": "tsc",
Expand All @@ -32,7 +43,8 @@
"test": "vitest run tests/**",
"test:e2e": "NODE_OPTIONS=--dns-result-order=ipv4first playwright test",
"test:coverage": "vitest run tests/** --coverage",
"prepare": "tsc"
"prepare": "tsc",
"prepack": "yarn build"
},
"dependencies": {
"events": "^3.3.0",
Expand Down Expand Up @@ -73,5 +85,6 @@
"*.(js|ts|tsx)": [
"npm run lint:check"
]
}
},
"packageManager": "[email protected]"
}
22 changes: 19 additions & 3 deletions packages/ts-client/src/webrtc/bitrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
RemoteTrackId,
TrackContext,
TrackEncoding,
TrackKind,
} from './types';
import { TrackContextImpl } from './internal';
import { findSender } from './RTCPeerConnectionUtils';
Expand Down Expand Up @@ -73,6 +74,9 @@ export const getTrackIdToTrackBitrates = <EndpointMetadata, TrackMetadata>(
return trackIdToTrackBitrates;
};

const isNotSimulcastTrack = (encodings: RTCRtpEncodingParameters[]) =>
encodings.length === 1 && !encodings[0].rid;

export const getTrackBitrates = <EndpointMetadata, TrackMetadata>(
connection: RTCPeerConnection | undefined,
localTrackIdToTrack: Map<
Expand All @@ -84,16 +88,28 @@ export const getTrackBitrates = <EndpointMetadata, TrackMetadata>(
const trackContext = localTrackIdToTrack.get(trackId);
if (!trackContext)
throw "Track with id ${trackId} not present in 'localTrackIdToTrack'";
const kind = trackContext.track?.kind as 'audio' | 'video' | undefined;

const kind = trackContext.track?.kind as TrackKind | undefined;

if (!trackContext.track) {
if (!trackContext.trackKind) {
throw new Error('trackContext.trackKind is empty');
}

return defaultBitrates[trackContext.trackKind];
}

const sender = findSender(connection, trackContext.track!.id);
const encodings = sender.getParameters().encodings;

if (encodings.length == 1 && !encodings[0].rid)
if (isNotSimulcastTrack(encodings)) {
return (
encodings[0].maxBitrate ||
(kind ? defaultBitrates[kind] : UNLIMITED_BANDWIDTH)
);
else if (kind == 'audio') throw 'Audio track cannot have multiple encodings';
} else if (kind === 'audio') {
throw 'Audio track cannot have multiple encodings';
}

const bitrates: Record<string, Bitrate> = {};

Expand Down
1 change: 1 addition & 0 deletions packages/ts-client/src/webrtc/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export type {
EncodingReason,
Config,
MetadataParser,
TrackKind
} from './types';

export { WebRTCEndpoint } from './webRTCEndpoint';
Expand Down
5 changes: 5 additions & 0 deletions packages/ts-client/src/webrtc/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@ import {
TrackContext,
TrackContextEvents,
TrackEncoding,
TrackKind,
TrackNegotiationStatus,
VadStatus,
} from './types';

export const isTrackKind = (kind: string): kind is TrackKind =>
kind === 'audio' || kind === 'video';

export class TrackContextImpl<EndpointMetadata, ParsedMetadata>
extends (EventEmitter as {
new <EndpointMetadata, ParsedMetadata>(): TypedEmitter<
Expand All @@ -24,6 +28,7 @@ export class TrackContextImpl<EndpointMetadata, ParsedMetadata>
endpoint: Endpoint<EndpointMetadata, ParsedMetadata>;
trackId: string;
track: MediaStreamTrack | null = null;
trackKind: TrackKind | null = null;
stream: MediaStream | null = null;
metadata?: ParsedMetadata;
rawMetadata: any;
Expand Down
49 changes: 49 additions & 0 deletions packages/ts-client/src/webrtc/transceivers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { EndpointWithTrackContext, TrackContextImpl } from "./internal";
import { RemoteTrackId } from "./types";

export const getMidToTrackId = <EndpointMetadata, TrackMetadata>(
connection: RTCPeerConnection | undefined,
localTrackIdToTrack: Map<RemoteTrackId, TrackContextImpl<EndpointMetadata, TrackMetadata>>,
midToTrackId: Map<string, string> = new Map(),
localEndpoint: EndpointWithTrackContext<EndpointMetadata, TrackMetadata>,
): Record<string, string> | null => {
if (!connection) return null;

const active = getActive(connection, localTrackIdToTrack)
const muted = getMuted(midToTrackId, localEndpoint)

return { ...active, ...muted };
};

export const getActive = <EndpointMetadata, TrackMetadata>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is active? isn't the name lacking something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the code and comments, i'd go for getActiveTracks/mutedTracks

connection: RTCPeerConnection,
localTrackIdToTrack: Map<RemoteTrackId, TrackContextImpl<EndpointMetadata, TrackMetadata>>,
): Record<string, string> | null =>
connection.getTransceivers()
.filter((transceiver) => transceiver.sender.track?.id && transceiver.mid)
.reduce((acc, transceiver) => {
const localTrackId = transceiver.sender.track!.id;
const mid = transceiver!.mid!;

const trackContext = Array.from(localTrackIdToTrack.values()).find(
(trackContext) => trackContext?.track?.id === localTrackId,
)!;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move it to a function?


acc[mid] = trackContext.trackId;

return acc
}, {} as Record<string, string>);


export const getMuted = <EndpointMetadata, TrackMetadata>(
midToTrackId: Map<string, string> = new Map(),
localEndpoint: EndpointWithTrackContext<EndpointMetadata, TrackMetadata>,
): Record<string, string> => {
return [...midToTrackId.entries()]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

midToTrackId.entries() would not be enough?

Copy link
Contributor Author

@kamil-stasiak kamil-stasiak Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need it to map the result of entries(), which is an IterableIterator, to a simple array

.filter(([_mid, trackId]) => localEndpoint.tracks.get(trackId))
.reduce((acc, [mid, trackId]) => {
acc[mid] = trackId;

return acc
}, {} as Record<string, string>);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this function should return muted tracks, where do we filter out tracks that are not muted here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the names of these functions in this file, and I've added some comments

2 changes: 2 additions & 0 deletions packages/ts-client/src/webrtc/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export type MetadataParser<ParsedMetadata> = (
export type LocalTrackId = string;
export type RemoteTrackId = string;

export type TrackKind = 'audio' | 'video';

/**
* Type describing Voice Activity Detection statuses.
*
Expand Down
Loading
Loading