Skip to content
This repository was archived by the owner on Jun 28, 2024. It is now read-only.

Mute track#79

Merged
kamil-stasiak merged 19 commits intomainfrom
mute-track
Jun 13, 2024
Merged

Mute track#79
kamil-stasiak merged 19 commits intomainfrom
mute-track

Conversation

@kamil-stasiak
Copy link
Copy Markdown
Contributor

Description

This PR introduces changes from: fishjam-dev/ts-client-sdk#49 and also:

useCamera, useMicrophone, useScreenshare

Hooks for managing a single track of a given type in the global state (similarly to the React Native API).

onDeviceStop

onDeviceStop?: "remove" | "mute"; This PR introduces a new onDeviceStopconfig parameter that allows the user to decide what to do if a track stops. Without it, the track has been removed fromRTCPeerConnection. Now, with the option mute, that track can be reused later without renegotiation. It is a companion event to onDeviceChange`.

Added onDeviceStop, onDeviceChange, and the ability to stop a device to use-camera-and-microphone-example.

onDeviceChange

onDeviceChange?: "replace" | "stop";
Determines whether a track should be replaced when the user requests a device change. Previously, broadcastOnDeviceChange.

Other changes

  • Names of types like UseMicrophoneResult changed to something simpler, like MicrophoneAPI.

Motivation and Context

Fishjam now offers the ability to reuse tracks.

Types of changes

  • Bug fix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a fix or feature that would cause existing functionality to not work as expected)
    • The replaceTrack method can handle a nullable track parameter.
    • The stream parameter was removed from the addTrack method.

Comment thread package.json Outdated
},
"dependencies": {
"@fishjam-dev/ts-client": "^0.5.0",
"@fishjam-dev/ts-client": "github:fishjam-dev/ts-client-sdk#mute-track",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replace with released version

@kamil-stasiak kamil-stasiak marked this pull request as ready for review June 6, 2024 09:11
Comment thread src/Client.ts Outdated
},
replaceTrack: async (newTrackMetadata?: TrackMetadata) => {
const prevTrack = Object.values(localTracks).find((track) => track.track?.id === this.currentCameraTrackId);
if (!this.currentCameraTrackId) throw Error("There is no audio track id");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

audio track?

Comment on lines 30 to 38
type OnDeviceChange = "stop" | "replace" | undefined;
type OnDeviceStop = "remove" | "mute" | undefined;

const isRestartChange = (e: string | undefined): e is RestartChange => {
return e === undefined || e === "stop" || e === "replace";
};
const isDeviceChangeValue = (e: string | undefined): e is OnDeviceChange =>
e === undefined || e === "stop" || e === "replace";

const isDeviceStopValue = (e: string | undefined): e is OnDeviceStop =>
e === undefined || e === "remove" || e === "mute";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't get it... why is "stop" in OnDeviceChange not in "OnDeviceStop"? Does the user also need to write such functions? Why undefined is ok?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Config accepts an optional string which can be stop or replace. Undefined is for situations when the user skips this parameter (which is valid because there is a default value). The user doesn't need to implement them because that config should be hardcoded. These validators are included because this tool tests this config.

Quick explanation:

OnDeviceChange
When the connection is established and the user changes a device, I saw two possible paths. Either the track from a new device replaces the old track, and other people would see it (replace), or the track would be stopped to prevent showing something that should not be shown (stop). I think I should change this word to remove to be consistent with onDeviceStop.

Now, there is 3 option, we are capable of muting this track and reusing it later. This whole idea is just a proof of concept, and I don't know if we should implement this behavior in all our clients, so I don't want to waste time to implement it right now.

OnDeviceStop
When the user stops their device, I see only two options: remove (or stop) or mute.

So, the word stop is confusing. I'll change it.

@kamil-stasiak kamil-stasiak requested a review from graszka22 June 12, 2024 09:01
Copy link
Copy Markdown

@graszka22 graszka22 left a comment

Choose a reason for hiding this comment

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

ok, I think I got it, LGTM

@kamil-stasiak kamil-stasiak merged commit cb4eeed into main Jun 13, 2024
@kamil-stasiak kamil-stasiak deleted the mute-track branch June 13, 2024 08:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants