Ahoyapps 960 stop video on disconnect

[timmydoza]

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):

• AHOYAPPS-960
• AHOYAPPS-917

Description

This PR fixes the following issues:

• Audio and Video tracks are now turned off when a user disconnects from the room (which means that the camera light will be turned off). AHOYAPPS-960
• Screen sharing tracks are also stopped when disconnecting from a room. AHOYAPPS-917
• When using firebase, the video track will now be enabled by default (this is how the app used to function until a regression was introduced) when clicking on a link that has the room name in it (e.g. https://my-example-app-1234/room/my-test-room). There is no ticket for this issue. This issue is solved by using useDevices inside useLocalTracks instead of using both useLocalVideoDevices and useLocalAudioDevices. This change means that enumerateDevices will be called once instead of twice, which fixes the issue.

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

[charliesantos]

Why can't we use useAudioInputDevices and useVideoInputDevices here? Is it because we are calling useDevices internally twice?

[timmydoza]

Is it because we are calling useDevices internally twice?

Yeah. So, there are two calls to useDevices, which means that there are two separate calls to enumerateDevices. It's a bit awkward since enumerateDevices resolves asynchronously. Because it's async, the initial value returned by useDevices is an empty array. So this is what happens when the app starts:

First:

  const localAudioDevices = useAudioInputDevices(); // returns []
  const localVideoDevices = useVideoInputDevices(); // returns []

Then:

  const localAudioDevices = useAudioInputDevices(); // returns [{deviceId: 'my-audio-device-id'}]
  const localVideoDevices = useVideoInputDevices(); // returns []

At this point, createLocalTracks() is called due to the logic on this line. And because there are no video devices yet, the app is started without acquiring video.

Then:

  const localAudioDevices = useAudioInputDevices(); // returns [{deviceId: 'my-audio-device-id'}]
  const localVideoDevices = useVideoInputDevices(); // returns [{deviceId: 'my-video-device-id'}]

So it's a little awkward that the two hooks resolve at different times. I could probably update the logic on line 51, but there's also no reason to call enumerateDevices more than once. So the change in the PR makes it so we get the list of audio devices and video devices at the exact same time.

[charliesantos]

Yeah, I get what was happening. Thanks for more details! I saw the same issue in the diagnostics app and ended up removing both useAudioInputDevices and useVideoInputDevices.

But for our use case in video app, this seems like a code smell since it's possible that we may run into this issue again if we or another developer use both of these hooks at the same time. What is your suggestion to prevent us from making this mistake in the future? My suggestion is, we can update both useAudioInputDevices and useVideoInputDevices such that when one is called, it waits for the other if the other is pending. Kinda like an async queue behavior. Personally, it feels cleaner this way.

[timmydoza]

it's possible that we may run into this issue again if we or another developer use both of these hooks at the same time. What is your suggestion to prevent us from making this mistake in the future?

Honestly, I'm leaning toward removing the useAudioInputDevices, useVideoInputDevices, and useAudioOutputDevices hooks altogether. I created them for the sake of convenience, but given the issue that arises when more than one of these hooks are used together (which I was unaware of when I made these hooks), they aren't really that convenient. Even without this issue, these hooks would save you one line of code at best. Since they don't provide much value, I think there's little harm in removing them.

Another idea would be to cache the initial value that is used when these hooks are called. Whenever any hook calls enumerateDevices, it could update the cached initial value. So when any of these hooks are called, they are initialized with the most recent list of devices, instead of an empty array.

we can update both useAudioInputDevices and useVideoInputDevices such that when one is called, it waits for the other if the other is pending.

I don't think that this is necessary. When useAudioInputDevices and useVideoInputDevices are called one after another, it results in two immediate calls to enumerateDevices. Calling enumerateDevices twice in a row will virtually guarantee that their return values will be identical. Thus, when the first enumerateDevices promise resolves, there is no need to wait for the second promise to resolve because we already have all the information that we need, and we know exactly what the value of the second promise will be. To me, using an async queue here feels a little bit like an anti-pattern. I think it would be best if enumerateDevices is called only once inside useLocalTracks.

[charliesantos]

Ok. It sounds like removing useAudioInputDevices, useVideoInputDevices, and useAudioOutputDevices hooks altogether is our best option?

[timmydoza]

@charliesantos I redid the deviceHooks file as we discussed on Slack. It involved quite a few changes all over the place. This PR is a bit bigger now, but I do like the new useDevices hook much more than what we had previously.

[charliesantos]

Great work!