Merge pull request #2802 from robintown/device-fall-back

Don't fall back as eagerly to unselected devices

src/livekit/MediaDevicesContext.tsx

@@ -24,6 +24,7 @@ import {
   audioInput as audioInputSetting,
   audioOutput as audioOutputSetting,
   videoInput as videoInputSetting,
+  Setting,
 } from "../settings/settings";
 import { isFirefox } from "../Platform";
 
@@ -58,7 +59,7 @@ function useObservableState<T>(
 
 function useMediaDevice(
   kind: MediaDeviceKind,
-  fallbackDevice: string | undefined,
+  setting: Setting<string | undefined>,
   usingNames: boolean,
   alwaysDefault: boolean = false,
 ): MediaDevice {
@@ -84,15 +85,21 @@ function useMediaDevice(
     [kind, requestPermissions],
   );
   const available = useObservableState(deviceObserver, []);
-  const [selectedId, select] = useState(fallbackDevice);
+  const [preferredId, select] = useSetting(setting);
 
   return useMemo(() => {
-    let devId;
-    if (available) {
-      devId = available.some((d) => d.deviceId === selectedId)
-        ? selectedId
-        : available.some((d) => d.deviceId === fallbackDevice)
-          ? fallbackDevice
+    let selectedId: string | undefined = undefined;
+    if (!alwaysDefault && available) {
+      // If the preferred device is available, use it. Or if every available
+      // device ID is falsy, the browser is probably just being paranoid about
+      // fingerprinting and we should still try using the preferred device.
+      // Worst case it is not available and the browser will gracefully fall
+      // back to some other device for us when requesting the media stream.
+      // Otherwise, select the first available device.
+      selectedId =
+        available.some((d) => d.deviceId === preferredId) ||
+        available.every((d) => d.deviceId === "")
+          ? preferredId
           : available.at(0)?.deviceId;
     }
 
@@ -102,10 +109,10 @@ function useMediaDevice(
           // device entries for the exact same device ID; deduplicate them
           [...new Map(available.map((d) => [d.deviceId, d])).values()]
         : [],
-      selectedId: alwaysDefault ? undefined : devId,
+      selectedId,
       select,
     };
-  }, [available, selectedId, fallbackDevice, select, alwaysDefault]);
+  }, [available, preferredId, select, alwaysDefault]);
 }
 
 const deviceStub: MediaDevice = {
@@ -141,36 +148,22 @@ export const MediaDevicesProvider: FC<Props> = ({ children }) => {
   // for ouput devices because the selector wont be shown on FF.
   const useOutputNames = usingNames && !isFirefox();
 
-  const [storedAudioInput, setStoredAudioInput] = useSetting(audioInputSetting);
-  const [storedAudioOutput, setStoredAudioOutput] =
-    useSetting(audioOutputSetting);
-  const [storedVideoInput, setStoredVideoInput] = useSetting(videoInputSetting);
-
-  const audioInput = useMediaDevice("audioinput", storedAudioInput, usingNames);
+  const audioInput = useMediaDevice(
+    "audioinput",
+    audioInputSetting,
+    usingNames,
+  );
   const audioOutput = useMediaDevice(
     "audiooutput",
-    storedAudioOutput,
+    audioOutputSetting,
     useOutputNames,
     alwaysUseDefaultAudio,
   );
-  const videoInput = useMediaDevice("videoinput", storedVideoInput, usingNames);
-
-  useEffect(() => {
-    if (audioInput.selectedId !== undefined)
-      setStoredAudioInput(audioInput.selectedId);
-  }, [setStoredAudioInput, audioInput.selectedId]);
-
-  useEffect(() => {
-    // Skip setting state for ff output. Redundent since it is set to always return 'undefined'
-    // but makes it clear while debugging that this is not happening on FF. + perf ;)
-    if (audioOutput.selectedId !== undefined && !isFirefox())
-      setStoredAudioOutput(audioOutput.selectedId);
-  }, [setStoredAudioOutput, audioOutput.selectedId]);
-
-  useEffect(() => {
-    if (videoInput.selectedId !== undefined)
-      setStoredVideoInput(videoInput.selectedId);
-  }, [setStoredVideoInput, videoInput.selectedId]);
+  const videoInput = useMediaDevice(
+    "videoinput",
+    videoInputSetting,
+    usingNames,
+  );
 
   const startUsingDeviceNames = useCallback(
     () => setNumCallersUsingNames((n) => n + 1),

src/room/useSwitchCamera.ts

@@ -20,7 +20,6 @@ import {
   TrackEvent,
 } from "livekit-client";
 import { useObservable, useObservableEagerState } from "observable-hooks";
-import { useEffect } from "react";
 import { logger } from "matrix-js-sdk/src/logger";
 
 import { useMediaDevices } from "../livekit/MediaDevicesContext";
@@ -35,6 +34,7 @@ export function useSwitchCamera(
   video: Observable<LocalVideoTrack | null>,
 ): (() => void) | null {
   const mediaDevices = useMediaDevices();
+  const setVideoInput = useLatest(mediaDevices.videoInput.select);
 
   // Produce an observable like the input 'video' observable, except make it
   // emit whenever the track is muted or the device changes
@@ -75,6 +75,12 @@ export function useSwitchCamera(
                   .restartTrack({
                     facingMode: facingMode === "user" ? "environment" : "user",
                   })
+                  .then(() => {
+                    // Inform the MediaDeviceContext which camera was chosen
+                    const deviceId =
+                      track.mediaStreamTrack.getSettings().deviceId;
+                    if (deviceId !== undefined) setVideoInput.current(deviceId);
+                  })
                   .catch((e) =>
                     logger.error("Failed to switch camera", facingMode, e),
                   );
@@ -83,16 +89,5 @@ export function useSwitchCamera(
     [videoTrack],
   );
 
-  const setVideoInput = useLatest(mediaDevices.videoInput.select);
-  useEffect(() => {
-    // Watch for device changes due to switching the camera and feed them back
-    // into the MediaDeviceContext
-    const subscription = videoTrack.subscribe((track) => {
-      const deviceId = track?.mediaStreamTrack.getSettings().deviceId;
-      if (deviceId !== undefined) setVideoInput.current(deviceId);
-    });
-    return (): void => subscription.unsubscribe();
-  }, [videoTrack, setVideoInput]);
-
   return useObservableEagerState(switchCamera);
 }

src/settings/DeviceSelection.module.css

@@ -0,0 +1,18 @@
+.selection {
+  gap: 0;
+}
+
+.title {
+  color: var(--cpd-color-text-secondary);
+  margin-block: var(--cpd-space-3x) 0;
+}
+
+.separator {
+  margin-block: 6px var(--cpd-space-4x);
+}
+
+.options {
+  display: flex;
+  flex-direction: column;
+  gap: var(--cpd-space-4x);
+}

src/settings/DeviceSelection.tsx

@@ -0,0 +1,71 @@
+/*
+Copyright 2024 New Vector Ltd.
+
+SPDX-License-Identifier: AGPL-3.0-only
+Please see LICENSE in the repository root for full details.
+*/
+
+import { ChangeEvent, FC, useCallback, useId } from "react";
+import {
+  Heading,
+  InlineField,
+  Label,
+  RadioControl,
+  Separator,
+} from "@vector-im/compound-web";
+
+import { MediaDevice } from "../livekit/MediaDevicesContext";
+import styles from "./DeviceSelection.module.css";
+
+interface Props {
+  devices: MediaDevice;
+  caption: string;
+}
+
+export const DeviceSelection: FC<Props> = ({ devices, caption }) => {
+  const groupId = useId();
+  const onChange = useCallback(
+    (e: ChangeEvent<HTMLInputElement>) => {
+      devices.select(e.target.value);
+    },
+    [devices],
+  );
+
+  if (devices.available.length == 0) return null;
+
+  return (
+    <div className={styles.selection}>
+      <Heading
+        type="body"
+        weight="semibold"
+        size="sm"
+        as="h4"
+        className={styles.title}
+      >
+        {caption}
+      </Heading>
+      <Separator className={styles.separator} />
+      <div className={styles.options}>
+        {devices.available.map(({ deviceId, label }, index) => (
+          <InlineField
+            key={deviceId}
+            name={groupId}
+            control={
+              <RadioControl
+                checked={deviceId === devices.selectedId}
+                onChange={onChange}
+                value={deviceId}
+              />
+            }
+          >
+            <Label>
+              {!!label && label.trim().length > 0
+                ? label
+                : `${caption} ${index + 1}`}
+            </Label>
+          </InlineField>
+        ))}
+      </div>
+    </div>
+  );
+};

src/settings/SettingsModal.tsx

@@ -5,10 +5,10 @@ SPDX-License-Identifier: AGPL-3.0-only
 Please see LICENSE in the repository root for full details.
 */
 
-import { ChangeEvent, FC, ReactNode, useCallback } from "react";
+import { ChangeEvent, FC, useCallback } from "react";
 import { Trans, useTranslation } from "react-i18next";
 import { MatrixClient } from "matrix-js-sdk/src/matrix";
-import { Dropdown, Separator, Text } from "@vector-im/compound-web";
+import { Root as Form, Text } from "@vector-im/compound-web";
 
 import { Modal } from "../Modal";
 import styles from "./SettingsModal.module.css";
@@ -19,7 +19,6 @@ import { ProfileSettingsTab } from "./ProfileSettingsTab";
 import { FeedbackSettingsTab } from "./FeedbackSettingsTab";
 import {
   useMediaDevices,
-  MediaDevice,
   useMediaDeviceNames,
 } from "../livekit/MediaDevicesContext";
 import { widget } from "../widget";
@@ -33,6 +32,7 @@ import {
 import { isFirefox } from "../Platform";
 import { PreferencesSettingsTab } from "./PreferencesSettingsTab";
 import { Slider } from "../Slider";
+import { DeviceSelection } from "./DeviceSelection";
 
 type SettingsTab =
   | "audio"
@@ -70,40 +70,6 @@ export const SettingsModal: FC<Props> = ({
   );
   const [duplicateTiles, setDuplicateTiles] = useSetting(duplicateTilesSetting);
 
-  // Generate a `SelectInput` with a list of devices for a given device kind.
-  const generateDeviceSelection = (
-    devices: MediaDevice,
-    caption: string,
-  ): ReactNode => {
-    if (devices.available.length == 0) return null;
-
-    const values = devices.available.map(
-      ({ deviceId, label }, index) =>
-        [
-          deviceId,
-          !!label && label.trim().length > 0
-            ? label
-            : `${caption} ${index + 1}`,
-        ] as [string, string],
-    );
-
-    return (
-      <Dropdown
-        label={caption}
-        defaultValue={
-          devices.selectedId === "" || !devices.selectedId
-            ? "default"
-            : devices.selectedId
-        }
-        onValueChange={(id): void => devices.select(id)}
-        values={values}
-        // XXX This is unused because we set a defaultValue. The component
-        // shouldn't require this prop.
-        placeholder=""
-      />
-    );
-  };
-
   const optInDescription = (
     <Text size="sm">
       <Trans i18nKey="settings.opt_in_description">
@@ -125,25 +91,30 @@ export const SettingsModal: FC<Props> = ({
     name: t("common.audio"),
     content: (
       <>
-        {generateDeviceSelection(devices.audioInput, t("common.microphone"))}
-        {!isFirefox() &&
-          generateDeviceSelection(
-            devices.audioOutput,
-            t("settings.speaker_device_selection_label"),
-          )}
-        <Separator />
-        <div className={styles.volumeSlider}>
-          <label>{t("settings.audio_tab.effect_volume_label")}</label>
-          <p>{t("settings.audio_tab.effect_volume_description")}</p>
-          <Slider
-            label={t("video_tile.volume")}
-            value={soundVolume}
-            onValueChange={setSoundVolume}
-            min={0}
-            max={1}
-            step={0.01}
+        <Form>
+          <DeviceSelection
+            devices={devices.audioInput}
+            caption={t("common.microphone")}
           />
-        </div>
+          {!isFirefox() && (
+            <DeviceSelection
+              devices={devices.audioOutput}
+              caption={t("settings.speaker_device_selection_label")}
+            />
+          )}
+          <div className={styles.volumeSlider}>
+            <label>{t("settings.audio_tab.effect_volume_label")}</label>
+            <p>{t("settings.audio_tab.effect_volume_description")}</p>
+            <Slider
+              label={t("video_tile.volume")}
+              value={soundVolume}
+              onValueChange={setSoundVolume}
+              min={0}
+              max={1}
+              step={0.01}
+            />
+          </div>
+        </Form>
       </>
     ),
   };
@@ -151,7 +122,14 @@ export const SettingsModal: FC<Props> = ({
   const videoTab: Tab<SettingsTab> = {
     key: "video",
     name: t("common.video"),
-    content: generateDeviceSelection(devices.videoInput, t("common.camera")),
+    content: (
+      <Form>
+        <DeviceSelection
+          devices={devices.videoInput}
+          caption={t("common.camera")}
+        />
+      </Form>
+    ),
   };
 
   const preferencesTab: Tab<SettingsTab> = {

src/state/CallViewModel.test.ts

@@ -30,7 +30,7 @@ import {
   mockLivekitRoom,
   mockLocalParticipant,
   mockMatrixRoom,
-  mockMember,
+  mockMatrixRoomMember,
   mockRemoteParticipant,
   withTestScheduler,
 } from "../utils/test";
@@ -42,10 +42,10 @@ import { E2eeType } from "../e2ee/e2eeType";
 
 vi.mock("@livekit/components-core");
 
-const alice = mockMember({ userId: "@alice:example.org" });
-const bob = mockMember({ userId: "@bob:example.org" });
-const carol = mockMember({ userId: "@carol:example.org" });
-const dave = mockMember({ userId: "@dave:example.org" });
+const alice = mockMatrixRoomMember({ userId: "@alice:example.org" });
+const bob = mockMatrixRoomMember({ userId: "@bob:example.org" });
+const carol = mockMatrixRoomMember({ userId: "@carol:example.org" });
+const dave = mockMatrixRoomMember({ userId: "@dave:example.org" });
 
 const aliceId = `${alice.userId}:AAAA`;
 const bobId = `${bob.userId}:BBBB`;
@@ -229,7 +229,7 @@ function withCallViewModel(
 }
 
 test("participants are retained during a focus switch", () => {
-  withTestScheduler(({ cold, expectObservable }) => {
+  withTestScheduler(({ hot, expectObservable }) => {
     // Participants disappear on frame 2 and come back on frame 3
     const participantInputMarbles = "a-ba";
     // Start switching focus on frame 1 and reconnect on frame 3
@@ -238,11 +238,11 @@ test("participants are retained during a focus switch", () => {
     const expectedLayoutMarbles = "  a";
 
     withCallViewModel(
-      cold(participantInputMarbles, {
+      hot(participantInputMarbles, {
         a: [aliceParticipant, bobParticipant],
         b: [],
       }),
-      cold(connectionInputMarbles, {
+      hot(connectionInputMarbles, {
         c: ConnectionState.Connected,
         s: ECAddonConnectionState.ECSwitchingFocus,
       }),
@@ -264,7 +264,7 @@ test("participants are retained during a focus switch", () => {
 });
 
 test("screen sharing activates spotlight layout", () => {
-  withTestScheduler(({ cold, schedule, expectObservable }) => {
+  withTestScheduler(({ hot, schedule, expectObservable }) => {
     // Start with no screen shares, then have Alice and Bob share their screens,
     // then return to no screen shares, then have just Alice share for a bit
     const participantInputMarbles = "    abcda-ba";
@@ -277,7 +277,7 @@ test("screen sharing activates spotlight layout", () => {
     const expectedLayoutMarbles = "      abcdaefeg";
     const expectedShowSpeakingMarbles = "y----nyny";
     withCallViewModel(
-      cold(participantInputMarbles, {
+      hot(participantInputMarbles, {
         a: [aliceParticipant, bobParticipant],
         b: [aliceSharingScreen, bobParticipant],
         c: [aliceSharingScreen, bobSharingScreen],
@@ -347,7 +347,7 @@ test("screen sharing activates spotlight layout", () => {
 });
 
 test("participants stay in the same order unless to appear/disappear", () => {
-  withTestScheduler(({ cold, schedule, expectObservable }) => {
+  withTestScheduler(({ hot, schedule, expectObservable }) => {
     const modeInputMarbles = "     a";
     // First Bob speaks, then Dave, then Alice
     const aSpeakingInputMarbles = "n- 1998ms - 1999ms y";
@@ -363,9 +363,9 @@ test("participants stay in the same order unless to appear/disappear", () => {
       of([aliceParticipant, bobParticipant, daveParticipant]),
       of(ConnectionState.Connected),
       new Map([
-        [aliceParticipant, cold(aSpeakingInputMarbles, { y: true, n: false })],
-        [bobParticipant, cold(bSpeakingInputMarbles, { y: true, n: false })],
-        [daveParticipant, cold(dSpeakingInputMarbles, { y: true, n: false })],
+        [aliceParticipant, hot(aSpeakingInputMarbles, { y: true, n: false })],
+        [bobParticipant, hot(bSpeakingInputMarbles, { y: true, n: false })],
+        [daveParticipant, hot(dSpeakingInputMarbles, { y: true, n: false })],
       ]),
       (vm) => {
         schedule(modeInputMarbles, {

src/state/CallViewModel.ts

@@ -306,7 +306,7 @@ class ScreenShare {
 
 type MediaItem = UserMedia | ScreenShare;
 
-function findMatrixMember(
+function findMatrixRoomMember(
   room: MatrixRoom,
   id: string,
 ): RoomMember | undefined {
@@ -341,12 +341,16 @@ export class CallViewModel extends ViewModel {
       }),
     );
 
-  private readonly rawRemoteParticipants = connectedParticipantsObserver(
-    this.livekitRoom,
-  ).pipe(this.scope.state());
+  /**
+   * The raw list of RemoteParticipants as reported by LiveKit
+   */
+  private readonly rawRemoteParticipants: Observable<RemoteParticipant[]> =
+    connectedParticipantsObserver(this.livekitRoom).pipe(this.scope.state());
 
-  // Lists of participants to "hold" on display, even if LiveKit claims that
-  // they've left
+  /**
+   * Lists of RemoteParticipants to "hold" on display, even if LiveKit claims that
+   * they've left
+   */
   private readonly remoteParticipantHolds: Observable<RemoteParticipant[][]> =
     this.connectionState.pipe(
       withLatestFrom(this.rawRemoteParticipants),
@@ -381,6 +385,9 @@ export class CallViewModel extends ViewModel {
       ),
     );
 
+  /**
+   * The RemoteParticipants including those that are being "held" on the screen
+   */
   private readonly remoteParticipants: Observable<RemoteParticipant[]> =
     combineLatest(
       [this.rawRemoteParticipants, this.remoteParticipantHolds],
@@ -402,6 +409,9 @@ export class CallViewModel extends ViewModel {
       },
     );
 
+  /**
+   * List of MediaItems that we want to display
+   */
   private readonly mediaItems: Observable<MediaItem[]> = combineLatest([
     this.remoteParticipants,
     observeParticipantMedia(this.livekitRoom.localParticipant),
@@ -418,7 +428,7 @@ export class CallViewModel extends ViewModel {
           function* (this: CallViewModel): Iterable<[string, MediaItem]> {
             for (const p of [localParticipant, ...remoteParticipants]) {
               const id = p === localParticipant ? "local" : p.identity;
-              const member = findMatrixMember(this.matrixRoom, id);
+              const member = findMatrixRoomMember(this.matrixRoom, id);
               if (member === undefined)
                 logger.warn(
                   `Ruh, roh! No matrix member found for SFU participant '${p.identity}': creating g-g-g-ghost!`,
@@ -471,6 +481,9 @@ export class CallViewModel extends ViewModel {
     this.scope.state(),
   );
 
+  /**
+   * List of MediaItems that we want to display, that are of type UserMedia
+   */
   private readonly userMedia: Observable<UserMedia[]> = this.mediaItems.pipe(
     map((mediaItems) =>
       mediaItems.filter((m): m is UserMedia => m instanceof UserMedia),
@@ -482,6 +495,9 @@ export class CallViewModel extends ViewModel {
       map((ms) => ms.find((m) => m.vm.local)!.vm as LocalUserMediaViewModel),
     );
 
+  /**
+   * List of MediaItems that we want to display, that are of type ScreenShare
+   */
   private readonly screenShares: Observable<ScreenShare[]> =
     this.mediaItems.pipe(
       map((mediaItems) =>
@@ -940,7 +956,7 @@ export class CallViewModel extends ViewModel {
     this.scope.state(),
   );
 
-  public readonly showFooter = this.windowMode.pipe(
+  public readonly showFooter: Observable<boolean> = this.windowMode.pipe(
     switchMap((mode) => {
       switch (mode) {
         case "pip":

src/utils/test.ts

@@ -99,7 +99,7 @@ function mockEmitter<T>(): EmitterMock<T> {
 // Maybe it'd be good to move this to matrix-js-sdk? Our testing needs are
 // rather simple, but if one util to mock a member is good enough for us, maybe
 // it's useful for matrix-js-sdk consumers in general.
-export function mockMember(member: Partial<RoomMember>): RoomMember {
+export function mockMatrixRoomMember(member: Partial<RoomMember>): RoomMember {
   return { ...mockEmitter(), ...member } as RoomMember;
 }
 
@@ -149,7 +149,7 @@ export async function withLocalMedia(
   const localParticipant = mockLocalParticipant({});
   const vm = new LocalUserMediaViewModel(
     "local",
-    mockMember(member),
+    mockMatrixRoomMember(member),
     localParticipant,
     {
       kind: E2eeType.PER_PARTICIPANT,
@@ -184,7 +184,7 @@ export async function withRemoteMedia(
   const remoteParticipant = mockRemoteParticipant(participant);
   const vm = new RemoteUserMediaViewModel(
     "remote",
-    mockMember(member),
+    mockMatrixRoomMember(member),
     remoteParticipant,
     {
       kind: E2eeType.PER_PARTICIPANT,