From 1de8d93b4bdb5090f4112aec0af49736b4e00820 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 25 Feb 2026 15:47:25 +0100 Subject: [PATCH 1/8] feat: video auto fit based on video stream size --- src/state/MediaViewModel.test.ts | 15 -- src/state/MediaViewModel.ts | 63 ++++++-- src/tile/GridTile.tsx | 41 ++--- src/tile/SpotlightTile.tsx | 22 ++- src/utils/videoFit.test.ts | 251 +++++++++++++++++++++++++++++++ src/utils/videoFit.ts | 94 ++++++++++++ 6 files changed, 441 insertions(+), 45 deletions(-) create mode 100644 src/utils/videoFit.test.ts create mode 100644 src/utils/videoFit.ts diff --git a/src/state/MediaViewModel.test.ts b/src/state/MediaViewModel.test.ts index 92868216..a7bbb571 100644 --- a/src/state/MediaViewModel.test.ts +++ b/src/state/MediaViewModel.test.ts @@ -92,21 +92,6 @@ test("control a participant's volume", () => { }); }); -test("toggle fit/contain for a participant's video", () => { - const vm = createRemoteMedia(rtcMembership, {}, mockRemoteParticipant({})); - withTestScheduler(({ expectObservable, schedule }) => { - schedule("-ab|", { - a: () => vm.toggleFitContain(), - b: () => vm.toggleFitContain(), - }); - expectObservable(vm.cropVideo$).toBe("abc", { - a: true, - b: false, - c: true, - }); - }); -}); - test("local media remembers whether it should always be shown", () => { const vm1 = createLocalMedia( rtcMembership, diff --git a/src/state/MediaViewModel.ts b/src/state/MediaViewModel.ts index 3da69c46..57b0428a 100644 --- a/src/state/MediaViewModel.ts +++ b/src/state/MediaViewModel.ts @@ -43,6 +43,8 @@ import { switchMap, throttleTime, distinctUntilChanged, + concat, + take, } from "rxjs"; import { alwaysShowSelf } from "../settings/settings"; @@ -55,6 +57,7 @@ import { platform } from "../Platform"; import { type MediaDevices } from "./MediaDevices"; import { type Behavior } from "./Behavior"; import { type ObservableScope } from "./ObservableScope"; +import { videoFit$, videoSizeFromParticipant$ } from "../utils/videoFit.ts"; export function observeTrackReference$( participant: Participant, @@ -67,6 +70,10 @@ export function observeTrackReference$( ); } +/** + * Helper function to observe the RTC stats for a given participant and track source. + * It polls the stats every second and emits the latest stats object. + */ export function observeRtpStreamStats$( participant: Participant, source: Track.Source, @@ -76,7 +83,9 @@ export function observeRtpStreamStats$( > { return combineLatest([ observeTrackReference$(participant, source), - interval(1000).pipe(startWith(0)), + // This is used also for detecting video orientation, + // and we want that to be more responsive than the connection stats, so we poll more frequently at the start. + concat(interval(300).pipe(take(3)), interval(1000)).pipe(startWith(0)), ]).pipe( switchMap(async ([trackReference]) => { const track = trackReference?.publication?.track; @@ -90,7 +99,6 @@ export function observeRtpStreamStats$( if (!report) { return undefined; } - for (const v of report.values()) { if (v.type === type) { return v; @@ -103,6 +111,13 @@ export function observeRtpStreamStats$( ); } +/** + * Helper function to observe the inbound RTP stats for a given participant and track source. + * To be used for remote participants' audio and video tracks. + * It polls the stats every second and emits the latest stats object. + * @param participant - The LiveKit participant whose track stats we want to observe. + * @param source - The source of the track (e.g. Track.Source.Camera or Track.Source.Microphone). + */ export function observeInboundRtpStreamStats$( participant: Participant, source: Track.Source, @@ -112,6 +127,13 @@ export function observeInboundRtpStreamStats$( ); } +/** + * Helper function to observe the outbound RTP stats for a given participant and track source. + * To be used for the local participant's audio and video tracks. + * It polls the stats every second and emits the latest stats object. + * @param participant - The LiveKit participant whose track stats we want to observe. + * @param source - The source of the track (e.g. Track.Source.Camera or Track.Source.Microphone). + */ export function observeOutboundRtpStreamStats$( participant: Participant, source: Track.Source, @@ -263,7 +285,6 @@ abstract class BaseMediaViewModel { protected readonly participant$: Observable< LocalParticipant | RemoteParticipant | null >, - encryptionSystem: EncryptionSystem, audioSource: AudioSource, videoSource: VideoSource, @@ -397,13 +418,12 @@ abstract class BaseUserMediaViewModel extends BaseMediaViewModel { return this._videoEnabled$; } - private readonly _cropVideo$ = new BehaviorSubject(true); /** - * Whether the tile video should be contained inside the tile or be cropped to fit. + * Whether the tile video should be contained inside the tile (video-fit contain) or be cropped to fit (video-fit cover). */ - public readonly cropVideo$: Behavior = this._cropVideo$; + public readonly videoFit$: Behavior<"cover" | "contain">; - public constructor( + protected constructor( scope: ObservableScope, id: string, userId: string, @@ -443,10 +463,12 @@ abstract class BaseUserMediaViewModel extends BaseMediaViewModel { this._videoEnabled$ = this.scope.behavior( media$.pipe(map((m) => m?.cameraTrack?.isMuted === false)), ); - } - public toggleFitContain(): void { - this._cropVideo$.next(!this._cropVideo$.value); + this.videoFit$ = videoFit$( + this.scope, + videoSizeFromParticipant$(participant$), + this.actualSize$, + ); } public get local(): boolean { @@ -456,9 +478,28 @@ abstract class BaseUserMediaViewModel extends BaseMediaViewModel { public abstract get audioStreamStats$(): Observable< RTCInboundRtpStreamStats | RTCOutboundRtpStreamStats | undefined >; + public abstract get videoStreamStats$(): Observable< RTCInboundRtpStreamStats | RTCOutboundRtpStreamStats | undefined >; + + private readonly _actualSize$ = new BehaviorSubject< + { width: number; height: number } | undefined + >(undefined); + public readonly actualSize$ = this._actualSize$.asObservable(); + + /** + * Set the actual dimensions of the html element. + * This can be used to determine the best video fit (fit to frame / keep ratio). + * @param width - The actual width of the html element displaying the video. + * @param height - The actual height of the html element displaying the video. + */ + public setActualDimensions(width: number, height: number): void { + this._actualSize$.next({ + width, + height, + }); + } } /** @@ -616,6 +657,7 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { // This private field is used to override the value from the superclass private __speaking$: Behavior; + public get speaking$(): Behavior { return this.__speaking$; } @@ -661,6 +703,7 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { // This private field is used to override the value from the superclass private __videoEnabled$: Behavior; + public get videoEnabled$(): Behavior { return this.__videoEnabled$; } diff --git a/src/tile/GridTile.tsx b/src/tile/GridTile.tsx index 92262f05..ad158db1 100644 --- a/src/tile/GridTile.tsx +++ b/src/tile/GridTile.tsx @@ -11,6 +11,7 @@ import { type ReactNode, type Ref, useCallback, + useEffect, useRef, useState, } from "react"; @@ -26,7 +27,6 @@ import { VolumeOffIcon, VisibilityOnIcon, UserProfileIcon, - ExpandIcon, VolumeOffSolidIcon, SwitchCameraSolidIcon, } from "@vector-im/compound-design-tokens/assets/web/icons"; @@ -37,6 +37,7 @@ import { Menu, } from "@vector-im/compound-web"; import { useObservableEagerState } from "observable-hooks"; +import useMeasure from "react-use-measure"; import styles from "./GridTile.module.css"; import { @@ -105,18 +106,26 @@ const UserMediaTile: FC = ({ const audioEnabled = useBehavior(vm.audioEnabled$); const videoEnabled = useBehavior(vm.videoEnabled$); const speaking = useBehavior(vm.speaking$); - const cropVideo = useBehavior(vm.cropVideo$); - const onSelectFitContain = useCallback( - (e: Event) => { - e.preventDefault(); - vm.toggleFitContain(); - }, - [vm], - ); + const videoFit = useBehavior(vm.videoFit$); + const rtcBackendIdentity = vm.rtcBackendIdentity; const handRaised = useBehavior(vm.handRaised$); const reaction = useBehavior(vm.reaction$); + // We need to keep track of the tile size. + // We use this to get the tile ratio, and compare it to the video ratio to decide + // whether to fit the video to frame or keep the ratio. + const [measureRef, bounds] = useMeasure(); + // There is already a ref being passed in, so we need to merge it with the measureRef. + const tileRef = useMergedRefs(ref, measureRef); + + // Whenever bounds change, inform the viewModel + useEffect(() => { + if (bounds.width > 0 && bounds.height > 0) { + vm.setActualDimensions(bounds.width, bounds.height); + } + }, [bounds.width, bounds.height, vm]); + const AudioIcon = locallyMuted ? VolumeOffSolidIcon : audioEnabled @@ -132,12 +141,10 @@ const UserMediaTile: FC = ({ const menu = ( <> {menuStart} - + {/* + No additional menu item (used to be the manual fit to frame. + Placeholder for future menu items that should be placed here. + */} {menuEnd} ); @@ -150,13 +157,13 @@ const UserMediaTile: FC = ({ const tile = ( = ({ vm, ...props }) => { - const cropVideo = useBehavior(vm.cropVideo$); + const videoFit = useBehavior(vm.videoFit$); const baseProps: SpotlightUserMediaItemBaseProps & RefAttributes = { - videoFit: cropVideo ? "cover" : "contain", + videoFit, ...props, }; @@ -147,7 +148,22 @@ const SpotlightItem: FC = ({ "aria-hidden": ariaHidden, }) => { const ourRef = useRef(null); - const ref = useMergedRefs(ourRef, theirRef); + + // We need to keep track of the tile size. + // We use this to get the tile ratio, and compare it to the video ratio to decide + // whether to fit the video to frame or keep the ratio. + const [measureRef, bounds] = useMeasure(); + + // Whenever bounds change, inform the viewModel + useEffect(() => { + if (bounds.width > 0 && bounds.height > 0) { + if (!(vm instanceof ScreenShareViewModel)) { + vm.setActualDimensions(bounds.width, bounds.height); + } + } + }, [bounds.width, bounds.height, vm]); + + const ref = useMergedRefs(ourRef, theirRef, measureRef); const focusUrl = useBehavior(vm.focusUrl$); const displayName = useBehavior(vm.displayName$); const mxcAvatarUrl = useBehavior(vm.mxcAvatarUrl$); diff --git a/src/utils/videoFit.test.ts b/src/utils/videoFit.test.ts new file mode 100644 index 00000000..9390e8d4 --- /dev/null +++ b/src/utils/videoFit.test.ts @@ -0,0 +1,251 @@ +/* +Copyright 2026 Element Creations Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE in the repository root for full details. +*/ + +import { describe, expect, test, vi } from "vitest"; +import { + LocalTrack, + type LocalTrackPublication, + type RemoteTrackPublication, + Track, +} from "livekit-client"; + +import { ObservableScope } from "../state/ObservableScope"; +import { videoFit$, videoSizeFromParticipant$ } from "./videoFit"; +import { constant } from "../state/Behavior"; +import { + flushPromises, + mockLocalParticipant, + mockRemoteParticipant, +} from "./test"; + +describe("videoFit$ defaults", () => { + test.each([ + { + videoSize: { width: 1920, height: 1080 }, + tileSize: undefined, + }, + { + videoSize: { width: 1080, height: 1920 }, + tileSize: undefined, + }, + { + videoSize: undefined, + tileSize: { width: 1920, height: 1080 }, + }, + { + videoSize: undefined, + tileSize: { width: 1080, height: 1920 }, + }, + ])( + "videoFit$ returns `cover` when videoSize is $videoSize and tileSize is $tileSize", + ({ videoSize, tileSize }) => { + const scope = new ObservableScope(); + const videoSize$ = constant(videoSize); + const tileSize$ = constant(tileSize); + + const fit = videoFit$(scope, videoSize$, tileSize$); + expect(fit.value).toBe("cover"); + }, + ); +}); + +const VIDEO_480_L = { width: 640, height: 480 }; +const VIDEO_720_L = { width: 1280, height: 720 }; +const VIDEO_1080_L = { width: 1920, height: 1080 }; + +// Some sizes from real world testing, which don't match the standard video sizes exactly +const TILE_SIZE_1_L = { width: 180, height: 135 }; +const TILE_SIZE_3_P = { width: 379, height: 542 }; +const TILE_SIZE_4_L = { width: 957, height: 542 }; +// This is the size of an iPhone Xr in portrait mode +const TILE_SIZE_5_P = { width: 414, height: 896 }; + +export function invertSize(size: { width: number; height: number }): { + width: number; + height: number; +} { + return { + width: size.height, + height: size.width, + }; +} + +test.each([ + { + videoSize: VIDEO_480_L, + tileSize: TILE_SIZE_1_L, + expected: "cover", + }, + { + videoSize: invertSize(VIDEO_480_L), + tileSize: TILE_SIZE_1_L, + expected: "contain", + }, + { + videoSize: VIDEO_720_L, + tileSize: TILE_SIZE_4_L, + expected: "cover", + }, + { + videoSize: invertSize(VIDEO_720_L), + tileSize: TILE_SIZE_4_L, + expected: "contain", + }, + { + videoSize: invertSize(VIDEO_1080_L), + tileSize: TILE_SIZE_3_P, + expected: "cover", + }, + { + videoSize: VIDEO_1080_L, + tileSize: TILE_SIZE_5_P, + expected: "contain", + }, + { + videoSize: invertSize(VIDEO_1080_L), + tileSize: TILE_SIZE_5_P, + expected: "cover", + }, + { + // square video + videoSize: { width: 400, height: 400 }, + tileSize: VIDEO_480_L, + expected: "contain", + }, +])( + "videoFit$ returns $expected when videoSize is $videoSize and tileSize is $tileSize", + ({ videoSize, tileSize, expected }) => { + const scope = new ObservableScope(); + const videoSize$ = constant(videoSize); + const tileSize$ = constant(tileSize); + + const fit = videoFit$(scope, videoSize$, tileSize$); + expect(fit.value).toBe(expected); + }, +); + +describe("extracting video size from participant stats", () => { + function createMockRtpStats( + isInbound: boolean, + props: Partial = {}, + ): RTCInboundRtpStreamStats | RTCOutboundRtpStreamStats { + const baseStats = { + id: "mock-stats-id", + timestamp: Date.now(), + type: isInbound ? "inbound-rtp" : "outbound-rtp", + kind: "video", + ...props, + }; + + return baseStats as RTCInboundRtpStreamStats | RTCOutboundRtpStreamStats; + } + + test("get stats for local user", async () => { + const localParticipant = mockLocalParticipant({ + identity: "@local:example.org:AAAAAA", + }); + + const mockReport: RTCStatsReport = new Map([ + [ + "OT01V639885149", + createMockRtpStats(false, { + frameWidth: 1280, + frameHeight: 720, + }), + ], + ]); + + const track = { + source: Track.Source.Camera, + getRTCStatsReport: vi + .fn() + .mockImplementation(async () => Promise.resolve(mockReport)), + } as Partial as LocalTrack; + + // Set up the prototype chain (there is an instanceof check in getRTCStatsReport) + Object.setPrototypeOf(track, LocalTrack.prototype); + + localParticipant.getTrackPublication = vi + .fn() + .mockImplementation((source: Track.Source) => { + if (source === Track.Source.Camera) { + return { + track, + } as unknown as LocalTrackPublication; + } else { + return undefined; + } + }); + + const videoDimensions$ = videoSizeFromParticipant$( + constant(localParticipant), + ); + + const publishedDimensions: { width: number; height: number }[] = []; + videoDimensions$.subscribe((dimensions) => { + if (dimensions) publishedDimensions.push(dimensions); + }); + + await flushPromises(); + + const dimension = publishedDimensions.pop(); + expect(dimension).toEqual({ width: 1280, height: 720 }); + }); + + test("get stats for remote user", async () => { + // vi.useFakeTimers() + const remoteParticipant = mockRemoteParticipant({ + identity: "@bob:example.org:AAAAAA", + }); + + const mockReport: RTCStatsReport = new Map([ + [ + "OT01V639885149", + createMockRtpStats(true, { + frameWidth: 480, + frameHeight: 640, + }), + ], + ]); + + const track = { + source: Track.Source.Camera, + getRTCStatsReport: vi + .fn() + .mockImplementation(async () => Promise.resolve(mockReport)), + } as Partial as LocalTrack; + + // Set up the prototype chain (there is an instanceof check in getRTCStatsReport) + Object.setPrototypeOf(track, LocalTrack.prototype); + + remoteParticipant.getTrackPublication = vi + .fn() + .mockImplementation((source: Track.Source) => { + if (source === Track.Source.Camera) { + return { + track, + } as unknown as RemoteTrackPublication; + } else { + return undefined; + } + }); + + const videoDimensions$ = videoSizeFromParticipant$( + constant(remoteParticipant), + ); + + const publishedDimensions: { width: number; height: number }[] = []; + videoDimensions$.subscribe((dimensions) => { + if (dimensions) publishedDimensions.push(dimensions); + }); + + await flushPromises(); + + const dimension = publishedDimensions.pop(); + expect(dimension).toEqual({ width: 480, height: 640 }); + }); +}); diff --git a/src/utils/videoFit.ts b/src/utils/videoFit.ts new file mode 100644 index 00000000..fdd91be7 --- /dev/null +++ b/src/utils/videoFit.ts @@ -0,0 +1,94 @@ +/* +Copyright 2026 Element Creations Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE in the repository root for full details. +*/ + +import { combineLatest, map, type Observable, of, switchMap } from "rxjs"; +import { + type LocalParticipant, + type RemoteParticipant, + Track, +} from "livekit-client"; + +import { type ObservableScope } from "../state/ObservableScope.ts"; +import { type Behavior } from "../state/Behavior.ts"; +import { + observeInboundRtpStreamStats$, + observeOutboundRtpStreamStats$, +} from "../state/MediaViewModel.ts"; + +type Size = { + width: number; + height: number; +}; + +export function videoFit$( + scope: ObservableScope, + videoSize$: Observable, + tileSize$: Observable, +): Behavior<"cover" | "contain"> { + const fit$ = combineLatest([videoSize$, tileSize$]).pipe( + map(([videoSize, tileSize]) => { + if (!videoSize || !tileSize) { + // If we don't have the sizes, default to cover to avoid black bars. + // This is a reasonable default as it will ensure the video fills the tile, even if it means cropping. + return "cover"; + } + const videoAspectRatio = videoSize.width / videoSize.height; + const tileAspectRatio = tileSize.width / tileSize.height; + + // If video is landscape (ratio > 1) and tile is portrait (ratio < 1) or vice versa, + // we want to use "contain" (fit) mode to avoid excessive cropping + const videoIsLandscape = videoAspectRatio > 1; + const tileIsLandscape = tileAspectRatio > 1; + + // If the orientations are the same, use the cover mode (Preserves the aspect ratio, and the image fills the container.) + // If they're not the same orientation, use the contain mode (Preserves the aspect ratio, but the image is letterboxed - black bars- to fit within the container.) + return videoIsLandscape === tileIsLandscape ? "cover" : "contain"; + }), + ); + + return scope.behavior(fit$, "cover"); +} + +/** + * Helper function to get the video size from a participant. + * It observes the participant's video track stats and extracts the frame width and height. + * @param participant$ - an Observable of a LocalParticipant or RemoteParticipant, or null if no participant is selected. + * @returns an Observable of the video size (width and height) or undefined if the size cannot be determined. + */ +export function videoSizeFromParticipant$( + participant$: Observable, +): Observable<{ width: number; height: number } | undefined> { + return participant$ + .pipe( + // If we have a participant, observe their video track stats. If not, return undefined. + switchMap((p) => { + if (!p) return of(undefined); + if (p.isLocal) { + return observeOutboundRtpStreamStats$(p, Track.Source.Camera); + } else { + return observeInboundRtpStreamStats$(p, Track.Source.Camera); + } + }), + ) + .pipe( + // Extract the frame width and height from the stats. If we don't have valid stats, return undefined. + map((stats) => { + if (!stats) return undefined; + if ( + // For video tracks, frameWidth and frameHeight should be numbers. If they're not, we can't determine the size. + typeof stats.frameWidth !== "number" || + typeof stats.frameHeight !== "number" + ) { + return undefined; + } + return { + width: stats.frameWidth, + height: stats.frameHeight, + }; + }), + ); +} From 15aa67ebb996119a577ffa0907b2742c1208b1cd Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 26 Feb 2026 16:46:08 +0100 Subject: [PATCH 2/8] remove unused fit to frame translation --- locales/en/app.json | 1 - 1 file changed, 1 deletion(-) diff --git a/locales/en/app.json b/locales/en/app.json index 0b0ac7b4..9a85478f 100644 --- a/locales/en/app.json +++ b/locales/en/app.json @@ -250,7 +250,6 @@ "video_tile": { "always_show": "Always show", "camera_starting": "Video loading...", - "change_fit_contain": "Fit to frame", "collapse": "Collapse", "expand": "Expand", "mute_for_me": "Mute for me", From ae8b1f840f188d6d428cd5f360dae17238a711d1 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 26 Feb 2026 17:02:43 +0100 Subject: [PATCH 3/8] add missing mocking --- src/tile/GridTile.test.tsx | 18 +++++++++++++++++- src/tile/SpotlightTile.test.tsx | 18 +++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/tile/GridTile.test.tsx b/src/tile/GridTile.test.tsx index 9bc0efb2..060119ef 100644 --- a/src/tile/GridTile.test.tsx +++ b/src/tile/GridTile.test.tsx @@ -6,7 +6,7 @@ Please see LICENSE in the repository root for full details. */ import { type RemoteTrackPublication } from "livekit-client"; -import { test, expect } from "vitest"; +import { test, expect, beforeAll } from "vitest"; import { render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import { type MatrixRTCSession } from "matrix-js-sdk/lib/matrixrtc"; @@ -28,6 +28,22 @@ global.IntersectionObserver = class MockIntersectionObserver { public disconnect(): void {} } as unknown as typeof IntersectionObserver; +// Mock ResizeObserver as it is needed by the useMeasure hook used in the GridTile, but is not implemented in JSDOM. +// We just need to mock it with empty methods as we don't need to test its functionality here. +beforeAll(() => { + window.ResizeObserver = class ResizeObserver { + public observe(): void { + // do nothing + } + public unobserve(): void { + // do nothing + } + public disconnect(): void { + // do nothing + } + }; +}); + test("GridTile is accessible", async () => { const vm = createRemoteMedia( mockRtcMembership("@alice:example.org", "AAAA"), diff --git a/src/tile/SpotlightTile.test.tsx b/src/tile/SpotlightTile.test.tsx index 981c0369..441b74c4 100644 --- a/src/tile/SpotlightTile.test.tsx +++ b/src/tile/SpotlightTile.test.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { test, expect, vi } from "vitest"; +import { test, expect, vi, beforeAll } from "vitest"; import { isInaccessible, render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import userEvent from "@testing-library/user-event"; @@ -27,6 +27,22 @@ global.IntersectionObserver = class MockIntersectionObserver { public unobserve(): void {} } as unknown as typeof IntersectionObserver; +// Mock ResizeObserver as it is needed by the useMeasure hook used in the SpotlightTile, but is not implemented in JSDOM. +// We just need to mock it with empty methods as we don't need to test its functionality here. +beforeAll(() => { + window.ResizeObserver = class ResizeObserver { + public observe(): void { + // do nothing + } + public unobserve(): void { + // do nothing + } + public disconnect(): void { + // do nothing + } + }; +}); + test("SpotlightTile is accessible", async () => { const vm1 = createRemoteMedia( mockRtcMembership("@alice:example.org", "AAAA"), From adc329a7e7538fd4e5545ce61d5c3dde2206ff66 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 2 Mar 2026 14:41:47 +0100 Subject: [PATCH 4/8] post merge fix --- src/tile/SpotlightTile.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tile/SpotlightTile.tsx b/src/tile/SpotlightTile.tsx index ba99f826..a0b1309b 100644 --- a/src/tile/SpotlightTile.tsx +++ b/src/tile/SpotlightTile.tsx @@ -183,7 +183,7 @@ const SpotlightItem: FC = ({ // Whenever bounds change, inform the viewModel useEffect(() => { if (bounds.width > 0 && bounds.height > 0) { - if (!(vm instanceof ScreenShareViewModel)) { + if (vm.type != "screen share") { vm.setActualDimensions(bounds.width, bounds.height); } } From 5165e95d82c8cb10f84883e35f2f20dc50a609cf Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 2 Mar 2026 15:38:43 +0100 Subject: [PATCH 5/8] fix: default to cover is size are 0 --- src/utils/videoFit.test.ts | 12 ++++++++++++ src/utils/videoFit.ts | 9 +++++++++ 2 files changed, 21 insertions(+) diff --git a/src/utils/videoFit.test.ts b/src/utils/videoFit.test.ts index 9390e8d4..5068526b 100644 --- a/src/utils/videoFit.test.ts +++ b/src/utils/videoFit.test.ts @@ -116,6 +116,18 @@ test.each([ tileSize: VIDEO_480_L, expected: "contain", }, + { + // Should default to cover if the initial size is 0:0. + // Or else it will cause a flash of "contain" mode until the real size is loaded, which can be jarring. + videoSize: VIDEO_480_L, + tileSize: { width: 0, height: 0 }, + expected: "cover", + }, + { + videoSize: { width: 0, height: 0 }, + tileSize: VIDEO_480_L, + expected: "cover", + }, ])( "videoFit$ returns $expected when videoSize is $videoSize and tileSize is $tileSize", ({ videoSize, tileSize, expected }) => { diff --git a/src/utils/videoFit.ts b/src/utils/videoFit.ts index c7b18f03..5f2cc2ce 100644 --- a/src/utils/videoFit.ts +++ b/src/utils/videoFit.ts @@ -36,6 +36,15 @@ export function videoFit$( // This is a reasonable default as it will ensure the video fills the tile, even if it means cropping. return "cover"; } + if ( + videoSize.width === 0 || + videoSize.height === 0 || + tileSize.width === 0 || + tileSize.height === 0 + ) { + // If we have invalid sizes (e.g. width or height is 0), default to cover to avoid black bars. + return "cover"; + } const videoAspectRatio = videoSize.width / videoSize.height; const tileAspectRatio = tileSize.width / tileSize.height; From 273fff20bd0d76b8901efea396479efea2b0231f Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 9 Mar 2026 09:12:03 +0100 Subject: [PATCH 6/8] review: add comment --- src/utils/videoFit.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/utils/videoFit.ts b/src/utils/videoFit.ts index 5f2cc2ce..39dc28c9 100644 --- a/src/utils/videoFit.ts +++ b/src/utils/videoFit.ts @@ -24,6 +24,14 @@ type Size = { height: number; }; +/** + * Computes the appropriate video fit mode ("cover" or "contain") based on the aspect ratios of the video and the tile. + * - If the video and tile have the same orientation (both landscape or both portrait), we use "cover" to fill the tile, even if it means cropping. + * - If the video and tile have different orientations, we use "contain" to ensure the entire video is visible, even if it means letterboxing (black bars). + * @param scope - the ObservableScope to create the Behavior in + * @param videoSize$ - an Observable of the video size (width and height) or undefined if the size is not yet known (no data yet received). + * @param tileSize$ - an Observable of the tile size (width and height) or undefined if the size is not yet known (not yet rendered). + */ export function videoFit$( scope: ObservableScope, videoSize$: Observable, From 513477d28039db9788f042a6f5e91b157faeca00 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 9 Mar 2026 09:45:25 +0100 Subject: [PATCH 7/8] review: Use targetWidth/Height instead of listening to element bounds --- src/grid/TileWrapper.tsx | 6 ++++++ src/state/media/UserMediaViewModel.ts | 18 ++++++++++-------- src/tile/GridTile.test.tsx | 18 +----------------- src/tile/GridTile.tsx | 20 ++++++++------------ src/tile/SpotlightTile.test.tsx | 18 +----------------- src/tile/SpotlightTile.tsx | 22 +++++++++++----------- 6 files changed, 37 insertions(+), 65 deletions(-) diff --git a/src/grid/TileWrapper.tsx b/src/grid/TileWrapper.tsx index 1bed08da..00689a78 100644 --- a/src/grid/TileWrapper.tsx +++ b/src/grid/TileWrapper.tsx @@ -27,7 +27,13 @@ interface Props { state: Parameters>[0], ) => void > | null; + /** + * The width this tile will have once its animations have settled. + */ targetWidth: number; + /** + * The width this tile will have once its animations have settled. + */ targetHeight: number; model: M; Tile: ComponentType>; diff --git a/src/state/media/UserMediaViewModel.ts b/src/state/media/UserMediaViewModel.ts index 1b9f6cbb..16af7f26 100644 --- a/src/state/media/UserMediaViewModel.ts +++ b/src/state/media/UserMediaViewModel.ts @@ -62,12 +62,12 @@ export interface BaseUserMediaViewModel extends MemberMediaViewModel { RTCInboundRtpStreamStats | RTCOutboundRtpStreamStats | undefined >; /** - * Set the actual dimensions of the HTML element. + * Set the target dimensions of the HTML element (final dimension after anim). * This can be used to determine the best video fit (fit to frame / keep ratio). - * @param width - The actual width of the HTML element displaying the video. - * @param height - The actual height of the HTML element displaying the video. + * @param targetWidth - The target width of the HTML element displaying the video. + * @param targetHeight - The target height of the HTML element displaying the video. */ - setActualDimensions: (width: number, height: number) => void; + setTargetDimensions: (targetWidth: number, targetHeight: number) => void; } export interface BaseUserMediaInputs extends Omit< @@ -98,7 +98,9 @@ export function createBaseUserMedia( ); const toggleCropVideo$ = new Subject(); - const actualSize$ = new BehaviorSubject< + // The target size of the video element, used to determine the best video fit. + // The target size is the final size of the HTML element after any animations have completed. + const targetSize$ = new BehaviorSubject< { width: number; height: number } | undefined >(undefined); @@ -130,7 +132,7 @@ export function createBaseUserMedia( videoFit$: videoFit$( scope, videoSizeFromParticipant$(participant$), - actualSize$, + targetSize$, ), toggleCropVideo: () => toggleCropVideo$.next(), rtcBackendIdentity, @@ -155,8 +157,8 @@ export function createBaseUserMedia( return observeRtpStreamStats$(p, Track.Source.Camera, statsType); }), ), - setActualDimensions: (width: number, height: number): void => { - actualSize$.next({ width, height }); + setTargetDimensions: (targetWidth: number, targetHeight: number): void => { + targetSize$.next({ width: targetWidth, height: targetHeight }); }, }; } diff --git a/src/tile/GridTile.test.tsx b/src/tile/GridTile.test.tsx index 2c6e0d15..02f09a17 100644 --- a/src/tile/GridTile.test.tsx +++ b/src/tile/GridTile.test.tsx @@ -6,7 +6,7 @@ Please see LICENSE in the repository root for full details. */ import { type RemoteTrackPublication } from "livekit-client"; -import { test, expect, beforeAll } from "vitest"; +import { test, expect } from "vitest"; import { render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import { type MatrixRTCSession } from "matrix-js-sdk/lib/matrixrtc"; @@ -28,22 +28,6 @@ global.IntersectionObserver = class MockIntersectionObserver { public disconnect(): void {} } as unknown as typeof IntersectionObserver; -// Mock ResizeObserver as it is needed by the useMeasure hook used in the GridTile, but is not implemented in JSDOM. -// We just need to mock it with empty methods as we don't need to test its functionality here. -beforeAll(() => { - window.ResizeObserver = class ResizeObserver { - public observe(): void { - // do nothing - } - public unobserve(): void { - // do nothing - } - public disconnect(): void { - // do nothing - } - }; -}); - test("GridTile is accessible", async () => { const vm = mockRemoteMedia( mockRtcMembership("@alice:example.org", "AAAA"), diff --git a/src/tile/GridTile.tsx b/src/tile/GridTile.tsx index a9ae188b..c8052a65 100644 --- a/src/tile/GridTile.tsx +++ b/src/tile/GridTile.tsx @@ -37,7 +37,6 @@ import { Menu, } from "@vector-im/compound-web"; import { useObservableEagerState } from "observable-hooks"; -import useMeasure from "react-use-measure"; import styles from "./GridTile.module.css"; import { Slider } from "../Slider"; @@ -88,6 +87,8 @@ const UserMediaTile: FC = ({ displayName, mxcAvatarUrl, focusable, + targetWidth, + targetHeight, ...props }) => { const { toggleRaisedHand } = useReactionsSender(); @@ -110,19 +111,12 @@ const UserMediaTile: FC = ({ const handRaised = useBehavior(vm.handRaised$); const reaction = useBehavior(vm.reaction$); - // We need to keep track of the tile size. - // We use this to get the tile ratio, and compare it to the video ratio to decide - // whether to fit the video to frame or keep the ratio. - const [measureRef, bounds] = useMeasure(); - // There is already a ref being passed in, so we need to merge it with the measureRef. - const tileRef = useMergedRefs(ref, measureRef); - // Whenever bounds change, inform the viewModel useEffect(() => { - if (bounds.width > 0 && bounds.height > 0) { - vm.setActualDimensions(bounds.width, bounds.height); + if (targetWidth > 0 && targetHeight > 0) { + vm.setTargetDimensions(targetWidth, targetHeight); } - }, [bounds.width, bounds.height, vm]); + }, [targetWidth, targetHeight, vm]); const AudioIcon = playbackMuted ? VolumeOffSolidIcon @@ -155,7 +149,7 @@ const UserMediaTile: FC = ({ const tile = ( = ({ audioStreamStats={audioStreamStats} videoStreamStats={videoStreamStats} rtcBackendIdentity={rtcBackendIdentity} + targetWidth={targetWidth} + targetHeight={targetHeight} {...props} /> ); diff --git a/src/tile/SpotlightTile.test.tsx b/src/tile/SpotlightTile.test.tsx index 6fa26695..a5332194 100644 --- a/src/tile/SpotlightTile.test.tsx +++ b/src/tile/SpotlightTile.test.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { test, expect, vi, beforeAll } from "vitest"; +import { test, expect, vi } from "vitest"; import { isInaccessible, render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import userEvent from "@testing-library/user-event"; @@ -27,22 +27,6 @@ global.IntersectionObserver = class MockIntersectionObserver { public unobserve(): void {} } as unknown as typeof IntersectionObserver; -// Mock ResizeObserver as it is needed by the useMeasure hook used in the SpotlightTile, but is not implemented in JSDOM. -// We just need to mock it with empty methods as we don't need to test its functionality here. -beforeAll(() => { - window.ResizeObserver = class ResizeObserver { - public observe(): void { - // do nothing - } - public unobserve(): void { - // do nothing - } - public disconnect(): void { - // do nothing - } - }; -}); - test("SpotlightTile is accessible", async () => { const vm1 = mockRemoteMedia( mockRtcMembership("@alice:example.org", "AAAA"), diff --git a/src/tile/SpotlightTile.tsx b/src/tile/SpotlightTile.tsx index a0b1309b..16a8679d 100644 --- a/src/tile/SpotlightTile.tsx +++ b/src/tile/SpotlightTile.tsx @@ -27,7 +27,6 @@ import { useObservableRef } from "observable-hooks"; import { useTranslation } from "react-i18next"; import classNames from "classnames"; import { type TrackReferenceOrPlaceholder } from "@livekit/components-core"; -import useMeasure from "react-use-measure"; import FullScreenMaximiseIcon from "../icons/FullScreenMaximise.svg?react"; import FullScreenMinimiseIcon from "../icons/FullScreenMinimise.svg?react"; @@ -152,7 +151,13 @@ const SpotlightRemoteScreenShareItem: FC< interface SpotlightItemProps { ref?: Ref; vm: MediaViewModel; + /** + * The width this tile will have once its animations have settled. + */ targetWidth: number; + /** + * The height this tile will have once its animations have settled. + */ targetHeight: number; focusable: boolean; intersectionObserver$: Observable; @@ -175,21 +180,16 @@ const SpotlightItem: FC = ({ }) => { const ourRef = useRef(null); - // We need to keep track of the tile size. - // We use this to get the tile ratio, and compare it to the video ratio to decide - // whether to fit the video to frame or keep the ratio. - const [measureRef, bounds] = useMeasure(); - - // Whenever bounds change, inform the viewModel + // Whenever target bounds change, inform the viewModel useEffect(() => { - if (bounds.width > 0 && bounds.height > 0) { + if (targetWidth > 0 && targetHeight > 0) { if (vm.type != "screen share") { - vm.setActualDimensions(bounds.width, bounds.height); + vm.setTargetDimensions(targetWidth, targetHeight); } } - }, [bounds.width, bounds.height, vm]); + }, [targetWidth, targetHeight, vm]); - const ref = useMergedRefs(ourRef, theirRef, measureRef); + const ref = useMergedRefs(ourRef, theirRef); const focusUrl = useBehavior(vm.focusUrl$); const displayName = useBehavior(vm.displayName$); const mxcAvatarUrl = useBehavior(vm.mxcAvatarUrl$); From ca3837f44ebdb3340340af1b95d8eae924f67667 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 9 Mar 2026 15:07:42 +0100 Subject: [PATCH 8/8] fix merge issue that added back a deprecated test --- src/state/media/MediaViewModel.test.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/state/media/MediaViewModel.test.ts b/src/state/media/MediaViewModel.test.ts index f64dd3ee..9d873ccb 100644 --- a/src/state/media/MediaViewModel.test.ts +++ b/src/state/media/MediaViewModel.test.ts @@ -160,21 +160,6 @@ test("control a participant's screen share volume", () => { }); }); -test("toggle fit/contain for a participant's video", () => { - const vm = mockRemoteMedia(rtcMembership, {}, mockRemoteParticipant({})); - withTestScheduler(({ expectObservable, schedule }) => { - schedule("-ab|", { - a: () => vm.toggleCropVideo(), - b: () => vm.toggleCropVideo(), - }); - expectObservable(vm.cropVideo$).toBe("abc", { - a: true, - b: false, - c: true, - }); - }); -}); - test("local media remembers whether it should always be shown", () => { const vm1 = mockLocalMedia( rtcMembership,