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

fix(plugin-meetings): moderated unmute when client is remotely muted #3995

Draft
wants to merge 2 commits into
base: next
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/@webex/plugin-meetings/src/locus-info/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1406,7 +1406,7 @@ export default class LocusInfo extends EventsScope {
}
);
}
if (parsedSelves.updates.isMutedByOthersChanged) {
if (parsedSelves.updates.isMutedByOthersChanged.changed) {
this.emitScoped(
{
file: 'locus-info',
Expand All @@ -1416,6 +1416,7 @@ export default class LocusInfo extends EventsScope {
{
muted: parsedSelves.current.remoteMuted,
unmuteAllowed: parsedSelves.current.unmuteAllowed,
isModifiedBySelf: parsedSelves.updates.isMutedByOthersChanged.isModifiedBySelf,
}
);
}
Expand Down
21 changes: 14 additions & 7 deletions packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,20 +419,27 @@ SelfUtils.mutedByOthersChanged = (oldSelf, changedSelf) => {
throw new ParameterError('New self must be defined to determine if self was muted by others.');
}

const isModifiedBySelf = changedSelf.selfIdentity === changedSelf.modifiedBy;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't do the full check if we should ignore or not here, because selfUtils.ts doesn't have a reference to the meeting or MuteState class, so I've added the field isModifiedBySelf that's emitted with the SELF_REMOTE_MUTE_STATUS_UPDATED event and I've put all the logic to decide if we ignore the Locus DTO update or not in MuteState.shouldIgnoreRemoteMuteUpdate()


if (!oldSelf || oldSelf.remoteMuted === null) {
if (changedSelf.remoteMuted) {
return true; // this happens when mute on-entry is enabled
return {changed: false, isModifiedBySelf}; // this happens when mute on-entry is enabled
}

// we don't want to be sending the 'meeting:self:unmutedByOthers' notification on meeting join
return false;
return {changed: false, isModifiedBySelf};
}

return (
changedSelf.remoteMuted !== null &&
(oldSelf.remoteMuted !== changedSelf.remoteMuted ||
(changedSelf.remoteMuted && oldSelf.unmuteAllowed !== changedSelf.unmuteAllowed))
console.log(
`marcin: mutedByOthersChanged: old.remoteMuted=${oldSelf.remoteMuted} new.remoteMuted=${changedSelf.remoteMuted} selfId=${changedSelf.selfIdentity} modifiedBy=${changedSelf.modifiedBy}`
);

return {
changed:
changedSelf.remoteMuted !== null &&
(oldSelf.remoteMuted !== changedSelf.remoteMuted ||
(changedSelf.remoteMuted && oldSelf.unmuteAllowed !== changedSelf.unmuteAllowed)),
isModifiedBySelf,
};
};

SelfUtils.localAudioUnmuteRequestedByServer = (oldSelf: any = {}, changedSelf: any) => {
Expand Down
48 changes: 28 additions & 20 deletions packages/@webex/plugin-meetings/src/meeting/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3144,26 +3144,34 @@ export default class Meeting extends StatelessWebexPlugin {

this.locusInfo.on(LOCUSINFO.EVENTS.SELF_REMOTE_MUTE_STATUS_UPDATED, (payload) => {
if (payload) {
if (this.audio) {
this.audio.handleServerRemoteMuteUpdate(this, payload.muted, payload.unmuteAllowed);
}
// with "mute on entry" server will send us remote mute even if we don't have media configured,
// so if being muted by others, always send the notification,
// but if being unmuted, only send it if we are also locally unmuted
if (payload.muted || !this.audio?.isMuted()) {
Trigger.trigger(
this,
{
file: 'meeting/index',
function: 'setUpLocusInfoSelfListener',
},
payload.muted
? EVENT_TRIGGERS.MEETING_SELF_MUTED_BY_OTHERS
: EVENT_TRIGGERS.MEETING_SELF_UNMUTED_BY_OTHERS,
{
payload,
}
);
const ignore = this.audio
? this.audio.shouldIgnoreRemoteMuteUpdate(payload.muted, payload.isModifiedBySelf)
: false;

if (!ignore) {
if (this.audio) {
this.audio.handleServerRemoteMuteUpdate(this, payload.muted, payload.unmuteAllowed);
}
// with "mute on entry" server will send us remote mute even if we don't have media configured,
// so if being muted by others, always send the notification,
// but if being unmuted, only send it if we are also locally unmuted
if (payload.muted || !this.audio?.isMuted()) {
Trigger.trigger(
this,
{
file: 'meeting/index',
function: 'setUpLocusInfoSelfListener',
},
payload.muted
? EVENT_TRIGGERS.MEETING_SELF_MUTED_BY_OTHERS
: EVENT_TRIGGERS.MEETING_SELF_UNMUTED_BY_OTHERS,
{
payload,
}
);
}
} else {
console.log(`marcin: ignoring remote mute update payload.muted=${payload.muted}`);
}
}
});
Expand Down
35 changes: 29 additions & 6 deletions packages/@webex/plugin-meetings/src/meeting/muteState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export class MuteState {
};
server: {localMute: boolean; remoteMute: boolean; unmuteAllowed: boolean};
syncToServerInProgress: boolean;
isRemoteUnmutePendingLocusDtoUpdate: boolean; // true if we've sent a remote unmute request to Locus and haven't received a Locus DTO confirming it happened, yet
};

type: any;
Expand Down Expand Up @@ -62,6 +63,7 @@ export class MuteState {
unmuteAllowed: type === AUDIO ? meeting.unmuteAllowed : meeting.unmuteVideoAllowed ?? true,
},
syncToServerInProgress: false,
isRemoteUnmutePendingLocusDtoUpdate: false,
};
}

Expand Down Expand Up @@ -327,6 +329,13 @@ export class MuteState {
`Meeting:muteState#sendRemoteMuteRequestToServer --> ${this.type}: sending remote mute:${remoteMute} to server`
);

this.state.isRemoteUnmutePendingLocusDtoUpdate = true;

setTimeout(() => {
console.log('marcin: resetting isRemoteUnmutePendingLocusDtoUpdate after timeout');
this.state.isRemoteUnmutePendingLocusDtoUpdate = false;
}, 10 * 1000);

return meeting.members
.muteMember(meeting.members.selfId, remoteMute, this.type === AUDIO)
.then(() => {
Expand All @@ -337,6 +346,8 @@ export class MuteState {
this.state.server.remoteMute = remoteMute;
})
.catch((remoteUpdateError) => {
this.state.isRemoteUnmutePendingLocusDtoUpdate = false;

LoggerProxy.logger.warn(
`Meeting:muteState#sendRemoteMuteRequestToServer --> ${this.type}: failed to apply remote mute ${remoteMute} to server: ${remoteUpdateError}`
);
Expand All @@ -359,6 +370,23 @@ export class MuteState {
}
}

public shouldIgnoreRemoteMuteUpdate(remoteMute: boolean, isModifiedBySelf: boolean) {
console.log(
`marcin: shouldIgnoreRemoteMuteUpdate: flag=${this.state.isRemoteUnmutePendingLocusDtoUpdate} remoteMute=${remoteMute}, isModifiedBySelf=${isModifiedBySelf}`
);
if (this.state.isRemoteUnmutePendingLocusDtoUpdate && isModifiedBySelf && !remoteMute) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only ever send remote unmute requests (not mute), so here I'm only checking for !remoteMute, but the method that sends out these requests (sendRemoteMuteRequestToServer()) is written in a way that suggests that it could do either mute or unmute, so I think I'll rename sendRemoteMuteRequestToServer() to sendRemoteUnmuteRequestToServer() and hardcode remoteMute = false in it to make it more clear that we only do remote unmute, then this will be more in-line with the hardcoded assumption here in shouldIgnoreRemoteMuteUpdate()

console.log('marcin: FIX: ignoring remote mute update');

this.state.isRemoteUnmutePendingLocusDtoUpdate = false;

return true;
}

console.log('marcin: FIX: NOT ignoring remote mute update');

return false;
}

/**
* This method should be called whenever the server remote mute state is changed
*
Expand All @@ -379,12 +407,7 @@ export class MuteState {
}
if (muted !== undefined) {
this.state.server.remoteMute = muted;

// We never want to unmute the local stream from a server remote mute update.
// Moderated unmute is handled by a different function.
if (muted) {
this.muteLocalStream(meeting, muted, 'remotelyMuted');
}
this.muteLocalStream(meeting, muted, 'remotelyMuted');
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,39 +345,37 @@ describe('plugin-meetings', () => {
});

describe('mutedByOthersChanged', () => {
it('throws an error if changedSelf is not provided', function () {
assert.throws(
() => SelfUtils.mutedByOthersChanged({}, null),
'New self must be defined to determine if self was muted by others.'
);
it('throws an error if changedSelf is not provided', function() {
assert.throws(() => SelfUtils.mutedByOthersChanged({}, null), 'New self must be defined to determine if self was muted by others.');
});

it('return false when oldSelf is not defined', function () {
assert.equal(SelfUtils.mutedByOthersChanged(null, {remoteMuted: false}), false);
it('return false when oldSelf is not defined', function() {
assert.equal(SelfUtils.mutedByOthersChanged(null, { remoteMuted: false }), false);
});

it('should return true when remoteMuted is true on entry', function () {
assert.equal(SelfUtils.mutedByOthersChanged(null, {remoteMuted: true}), true);
it('should return true when remoteMuted is true on entry', function() {
assert.equal(SelfUtils.mutedByOthersChanged(null, { remoteMuted: true }), true);
});

it('should return true when remoteMuted values are different', function () {
assert.equal(
SelfUtils.mutedByOthersChanged(
{remoteMuted: false},
{remoteMuted: true, selfIdentity: 'user1', modifiedBy: 'user2'}
),
true
);
it('should return false when selfIdentity and modifiedBy are the same', function() {
assert.equal(SelfUtils.mutedByOthersChanged(
{ remoteMuted: false },
{ remoteMuted: true, selfIdentity: 'user1', modifiedBy: 'user1' }
), false);
});

it('should return true when remoteMuted is true and unmuteAllowed has changed', function () {
assert.equal(
SelfUtils.mutedByOthersChanged(
{remoteMuted: true, unmuteAllowed: false},
{remoteMuted: true, unmuteAllowed: true, selfIdentity: 'user1', modifiedBy: 'user2'}
),
true
);
it('should return true when remoteMuted values are different', function() {
assert.equal(SelfUtils.mutedByOthersChanged(
{ remoteMuted: false },
{ remoteMuted: true, selfIdentity: 'user1', modifiedBy: 'user2' }
), true);
});

it('should return true when remoteMuted is true and unmuteAllowed has changed', function() {
assert.equal(SelfUtils.mutedByOthersChanged(
{ remoteMuted: true, unmuteAllowed: false },
{ remoteMuted: true, unmuteAllowed: true, selfIdentity: 'user1', modifiedBy: 'user2' }
), true);
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,30 +113,6 @@ describe('plugin-meetings', () => {
assert.isTrue(audio.isRemotelyMuted());
});

it('does not locally unmute on a server unmute', async () => {
const setServerMutedSpy = meeting.mediaProperties.audioStream.setServerMuted;

// simulate remote mute
audio.handleServerRemoteMuteUpdate(meeting, true, true);

assert.isTrue(audio.isRemotelyMuted());
assert.isTrue(audio.isLocallyMuted());

// mutes local
assert.calledOnceWithExactly(setServerMutedSpy, true, 'remotelyMuted');

setServerMutedSpy.resetHistory();

// simulate remote unmute
audio.handleServerRemoteMuteUpdate(meeting, false, true);

assert.isFalse(audio.isRemotelyMuted());
assert.isTrue(audio.isLocallyMuted());

// does not unmute local
assert.notCalled(setServerMutedSpy);
});

it('does local audio unmute if localAudioUnmuteRequired is received', async () => {
// first we need to have the local stream user muted
meeting.mediaProperties.audioStream.userMuted = true;
Expand Down
Loading