-
Notifications
You must be signed in to change notification settings - Fork 254
fix(video-stream): switch to SOCK_STREAM between main and video #1041
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 fixes a critical issue where large H.264 I-frames (keyframes) exceed the ~360KB message size limit of unixpacket sockets, causing "message too long" errors that crash the video process and can trigger Failsafe mode.
Key changes:
- Switch from
SOCK_SEQPACKET(unixpacket) toSOCK_STREAM(unix) sockets for video streaming - Implement length-prefix framing protocol using 4-byte little-endian headers to delimit frames
- Add frame size validation in the receiver to prevent buffer overflows
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| internal/native/server.go | Updated video frame sender to use unix socket with 4-byte length prefix protocol |
| internal/native/proxy.go | Updated video frame receiver to read length-prefixed frames from unix socket with validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/native/server.go
Outdated
| if err != nil { | ||
| // Write 4-byte frame length prefix, then frame data | ||
| frameSizeBuffer := make([]byte, 4) | ||
| binary.LittleEndian.PutUint32(frameSizeBuffer, uint32(len(frame))) |
Copilot
AI
Dec 4, 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 frame length is cast to uint32, which limits the maximum frame size to 4GB. While this is likely sufficient for video frames, on 64-bit systems len(frame) returns an int that could theoretically exceed uint32 max value, causing silent truncation.
Consider adding a check:
if len(frame) > 0xFFFFFFFF {
logger.Error().Int("frameSize", len(frame)).Msg("frame size exceeds uint32 maximum")
return
}
binary.LittleEndian.PutUint32(frameSizeBuffer, uint32(len(frame)))Or more practically, combine this with the maxFrameSize validation suggested in another comment.
internal/native/proxy.go
Outdated
| defer conn.Close() | ||
|
|
||
| inboundPacket := make([]byte, maxFrameSize) | ||
| frameSizeBuffer := make([]byte, 4) |
Copilot
AI
Dec 4, 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 frameSizeBuffer is allocated once before the loop and reused for all frames, which is good. However, consider using a stack-allocated array instead of a heap-allocated slice for better performance:
var frameSizeBuffer [4]byteThen use frameSizeBuffer[:] when calling io.ReadFull. This eliminates a heap allocation since the buffer is stack-allocated.
internal/native/proxy.go
Outdated
| if frameSize > maxFrameSize { | ||
| p.logger.Error().Uint32("frameSize", frameSize).Uint32("maxFrameSize", maxFrameSize). | ||
| Msg("received frame size exceeds maximum, rejecting") |
Copilot
AI
Dec 4, 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.
There's no validation for zero-sized frames. While unlikely, if frameSize is 0, io.ReadFull(conn, inboundPacket[:0]) would succeed without reading anything, and a zero-length slice would be passed to OnVideoFrameReceived.
Consider adding a check:
frameSize := binary.LittleEndian.Uint32(frameSizeBuffer)
if frameSize == 0 || frameSize > maxFrameSize {
p.logger.Error().Uint32("frameSize", frameSize).Uint32("maxFrameSize", maxFrameSize).
Msg("received invalid frame size")
break
}This prevents processing of invalid zero-length frames.
| if frameSize > maxFrameSize { | |
| p.logger.Error().Uint32("frameSize", frameSize).Uint32("maxFrameSize", maxFrameSize). | |
| Msg("received frame size exceeds maximum, rejecting") | |
| if frameSize == 0 || frameSize > maxFrameSize { | |
| p.logger.Error().Uint32("frameSize", frameSize).Uint32("maxFrameSize", maxFrameSize). | |
| Msg("received invalid frame size") |
internal/native/server.go
Outdated
| if _, err := conn.Write(frameSizeBuffer); err != nil { | ||
| logger.Fatal().Err(err).Msg("failed to write frame size to video stream socket") | ||
| } | ||
| if _, err := conn.Write(frame); err != nil { |
Copilot
AI
Dec 4, 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 two separate Write() calls could lead to protocol desynchronization if an error occurs between them. While SOCK_STREAM preserves byte order, if the first write succeeds but the second fails, the receiver will read a frame size prefix without corresponding data.
Consider combining the writes into a single operation for better atomicity:
// Write 4-byte frame length prefix, then frame data
frameSizeBuffer := make([]byte, 4)
binary.LittleEndian.PutUint32(frameSizeBuffer, uint32(len(frame)))
packet := append(frameSizeBuffer, frame...)
if _, err := conn.Write(packet); err != nil {
logger.Fatal().Err(err).Msg("failed to write frame to video stream socket")
}This ensures both the prefix and data are written together, reducing the chance of partial writes.
| if _, err := conn.Write(frameSizeBuffer); err != nil { | |
| logger.Fatal().Err(err).Msg("failed to write frame size to video stream socket") | |
| } | |
| if _, err := conn.Write(frame); err != nil { | |
| packet := append(frameSizeBuffer, frame...) | |
| if _, err := conn.Write(packet); err != nil { |
internal/native/server.go
Outdated
| frameSizeBuffer := make([]byte, 4) | ||
| binary.LittleEndian.PutUint32(frameSizeBuffer, uint32(len(frame))) | ||
|
|
||
| if _, err := conn.Write(frameSizeBuffer); err != nil { |
Copilot
AI
Dec 4, 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 frameSizeBuffer is allocated on every frame received. Since video frames arrive frequently (likely 30-60 fps), this creates unnecessary garbage collection pressure.
Consider reusing the buffer:
nativeOptions.OnVideoFrameReceived = func(frame []byte, duration time.Duration) {
var frameSizeBuffer [4]byte
binary.LittleEndian.PutUint32(frameSizeBuffer[:], uint32(len(frame)))
packet := append(frameSizeBuffer[:], frame...)
if _, err := conn.Write(packet); err != nil {
logger.Fatal().Err(err).Msg("failed to write frame to video stream socket")
}
}Using a stack-allocated array [4]byte instead of a heap-allocated slice eliminates this allocation overhead.
| frameSizeBuffer := make([]byte, 4) | |
| binary.LittleEndian.PutUint32(frameSizeBuffer, uint32(len(frame))) | |
| if _, err := conn.Write(frameSizeBuffer); err != nil { | |
| var frameSizeBuffer [4]byte | |
| binary.LittleEndian.PutUint32(frameSizeBuffer[:], uint32(len(frame))) | |
| if _, err := conn.Write(frameSizeBuffer[:]); err != nil { |
| logger.Fatal().Err(err).Msg("failed to write frame size to video stream socket") | ||
| } | ||
| if _, err := conn.Write(frame); err != nil { | ||
| logger.Fatal().Err(err).Msg("failed to write frame to video stream socket") |
Copilot
AI
Dec 4, 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.
Using logger.Fatal() for write errors will terminate the entire process on any transient network issue. This seems overly severe for a video streaming error.
Consider using a non-fatal error handling approach that logs the error and allows the native process to continue or gracefully restart just the video stream connection:
if _, err := conn.Write(frameSizeBuffer); err != nil {
logger.Error().Err(err).Msg("failed to write frame size to video stream socket")
return
}This would prevent the entire native process from crashing when the video stream encounters issues.
| logger.Fatal().Err(err).Msg("failed to write frame size to video stream socket") | |
| } | |
| if _, err := conn.Write(frame); err != nil { | |
| logger.Fatal().Err(err).Msg("failed to write frame to video stream socket") | |
| logger.Error().Err(err).Msg("failed to write frame size to video stream socket") | |
| return | |
| } | |
| if _, err := conn.Write(frame); err != nil { | |
| logger.Error().Err(err).Msg("failed to write frame to video stream socket") | |
| return |
| nativeOptions.OnVideoFrameReceived = func(frame []byte, duration time.Duration) { | ||
| _, err := conn.Write(frame) | ||
| if err != nil { | ||
| // Write 4-byte frame length prefix, then frame data | ||
| frameSizeBuffer := make([]byte, 4) | ||
| binary.LittleEndian.PutUint32(frameSizeBuffer, uint32(len(frame))) | ||
|
|
||
| if _, err := conn.Write(frameSizeBuffer); err != nil { | ||
| logger.Fatal().Err(err).Msg("failed to write frame size to video stream socket") | ||
| } | ||
| if _, err := conn.Write(frame); err != nil { | ||
| logger.Fatal().Err(err).Msg("failed to write frame to video stream socket") | ||
| } |
Copilot
AI
Dec 4, 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.
There's no validation that the frame size fits within the protocol's constraints before writing. While the receiver validates against maxFrameSize, the sender should also validate to prevent sending invalid data.
Consider adding validation:
nativeOptions.OnVideoFrameReceived = func(frame []byte, duration time.Duration) {
if len(frame) > maxFrameSize {
logger.Error().Int("frameSize", len(frame)).Msg("frame exceeds maximum size, dropping")
return
}
// Write 4-byte frame length prefix, then frame data
...
}Note: You'll need to define or import maxFrameSize constant in this file for consistency with the receiver side.
Fixes: #1017
The device in the issue crashes multiple times, with the error
@jetkvm/native/video-stream/78c05999: write: message too long. This restarts the video process, and eventually triggers Failsafe mode. Before each crash this happens:Detailed Logs
The video stream uses a unixpacket socket which has a kernel-enforced size limit of ~360 KB per message. While most H.264 frames are small, I-frames (keyframes) can exceed this limit during complex scenes or stream initialization.
I was not able to reproduce the issue by simply using the JetKVM. I did however, write a small golang program that spawns a child process talking over
unixpacketto the message size limit. The program correctly confirms message the size limit.The fix is to switch from unixpacket (SOCK_SEQPACKET) to unix (SOCK_STREAM) with a 4-byte length prefix for framing. Stream sockets have no per-message size limit.