amazon-chime-sdk-component-library-react icon indicating copy to clipboard operation
amazon-chime-sdk-component-library-react copied to clipboard

MeetingManager cannot be shared with multiple MeetingProviders in different React branches

Open jamsesso opened this issue 3 years ago • 9 comments

Describe the bug I have a single MeetingManager instance that I need to use to render a Chime meeting at different areas of a layout that cannot be wrapped by a parent. Illustration:

const mm = new MeetingManager(...);

<>
  <MeetingProvider meetingManager={mm}>
    <AppSection1 />
  </MeetingProvider>

  // further down...

  <MeetingProvider meetingManager={mm}>
    <AppSection2 />
  </MeetingProvider>
</>

This does not work. According to the docs here: https://aws.github.io/amazon-chime-sdk-component-library-react/?path=/story/sdk-providers-meetingprovider--page it should be working.

Neither AppSection1 nor AppSection2 behave correctly when using the React library hooks.

To Reproduce

  1. Create a Chime meeting and pass in the meetingInfo object to Page.
  2. Join the meeting in some other tab as jamsesso (externalUserId)
  3. Source to reproduce:
import React, { useEffect, useMemo, useRef, useState } from 'react';

import { MeetingProvider, MeetingManager, useRosterState, useMeetingManager, useRemoteVideoTileState, useAudioVideo } from 'amazon-chime-sdk-component-library-react';
import { LogLevel } from 'amazon-chime-sdk-js';

export default function Page({ meetingInfo }) {
  // `meetingInfo` contains { meetingInfo, attendeeInfo } as you'd pass to `MeetingProvider`.
  const meetingManager = useMemo(
    () => new MeetingManager({ logLevel: LogLevel.INFO }),
    []
  );

  return (
    <div style={{display: 'flex', height: '100%'}}>
      <div style={{flex: 1, background: 'pink', height: '100%'}}>
        <MeetingProvider meetingManager={meetingManager}>
          <VideoPlayer meetingInfo={meetingInfo} />
        </MeetingProvider>
      </div>
      <div style={{flex: 1, background: 'red', height: '100%'}}>
        <MeetingProvider meetingManager={meetingManager}>
          <VideoPlayer meetingInfo={meetingInfo} />
        </MeetingProvider>
      </div>
    </div>
  );
}

function VideoPlayer({ meetingInfo }) {
  const [ready, setReady] = useState(false);
  const meetingManager = useMeetingManager();
  const videoRef = useRef(null);
  const speakerTiles = useSpeakerTiles();
  const av = useAudioVideo();

  useEffect(
    () => {
      (async () => {
        await meetingManager.join(meetingInfo);
        await meetingManager.start();
        setReady(true);
      })();
    },
    [meetingManager, meetingInfo]
  );

  useEffect(
    () => {
      // Always render attendee "jamsesso" into the video tile.
      if (ready && speakerTiles['jamsesso']) {
        av.bindVideoElement(speakerTiles['jamsesso'], videoRef.current);

        return () => {
          av.unbindVideoElement(speakerTiles['jamsesso']);
        }
      }
    },
    [ready, speakerTiles]
  );

  return <video ref={videoRef} style={{width: '300px'}} />;
}

function useSpeakerTiles() {
  const { roster } = useRosterState();
  const remoteVideoTileState = useRemoteVideoTileState();

  return useMemo(
    () =>
      Object.keys(roster).reduce((map, chimeAttendeeId) => {
        map[roster[chimeAttendeeId].externalUserId] = remoteVideoTileState.attendeeIdToTileId[chimeAttendeeId];
        return map;
      }, {}),
    [roster, remoteVideoTileState]
  );
}

Expected behavior A video should start playing in both the pink and red box. If you comment out either the red VideoPlayer or the pink VideoPlayer, it works fine. But when there are two, it does not.

Screenshots N/A

Desktop (please complete the following information):

  • OS: MacOS 10.15.6 Catalina
  • Browser: Chrome
  • Version: 89.0.4389.128

Additional context N/A

jamsesso avatar Apr 27 '21 22:04 jamsesso

I haven’t dug into this deeply, but I suspect what you’re seeing is that the same video stream cannot be bound to two elements in the page. Try joining the meeting with two users, A and B, and make A play in the pink element and B in the other. Does that work?

rnewman avatar Apr 28 '21 15:04 rnewman

@rnewman Doesn't look like it. Here are the approximate changes I made:

- <VideoPlayer meetingInfo={meetingInfo} />
+ <VideoPlayer meetingInfo={meetingInfo} speakerId="jamsesso1" />

and

  useEffect(
    () => {
      // Always render attendee "jamsesso" into the video tile.
-      if (ready && speakerTiles['jamsesso']) {
-        av.bindVideoElement(speakerTiles['jamsesso'], videoRef.current);
+      if (ready && speakerTiles[speakerId]) {
+        av.bindVideoElement(speakerTiles[speakerId], videoRef.current);

        return () => {
-          av.unbindVideoElement(speakerTiles['jamsesso']);
+          av.unbindVideoElement(speakerTiles[speakerId]);
        }
      }
    },
    [ready, speakerTiles]
  );

Side note: For what it's worth, I am able to bind a single user to multiple <video> elements in a single MeetingProvider by cloning the VideoTile instance returned from audioVideo.getVideoTile(tileId).

jamsesso avatar Apr 28 '21 16:04 jamsesso

To switch into core JS library terminology and go into more depth now I'm at a computer:

As far as I can tell, the root of the issue is that a DefaultVideoTile tracks which HTML element it's bound to, and you're using the same tiles in both 'branches', because you're using the same remote video tile state in both. That is, both of your useSpeakerTiles calls are referring to the same state in:

https://github.com/aws/amazon-chime-sdk-component-library-react/blob/d4c837f653912ddda1c3034da76a1b64c5181cf6/src/providers/RemoteVideoTileProvider/index.tsx

In order to make this work you would need to have two different tile providers, but there's only one in the stock MeetingProvider:

https://github.com/aws/amazon-chime-sdk-component-library-react/blob/d4c837f653912ddda1c3034da76a1b64c5181cf6/src/providers/MeetingProvider/index.tsx#L49

When you clone the tile instance you are preventing it from unbinding the previous video element in the original tile state when you bind the new one.

It should be possible for you to do this:

        <MeetingProvider meetingManager={meetingManager}>
          <VideoPlayer meetingInfo={meetingInfo} speakerId="bbbb" />
        </MeetingProvider>

…

        <MeetingProvider meetingManager={meetingManager}>
          <VideoPlayer meetingInfo={meetingInfo} speakerId="aaaa" />
        </MeetingProvider>

because the tiles in each won't overlap — the state for attendee aaaa will be a sibling of bbbb. Does that work?

rnewman avatar Apr 28 '21 16:04 rnewman

I've confirmed that attempting to bind aaaa and bbbb still does not work.

jamsesso avatar Apr 28 '21 16:04 jamsesso

@jamsesso In your console log, did you see this MeetingSessionStatusCode.AudioJoinedFromAnotherDevice? From your code, it looks like you call meetingManager.join(meetingInfo) and start for each video player instance with the same meetingInfo, which I don't think we support right now. It should cause the previous join to stop with the above message.

ltrung avatar Apr 28 '21 18:04 ltrung

@ltrung I added a flag so that only one VideoPlayer calls meetingManager.join() and meetingManager.start(). See:

  useEffect(
    (): void => {
      if (shouldJoin) {
        (async () => {
          await meetingManager.join(meetingInfo);
          await meetingManager.start();
          setReady(true);
        })();
      }
    },
    [meetingManager, meetingInfo]
  );

And now only the side that calls join has a video bound - even if there are two different attendees on each side. Screenshot:

Screen Shot 2021-04-28 at 6 20 32 PM

jamsesso avatar Apr 28 '21 21:04 jamsesso

@jamsesso Could you give more details on your use case? Could you just create a new attendee and use a different meetingInfo (same meetingId but different attendeeId)?

ltrung avatar Apr 28 '21 21:04 ltrung

@ltrung Our application lets you drag and drop the video tile from the pink side to the red side. When the drop event occurs, we want both videos to be available.

jamsesso avatar Apr 29 '21 12:04 jamsesso

Investigation on the re-usable MeetingManager:

  1. SDK hooks, providers, components who depend either directly on the meeting manager states and observe the changes to those states and then update themselve may not be affected if you re-use the meeting manager.

  2. The LocalVideo and LocalVideoProvider is affected as JS SDK currently only can bind a single video tile to a video stream. Meaning, if you are sharing the MeetingManager instance across multiple MeetingProvider's, turning on a video tile will work but once you toggle one video the other video will also go black since we disconnect the video stream bound to the video tile.

  3. I have attached an testing app zip wherein, I tried to re-use a single meeting manager with two different branches which ran into problems and the components did not worked as expected.

  4. You will be able to use the same MeetingManager instance for different MeetingProvider's but the attendee join info still has to be different, if not you will run into AudioJoinedFromAnotherDevice error. Hence, the attached sample code tries to join the same meeting with different attendees. But it tries to simulate and check whether the local video toggling works and mute toggling works.

  5. Since, the mute toggling is directly dependent on JS SDK APIs, muting one mutes other since the same hook is used in two places under two MeetingProviders which is using the same state. I verified this by monitoring the muted states from the useToggleLocalMute hook. But, this is not the case for the video.

  6. Toggling video toggles the isVideoEnabled state but due to 1:1 mapping between video tile to video stream as explained earlier, does not work as expected.

After testing and working with the findings we found that:

  1. Sharing MeetingManager actually is not useful and we intended it to only be used to provide access to meeting manager instance variables to be available across different MeetingProvider but that internally affects other wrapped providers.

  2. Hence, we added this prop for very few specific cases where you are migrating and dont need other providers / hooks in the migration part of the app which run into trouble.

  3. We also learned that this change was poorly documented and tested. We will update the documentation to note not to use this prop or see whether it really satisfies your use-case.

  4. There is no action item we can take to fix this soon as the React SDK depends on JS SDK and would be an enhancement.

The testing app:

  1. Creates a meeting and different attendees on page load.
  2. Provides toggle video and mute buttons.
  3. If clicked on toggle video for the first time both video tiles show the video stream.
  4. But once you toggle again one video goes black and other goes off completely until to toggle both back online.

Note: The attached code is just a testing code based on the code attached in the GitHub issue.

Action Items:

  • Update the documentation on the limitation, use of re-usable meeting managers.

Testing App

devalevenkatesh avatar May 22 '21 00:05 devalevenkatesh