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

feat(track-state): Log on add/remove/mute/owner. #13934

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Conversation

hristoterezov
Copy link
Member

No description provided.

@@ -845,7 +845,7 @@ export default {
// This will only modify base/media.audio.muted which is then synced
// up with the track at the end of local tracks initialization.
muteLocalAudio(mute);
this.setAudioMuteStatus(mute);
this.updateAudioIconEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need to call it here? What is the purpose of checking here if the audio icon needs to be enabled or disabled.

@@ -1394,7 +1394,7 @@ export default {
APP.store.dispatch(
replaceLocalTrack(oldTrack, newTrack, room))
.then(() => {
this.setAudioMuteStatus(this.isLocalAudioMuted());
this.updateAudioIconEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

APP.conference.setAudioMuteStatus(muted);
} else {
APP.UI.setAudioMuted(participantID, muted);
APP.conference.updateAudioIconEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

Same question here again. If we already know that a local audio track exists, why do we need to check if the mic icon needs to be enabled or disabled?

@@ -62,7 +71,32 @@ MiddlewareRegistry.register(store => next => action => {

case TRACK_REMOVED: {
_removeNoDataFromSourceNotification(store, action.track);
break;

const result = next(action);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to log it here since with ssrc-rewriting, tracks will never be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be useful for the case where the ssrc rewriting is not enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Even without ssrc rewriting, remote tracks are removed only when participants leave. Do you think track states will be helpful in that case. I am just worried about all the unnecessary logging.

@hristoterezov
Copy link
Member Author

About your questions related to the first commit:
Currently I don't have a good answer. I just noticed that we are calling a function from UI that calls a function from conference.js and works only for local tracks. So basically my idea was to simplify the code by leaving only the function in conference.js. Therefore the current code execution will do exactly what the previous version was doing.

Will check on the specific places that you pointed out if we actually we need to call the function although IMHO this is totally different story.

@hristoterezov
Copy link
Member Author

hristoterezov commented Oct 11, 2023

It seems that we try to update it whenever we do changes to the local tracks or devices. On some places it may be redundant but I think the whole thing needs to be refactored. In addition this is old logic which dispatches an action and changes the store which may have other implications. I'm worried to just remove some of the places in order not to break some weird use case. So lets leave it for another PR.

@hristoterezov hristoterezov merged commit 0becc89 into master Oct 11, 2023
6 checks passed
@hristoterezov hristoterezov deleted the track-state-log branch October 11, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants