Video 8472 add dominant speaker #676

olipyskoty · 2022-05-10T19:00:13Z

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

VIDEO-8472

Description

This PR adds the dominantSpeaker functionality for grid mode. The dominant speaker UI adds a green highlight around the participant's video when they are actively speaking.

When the dominantSpeaker is "on screen" (on the first page of the grid), their video will be outlined in green, and their position in the grid will not change.
When the dominantSpeaker is "off screen" (not on the first page of the grid), their video will be outlined in green and they will replace the least recent (oldest) dominantSpeaker on the first page. The least recent dominantSpeaker is moved to the end of the participant list.

Burndown

Before review

Updated CHANGELOG.md if necessary
Added unit tests if necessary
Updated affected documentation
Verified locally with npm test
Manually sanity tested running locally
Included screenshot as PR comment (if needed)
Ready for review

Before merge

Got one or more +1s
Re-tested if necessary

timrozum · 2022-05-10T20:16:32Z

src/hooks/useGridParticipants/useGridParticipants.ts

+            1,
+            newDominantSpeakerWithStartTime!
+          );
+          // Add the least recent dominant speaker back into the  array at the end:


I think this is the only discrepancy between iOS and web. Currently iOS moves the oldest dominant speaker on the first page to the start of the second page. This way the entire grid is ordered by dominant speaker time + connection time which seems intuitive. I am thinking this is how zoom works.

@timmydoza I think what @timrozum did with iOS makes sense. However, it is kind of tricky with the current implementation for the pagination feature. Since we are filling each page with the number of maxGridParticipants, the first participant on the second page may actually be an onscreenParticipant from the first page.

For instance, if a room has 13 participants, and maxGridParticipants is 9, the first participant on the second page will actually be the fifth participant from the first page (when on iOS the first participant on the second page would be the 10th participant). Did we come to a decision about updating the usePagination hook to mirror was iOS does?

timmydoza

Nice work! It functions great, and the binary search stuff looks awesome.

I've been thinking about the use of useCollaborationParticipants in useMainParticipant, MenuBar, and ParticipantAudioTracks. None of those hooks/components need to know when the domainant speaker changes, but useCollaborationParticipants adds a listener to that event and updates its state when the dominant speaker changes.

I'm wondering if we could use a plain useParticipants hooks in these instances, where the array of participants is not updated when the dominant speaker is updated. I originally thought that a plain useParticipants hook could also provide an array of participants to the useCollaborationParticipants and useGridParticipants hooks, but I think that might be a bit trickier than I first thought.

So I'm going to think about this a little more. There's an opportunity here to get rid of some rerenders, but I'm not sure if it's really worth the effort. We can chat more about it later.

timmydoza · 2022-05-11T22:46:04Z

src/hooks/useDominantSpeaker/useDominantSpeaker.tsx

+        if (newDominantSpeaker !== null || (newDominantSpeaker === null && includeNull)) {
          setDominantSpeaker(newDominantSpeaker);
        }


I think this can be simplified a little to be easier to understand:

Suggested change

if (newDominantSpeaker !== null || (newDominantSpeaker === null && includeNull)) {

setDominantSpeaker(newDominantSpeaker);

}

if (includeNull || newDominantSpeaker !== null) {

setDominantSpeaker(newDominantSpeaker);

}

This caught my eye because we see newDominantSpeaker !== null and newDominantSpeaker === null in the same if statement. We don't need both, because if newDominantSpeaker === null is true, then we can infer that newDominantSpeaker !== null is false without having to check.

timmydoza · 2022-05-11T22:46:51Z

src/hooks/useDominantSpeaker/useDominantSpeaker.test.tsx

@@ -33,6 +33,15 @@ describe('the useDominantSpeaker hook', () => {
    expect(result.current).toBe('mockDominantSpeaker');
  });

+  it('should set "null" when there is no dominant speaker and excludeNull is true', () => {


Suggested change

it('should set "null" when there is no dominant speaker and excludeNull is true', () => {

it('should set "null" when there is no dominant speaker and includeNull is true', () => {

timmydoza · 2022-05-11T22:47:41Z

src/hooks/useGridLayout/useGridLayout.ts

+    // Here we use binary search to guess the new size of each video in the grid
+    // so that they all fit nicely for any screen size up to a width of 16384px.
+    let minVideoWidth = 0;
+    let maxVideoWidth = 2 ** 14;

-    // Here we try to guess the new size of each video by increasing the width by 1.
-    // Once layoutIsTooSmall becomes false, we have found the correct video width:
-    while (layoutIsTooSmall(newParticipantVideoWidth + 1, participantCount, containerWidth, containerHeight)) {
-      newParticipantVideoWidth++;
+    while (maxVideoWidth - minVideoWidth > 1) {
+      let mid = (maxVideoWidth - minVideoWidth) / 2 + minVideoWidth;
+      const isLower = layoutIsTooSmall(mid, participantCount, containerWidth, containerHeight);
+
+      if (isLower) {
+        minVideoWidth = mid;
+      } else {
+        maxVideoWidth = mid;
+      }
    }


This looks great!

timmydoza · 2022-05-18T04:33:32Z

src/components/ParticipantList/ParticipantList.tsx

@@ -3,7 +3,7 @@ import clsx from 'clsx';
 import Participant from '../Participant/Participant';
 import { makeStyles, createStyles, Theme } from '@material-ui/core/styles';
 import useMainParticipant from '../../hooks/useMainParticipant/useMainParticipant';
-import useParticipants from '../../hooks/useParticipants/useParticipants';
+import useParticipants from '../../hooks/useCollaborationParticipants/useCollaborationParticipants';


Wondering if we should also change the name of the variable here:
import useCollaborationParticipants from //...?

timmydoza · 2022-05-18T04:33:54Z

src/hooks/useCollaborationParticipants/useCollaborationParticipants.test.tsx

@@ -1,7 +1,7 @@
 import { act, renderHook } from '@testing-library/react-hooks';
 import EventEmitter from 'events';
 import useDominantSpeaker from '../useDominantSpeaker/useDominantSpeaker';
-import useParticipants from './useParticipants';
+import useParticipants from './useCollaborationParticipants';


Same here: import useCollaborationParticipants?

timmydoza · 2022-05-18T04:42:24Z

src/hooks/useGridParticipants/useGridParticipants.ts

+          // update the participant's dominantSpeakerStartTime to when they became the new dominant speaker:
+          newDominantSpeaker.dominantSpeakerStartTime = Date.now();
+        } else {
+          return newParticipantsArray;


Here, I wonder if it would be better to return prevParticipants instead.

On this line, we just want to bail out if there is no dominant speaker. So if we return newParticipantsArray, it's going to be a different reference than the array that is stored in state (thanks to .slice()), which (I think) would cause this hook to update it's state, which would then cause components to rerender.

Returning prevParticipants is the same reference, so the state would not update, and components wouldn't update either.

I don't think this makes any real difference, but it crossed my mind.

timmydoza · 2022-05-18T04:43:20Z

src/hooks/useGridParticipants/useGridParticipants.ts

+        }
+
+        // Here we use maxGridParticipants - 1 since the localParticipant will always be the first in the grid
+        let maxOnScreenParticipants = maxGridParticipants - 1;


Could this be const?

timmydoza · 2022-05-18T04:44:42Z

src/hooks/useGridParticipants/useGridParticipants.ts

+
+        // Here we use maxGridParticipants - 1 since the localParticipant will always be the first in the grid
+        let maxOnScreenParticipants = maxGridParticipants - 1;
+        const onscreenParticipants = newParticipantsArray.slice(0, maxOnScreenParticipants);


In this app, I don't think onscreenParticipants is the right variable name. Maybe something like firstPageParticipants? This hook is just moving dominant speakers to the first page, but they may not be 'on screen' if the user is looking at a page other than the first.

timmydoza · 2022-05-18T04:49:16Z

src/hooks/useGridParticipants/useGridParticipants.ts

+            (a, b) => a.dominantSpeakerStartTime - b.dominantSpeakerStartTime
+          );
+          const leastRecentDominantSpeaker = sortedOnscreenParticipants[0];
+          const newDominantSpeakerWithStartTime = newParticipantsArray.find(p => p.participant === dominantSpeaker);


Here I don't think we need to save a new variable, since it's going to be the same object reference as newDominantSpeaker.

With this we can just use newDominantSpeaker in the following lines (without the ! since TypeScript knows we can't get to this point without this variable thanks to the return on line 35.

/** Reorder the onscreen participants */ // Temporarily remove the newest dominant speaker: newParticipantsArray.splice(newParticipantsArray.indexOf(newDominantSpeaker), 1); // Remove the least recent dominant speaker and replace them with the newest: newParticipantsArray.splice(newParticipantsArray.indexOf(leastRecentDominantSpeaker), 1, newDominantSpeaker); // Add the least recent dominant speaker back into the array after the last onscreen participant. newParticipantsArray.splice(maxGridParticipants - 1, 0, leastRecentDominantSpeaker);

timmydoza · 2022-05-18T05:07:21Z

src/hooks/useGridParticipants/useGridParticipants.ts

+export default function useGridParticipants() {
+  const [orderedParticipants, setOrderedParticipants] = useState<OrderedParticipant[]>([]);
+  const { room } = useVideoContext();
+  const dominantSpeaker = useDominantSpeaker(true);


Do we want to includeNull here?

Nope, looks like it isn't needed. I thought it was so that when the dominant speaker stops talking (and becomes null), they wouldn't be highlighted in green any more. Now I realize that by passing includeNull in the <GridView /> component, this is taken care of.

timmydoza

Looks good! My only thought is that some comments in useGridParticipant still refer to 'onscreen' participants, while the new variable name is firstPageParticipants.

olipyskoty added 8 commits May 3, 2022 15:39

use collab participants

e30bb3f

add useGridParticipants to app

4b12bcc

update variable names

d7fa279

add binary search algo

b255bf1

update failing tests for mocks and imports

d63937b

remove room prop from useGridParticipants

5860f60

change helper function back to arrow function

48290f5

change border size

1918f2b

olipyskoty requested review from timrozum and timmydoza May 10, 2022 19:00

timrozum reviewed May 10, 2022

View reviewed changes

olipyskoty added 8 commits May 11, 2022 13:21

storybook: randomize participant backgrounds

b67069a

account for null values in useDominantSpeaker

9cc5bdf

update splice to account for local participant

399942c

update failing tests

b4557c8

change variable name from excludeNull to includeNull

5aeb1ed

fix failing test

24547e5

fix typo in test description

d74ff84

fix comment to accurately describe code

073712d

timmydoza suggested changes May 18, 2022

View reviewed changes

olipyskoty added 5 commits May 18, 2022 13:00

fix import name

1c09e4c

fix typo in test description

49beacf

update variable names for clarity

a2bf2e5

update if statement logic

277b4aa

add comment to useParticipants

84373a8

timmydoza approved these changes May 19, 2022

View reviewed changes

update comments to reflect firstPageParticipants

c6b0c64

olipyskoty merged commit e923527 into feature/grid-mode May 19, 2022

olipyskoty deleted the VIDEO-8472-add-dominant-speaker branch May 19, 2022 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video 8472 add dominant speaker #676

Video 8472 add dominant speaker #676

olipyskoty commented May 10, 2022

timrozum May 10, 2022

olipyskoty May 10, 2022

timmydoza left a comment

timmydoza May 11, 2022 •

edited

Loading

timmydoza May 11, 2022

timmydoza May 11, 2022

timmydoza May 18, 2022

timmydoza May 18, 2022

timmydoza May 18, 2022

timmydoza May 18, 2022

timmydoza May 18, 2022

timmydoza May 18, 2022

timmydoza May 18, 2022

olipyskoty May 18, 2022

timmydoza left a comment •

edited

Loading

	it('should set "null" when there is no dominant speaker and excludeNull is true', () => {
	it('should set "null" when there is no dominant speaker and includeNull is true', () => {

Video 8472 add dominant speaker #676

Video 8472 add dominant speaker #676

Conversation

olipyskoty commented May 10, 2022

Pull Request Details

JIRA link(s):

Description

Burndown

Before review

Before merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmydoza left a comment

Choose a reason for hiding this comment

timmydoza May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmydoza left a comment • edited Loading

Choose a reason for hiding this comment

timmydoza May 11, 2022 •

edited

Loading

timmydoza left a comment •

edited

Loading