Merge pull request #3536 from robintown/stable-speaker-switches

Avoid gratuitous animations when the speaker changes

src/state/CallViewModel.test.ts

@@ -193,7 +193,6 @@ function summarizeLayout$(l$: Observable<Layout>): Observable<LayoutSummary> {
               l.spotlight?.media$ ?? constant(undefined),
               ...l.grid.map((vm) => vm.media$),
             ],
-            // eslint-disable-next-line rxjs/finnish -- false positive
             (spotlight, ...grid) => ({
               type: l.type,
               spotlight: spotlight?.map((vm) => vm.id),
@@ -213,7 +212,6 @@ function summarizeLayout$(l$: Observable<Layout>): Observable<LayoutSummary> {
         case "spotlight-expanded":
           return combineLatest(
             [l.spotlight.media$, l.pip?.media$ ?? constant(undefined)],
-            // eslint-disable-next-line rxjs/finnish -- false positive
             (spotlight, pip) => ({
               type: l.type,
               spotlight: spotlight.map((vm) => vm.id),
@@ -677,6 +675,21 @@ test("spotlight speakers swap places", () => {
             },
           },
         );
+
+        // While we expect the media on tiles to change, layout$ itself should
+        // *never* meaningfully change. That is, we expect there to be no layout
+        // shifts as the spotlight speaker changes; instead, the same tiles
+        // should be reused for the whole duration of the test and simply have
+        // their media swapped out. This is meaningful for keeping the interface
+        // not too visually distracting during back-and-forth conversations,
+        // while still animating tiles to express people joining, leaving, etc.
+        expectObservable(
+          vm.layout$.pipe(
+            distinctUntilChanged(deepCompare),
+            debounceTime(0),
+            map(() => "x"),
+          ),
+        ).toBe("x"); // Expect just one emission
       },
     );
   });
@@ -719,6 +732,84 @@ test("layout enters picture-in-picture mode when requested", () => {
   });
 });
 
+test("PiP tile in expanded spotlight layout switches speakers without layout shifts", () => {
+  withTestScheduler(({ behavior, schedule, expectObservable }) => {
+    // Switch to spotlight immediately
+    const modeInputMarbles = "     s";
+    // And expand the spotlight immediately
+    const expandInputMarbles = "   a";
+    // First Bob speaks, then Dave, then Bob again
+    const bSpeakingInputMarbles = "n-yn--yn";
+    const dSpeakingInputMarbles = "n---yn";
+    // Should show Alice (presenter) in the PiP, then Bob, then Dave, then Bob
+    // again
+    const expectedLayoutMarbles = "a-b-c-b";
+
+    withCallViewModel(
+      {
+        remoteParticipants$: constant([
+          aliceSharingScreen,
+          bobParticipant,
+          daveParticipant,
+        ]),
+        rtcMembers$: constant([
+          localRtcMember,
+          aliceRtcMember,
+          bobRtcMember,
+          daveRtcMember,
+        ]),
+        speaking: new Map([
+          [bobParticipant, behavior(bSpeakingInputMarbles, yesNo)],
+          [daveParticipant, behavior(dSpeakingInputMarbles, yesNo)],
+        ]),
+      },
+      (vm) => {
+        schedule(modeInputMarbles, {
+          s: () => vm.setGridMode("spotlight"),
+        });
+        schedule(expandInputMarbles, {
+          a: () => vm.toggleSpotlightExpanded$.value!(),
+        });
+
+        expectObservable(summarizeLayout$(vm.layout$)).toBe(
+          expectedLayoutMarbles,
+          {
+            a: {
+              type: "spotlight-expanded",
+              spotlight: [`${aliceId}:0:screen-share`],
+              pip: `${aliceId}:0`,
+            },
+            b: {
+              type: "spotlight-expanded",
+              spotlight: [`${aliceId}:0:screen-share`],
+              pip: `${bobId}:0`,
+            },
+            c: {
+              type: "spotlight-expanded",
+              spotlight: [`${aliceId}:0:screen-share`],
+              pip: `${daveId}:0`,
+            },
+          },
+        );
+
+        // While we expect the media on the PiP tile to change, layout$ itself
+        // should *never* meaningfully change. That is, we expect the same PiP
+        // tile to exist throughout the test and just have its media swapped out
+        // when the speaker changes, rather than for tiles to animate in/out.
+        // This is meaningful for keeping the interface not too visually
+        // distracting during back-and-forth conversations.
+        expectObservable(
+          vm.layout$.pipe(
+            distinctUntilChanged(deepCompare),
+            debounceTime(0),
+            map(() => "x"),
+          ),
+        ).toBe("x"); // Expect just one emission
+      },
+    );
+  });
+});
+
 test("spotlight remembers whether it's expanded", () => {
   withTestScheduler(({ schedule, expectObservable }) => {
     // Start in spotlight mode, then switch to grid and back to spotlight a
@@ -741,11 +832,7 @@ test("spotlight remembers whether it's expanded", () => {
           g: () => vm.setGridMode("grid"),
         });
         schedule(expandInputMarbles, {
-          a: () => {
-            let toggle: () => void;
-            vm.toggleSpotlightExpanded$.subscribe((val) => (toggle = val!));
-            toggle!();
-          },
+          a: () => vm.toggleSpotlightExpanded$.value!(),
         });
 
         expectObservable(summarizeLayout$(vm.layout$)).toBe(

src/state/SpotlightExpandedLayout.ts

@@ -20,7 +20,7 @@ export function spotlightExpandedLayout(
 ): [SpotlightExpandedLayout, TileStore] {
   const update = prevTiles.from(1);
   update.registerSpotlight(media.spotlight, true);
-  if (media.pip !== undefined) update.registerGridTile(media.pip);
+  if (media.pip !== undefined) update.registerPipTile(media.pip);
   const tiles = update.build();
 
   return [

src/state/TileStore.ts

@@ -118,10 +118,11 @@ export class TileStore {
  */
 export class TileStoreBuilder {
   private spotlight: SpotlightTileData | null = null;
-  private readonly prevSpotlightSpeaker =
+  private readonly prevSpotlightSpeaker: UserMediaViewModel | null =
     this.prevSpotlight?.media.length === 1 &&
-    "speaking" in this.prevSpotlight.media[0] &&
-    this.prevSpotlight.media[0];
+    "speaking$" in this.prevSpotlight.media[0]
+      ? this.prevSpotlight.media[0]
+      : null;
 
   private readonly prevGridByMedia: Map<
     MediaViewModel,
@@ -201,8 +202,9 @@ export class TileStoreBuilder {
       if (
         media === this.prevSpotlightSpeaker &&
         this.spotlight.media.length === 1 &&
-        "speaking" in this.spotlight.media[0] &&
-        this.prevSpotlightSpeaker !== this.spotlight.media[0]
+        "speaking$" in this.spotlight.media[0] &&
+        this.prevSpotlightSpeaker !==
+          (this.spotlight.media[0] satisfies UserMediaViewModel)
       ) {
         const prev = this.prevGridByMedia.get(this.spotlight.media[0]);
         if (prev !== undefined) {
@@ -260,6 +262,33 @@ export class TileStoreBuilder {
     this.numGridEntries++;
   }
 
+  /**
+   * Sets up a PiP tile for the given media. This is a special kind of grid tile
+   * that is expected to stand on its own and switch between speakers, so this
+   * method will more eagerly try to reuse an existing tile, replacing its
+   * media, than registerGridTile would.
+   */
+  public registerPipTile(media: UserMediaViewModel): void {
+    if (DEBUG_ENABLED)
+      logger.debug(
+        `[TileStore, ${this.generation}] register PiP tile: ${media.member?.rawDisplayName ?? "[👻]"}`,
+      );
+
+    // If there is a single grid tile that we can reuse
+    if (this.prevGrid.length === 1) {
+      const entry = this.prevGrid[0];
+      this.stationaryGridEntries[0] = entry;
+      // Do the media swap
+      entry.media = media;
+      this.prevGridByMedia.delete(entry.media);
+      this.prevGridByMedia.set(media, [entry, 0]);
+    } else {
+      this.visibleGridEntries.push(new GridTileData(media));
+    }
+
+    this.numGridEntries++;
+  }
+
   /**
    * Constructs a new collection of all registered tiles, transferring ownership
    * of the tiles to the new collection. Any tiles present in the previous