-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
Supports audio sharing #181
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this feature doesn't entirely fit within the project's scope. However, it can be argued that system audio is a part of screen sharing and thus enhances the experience of using screego.
I expelicitly set |
For consistency with other features, I added the hotkey 'a' for audio. Along with this, an issue arose from the timing of initilizing the audio element reference, so I replaced it with the more appropriate useRef. |
stream.addTrack(event.track); | ||
} | ||
} | ||
|
||
onTrack(stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved inside the if (event.track.kind === 'video)
, then we don't have to adjust the onTrack setState implementation at all.
@@ -215,6 +258,13 @@ export const Room = ({ | |||
</Typography> | |||
)} | |||
|
|||
{audioStream && ( | |||
<audio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <video>
element should be able to play audio, we should use this instead of creating another audio element. the muted property should be used to control if the audio should be played or not.
@@ -161,6 +203,7 @@ export const Room = ({ | |||
}, | |||
[state.clientStreams, selectedStream] | |||
); | |||
useHotkeys('a', toggleAudio, [playingAudio]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the hotkey should be m
for mute.
if (videoElement && stream) { | ||
videoElement.srcObject = stream; | ||
if (videoElement && videoStream) { | ||
videoElement.srcObject = videoStream; | ||
videoElement.play().catch((e) => console.log('Could not play main video', e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The video audio can be muted when in fullscreen via native controls, the playingAudio state in screego should reflect that. PlayingAudio should be updated via volume events from the video e.g.:
videoElement.onvolumechange = () => setPlayingAudio(videoElement.muted)
I understand that Screego aims to remain extremely simple, and that audio support was previously closed in issue #102. However, in my daily work, I find it inconvenient not to be able to share audio when I need to demonstrate code and its results to colleagues. As a game programmer, a lot of sound is produced as a result of my code. Not being able to convey this to my colleagues is a significant drawback.
Considering the context of issue #102, I have implemented a slightly different concept, which I will explain below.
Audio Transmission Settings
In the browser menu, users can choose whether to share audio. If enabled, both video and audio tracks will be sent; if disabled, only the video track will be sent. In browsers that do not support audio sharing, such as Firefox, this menu will not be displayed, and only the video track will always be sent.
Audio Listening Settings
First, audio can only be listened to under the following conditions:
The reason for 1 is straightforward—nobody wants to hear their own screen's audio twice.
For 2, the concept of this implementation is the reason. When receiving multiple screens, I couldn't think of a useful scenario where overlapping audio is helpful. Also, having many mute buttons displayed would not be simple. Therefore, by limiting the audio track used for playback to the selected stream, users can choose which audio to hear.
For 3, this is the only additional button in this implementation. With just 2, you can choose which audio to hear but cannot choose not to hear any audio at all. So, I added a button to the control menu to toggle the audio on and off. The default state is Mute. If the selected stream is your own screen or does not include an audio track, the button will automatically be disabled.
Of course, even in browsers that do not support audio sharing, such as Firefox, you can still receive audio.
I believe this approach maintains simplicity.