Skip to content

Conversation

@danxuliu
Copy link
Member

@danxuliu danxuliu commented Apr 18, 2024

Follow up to #12108

@DorraJaouad After being working on this for quite some time I saw that you fixed it several hours ago already 😢

Nevertheless, I took the freedom to push my version anyway as it also handles a special behaviour of Chromium*, and it is a pure fix rather than including an enhancement (showing the devices section when no preferences are set yet). While I think it that is a nice feature I would prefer to see the issue fixed first to then focus on the improvements :-) (specially given that the RC is so close).

*Actually it is in the spec, it is just that Firefox does not implement it yet, but I forgot about it 🤦 Thanks @Antreesy for the digging :-)

How to test

  • Create a public conversation
  • In a private window, open the conversation
  • Show the media settings

Result with this pull request

The media permissions are requested for and the first available devices selected and started

Result without this pull request

The media permissions are not requested; no devices are selected nor started. Additionally, if the devices tab is shown you first need to choose None and then a device for the permissions to be requested, just selecting the first device (Default in Chromium) is not enough.

@danxuliu danxuliu added this to the v19.0.0-rc.5 milestone Apr 18, 2024
The lists of preferences are used to select the default input devices
when none was selected yet, so they need to be populated first.
Otherwise the first time that the devices are updated the lists will be
empty, so no default device would be set.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When no permissions were given yet a single device of each kind with an
empty deviceId may be returned (it is in the MediaDevices spec, but
implemented by Chromium and not by Firefox yet). Those devices are not
registered in the preference lists, so the lists are empty and no
default devices are set. Indirectly this causes that no media is
requested by the media settings dialog, so no permissions are asked for
and thus the list is not updated with the real devices either.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@danxuliu danxuliu force-pushed the fix-initial-selection-of-devices branch from 704ab67 to 6dc4b53 Compare April 18, 2024 15:30
@danxuliu danxuliu changed the title Fix initial selection of devices fix(media): Fix initial selection of devices Apr 18, 2024
@danxuliu
Copy link
Member Author

/backport to stable29

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested against Chrome 124 and Firefox 124, seems to work correctly now in private windows / as logged-in users without permissions granted

@DorraJaouad
Copy link
Contributor

Thanks for the info :) and it's good to have this fixed before introducing improvements.

Based on this PR, first id is now '' which does not validate the condition

		if (audioInputId.value === null || audioInputId.value === undefined) {
			return
		}

and that's how permissions are requested after.

Instead of adding an empty device, what about removing that condition specifically?
In both cases of null ( "None" is selected) or undefined ( no device selected), we still need permissions ( to show the list).

@DorraJaouad
Copy link
Contributor

and it will only solve the issue for chromium based browsers only. The others will still be left with an empty list, won't be prompted to accept permissions, thus no devices listed eventually.

@danxuliu
Copy link
Member Author

Instead of adding an empty device, what about removing that condition specifically? In both cases of null ( "None" is selected) or undefined ( no device selected), we still need permissions ( to show the list).

If None is explicitly selected (null) no permissions should be requested, even if that means that the list will be incomplete. As a user I would be badly surprised if I open the media settings and I am asked to give permissions for audio and video even if I explicitly said in the past that I did not want to use audio or video.

If audio/videoInputId is undefined... rather than not selected it means that there is no available device; if there is any available device MediaDevicesManager should fall back to it. If the condition is simply removed then, for example, if a camera was selected, it is then unplugged and there are no other cameras MediaDevicesManager.attributes.videoInputId would be set to undefined, but as it changed updateAudioStream would be called and the call to getUserMedia would then fail.

and it will only solve the issue for chromium based browsers only. The others will still be left with an empty list, won't be prompted to accept permissions, thus no devices listed eventually.

It should work for both Chromium and Firefox (I have not tested Safari, although it should work too 🤞).

In the case of Chromium, as it already implements the spec, when no permissions are given there will be a device with an empty id, so getFirstAvailableMediaDevice(devices, this._preferenceXXXInputList) will not return anything and the fall back will trigger, which just selects the first available device (note that there is no check for deviceId === '' by design to ensure that the fallback will be applied in any other case that getFirstAvailableMediaDevices does not return anything). And if there is no device at all then the xxxInputId will be set to undefined :-)

In the case of Firefox the list of available devices will be filled with the actual devices, but they will simply use an empty label. Therefore getFirstAvailableMediaDevice(devices, this._preferenceXXXInputList) will already return a device, without needing the fallback (and even if the fallback was triggered it would select the first device/undefined as explained above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants