-
Notifications
You must be signed in to change notification settings - Fork 2
fixed issue with audio on screen recording #78
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
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.
Pull Request Overview
This PR addresses audio recording issues in screen capture functionality by implementing a more robust audio handling system. The changes ensure audio tracks are properly captured and composed with video streams, fixing problems where audio was not being included in screen recordings.
- Refactored audio handling to use separate streams for display and microphone capture
- Added comprehensive MIME type support with audio codec preferences
- Introduced proper error handling and resource cleanup for all media streams
Reviewed Changes
Copilot reviewed 2 out of 12 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/GleapScreenRecorder.js | Major refactoring to fix audio capture by separating display and microphone streams, adding constants, improving error handling, and modernizing syntax |
| demo/main.js | Configuration changes to commented out development URLs and updated API key |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for (let i = 0; i < audioTracks.length; i++) { | ||
| const audioTrack = audioTracks[i]; |
Copilot
AI
Oct 6, 2025
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.
[nitpick] Consider using a for...of loop or forEach for better readability: for (const audioTrack of audioTracks) or audioTracks.forEach(audioTrack => { ... })
| for (let i = 0; i < audioTracks.length; i++) { | |
| const audioTrack = audioTracks[i]; | |
| for (const audioTrack of audioTracks) { |
| this.mediaRecorder = new MediaRecorder(stream, { | ||
| mimeType: this.getSupportedMimeType(), | ||
| }); |
Copilot
AI
Oct 6, 2025
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 MediaRecorder constructor could throw an exception if the MIME type is not supported. Consider wrapping in a try-catch block and providing a fallback MIME type or meaningful error message.
| this.mediaRecorder = new MediaRecorder(stream, { | |
| mimeType: this.getSupportedMimeType(), | |
| }); | |
| try { | |
| this.mediaRecorder = new MediaRecorder(stream, { | |
| mimeType: this.getSupportedMimeType(), | |
| }); | |
| } catch (e) { | |
| // Fallback: try without mimeType or with a common one | |
| try { | |
| this.mediaRecorder = new MediaRecorder(stream); | |
| } catch (fallbackError) { | |
| // Optionally, try a very common mimeType | |
| try { | |
| this.mediaRecorder = new MediaRecorder(stream, { | |
| mimeType: "video/webm", | |
| }); | |
| } catch (finalError) { | |
| // If all attempts fail, show a meaningful error and abort | |
| alert("Screen recording is not supported in this browser or configuration."); | |
| this.isRecording = false; | |
| return; | |
| } | |
| } | |
| } |
| this.audioAvailable = this.stream | ||
| ? this.stream.getAudioTracks().length > 0 | ||
| : this.audioAvailable; |
Copilot
AI
Oct 6, 2025
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.
[nitpick] This ternary expression is unnecessarily complex. Consider simplifying to: this.audioAvailable = this.stream?.getAudioTracks().length > 0 ?? this.audioAvailable; or use a clearer if-else structure.
| this.audioAvailable = this.stream | |
| ? this.stream.getAudioTracks().length > 0 | |
| : this.audioAvailable; | |
| this.audioAvailable = this.stream?.getAudioTracks().length > 0 ?? this.audioAvailable; |
| }; | ||
| try { | ||
| this.mediaRecorder.stop(); | ||
| } catch (_) {} |
Copilot
AI
Oct 6, 2025
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.
Empty catch blocks should be avoided or at least include a comment explaining why the error is being ignored. Consider logging the error for debugging purposes.
| } catch (_) {} | |
| } catch (error) { | |
| // Log the error for debugging purposes; stopping may fail if already stopped or in an invalid state. | |
| console.error("Error stopping mediaRecorder:", error); | |
| } |
No description provided.