Skip to content

Commit

Permalink
fix(meetings): webrtc dumps are sometimes empty when users join a mee…
Browse files Browse the repository at this point in the history
…ting via lobby (#3836)
  • Loading branch information
marcin-bazyl authored Sep 16, 2024
1 parent cd652ad commit 9bb81a2
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 29 deletions.
14 changes: 5 additions & 9 deletions packages/@webex/plugin-meetings/src/media/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,7 @@ Media.getDirection = (forceSendRecv: boolean, receive: boolean, send: boolean) =
*
* @param {boolean} isMultistream
* @param {string} debugId string useful for debugging (will appear in media connection logs)
* @param {object} webex main `webex` object.
* @param {string} meetingId id for the meeting using this connection
* @param {string} correlationId id used in requests to correlate to this session
* @param {Object} options
* @param {Object} [options.mediaProperties] contains mediaDirection and local tracks:
* audioTrack, videoTrack, shareVideoTrack, and shareAudioTrack
Expand All @@ -120,10 +118,9 @@ Media.getDirection = (forceSendRecv: boolean, receive: boolean, send: boolean) =
Media.createMediaConnection = (
isMultistream: boolean,
debugId: string,
webex: object,
meetingId: string,
correlationId: string,
options: {
rtcMetrics?: RtcMetrics;
mediaProperties: {
mediaDirection?: {
receiveAudio: boolean;
Expand All @@ -150,6 +147,7 @@ Media.createMediaConnection = (
}
) => {
const {
rtcMetrics,
mediaProperties,
remoteQualityLevel,
enableRtx,
Expand Down Expand Up @@ -192,15 +190,13 @@ Media.createMediaConnection = (
config.bundlePolicy = bundlePolicy;
}

const rtcMetrics = new RtcMetrics(webex, meetingId, correlationId);

return new MultistreamRoapMediaConnection(
config,
meetingId,
/* the rtc metrics objects callbacks */
(data) => rtcMetrics.addMetrics(data),
() => rtcMetrics.closeMetrics(),
() => rtcMetrics.sendMetricsInQueue()
(data) => rtcMetrics?.addMetrics(data),
() => rtcMetrics?.closeMetrics(),
() => rtcMetrics?.sendMetricsInQueue()
);
}

Expand Down
12 changes: 9 additions & 3 deletions packages/@webex/plugin-meetings/src/meeting/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ import ControlsOptionsManager from '../controls-options-manager';
import PermissionError from '../common/errors/permission';
import {LocusMediaRequest} from './locusMediaRequest';
import {ConnectionStateHandler, ConnectionStateEvent} from './connectionStateHandler';
import RtcMetrics from '../rtcMetrics';

// default callback so we don't call an undefined function, but in practice it should never be used
const DEFAULT_ICE_PHASE_CALLBACK = () => 'JOIN_MEETING_FINAL';
Expand Down Expand Up @@ -696,6 +697,7 @@ export default class Meeting extends StatelessWebexPlugin {
private connectionStateHandler?: ConnectionStateHandler;
private iceCandidateErrors: Map<string, number>;
private iceCandidatesCount: number;
private rtcMetrics?: RtcMetrics;

/**
* @param {Object} attrs
Expand Down Expand Up @@ -3156,6 +3158,7 @@ export default class Meeting extends StatelessWebexPlugin {
options: {meetingId: this.id},
});
}
this.rtcMetrics?.sendNextMetrics();
this.updateLLMConnection();
});

Expand Down Expand Up @@ -6307,14 +6310,17 @@ export default class Meeting extends StatelessWebexPlugin {
* @returns {RoapMediaConnection | MultistreamRoapMediaConnection}
*/
private async createMediaConnection(turnServerInfo, bundlePolicy?: BundlePolicy) {
this.rtcMetrics = this.isMultistream
? // @ts-ignore
new RtcMetrics(this.webex, this.id, this.correlationId)
: undefined;

const mc = Media.createMediaConnection(
this.isMultistream,
this.getMediaConnectionDebugId(),
// @ts-ignore
this.webex,
this.id,
this.correlationId,
{
rtcMetrics: this.rtcMetrics,
mediaProperties: this.mediaProperties,
remoteQualityLevel: this.mediaProperties.remoteQualityLevel,
// @ts-ignore - config coming from registerPlugin
Expand Down
20 changes: 16 additions & 4 deletions packages/@webex/plugin-meetings/src/rtcMetrics/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export default class RtcMetrics {

connectionId: string;

initialMetricsSent: boolean;
shouldSendMetricsOnNextStatsReport: boolean;

/**
* Initialize the interval.
Expand Down Expand Up @@ -64,6 +64,18 @@ export default class RtcMetrics {
}
}

/**
* Forces sending metrics when we get the next stats-report
*
* This is useful for cases when something important happens that affects the media connection,
* for example when we move from lobby into the meeting.
*
* @returns {void}
*/
public sendNextMetrics() {
this.shouldSendMetricsOnNextStatsReport = true;
}

/**
* Add metrics items to the metrics queue.
*
Expand All @@ -79,11 +91,11 @@ export default class RtcMetrics {

this.metricsQueue.push(data);

if (!this.initialMetricsSent && data.name === 'stats-report') {
if (this.shouldSendMetricsOnNextStatsReport && data.name === 'stats-report') {
// this is the first useful set of data (WCME gives it to us after 5s), send it out immediately
// in case the user is unhappy and closes the browser early
this.sendMetricsInQueue();
this.initialMetricsSent = true;
this.shouldSendMetricsOnNextStatsReport = false;
}

try {
Expand Down Expand Up @@ -139,7 +151,7 @@ export default class RtcMetrics {
*/
private resetConnection() {
this.connectionId = uuid.v4();
this.initialMetricsSent = false;
this.shouldSendMetricsOnNextStatsReport = true;
}

/**
Expand Down
40 changes: 33 additions & 7 deletions packages/@webex/plugin-meetings/test/unit/spec/media/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,12 @@ import Media from '@webex/plugin-meetings/src/media/index';
import {assert} from '@webex/test-helper-chai';
import sinon from 'sinon';
import StaticConfig from '@webex/plugin-meetings/src/common/config';
import MockWebex from '@webex/test-helper-mock-webex';

describe('createMediaConnection', () => {
let clock;
beforeEach(() => {
clock = sinon.useFakeTimers();
});
const webex = MockWebex();

const fakeRoapMediaConnection = {
id: 'roap media connection',
Expand Down Expand Up @@ -61,7 +59,7 @@ describe('createMediaConnection', () => {
const ENABLE_EXTMAP = false;
const ENABLE_RTX = true;

Media.createMediaConnection(false, 'some debug id', webex, 'meetingId', 'correlationId', {
Media.createMediaConnection(false, 'some debug id', 'meetingId', {
mediaProperties: {
mediaDirection: {
sendAudio: false,
Expand Down Expand Up @@ -139,7 +137,13 @@ describe('createMediaConnection', () => {
.stub(InternalMediaCoreModule, 'MultistreamRoapMediaConnection')
.returns(fakeRoapMediaConnection);

Media.createMediaConnection(true, 'some debug id', webex, 'meeting id', 'correlationId', {
const rtcMetrics = {
addMetrics: sinon.stub(),
closeMetrics: sinon.stub(),
sendMetricsInQueue: sinon.stub(),
};

Media.createMediaConnection(true, 'some debug id', 'meeting id', {
mediaProperties: {
mediaDirection: {
sendAudio: true,
Expand All @@ -150,6 +154,7 @@ describe('createMediaConnection', () => {
receiveShare: true,
},
},
rtcMetrics,
turnServerInfo: {
url: 'turns:turn-server-url:443?transport=tcp',
username: 'turn username',
Expand Down Expand Up @@ -177,6 +182,27 @@ describe('createMediaConnection', () => {
},
'meeting id'
);

// check if rtcMetrics callbacks are configured correctly
const addMetricsCallback = multistreamRoapMediaConnectionConstructorStub.getCalls()[0].args[2];
const closeMetricsCallback = multistreamRoapMediaConnectionConstructorStub.getCalls()[0].args[3];
const sendMetricsInQueueCallback = multistreamRoapMediaConnectionConstructorStub.getCalls()[0].args[4];

assert.isFunction(addMetricsCallback);
assert.isFunction(closeMetricsCallback);
assert.isFunction(sendMetricsInQueueCallback);

const fakeMetricsData = {id: 'metrics data'};

addMetricsCallback(fakeMetricsData);
assert.calledOnceWithExactly(rtcMetrics.addMetrics, fakeMetricsData);

closeMetricsCallback();
assert.calledOnce(rtcMetrics.closeMetrics);

sendMetricsInQueueCallback();
assert.calledOnce(rtcMetrics.sendMetricsInQueue);

});

[
Expand All @@ -191,7 +217,7 @@ describe('createMediaConnection', () => {
.stub(InternalMediaCoreModule, 'MultistreamRoapMediaConnection')
.returns(fakeRoapMediaConnection);

Media.createMediaConnection(true, 'debug string', webex, 'meeting id', 'correlationId', {
Media.createMediaConnection(true, 'debug string', 'meeting id', {
mediaProperties: {
mediaDirection: {
sendAudio: true,
Expand Down Expand Up @@ -220,7 +246,7 @@ describe('createMediaConnection', () => {
.stub(InternalMediaCoreModule, 'MultistreamRoapMediaConnection')
.returns(fakeRoapMediaConnection);

Media.createMediaConnection(true, 'debug string', webex, 'meeting id', 'correlationId', {
Media.createMediaConnection(true, 'debug string', 'meeting id', {
mediaProperties: {
mediaDirection: {
sendAudio: true,
Expand Down Expand Up @@ -260,7 +286,7 @@ describe('createMediaConnection', () => {
const ENABLE_EXTMAP = false;
const ENABLE_RTX = true;

Media.createMediaConnection(false, 'some debug id', webex, 'meeting id', 'correlationId', {
Media.createMediaConnection(false, 'some debug id', 'meeting id', {
mediaProperties: {
mediaDirection: {
sendAudio: true,
Expand Down
51 changes: 45 additions & 6 deletions packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import 'jsdom-global/register';
import {cloneDeep, forEach, isEqual, isUndefined} from 'lodash';
import sinon from 'sinon';
import * as InternalMediaCoreModule from '@webex/internal-media-core';
import * as RtcMetricsModule from '@webex/plugin-meetings/src/rtcMetrics';
import * as RemoteMediaManagerModule from '@webex/plugin-meetings/src/multistream/remoteMediaManager';
import StateMachine from 'javascript-state-machine';
import uuid from 'uuid';
import {assert, expect} from '@webex/test-helper-chai';
Expand Down Expand Up @@ -2405,9 +2407,7 @@ describe('plugin-meetings', () => {
Media.createMediaConnection,
false,
meeting.getMediaConnectionDebugId(),
webex,
meeting.id,
meeting.correlationId,
sinon.match({turnServerInfo: undefined})
);
assert.calledOnce(meeting.setMercuryListener);
Expand Down Expand Up @@ -2449,6 +2449,44 @@ describe('plugin-meetings', () => {
checkWorking({allowMediaInLobby: true});
});

it('should create rtcMetrics and pass them to Media.createMediaConnection()', async () => {
const fakeRtcMetrics = {id: 'fake rtc metrics object'};
const rtcMetricsCtor = sinon.stub(RtcMetricsModule, 'default').returns(fakeRtcMetrics);

// setup the minimum mocks required for multistream connection
fakeMediaConnection.createSendSlot = sinon.stub().returns({
publishStream: sinon.stub(),
unpublishStream: sinon.stub(),
setNamedMediaGroups: sinon.stub(),
});
sinon.stub(RemoteMediaManagerModule, 'RemoteMediaManager').returns({
start: sinon.stub().resolves(),
on: sinon.stub(),
logAllReceiveSlots: sinon.stub(),
});

meeting.meetingState = 'ACTIVE';
meeting.isMultistream = true;

await meeting.addMedia({
mediaSettings: {},
});

assert.calledOnceWithExactly(rtcMetricsCtor, webex, meeting.id, meeting.correlationId);

// check that rtcMetrics was passed to Media.createMediaConnection
assert.calledOnce(Media.createMediaConnection);
assert.calledWith(
Media.createMediaConnection,
true,
meeting.getMediaConnectionDebugId(),
meeting.id,
sinon.match({
rtcMetrics: fakeRtcMetrics,
})
);
});

it('should pass the turn server info to the peer connection', async () => {
const FAKE_TURN_URL = 'turns:webex.com:3478';
const FAKE_TURN_USER = 'some-turn-username';
Expand Down Expand Up @@ -2478,9 +2516,7 @@ describe('plugin-meetings', () => {
Media.createMediaConnection,
false,
meeting.getMediaConnectionDebugId(),
webex,
meeting.id,
meeting.correlationId,
sinon.match({
turnServerInfo: {
url: FAKE_TURN_URL,
Expand Down Expand Up @@ -3401,9 +3437,7 @@ describe('plugin-meetings', () => {
Media.createMediaConnection,
false,
meeting.getMediaConnectionDebugId(),
webex,
meeting.id,
meeting.correlationId,
sinon.match({
turnServerInfo: {
url: FAKE_TURN_URL,
Expand Down Expand Up @@ -8454,6 +8488,9 @@ describe('plugin-meetings', () => {
it('listens to the self admitted guest event', (done) => {
meeting.stopKeepAlive = sinon.stub();
meeting.updateLLMConnection = sinon.stub();
meeting.rtcMetrics = {
sendNextMetrics: sinon.stub(),
};
meeting.locusInfo.emit({function: 'test', file: 'test'}, 'SELF_ADMITTED_GUEST', test1);
assert.calledOnceWithExactly(meeting.stopKeepAlive);
assert.calledThrice(TriggerProxy.trigger);
Expand All @@ -8465,6 +8502,8 @@ describe('plugin-meetings', () => {
{payload: test1}
);
assert.calledOnce(meeting.updateLLMConnection);
assert.calledOnceWithExactly(meeting.rtcMetrics.sendNextMetrics);

done();
});

Expand Down

0 comments on commit 9bb81a2

Please sign in to comment.