Improvements to group calling video requests

ts/calling/constants.ts

@@ -8,5 +8,6 @@ export const REQUESTED_VIDEO_WIDTH = 640;
 export const REQUESTED_VIDEO_HEIGHT = 480;
 export const REQUESTED_VIDEO_FRAMERATE = 30;
 
-export const MAX_FRAME_SIZE = 1920 * 1080;
+export const MAX_FRAME_WIDTH = 1920;
-export const FRAME_BUFFER_SIZE = MAX_FRAME_SIZE * 4;
+export const MAX_FRAME_HEIGHT = 1080;
+export const FRAME_BUFFER_SIZE = MAX_FRAME_WIDTH * MAX_FRAME_HEIGHT * 4;

ts/components/CallingPipRemoteVideo.tsx

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 
 import React, { useMemo, useEffect } from 'react';
-import { maxBy } from 'lodash';
+import { clamp, maxBy } from 'lodash';
 import type { VideoFrameSource } from 'ringrtc';
 import { Avatar } from './Avatar';
 import { CallBackgroundBlur } from './CallBackgroundBlur';
@@ -18,11 +18,13 @@ import { CallMode } from '../types/Calling';
 import { AvatarColors } from '../types/Colors';
 import type { SetRendererCanvasType } from '../state/ducks/calling';
 import { useGetCallingFrameBuffer } from '../calling/useGetCallingFrameBuffer';
+import { MAX_FRAME_WIDTH } from '../calling/constants';
 import { usePageVisibility } from '../hooks/usePageVisibility';
 import { missingCaseError } from '../util/missingCaseError';
 import { nonRenderedRemoteParticipant } from '../util/ringrtc/nonRenderedRemoteParticipant';
 
-// This value should be kept in sync with the hard-coded CSS height.
+// This value should be kept in sync with the hard-coded CSS height. It should also be
+//   less than `MAX_FRAME_HEIGHT`.
 const PIP_VIDEO_HEIGHT_PX = 120;
 
 const NoVideo = ({
@@ -110,14 +112,13 @@ export const CallingPipRemoteVideo = ({
     if (isPageVisible) {
       setGroupCallVideoRequest(
         activeCall.remoteParticipants.map(participant => {
-          const isVisible =
+          if (participant === activeGroupCallSpeaker) {
-            participant === activeGroupCallSpeaker &&
-            participant.hasRemoteVideo;
-          if (isVisible) {
             return {
               demuxId: participant.demuxId,
-              width: Math.floor(
+              width: clamp(
-                PIP_VIDEO_HEIGHT_PX * participant.videoAspectRatio
+                Math.floor(PIP_VIDEO_HEIGHT_PX * participant.videoAspectRatio),
+                1,
+                MAX_FRAME_WIDTH
               ),
               height: PIP_VIDEO_HEIGHT_PX,
             };

ts/components/GroupCallRemoteParticipant.tsx

@@ -22,7 +22,7 @@ import { ConfirmationDialog } from './ConfirmationDialog';
 import { Intl } from './Intl';
 import { ContactName } from './conversation/ContactName';
 import { useIntersectionObserver } from '../hooks/useIntersectionObserver';
-import { MAX_FRAME_SIZE } from '../calling/constants';
+import { MAX_FRAME_HEIGHT, MAX_FRAME_WIDTH } from '../calling/constants';
 
 const MAX_TIME_TO_SHOW_STALE_VIDEO_FRAMES = 5000;
 const MAX_TIME_TO_SHOW_STALE_SCREENSHARE_FRAMES = 60000;
@@ -150,7 +150,8 @@ export const GroupCallRemoteParticipant: React.FC<PropsType> = React.memo(
       if (
         frameWidth < 2 ||
         frameHeight < 2 ||
-        frameWidth * frameHeight > MAX_FRAME_SIZE
+        frameWidth > MAX_FRAME_WIDTH ||
+        frameHeight > MAX_FRAME_HEIGHT
       ) {
         return;
       }

ts/components/GroupCallRemoteParticipants.tsx

@@ -3,7 +3,7 @@
 
 import React, { useCallback, useState, useMemo, useEffect } from 'react';
 import Measure from 'react-measure';
-import { takeWhile, chunk, maxBy, flatten, noop } from 'lodash';
+import { takeWhile, clamp, chunk, maxBy, flatten, noop } from 'lodash';
 import type { VideoFrameSource } from 'ringrtc';
 import { GroupCallRemoteParticipant } from './GroupCallRemoteParticipant';
 import {
@@ -21,9 +21,10 @@ import { useDevicePixelRatio } from '../hooks/useDevicePixelRatio';
 import { nonRenderedRemoteParticipant } from '../util/ringrtc/nonRenderedRemoteParticipant';
 import { missingCaseError } from '../util/missingCaseError';
 import { SECOND } from '../util/durations';
-import { filter } from '../util/iterables';
+import { filter, join } from '../util/iterables';
 import * as setUtil from '../util/setUtil';
 import * as log from '../logging/log';
+import { MAX_FRAME_HEIGHT, MAX_FRAME_WIDTH } from '../calling/constants';
 
 const MIN_RENDERED_HEIGHT = 180;
 const PARTICIPANT_MARGIN = 10;
@@ -52,9 +53,9 @@ type PropsType = {
 };
 
 enum VideoRequestMode {
-  Normal,
+  Normal = 'Normal',
-  LowResolution,
+  LowResolution = 'LowResolution',
-  NoVideo,
+  NoVideo = 'NoVideo',
 }
 
 // This component lays out group call remote participants. It uses a custom layout
@@ -298,6 +299,9 @@ export const GroupCallRemoteParticipants: React.FC<PropsType> = ({
   );
 
   const videoRequestMode = useVideoRequestMode();
+  useEffect(() => {
+    log.info(`Group call now using ${videoRequestMode} video request mode`);
+  }, [videoRequestMode]);
 
   useEffect(() => {
     let videoRequest: Array<GroupCallVideoRequest>;
@@ -310,35 +314,44 @@ export const GroupCallRemoteParticipants: React.FC<PropsType> = ({
             if (participant.sharingScreen) {
               // We want best-resolution video if someone is sharing their screen.
               scalar = Math.max(devicePixelRatio, 1);
-            } else if (participant.hasRemoteVideo) {
-              scalar = VIDEO_REQUEST_SCALAR;
             } else {
-              scalar = 0;
+              scalar = VIDEO_REQUEST_SCALAR;
             }
             return {
               demuxId: participant.demuxId,
-              width: Math.floor(
+              width: clamp(
-                gridParticipantHeight * participant.videoAspectRatio * scalar
+                Math.floor(
+                  gridParticipantHeight * participant.videoAspectRatio * scalar
+                ),
+                1,
+                MAX_FRAME_WIDTH
+              ),
+              height: clamp(
+                Math.floor(gridParticipantHeight * scalar),
+                1,
+                MAX_FRAME_HEIGHT
               ),
-              height: Math.floor(gridParticipantHeight * scalar),
             };
           }),
           ...overflowedParticipants.map(participant => {
-            if (
+            if (invisibleDemuxIds.has(participant.demuxId)) {
-              !participant.hasRemoteVideo ||
-              invisibleDemuxIds.has(participant.demuxId)
-            ) {
               return nonRenderedRemoteParticipant(participant);
             }
 
             return {
               demuxId: participant.demuxId,
-              width: Math.floor(
+              width: clamp(
-                OVERFLOW_PARTICIPANT_WIDTH * VIDEO_REQUEST_SCALAR
+                Math.floor(OVERFLOW_PARTICIPANT_WIDTH * VIDEO_REQUEST_SCALAR),
+                1,
+                MAX_FRAME_WIDTH
               ),
-              height: Math.floor(
+              height: clamp(
-                (OVERFLOW_PARTICIPANT_WIDTH / participant.videoAspectRatio) *
+                Math.floor(
-                  VIDEO_REQUEST_SCALAR
+                  (OVERFLOW_PARTICIPANT_WIDTH / participant.videoAspectRatio) *
+                    VIDEO_REQUEST_SCALAR
+                ),
+                1,
+                MAX_FRAME_HEIGHT
               ),
             };
           }),
@@ -454,6 +467,12 @@ function useInvisibleParticipants(
     []
   );
 
+  useEffect(() => {
+    log.info(
+      `Invisible demux IDs changed to [${join(invisibleDemuxIds, ',')}]`
+    );
+  }, [invisibleDemuxIds]);
+
   useEffect(() => {
     const remoteParticipantDemuxIds = new Set<number>(
       remoteParticipants.map(r => r.demuxId)

ts/test-both/util/iterables_test.ts

@@ -12,6 +12,7 @@ import {
   groupBy,
   isEmpty,
   isIterable,
+  join,
   map,
   reduce,
   repeat,
@@ -320,6 +321,31 @@ describe('iterable utilities', () => {
     });
   });
 
+  describe('join', () => {
+    it('returns the empty string for empty iterables', () => {
+      assert.isEmpty(join([], 'x'));
+      assert.isEmpty(join(new Set(), 'x'));
+    });
+
+    it("returns the stringified value if it's the only value", () => {
+      assert.strictEqual(join(new Set(['foo']), 'x'), 'foo');
+      assert.strictEqual(join(new Set([123]), 'x'), '123');
+      assert.strictEqual(join([{ toString: () => 'foo' }], 'x'), 'foo');
+    });
+
+    it('returns each value stringified, joined by separator', () => {
+      assert.strictEqual(
+        join(new Set(['foo', 'bar', 'baz']), ' '),
+        'foo bar baz'
+      );
+      assert.strictEqual(join(new Set([1, 2, 3]), '--'), '1--2--3');
+    });
+
+    it('handles undefined and null like Array.prototype.join', () => {
+      assert.strictEqual(join(new Set([undefined, null]), ','), ',');
+    });
+  });
+
   describe('map', () => {
     it('returns an empty iterable when passed an empty iterable', () => {
       const fn = sinon.fake();

ts/util/iterables.ts

@@ -133,6 +133,21 @@ export function groupBy<T>(
 export const isEmpty = (iterable: Iterable<unknown>): boolean =>
   Boolean(iterable[Symbol.iterator]().next().done);
 
+export function join(iterable: Iterable<unknown>, separator: string): string {
+  let hasProcessedFirst = false;
+  let result = '';
+  for (const value of iterable) {
+    const stringifiedValue = value == null ? '' : String(value);
+    if (hasProcessedFirst) {
+      result += separator + stringifiedValue;
+    } else {
+      result = stringifiedValue;
+    }
+    hasProcessedFirst = true;
+  }
+  return result;
+}
+
 export function map<T, ResultT>(
   iterable: Iterable<T>,
   fn: (value: T) => ResultT