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.

Sign up for GitHub

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

Jump to bottom

fix(prosody-auth): Don't loose initial tracks. #14358

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+194 −103
Open

fix(prosody-auth): Don't loose initial tracks. #14358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51a0133
fix(prosody-auth): Don't loose initial tracks.
hristoterezov Feb 13, 2024
ec46340
ref(web): startConference and initial GUM tracks management.
hristoterezov Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 22 additions & 41 deletions conference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ import {
setAudioAvailable,
setAudioMuted,
setAudioUnmutePermissions,
setInitialGUMPromise,
setVideoAvailable,
setVideoMuted,
setVideoUnmutePermissions
Expand Down Expand Up @@ -699,7 +700,7 @@ export default {
const handleInitialTracks = (options, tracks) => {
let localTracks = tracks;

if (options.startWithAudioMuted || room?.isStartAudioMuted()) {
if (options.startWithAudioMuted) {
// Always add the track on Safari because of a known issue where audio playout doesn't happen
// if the user joins audio and video muted, i.e., if there is no local media capture.
if (browser.isWebKitBased()) {
Expand All @@ -708,58 +709,38 @@ export default {
localTracks = localTracks.filter(track => track.getType() !== MEDIA_TYPE.AUDIO);
}
}
if (room?.isStartVideoMuted()) {
localTracks = localTracks.filter(track => track.getType() !== MEDIA_TYPE.VIDEO);
}

return localTracks;
};
const { dispatch, getState } = APP.store;
const { tryCreateLocalTracks, errors } = this.createInitialLocalTracks(initialOptions);

if (isPrejoinPageVisible(state)) {
const { tryCreateLocalTracks, errors } = this.createInitialLocalTracks(initialOptions);
const localTracks = await tryCreateLocalTracks;
dispatch(setInitialGUMPromise(tryCreateLocalTracks.then(async tr => {
const tracks = handleInitialTracks(initialOptions, tr);

// Initialize device list a second time to ensure device labels get populated in case of an initial gUM
// acceptance; otherwise they may remain as empty strings.
this._initDeviceList(true);

if (isPrejoinPageVisible(state)) {
APP.store.dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE));
if (isPrejoinPageVisible(getState())) {
dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE));
dispatch(setInitialGUMPromise());

return APP.store.dispatch(initPrejoin(localTracks, errors));
// Note: Not sure if initPrejoin needs to be async. But let's wait for it just to be sure the
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't need to be. It doesn't even need access to the store so it doesn't even need to be async.

export function initPrejoin(tracks: Object[], errors: Object) {
    return async function(dispatch: IStore['dispatch']) {
        dispatch(setPrejoinDeviceErrors(errors));
        dispatch(prejoinInitialized());

        tracks.forEach(track => dispatch(trackAdded(track)));
    };
}

You could rewrite this to take the store as a parameter and just call it, since it's not really an action. Then in there you can batch-dispatch the actual actions.

// tracks are added.
await dispatch(initPrejoin(tracks, errors));
} else {
this._displayErrorsForCreateInitialLocalTracks(errors);
setGUMPendingStateOnFailedTracks(tracks);
}

logger.debug('Prejoin screen no longer displayed at the time when tracks were created');

this._displayErrorsForCreateInitialLocalTracks(errors);

const tracks = handleInitialTracks(initialOptions, localTracks);

setGUMPendingStateOnFailedTracks(tracks);
return {
tracks,
errors
};
})));

return this._setLocalAudioVideoStreams(tracks);
if (!isPrejoinPageVisible(getState())) {
dispatch(connect());
}

const { tryCreateLocalTracks, errors } = this.createInitialLocalTracks(initialOptions);

return Promise.all([
tryCreateLocalTracks.then(tr => {
this._displayErrorsForCreateInitialLocalTracks(errors);

return tr;
}).then(tr => {
this._initDeviceList(true);

const filteredTracks = handleInitialTracks(initialOptions, tr);

setGUMPendingStateOnFailedTracks(filteredTracks);

return filteredTracks;
}),
APP.store.dispatch(connect())
]).then(([ tracks, _ ]) => {
this.startConference(tracks).catch(logger.error);
});
},

/**
Expand Down
6 changes: 2 additions & 4 deletions react/features/authentication/components/web/LoginDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import { connect as reduxConnect } from 'react-redux';
import { IReduxState, IStore } from '../../../app/types';
import { IJitsiConference } from '../../../base/conference/reducer';
import { IConfig } from '../../../base/config/configType';
import { connect } from '../../../base/connection/actions.web';
import { toJid } from '../../../base/connection/functions';
import { translate, translateToHTML } from '../../../base/i18n/functions';
import { JitsiConnectionErrors } from '../../../base/lib-jitsi-meet';
import Dialog from '../../../base/ui/components/web/Dialog';
import Input from '../../../base/ui/components/web/Input';
import { joinConference } from '../../../prejoin/actions.web';
import {
authenticateAndUpgradeRole,
cancelLogin
Expand Down Expand Up @@ -134,9 +134,7 @@ class LoginDialog extends Component<IProps, IState> {
if (conference) {
dispatch(authenticateAndUpgradeRole(jid, password, conference));
} else {
// dispatch(connect(jid, password));
// FIXME: Workaround for the web version. To be removed once we get rid of conference.js
dispatch(joinConference(undefined, false, jid, password));
dispatch(connect(jid, password));
Copy link
Member

Choose a reason for hiding this comment

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

Has this been tested with XMPP auth? I recall we've broken it a number of times in the past.

}
}

Expand Down
19 changes: 18 additions & 1 deletion react/features/authentication/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { batch } from 'react-redux';

import { IStore } from '../app/types';
import { APP_WILL_NAVIGATE } from '../base/app/actionTypes';
import {
Expand All @@ -13,7 +15,9 @@ import {
JitsiConferenceErrors,
JitsiConnectionErrors
} from '../base/lib-jitsi-meet';
import { gumPending, setInitialGUMPromise } from '../base/media/actions';
import { MEDIA_TYPE } from '../base/media/constants';
import { IGUMPendingState } from '../base/media/types';
import MiddlewareRegistry from '../base/redux/MiddlewareRegistry';
import { isLocalTrackMuted } from '../base/tracks/functions.any';
import { parseURIString } from '../base/util/uri';
Expand Down Expand Up @@ -143,7 +147,8 @@ MiddlewareRegistry.register(store => next => action => {

case CONNECTION_FAILED: {
const { error } = action;
const state = store.getState();
const { dispatch, getState } = store;
const state = getState();
const { jwt } = state['features/base/jwt'];

if (error
Expand All @@ -153,6 +158,11 @@ MiddlewareRegistry.register(store => next => action => {
error.recoverable = true;

_handleLogin(store);
} else {
batch(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This feeels like the wrong place to be handling this. Authentication should not be handling track stuff.

You can handle this in the media middleware, since the recoverable flag will already be set once you calll next when handling this action.

dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE));
dispatch(setInitialGUMPromise());
});
}

break;
Expand Down Expand Up @@ -264,6 +274,11 @@ function _handleLogin({ dispatch, getState }: IStore) {
const videoMuted = isLocalTrackMuted(state['features/base/tracks'], MEDIA_TYPE.VIDEO);

if (!room) {
batch(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

dispatch(setInitialGUMPromise());
dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE));
});

logger.warn('Cannot handle login, room is undefined!');

return;
Expand All @@ -275,6 +290,8 @@ function _handleLogin({ dispatch, getState }: IStore) {
return;
}

dispatch(setInitialGUMPromise());

getTokenAuthUrl(
config,
locationURL,
Expand Down
11 changes: 0 additions & 11 deletions react/features/base/conference/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ import { JITSI_CONNECTION_CONFERENCE_KEY } from '../connection/constants';
import { hasAvailableDevices } from '../devices/functions.any';
import { JitsiConferenceEvents, JitsiE2ePingEvents } from '../lib-jitsi-meet';
import {
gumPending,
setAudioMuted,
setAudioUnmutePermissions,
setVideoMuted,
setVideoUnmutePermissions
} from '../media/actions';
import { MEDIA_TYPE, VIDEO_MUTISM_AUTHORITY } from '../media/constants';
import { IGUMPendingState } from '../media/types';
import {
dominantSpeakerChanged,
participantKicked,
Expand Down Expand Up @@ -1060,10 +1058,6 @@ export function redirect(vnode: string, focusJid: string, username: string) {
.then(() => dispatch(conferenceWillInit()))
.then(() => dispatch(connect()))
.then(() => {
// Clear the gum pending state in case we have set it to pending since we are starting the
// conference without tracks.
dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE));

if (!vnode) {
const state = getState();
const { enableMediaOnPromote = {} } = state['features/base/config'].visitors ?? {};
Expand Down Expand Up @@ -1096,11 +1090,6 @@ export function redirect(vnode: string, focusJid: string, username: string) {
}
}
}

// FIXME: Workaround for the web version. To be removed once we get rid of conference.js
if (typeof APP !== 'undefined') {
APP.conference.startConference([]);
}
});
};
}
15 changes: 2 additions & 13 deletions react/features/base/conference/middleware.any.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import LocalRecordingManager from '../../recording/components/Recording/LocalRec
import { iAmVisitor } from '../../visitors/functions';
import { overwriteConfig } from '../config/actions';
import { CONNECTION_ESTABLISHED, CONNECTION_FAILED } from '../connection/actionTypes';
import { connect, connectionDisconnected, disconnect } from '../connection/actions';
import { connectionDisconnected, disconnect } from '../connection/actions';
import { validateJwt } from '../jwt/functions';
import { JitsiConferenceErrors, JitsiConferenceEvents, JitsiConnectionErrors } from '../lib-jitsi-meet';
import { PARTICIPANT_UPDATED, PIN_PARTICIPANT } from '../participants/actionTypes';
Expand All @@ -37,7 +37,6 @@ import {
import MiddlewareRegistry from '../redux/MiddlewareRegistry';
import StateListenerRegistry from '../redux/StateListenerRegistry';
import { TRACK_ADDED, TRACK_REMOVED } from '../tracks/actionTypes';
import { getLocalTracks } from '../tracks/functions.any';

import {
CONFERENCE_FAILED,
Expand Down Expand Up @@ -204,17 +203,7 @@ function _conferenceFailed({ dispatch, getState }: IStore, next: Function, actio
dispatch(overwriteConfig(newConfig)) // @ts-ignore
.then(() => dispatch(conferenceWillLeave(conference)))
.then(() => conference.leave())
.then(() => dispatch(disconnect()))
.then(() => dispatch(connect()))
.then(() => {
// FIXME: Workaround for the web version. To be removed once we get rid of conference.js
if (typeof APP !== 'undefined') {
const localTracks = getLocalTracks(getState()['features/base/tracks']);
const jitsiTracks = localTracks.map((t: any) => t.jitsiTrack);

APP.conference.startConference(jitsiTracks).catch(logger.error);
}
});
.then(() => dispatch(disconnect()));
Copy link
Member

Choose a reason for hiding this comment

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

When will the connection begin again?

Copy link
Member

Choose a reason for hiding this comment

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

I see some of the logic got moved to base/conference, but only to the web middleware. When does mobile start connecting again?

}

break;
Expand Down
39 changes: 39 additions & 0 deletions react/features/base/conference/middleware.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,14 @@ import {
setPrejoinPageVisibility,
setSkipPrejoinOnReload
} from '../../prejoin/actions.web';
import { isPrejoinPageVisible } from '../../prejoin/functions';
import { iAmVisitor } from '../../visitors/functions';
import { CONNECTION_DISCONNECTED, CONNECTION_ESTABLISHED } from '../connection/actionTypes';
import { hangup } from '../connection/actions.web';
import { JitsiConferenceErrors } from '../lib-jitsi-meet';
import { gumPending, setInitialGUMPromise } from '../media/actions';
import { MEDIA_TYPE } from '../media/constants';
import { IGUMPendingState } from '../media/types';
import MiddlewareRegistry from '../redux/MiddlewareRegistry';

import {
Expand Down Expand Up @@ -131,6 +137,39 @@ MiddlewareRegistry.register(store => next => action => {
releaseScreenLock();

break;
case CONNECTION_DISCONNECTED: {
const { initialGUMPromise } = getState()['features/base/media'].common;

if (initialGUMPromise) {
store.dispatch(setInitialGUMPromise());
}

break;
}
case CONNECTION_ESTABLISHED: {
const state = getState();

if (!isPrejoinPageVisible(state)) {
const { initialGUMPromise = Promise.resolve({ tracks: [] }) } = state['features/base/media'].common;

initialGUMPromise.then(({ tracks }) => {
let tracksToUse = tracks ?? [];

if (iAmVisitor(getState())) {
tracksToUse = [];
tracks.forEach(track => track.dispose().catch(logger.error));
dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE));
}

dispatch(setInitialGUMPromise());

return APP.conference.startConference(tracksToUse);
})
.catch(logger.error);
}

break;
}
}

return next(action);
Expand Down
10 changes: 10 additions & 0 deletions react/features/base/media/actionTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,16 @@ export const SET_AUDIO_UNMUTE_PERMISSIONS = 'SET_AUDIO_UNMUTE_PERMISSIONS';
*/
export const SET_CAMERA_FACING_MODE = 'SET_CAMERA_FACING_MODE';

/**
* Sets the initial GUM promise.
*
* {
* type: SET_INITIAL_GUM_PROMISE,
* promise: Promise
* }}
*/
export const SET_INITIAL_GUM_PROMISE = 'SET_INITIAL_GUM_PROMISE';

/**
* The type of (redux) action to set the muted state of the local screenshare.
*
Expand Down
17 changes: 17 additions & 0 deletions react/features/base/media/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
SET_AUDIO_MUTED,
SET_AUDIO_UNMUTE_PERMISSIONS,
SET_CAMERA_FACING_MODE,
SET_INITIAL_GUM_PROMISE,
SET_SCREENSHARE_MUTED,
SET_VIDEO_AVAILABLE,
SET_VIDEO_MUTED,
Expand Down Expand Up @@ -93,6 +94,22 @@ export function setCameraFacingMode(cameraFacingMode: string) {
};
}

/**
* Sets the initial GUM promise.
*
* @param {Promise<Array<Object>> | undefined} promise - The promise.
* @returns {{
* type: SET_INITIAL_GUM_PROMISE,
* promise: Promise
* }}
*/
export function setInitialGUMPromise(promise?: Promise<{ errors: any; tracks: Array<any>; }>) {
return {
type: SET_INITIAL_GUM_PROMISE,
promise
};
}

/**
* Action to set the muted state of the local screenshare.
*
Expand Down