feat(share-tab-audio): Add share audio flag on desktop share. #1732
Conversation
|
Hi, thanks for your contribution! |
|
Jenkins add to whitelist. |
|
@tudor-phd What happens when the audio constraint is set to false for getDisplayMedia() ? Does the share audio checkbox not show up in the screen selection dialog ? If that is the case are you planning to pass this new option set to false while creating a desktop track for virtual background ? |
Yes and yes |
Also if the value will be undefined/not specified it will be ignored and set to true by default. The remaining cases are setting to: true / false |
modules/RTC/ScreenObtainer.js
Outdated
| const video = typeof desktopSharingFrameRate === 'object' ? { frameRate: desktopSharingFrameRate } : true; | ||
| const audio = this._getAudioConstraints(); | ||
| // eslint-disable-next-line no-negated-condition |
There was a problem hiding this comment.
Please reverse this instead of disabling the eslint rule.
JitsiMeetJS.js
Outdated
| const firePermissionPrompt = firePermissionPromptIsShownEvent || oldfirePermissionPromptIsShownEvent; | ||
|
|
||
| if (typeof desktopSharingAudio !== undefined) { |
There was a problem hiding this comment.
Why is this necessary here? IIRC we initialize it in RTC.js already.
There was a problem hiding this comment.
I saw that in RTCUtils.js we call screenObtainer.init(options); -> but looks like we pass all options at the start -> then we want to change an option. This is the reason that I call again screenObtainer.init(options); with the new desktopSharingAudio option. @jallamsetty1 What do you think about this? Looks fine for you or would you have another approach to fix this?
There was a problem hiding this comment.
Doesn't this mean that once someone sets their desktop as virtual background, the audio share option will no longer be available for subsequent screenshares and audio-only shares ?
There was a problem hiding this comment.
No. Already tested that. For example, I tried to apply the option just on the virtual background. Worked. After that, I wanted to test this case that you specified and I clicked the screen share option from the bottom toolbar to see the behavior on browser tab sharing. The audio share checkbox was there.
There was a problem hiding this comment.
I think we should find a better way or workaround for this change in Chrome's behavior where the audio share gets checked by default whenever audio is requested as this affects regular screenshare as well.
|
@tudor-phd Is it possible to ignore the audio track here when the desktop track is being used for virtual background ? |
Already tried to do that with the flag that I created. Is not working. Same reason like here: #1732 (comment) |
| if (virtualBackgroundSharing) { | ||
| screenObtainer.virtualBackgroundSharing = true; | ||
| } else { | ||
| screenObtainer.virtualBackgroundSharing = false; |
There was a problem hiding this comment.
Suggestion: screenObtainer.virtualBackgroundSharing = virtualBackgroundSharing ?? false
|
@tudor-phd this LGTM with an optional suggestion. |
|
I won't block this PR because Jaya approved it, but I will state my strong -1. That option bears no meaning in lib-jitsi-meet. Virtual backgrounds are part of JM, all this library needs to offer is a way for the user of the API to specify some audio constraints. As it stands it's a leaky abstraction. If we come up with another case which needs audio sharing disabled (maybe presenter mode?) the flag name would be even more confusing. |
In that case, can we make "audio: false" the default constraints and pass option (shareDesktopAudio or something similar) for explicitly enabling it ? |
This is exactly how this should be implemented IMHO. |
No description provided.