Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for getDisplayMedia audio capture #3173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rjnpnfigueiredo
Copy link
Contributor

This is meant to address the issue of getDisplayMedia requests not asking for audio capture when capturing the screen.

The example of screensharingtest.html actually captures microphone audio and not the desktop audio which can cause unintended effects.

This is a possible fix that adds a new track type of "desktopAudio" which can be used in the track list:

tracks: [
{ type: 'desktopAudio', capture: true, recv: false },
{ type: 'screen', capture: true, recv: false }
]

This allows for specific request of audio capture while screen capturing, avoiding confusing with microphone audio.

Since the audio and video capture request must be made with a single request and single constraint, the captured stream is buffered to be split into audio and video tracks as per regular behavior.

Audio capture in getDisplayMedia is still dependent on:

  • Browser
  • Operating System
  • User permission

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, but I don't think it fits the way the tracks functionality works right now. Please revise and make use of group instead, where needed.

html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
@@ -147,7 +147,7 @@ $(document).ready(function() {
{
// We want sendonly audio and screensharing
tracks: [
{ type: 'audio', capture: true, recv: false },
{ type: 'desktopAudio', capture: true, recv: false },
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good idea, in the demo, or at the very least it should be optional and/or configurable. At least some time ago, desktop audio was only supported on Windows, and not other systems, which means you thought you asked for something and got something else entirely. This is why we fell back to "regular" audio in this demo, as it was meant as a way to "talk on top of your screen".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently Chrome/Edge on windows can share audio in all modes (tab, window, screen) as long as the user ticks the permission box in the share screen prompt (tick for audio only appears if you request audio in the getDisplayMedia)

For other systems, Chrome/Edge can still share audio, but only of tabs.

html/janus.js Outdated
//double check if audio capture is required (messy way)
for(const othertrack of tracks) {
if(othertrack.type === 'desktopAudio' && othertrack.capture) {
constraints.audio = othertrack.capture;
Copy link
Member

Choose a reason for hiding this comment

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

For some reason this comment I added disappeared, so I'll add a new single comment again. I don't like this approach, as it breaks the way tracks is supposed to work. You're not meant to simply pick any other random audio track from that list and use it here: we explicitly conceived tracks to be a collection of independent tracks, that you can either ask separately, or in some cases group together, e.g., in a single getUserMedia (which makes sense for instance when you're just asking for mic and webcam). Asking things together is done via the concept of group, which is explained in the original PR #3003. What you'e doing breaks that assumption, and will cause undefined or unexpected behaviours when one uses tracks to capture many different tracks at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,

Thanks for the feedback on this, I tried to stick to the code style conventions better this time.
I've reworked the code to make 'screen' work along side 'video' and 'audio' in the groups:

  • The 'screen' track type has its own default group name to avoid collisions with expected behaviors of default audio/video group
  • A choice is made to acquire stream from getDisplayMedia rather than getUserMedia if the track is of the 'screen' type or the track belongs to a group where 'screen' is the type of the other track.
  • Current demo for screenshare works the same as before (reverted changes)
  • Actually sharing screen with system audio now requires that the 'screen' track is in a group with an 'audio' track

I'm still evaluating combinations of tracks to ensure proper behavior, but I wanted to check if this would an acceptable direction for a solution.

Copy link
Member

Choose a reason for hiding this comment

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

It does make sense, thanks! It looks like it will indeed preserve backwards compatibility, and allow for what you're looking for as an additional extra feature.

@lminiero lminiero added the multistream Related to Janus 1.x label Mar 6, 2023
Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Added a couple of extra notes.

html/janus.js Outdated
continue;
if(!track.capture || track.capture instanceof MediaStreamTrack)
continue;
let group = track.group ? track.group : 'default';
// Avoid assigning regular default group if it is a screen capture
let group = track.group ? track.group : track.type=='screen'?'sharescreendefault':'default';
Copy link
Member

Choose a reason for hiding this comment

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

Please put the second block of the ternary operator in brackets to avoid ambiguity, and space things accordingly to make it more readable, e.g.:

(track.type === 'screen' ? 'sharescreendefault' : 'default')

As a side note, I'm not fond sharescreendefault as a name, too long and overly verbose: maybe just screendefault? I was going to suggest updating the documentation in mainpage.dox accordingly, but I just noticed we don't have any text about group in there at all where we talk of tracks, so I can add something myself after this is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaked code to better fit style as per comments.
Chained ternaries are not really readable but I don't think spinning it off into a function would fit into the current code style.
I'd say screendefault sounds good, lets keep that.

html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
html/screensharingtest.js Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

@rjnpnfigueiredo apologies for the lack of feedback: I'm currently abroad for a conference, so I won't be able to look at this for a little while longer. Pinging @atoppi so that he can check them too.

html/janus.js Outdated Show resolved Hide resolved
Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Apologies for this late review, I've been abroad for quite some time and spent last week catching up with other activities. I added some notes inline: please also address @atoppi's comment, which seems to point out a syntax error (double closing bracket where only one was opened).

html/janus.js Outdated Show resolved Hide resolved
html/janus.js Outdated
if(track.gumGroup) {
let otherType = (track.type === 'audio' ? 'video' : 'audio');
if(groups[track.gumGroup] && groups[track.gumGroup][otherType]) {
let otherTrack = groups[track.gumGroup][otherType];
if(!otherTrack && otherType === 'video') {
// If there other track is not video, then it is screen and should be a getDisplayMedia capture
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to wrap up my mind around this check and the action it implements as a consequence, but I'm struggling to understand the logic behind it. Could you elaborate on why this should work that way? The check and the comment seem at odds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my own late reply, been otherwise occupied.

Logic is a bit strange, yes. This is meant to address the issue of asking for audio before screen in the group.
If the audio track is handled first, we need to know what the other track type is. If it fails to get the other track and we had the type at video (from the other type ternary above) then it means the other type is actually screen and we should treat this as a display media request.

@lminiero
Copy link
Member

lminiero commented Sep 6, 2024

@rjnpnfigueiredo any chance you can rebase this? I wanted to have another look (apologies for the delay) but apparently there's a conflict now.

@rjnpnfigueiredo
Copy link
Contributor Author

Hello, rebasing was proving a bit tricky so I just redid the code changes based on the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants