Added support for getDisplayMedia audio capture #3173
Conversation
html/screensharingtest.js
Outdated
| @@ -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 }, | |||
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
getDisplayMediarather thangetUserMediaif 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.
There was a problem hiding this comment.
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.
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'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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. |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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
force-pushed
the
master
branch
from
1b703bf
to
449d7c8
Compare
|
Hello, rebasing was proving a bit tricky so I just redid the code changes based on the latest master. |
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:
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: