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.

Sign up for GitHub

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

Jump to bottom

Added support for getDisplayMedia audio capture #3173

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added support for getDisplayMedia audio capture #3173

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4499f29
Added support for getDisplayMedia audio capture
rjnpnfigueiredo Feb 28, 2023
ac39e3a
Refactored getDisplayMedia audio into group
rjnpnfigueiredo Mar 4, 2023
17cf23e
Added back empty line at the end of file
rjnpnfigueiredo Mar 7, 2023
ccfc680
Improved coding style
rjnpnfigueiredo Mar 7, 2023
d5af913
Addressed coding style
rjnpnfigueiredo May 6, 2023
1b703bf
Merge pull request #1 from meetecho/master
rjnpnfigueiredo May 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 28 additions & 2 deletions html/janus.js
View file
Open in desktop
Expand Up @@ -2293,6 +2293,7 @@ function Janus(gatewayCallbacks) {
}
}
let answer = (callbacks.jsep ? true : false);
let desktopAudio = null;
for(let track of tracks) {
if(!track.type) {
Janus.warn('Missing track type:', track);
Expand Down Expand Up @@ -2332,6 +2333,8 @@ function Janus(gatewayCallbacks) {
let kind = track.type;
if(track.type === 'screen')
kind = 'video'; // FIXME
if(track.type === 'desktopAudio')
kind = 'audio'; // FIXME
let transceiver = null, sender = null;
if(track.mid) {
// Search by mid
Expand Down Expand Up @@ -2403,9 +2406,32 @@ function Janus(gatewayCallbacks) {
}
} else {
// Use getDisplayMedia
stream = await navigator.mediaDevices.getDisplayMedia(constraints);
// Figure out if it is a video or audio capture request
if(track.type === 'screen') {

rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
//double check if audio capture is required (messy way)
rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
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.

}
}
stream = await navigator.mediaDevices.getDisplayMedia(constraints);

rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
desktopAudio = stream;
}
if(track.type === 'desktopAudio') {
stream = desktopAudio;
}
}
if(track.type === 'desktopAudio' && (desktopAudio == null || desktopAudio.getAudioTracks().length == 0) ) {
continue; //ignore no audio in screen capture as it is set at the moment of screen capture
rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
}

rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
nt = (track.type === 'audio' || track.type === 'desktopAudio' ? stream.getAudioTracks()[0] : stream.getVideoTracks()[0]);

rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
if(track.type === 'desktopAudio') {
track.type = 'audio'; //overrulling from here
rjnpnfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
}
nt = (track.type === 'audio' ? stream.getAudioTracks()[0] : stream.getVideoTracks()[0]);
}
if(track.replace) {
// Replace the track
Expand Down
2 changes: 1 addition & 1 deletion html/screensharingtest.js
View file
Open in desktop
Expand Up @@ -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.

{ type: 'screen', capture: true, recv: false }
],
success: function(jsep) {
Expand Down