fix: stop video stream before enabling sleep mode #999
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where entering sleep mode could occur while the video stream is still active, which could cause system instability. The fix ensures that the video stream is properly stopped before enabling sleep mode.
Key changes:
- Added
VideoIsStreaming()function to check if video streaming is active or stopping - Modified
setSleepMode()to stop video streaming before enabling sleep mode - Implemented underlying C function
video_is_streaming()that returns streaming status (active, stopping, or stopped)
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/native/video.go | Added logic to check and stop video streaming before enabling sleep mode, with a grace period |
| internal/native/cgo_linux.go | Implemented Go wrapper for the new video streaming status check function |
| internal/native/cgo_notlinux.go | Added stub implementation for non-Linux platforms |
| internal/native/cgo/video.h | Added function declaration and documentation for video_is_streaming() |
| internal/native/cgo/video.c | Implemented video_is_streaming() to return streaming state (1=active, 2=stopping, 0=stopped) and added sleep mode check on select timeout |
| internal/native/cgo/ctrl.h | Added wrapper function declaration for jetkvm_is_video_streaming() |
| internal/native/cgo/ctrl.c | Implemented wrapper function that calls video_is_streaming() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/native/video.go
Outdated
| return fmt.Errorf("video stop failed, won't enable sleep mode: %w", err) | ||
| } | ||
|
|
||
| // wait few seconds to ensure the video stream is stopped |
There was a problem hiding this comment.
Grammar: "wait few seconds" should be "wait a few seconds"
| // wait few seconds to ensure the video stream is stopped | |
| // wait a few seconds to ensure the video stream is stopped |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/native/cgo/video.c
Outdated
| if (should_restart) { | ||
| log_info("restarting video streaming due to format change"); | ||
| video_restart_streaming(); | ||
| video_restart_streaming(false); |
There was a problem hiding this comment.
The function video_restart_streaming() is defined without parameters at line 770, but it's being called with a false argument here. This will cause a compilation error. Remove the false argument from this call.
| video_restart_streaming(false); | |
| video_restart_streaming(); |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.