Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect error being thrown when starting video input device from MeetingManager #888

Closed
6 tasks done
Nanosync opened this issue Apr 18, 2023 · 5 comments
Closed
6 tasks done
Labels
Bug Something isn't working

Comments

@Nanosync
Copy link

Nanosync commented Apr 18, 2023

What happened and what did you expect to happen?

I am using meetingManager.startVideoInputDevice() and meetingManager.stopVideoInputDevice to start/stop the video input device, and if an error is thrown, the error name is used to display an appropriate error message.

Current Result:

These 2 functions are wrapping the original Error coming from handleGetUserMedia (in device controller from the JS sdk) and throwing a generic Error.

Expected Result:

The original Error should be thrown for these 2 functions, and possibly all the other functions (startAudioInputDevice, stopAudioInputDevice, startAudioOutputDevice, stopAudioOutputDevice).

I also note that in setupDeviceLabelTrigger, the error is being wrapped (throw new Error(error)), it should throw error directly.

Have you reviewed our existing documentation?

Reproduction steps

  1. Block browser permissions or if on Windows, ensure that browser permissions are granted and the video device is being used in another application.
  2. Call await meetingManager.startVideoInputDevice().
  3. When PermissionDeniedError / NotReadableError is thrown, the Error name is "Error" with reason "MeetingManager failed to select video input device"

Amazon Chime SDK React Components Library version

3.5.0

What browsers are you seeing the problem on?

Chrome

Browser version

Version 112.0.5615.49 (Official Build) (arm64)

Device Information

macOS Monterey 12.6.1

Meeting and Attendee ID Information.

No response

Browser console logs

MeetingManager failed to select video input device PermissionDeniedError: Permission denied by browser
    at DefaultDeviceController.handleGetUserMediaError (DefaultDeviceController.js:885:1)
    at DefaultDeviceController.eval (DefaultDeviceController.js:1010:1)
    at Generator.throw (<anonymous>)
    at rejected (DefaultDeviceController.js:8:42)

An error was thrown (name: Error, message: MeetingManager failed to select video input device)

Add any other context about the problem here.

No response

@Nanosync
Copy link
Author

Hi team,

Could I check if there are any updates on this issue?

Thank you.

@xuesichao
Copy link
Contributor

Hi @Nanosync thanks for opening this issue and providing lots of details here. These are very helpful. I think you're right, we should directly throw this error instead of wrapping it with another general Error. I'll check with team to see if this should be considered as a breaking change and get back to this issue earlier next week.

Most likely it's not breaking and we could have a PR to update this. Thank you.

@xuesichao
Copy link
Contributor

xuesichao commented May 2, 2023

Hi @Nanosync I checked the code. The deviceLabelTrigger is only used when SDK can not list devices with labels via enumerateDevices call and thus need to use getUserMedia call to get permission and then list the device with labels.

So the error here in line 371 will not be thrown when you call startVideoInputDevice(). It's a different code path.

callback = async (): Promise<MediaStream> => {
this.deviceLabelTriggerStatus = DeviceLabelTriggerStatus.IN_PROGRESS;
this.publishDeviceLabelTriggerStatus();
try {
const devices = await navigator.mediaDevices.enumerateDevices();
const hasVideoInput = devices.some(
(value) => value.kind === 'videoinput'
);
const stream = await navigator.mediaDevices.getUserMedia({
audio: constraints.audio,
video: constraints.video && hasVideoInput,
});
this.deviceLabelTriggerStatus = DeviceLabelTriggerStatus.GRANTED;
this.publishDeviceLabelTriggerStatus();
return stream;
} catch (error) {
console.error('MeetingManager failed to get device permissions');
this.deviceLabelTriggerStatus = DeviceLabelTriggerStatus.DENIED;
this.publishDeviceLabelTriggerStatus();
throw new Error(error);
}
};
}
this.audioVideo?.setDeviceLabelTrigger(callback);

If you block device permission in browser and then call meetingManager.join() to joins the meeting, the error in line 368 MeetingManager failed to get device permissions will be printed and the error in line 371 will be handled in JS SDK, because SDK/browser does not have permission to call getUserMedia to list devices with labels.
https://github.com/aws/amazon-chime-sdk-js/blob/3ea91b88087035e8a036e0e60ceab8c7466936f0/src/devicecontroller/DefaultDeviceController.ts#L823-L840

The error message you got is actually from startVideoInputDevice() at line 508.

An error was thrown (name: Error, message: MeetingManager failed to select video input device)

startVideoInputDevice = async (device: VideoInputDevice): Promise<void> => {
try {
await this.audioVideo?.startVideoInput(device);
this.selectedVideoInputDevice = device;
this.publishSelectedVideoInputDevice();
} catch (error) {
console.error(
'MeetingManager failed to select video input device',
error
);
throw new Error('MeetingManager failed to select video input device');
}
};

JS SDK use handleGetUserMediaError to handle this error, and in React SDK we wrap it with a general Error type.
https://github.com/aws/amazon-chime-sdk-js/blob/3ea91b88087035e8a036e0e60ceab8c7466936f0/src/devicecontroller/DefaultDeviceController.ts#L1155-L1175

@xuesichao xuesichao added the Bug Something isn't working label May 2, 2023
@xuesichao
Copy link
Contributor

You're right, we should:

  1. throw error directly in setupDeviceLabelTrigger.
  2. throw the original error from handleGetUserMediaError for start and stop device methods.

Thanks a lot for your investigation, these are really helpful.

@xuesichao
Copy link
Contributor

Change is merged into main line and will be released in next version. Closing this for now. Let me know if you have any other thoughts. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants