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

Merged
merged 5 commits into from
Jul 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f663bd
fix(prosody-auth): Don't loose initial tracks.
hristoterezov Feb 13, 2024
c602d77
ref(web): startConference and initial GUM tracks management.
hristoterezov Apr 29, 2024
ccf6c9a
feat(prejoin): make initPrejoin sync.
hristoterezov Jun 25, 2024
930503e
ref(initialGUMPromise): Move out of _common reducer.
hristoterezov Jul 26, 2024
944d1b4
feat(prejoin): Move startConference logic to conference middleware.
hristoterezov Jul 26, 2024
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
66 changes: 23 additions & 43 deletions conference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import {
setAudioAvailable,
setAudioMuted,
setAudioUnmutePermissions,
setInitialGUMPromise,
setVideoAvailable,
setVideoMuted,
setVideoUnmutePermissions
Expand Down Expand Up @@ -154,8 +155,7 @@ import {
import { isModerationNotificationDisplayed } from './react/features/notifications/functions';
import { mediaPermissionPromptVisibilityChanged } from './react/features/overlay/actions';
import { suspendDetected } from './react/features/power-monitor/actions';
import { initPrejoin } from './react/features/prejoin/actions';
import { isPrejoinPageVisible } from './react/features/prejoin/functions';
import { initPrejoin, isPrejoinPageVisible } from './react/features/prejoin/functions';
import { disableReceiver, stopReceiver } from './react/features/remote-control/actions';
import { setScreenAudioShareState } from './react/features/screen-share/actions.web';
import { isScreenAudioShared } from './react/features/screen-share/functions';
Expand Down Expand Up @@ -591,7 +591,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 @@ -600,58 +600,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.
initPrejoin(tracks, errors, dispatch);
} else {
APP.store.dispatch(displayErrorsForCreateInitialLocalTracks(errors));
setGUMPendingStateOnFailedTracks(tracks, APP.store.dispatch);
}

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

APP.store.dispatch(displayErrorsForCreateInitialLocalTracks(errors));

const tracks = handleInitialTracks(initialOptions, localTracks);

setGUMPendingStateOnFailedTracks(tracks, APP.store.dispatch);
return {
tracks,
errors
};
})));

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

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

return Promise.all([
tryCreateLocalTracks.then(tr => {
APP.store.dispatch(displayErrorsForCreateInitialLocalTracks(errors));

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

const filteredTracks = handleInitialTracks(initialOptions, tr);

setGUMPendingStateOnFailedTracks(filteredTracks, APP.store.dispatch);

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));
saghul marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
3 changes: 2 additions & 1 deletion react/features/authentication/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ MiddlewareRegistry.register(store => next => action => {

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

if (error
Expand Down
6 changes: 0 additions & 6 deletions react/features/base/conference/actions.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,5 @@ export function setupVisitorStartupMedia(media: Array<MediaType>) {
if (media && Array.isArray(media) && media.length > 0) {
dispatch(createAndAddInitialAVTracks(media));
}

// FIXME: The name of the function doesn't fit the startConference execution but another PR will removes
// this and calls startConference based on the connection status. This will stay here temporary.
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 @@ -209,17 +208,7 @@ function _conferenceFailed({ dispatch, getState }: IStore, next: Function, actio
dispatch(conferenceWillLeave(conference));

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

we decided with @damencho to remove the reconnect in order not to flood the infrastructure with reconnects. IIRC the UX now will be that the user will see an error and can reload or press the Join button again.

}

break;
Expand Down
79 changes: 78 additions & 1 deletion 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,9 +4,17 @@ 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 { JitsiConferenceErrors, browser } 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 { replaceLocalTrack } from '../tracks/actions.any';
import { getLocalTracks } from '../tracks/functions.any';

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

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

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

break;
}
case CONNECTION_ESTABLISHED: {
if (isPrejoinPageVisible(getState())) {
let { initialGUMPromise } = getState()['features/base/media'];

initialGUMPromise = initialGUMPromise || Promise.resolve({ tracks: [] });

initialGUMPromise.then(() => {
const state = getState();
let localTracks = getLocalTracks(state['features/base/tracks']);
const trackReplacePromises = [];

// Do not signal audio/video tracks if the user joins muted.
for (const track of localTracks) {
// Always add the audio track on Safari because of a known issue where audio playout doesn't happen
// if the user joins audio and video muted.
if ((track.muted && !(browser.isWebKitBased() && track.jitsiTrack
&& track.jitsiTrack.getType() === MEDIA_TYPE.AUDIO)) || iAmVisitor(state)) {
trackReplacePromises.push(dispatch(replaceLocalTrack(track.jitsiTrack, null))
.catch((error: any) => {
logger.error(`Failed to replace local track (${track.jitsiTrack}) with null: ${error}`);
}));
}
}

Promise.allSettled(trackReplacePromises).then(() => {

// Re-fetch the local tracks after muted tracks have been removed above.
// This is needed, because the tracks are effectively disposed by the replaceLocalTrack and should
// not be used anymore.
localTracks = getLocalTracks(getState()['features/base/tracks']);

const jitsiTracks = localTracks.map((t: any) => t.jitsiTrack);


return APP.conference.startConference(jitsiTracks);
});
});
} else {
let { initialGUMPromise } = getState()['features/base/media'];

initialGUMPromise = initialGUMPromise || Promise.resolve({ tracks: [] });

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>; }> | null = null) {
return {
type: SET_INITIAL_GUM_PROMISE,
promise
};
}

/**
* Action to set the muted state of the local screenshare.
*
Expand Down
24 changes: 24 additions & 0 deletions react/features/base/media/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,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 @@ -87,6 +88,22 @@ function _audio(state: IAudioState = _AUDIO_INITIAL_MEDIA_STATE, action: AnyActi
}
}

/**
* Reducer fot the common properties in media state.
*
* @param {ICommonState} state - Common media state.
* @param {Object} action - Action object.
* @param {string} action.type - Type of action.
* @returns {ICommonState}
*/
function _initialGUMPromise(state: initialGUMPromise | null = null, action: AnyAction) {
if (action.type === SET_INITIAL_GUM_PROMISE) {
return action.promise ?? null;
}

return state;
}

/**
* Media state object for local screenshare.
*
Expand Down Expand Up @@ -247,6 +264,11 @@ interface IAudioState {
unmuteBlocked: boolean;
}

type initialGUMPromise = Promise<{
errors?: any;
tracks: Array<any>;
}> | null;

interface IScreenshareState {
available: boolean;
muted: number;
Expand All @@ -264,6 +286,7 @@ interface IVideoState {

export interface IMediaState {
audio: IAudioState;
initialGUMPromise: initialGUMPromise;
screenshare: IScreenshareState;
video: IVideoState;
}
Expand All @@ -280,6 +303,7 @@ export interface IMediaState {
*/
ReducerRegistry.register<IMediaState>('features/base/media', combineReducers({
audio: _audio,
initialGUMPromise: _initialGUMPromise,
screenshare: _screenshare,
video: _video
}));
Expand Down
Loading