-
Notifications
You must be signed in to change notification settings - Fork 126
RSDK-11732 — Remove feature flag and make GetImages the main code path in stream server #5603
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
robot/web/stream/camera/camera.go
Outdated
| imgBytes, err := namedImage.Bytes(ctx) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| _, format, err := image.DecodeConfig(bytes.NewReader(imgBytes)) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if _, ok := StreamableImageMIMETypes[FormatStringToMimeType(format)]; ok { | ||
| return namedImage, nil |
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.
almost all code except the highlighted is directly copy and pasted from camera2/camera.go (now deleted since it used to be gated behind a feature flag). The FormatStringToMimeType helper is also new along with the corresponding test.
I added this new code because I want the stream server to be more robust i.e. if a mimeType is missing/wrong somehow, we still want to be able to stream if the underlying bytes are healthy. DecodeConfig is a lightweight function that checks the header bytes of the actual image bytes and tells us what the format is for a stronger source of truth vs. mime type.
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.
seanavery
left a comment
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.
Nice DecodeConfig use!
Let me know if you need a hand with manual tests
SebastianMunozP
left a comment
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.
LGTM
https://viam.atlassian.net/browse/RSDK-11732
This PR removes GetImage as the source of frames for image-based live streams. It makes the previously gated by feature flag GetImages path the main and only path.
This is the PR that added the feature flag and GetImages path originally: #5258
Manual Tests