-
Notifications
You must be signed in to change notification settings - Fork 35
[Extending API] /screenshot #54
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
[Extending API] /screenshot #54
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.
Performed full review of fe02e69...0377a23
7 files reviewed | 0 comments | Review on Mesa | Edit Reviewer Settings
| if err != nil { | ||
| // Fallback to default dimensions if xdpyinfo fails | ||
| return 1024, 768, 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.
Bug: Screen Dimensions Fallback Hides Errors
The getScreenDimensions function returns a nil error even when xdpyinfo fails, causing it to silently use fallback dimensions. This prevents captureScreenshot from logging a warning and leads to incorrect bounds validation, potentially rejecting valid screenshot regions or allowing invalid ones that cause ffmpeg to fail.
|
This feature will be nice to use!
|
| } | ||
|
|
||
| // Default dimensions if parsing fails | ||
| return 1024, 768, 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.
If the fallback is needed would it be useful to read fallback dimensions from something such as environment variables before fixed values?
rgarcia
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.
thanks for this @raiden-staging !
| @@ -1,6 +1,6 @@ | |||
| // Package oapi provides primitives to interact with the openapi HTTP API. | |||
| // | |||
| // Code generated by github.com/oapi-codegen/oapi-codegen/v2 version v2.5.0 DO NOT EDIT. | |||
| // Code generated by github.com/deepmap/oapi-codegen version v1.12.4 DO NOT EDIT. | |||
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 doesn't look right
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.
oapi/go version conflicts fix induced slop 🙃 sorry for the mess will fix
| "400": | ||
| $ref: "#/components/responses/BadRequestError" | ||
| "500": | ||
| $ref: "#/components/responses/InternalError" |
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 I'd prefer merging these into one POST request
| } | ||
|
|
||
| // Read the screenshot data | ||
| data, err := io.ReadAll(screenshot) |
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.
not sure how oapi does response body reading under the hood but I'd assume it's doing this read of the reader anyway, so this feels like duplicate / unnecessary reading of the screenshot into memory
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.
should we serve as base64 string [ @matthewjmarangoni ] or binary (or add optional ?format={} ) ?
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.
If I understand the question correctly, offering both with a way for the client to choose either the original binary or base 64 encoded string is what I had in mind.
|
thanks again for this @raiden-staging -- screenshot functionality has been added as part of #76 |
serverwith/screenshotendpointTL;DR
Added new
/screenshotAPI endpoints to capture full or partial screen images, and an uptime health check.Why we made these changes
To extend the server's API capabilities, providing programmatic screenshot capture for integration or automation, and to expose server health and uptime information.
What changed?
server/cmd/api/api/screenshot.go: New file introducingGET /screenshotfor full-screen captures andPOST /screenshotfor region-specific captures, leveragingffmpeg.server/cmd/api/api/api.go: Implemented server uptime tracking and exposed it via a newGetHealthendpoint.server/README.md&server/openapi.yaml: Updated documentation to reflect the new/screenshotAPI routes and their usage.server/cmd/api/api/screenshot_test.go: Added comprehensive unit tests for the health check and screenshot capture functionalities, including input validation.server/go.mod&server/go.sum: Updated Go module dependencies.Description generated by Mesa. Update settings