fix(conference) fix processing device list while initial gUM is pending #16318
Conversation
When not using the pre-join screen (like in 8x8 Video Elevation) the 1st device list change will happen while the initial tracks haven't fully been resolved yet. Yes, they were created, but they weren't comitted to the store. Thus, since a new track is already on the way, skip the processing of the device list so it doesn't interfere. How does it interfere? Great question! If the initial track was created for a device ID which is NOT the default (aka the first one) then our logic will replace the current track with a new one, using the default device, which is not what we want.
Its does not need to wait on getAvailableDevices() since it's always called before we enter here. What's more, that was delaying the execution enough to miss the device update that triggers processing the device requests queue.
saghul
force-pushed
the
gumpending-devicelistchange
branch
from
4e7b994 to
63ee428
Compare
| * @returns {Function} | ||
| */ | ||
| export function configureInitialDevices() { | ||
| return (dispatch: IStore['dispatch'], getState: IStore['getState']) => { |
There was a problem hiding this comment.
I would add a comment inside the function and also here https://github.com/jitsi/jitsi-meet/blob/master/react/features/conference/actions.web.ts#L56 that we expect getAvailableDevices() to be called before hand. Otherwise I can very well forget about this and remove it by mistake :)
Otherwise kudos for finding this and changing it. I myself was doing a similar cleanup a year ago probably and there was a lot of getAvailableDevices calls happening at the beginning of the call (4-5 IIRC).
| * Check if we have gUM pending while the device list has changed. This can happen when joining a meeting | ||
| * without the pre-join screen since we don't have the device list while the initial tracks are being created. | ||
| */ | ||
| const { gumPending: audioGumPending } = state['features/base/media'].audio; |
There was a problem hiding this comment.
Well looking into the code the scenario explained above seems impossible to me.
_onDeviceListChanged seems to be attached as a listener by _initDeviceList only when the passed setDeviceListChangeHandler is true and the only time this method is called with parameter true is here https://github.com/jitsi/jitsi-meet/blob/master/conference.js#L591 . At this point the tracks are already created.
Why do we need this change? Am I missing something?
There was a problem hiding this comment.
I saw your commit comment and if the problem is that the tracks are not in Redux yet but they are created I think you should at least fix your comment above.
Also if this is the case I feel like there should be better solution!
Maybe call _initDeviceList(true) only once the tracks are actually stored in redux instead of attaching it too early? WDYT? Or we still want to update the device list?
The scenario you are describing if I understand correctly is the following:
- initial tracks are created.
- this handler is called and we skip it.
- we add the tracks to redux.
In this case shouldn't we call the handler once more to go trough the checks and potentially modify the tracks once the local tracks are added into redux.
I see you are using the gumPending flags. I'm not sure if these flags are the best choice because the condition could be true also during a regular mute/unmute or replace track operation and if I understand correctly we are trying to fix something related only to the initial GUM creation, isn't it? Maybe you can use initialGUMPromise and maybe you can chain the checks for the track change related to the devices changed to the initialGUMPromise (speculating here, not sure exactly when do we resolve those).
There was a problem hiding this comment.
Also if the problem is that the tracks are not in redux yet maybe we should start putting tracks in redux earlier? When I was looking into the code now we add the into the store once we have connected, the conference have started and we have added the tracks to LJM. WDYT?
There was a problem hiding this comment.
Added some comments, suggestions and questions.
The core solution doesn't feel like addressing the root cause but more like a workaround. Not sure if it will work in all cases (see my comments).
Also can you explain the exact use case we are trying to solve. If I look into the code the and have in mind the scenario described in the description. In order to have the initial gum request a device that is not the default one then I guess the device id would be coming from getUserSelectedCameraDeviceId and the settings values in redux (which IIRC are coming from local storage). I see that we have similar logic in the _onDeviceListChanged handler and we would select the device we want to use based on what we have in settings and what getUserSelectedCameraDeviceId will return. So I think we will again choose and create a track with the correct device id, isn't it? Now the whole thing doesn't look perfect at all and it will replace the same track with the newly created track for the same device which doesn't make any sense but shouldn't cause an issue. Am I missing something?
It is interesting what will happen if the device used for the initial track gets disconnected. Would we have a race which track will be added last? Maybe this may cause issues as well?
2 participants
When not using the pre-join screen (like in 8x8 Video Elevation) the 1st device list change will happen while the initial tracks haven't fully been resolved yet. Yes, they were created, but they weren't comitted to the store. Thus, since a new track is already on the way, skip the processing of the device list so it doesn't interfere.
How does it interfere? Great question! If the initial track was created for a device ID which is NOT the default (aka the first one) then our logic will replace the current track with a new one, using the default device, which is not what we want.