Fix media automatically enabled after selecting a device during a call #7014
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
As described in the TrackSinkMixin documentation when a different source track is set the "_handleInputTrackEnabled()" method will not be called even if the new track has different state than the previous track. Therefore, in that case TrackToStream should trigger the "trackEnabled" event, as otherwise the state change would not be noticed. Currently this has no effect, as the input track was never set again with a different enabled state, but it should make the code more robust. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The TrackEnabler node forces the enable state of the track when an input track is set. However, this was done after setting the output track: first the output track was set with the original state and then the expected one was forced. Now the state is set before setting the output track to ensure that the state will be the expected one from the start, instead of having a transient state. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This should not make any difference, as the configuration reflects the current tracks, but it makes it more consistent with "_updateMediaAvailability()" and prepares it for upcoming changes. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Until now the enabled media state was based on the "audioOn/audioOff/VideoOn/VideoOff" events triggered when enabling and disabling the media. However, the enabled state was also explicitly disabled when media was not available, for example after leaving a call. Due to this, the enabled media state in the LocalMediaModel did not necessarily reflect the current enabled media state in the tracks. For example, if video was enabled in a previous call and a call is joined now without camera, once a camera is selected again the track will be enabled, but the video will be still disabled in the model until something else causes it to change (like sending the current media state to other participants). To solve that the state in the LocalMediaModel is now set based on the tracks, just like done in the peer connections, which should ensure that the model reflects the local media state at all times. The enabled state is still explicitly disabled when media is not available, but that implicitly matches with the track state too (as if there is no track it can not be enabled). Moreover, media is no longer enabled and disabled by the model once available; if media is available the model just reflects the enabled state from the tracks, but it does not modify it. Therefore if media is disabled when not available it may not be automatically disabled once available, but this was not always working anyway since defe617. Finally, currently the local stream will contain at most one track of each kind (audio or video), so the code assumes that and just checks the first track of each kind, if any. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When media is not available the media was explicitly disabled to ensure that it would not be unexpectedly enabled once available again. However, due to the conditions inside "disableAudio()/disableVideo()" only the local store was modified; the pipeline nodes that control whether tracks are enabled or disabled were not modified, so when media was available again in the same call it was automatically enabled back. The conditions (and some supporting code) were needed in the past, but since the introduction of the media pipeline it is possible to enable and disable media even if there are no actual tracks (the tracks will get enabled or disabled once they are started). Therefore the conditions can be removed not only for disabling media but also for enabling it. Besides that, disabling media when not available was done only when an stream was updated; now it is also done when media is not available in the initial stream. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The enabled state of media is saved per conversation using the conversation token. However, the token was not initialized when joining a call without media, so either an empty token or a token from a previous call was used. To solve this now the token is set not only if media was started, but also if it failed to start. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The saved state was restored only when joining a call with media. This was not a problem for audio or video, as in that case they were automatically disabled due not having media available, but it caused the virtual background state to be the default one or the one set in a previous call. Rather than also restoring the state after failing to start media the state is now restored even before the media is tried to be initialized. This ensures that the state will be the expected one once the media is initialized, and with that prevent some unneeded changes back and forth of the tracks caused by adjusting the audio, video and virtual background once the tracks were already started. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Member
Author
|
/backport to stable23 |
nickvergessen
deleted the
fix-media-automatically-enabled-after-selecting-a-device-during-a-call
branch
Member
|
/backport to stable23 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Call flags may not be updated to the right values without #7013
Since defe617 when a media device (microphone or camera) is selected during a call in which the user joined without media of that kind the media is automatically enabled. This is an unexpected behaviour for the user, so even if a media device is selected it should be kept disabled until explicitly enabled by the user.
However, just reverting the commit to explicitly disable again the media is not a valid approach, as in some scenarios the event handler could disable again the media that was being enabled even before the other event handlers were executed (thus causing them to handle first the disabled event and then the enabled event), which ends messing up the state of the objects.
For example, when joining a call with video (without having joined any other call first in that session) the sent video would be a black stream, and as soon as another participant joins the video would be automatically disabled.
Therefore, rather than reverting the commit, this pull request changes the LocalMediaModel to base its enabled state on the state of the tracks (so the UI always reflects the actual track state rather than the expected track state) and removes the restriction of enabling and disabling the media only when available (which ensures that if media is not available it will be initially disabled once a device is selected).
Besides that it also ensures that media will be disabled when starting a call without microphone nor camera, and fixes some other minor things to make the code more robust.
Note that this pull request does not address the problem of media getting disabled when switching to a different device. That behaviour is a side effect of disabling the media when not available, as during the device change the media becomes temporary unavailable. It would make sense to keep the media enabled when switching to a different device and only disabling it if it is actually unavailable (either because no device was selected or because there was an error when getting the media), but this is something for another pull request.
The scenarios below can be performed with or without HPB. Nevertheless, they have been tested ad nauseam, so trying any other creative scenario would be better to find wrong behaviours that may have slipped :-)
How to test (scenario 1)
Result with this pull request
The UI shows the media as disabled, and
OCA.Talk.SimpleWebRTC.webrtc.localStreams[0].getTracks()[0].enabledis falseResult without this pull request
The UI shows the media as disabled, but
OCA.Talk.SimpleWebRTC.webrtc.localStreams[0].getTracks()[0].enabledis trueHow to test (scenario 1b)
Result with this pull request
The UI shows the media as disabled, and
OCA.Talk.SimpleWebRTC.webrtc.localStreams[0].getTracks()[0].enabledis falseResult without this pull request
The UI shows the media as disabled, but
OCA.Talk.SimpleWebRTC.webrtc.localStreams[0].getTracks()[0].enabledis trueHow to test (scenario 2)
Result with this pull request
The UI shows the media as disabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis not setResult without this pull request
The UI shows the media as enabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis setHow to test (scenario 2b)
Result with this pull request
The UI shows the media as disabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis not setResult without this pull request
The UI shows the media as enabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis setHow to test (scenario 3)
Result with this pull request
The UI shows the media as disabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis not set; background blur is disabled and it will not be visible if video is enabledResult without this pull request
The UI shows the media as enabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis set; background blur is also enabled and it is visibleHow to test (scenario 4 - Mimics what would happen if a renegotiation happened 30 seconds after the last participant joined and the messages to send the current media state to other participants are no longer being sent)
Result with this pull request
The UI shows the media as disabled, and
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis not setResult without this pull request
The UI shows the media as disabled, but
OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders()[0].trackis set. Moreover, the participant in the private window is able to hear or see the participant in the original window!How to test (scenario 5)
Result with this pull request
In the device checker the audio or video is disabled (as audio and video were disabled in the previous call)
Result without this pull request
In the device checker the audio or video is enabled