feat: add device.screencapture.setConfiguration RPC#290
Open
gmegidish wants to merge 3 commits into
Open
Conversation
FleetAllocateResponse.SessionID → AllocationID (json:"allocationId") DeviceProvider.SessionID → AllocationID (json:"allocationId,omitempty") All call sites in cli/fleet.go and cli/remote.go updated.
… types" This reverts commit 3159f44.
Adds a new `device.screencapture.setConfiguration` method that applies live encoder settings to an in-flight AVC capture without restarting the stream. Currently supports adjusting bitrate on the fly.
WalkthroughA new AVC control channel is added for live screen-capture bitrate adjustment. A global mutex-protected registry ( 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/ios.go (1)
1452-1460: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the missing-app error to match the new bundle lookup.
The lookup now requires a
devicekit-h264suffix, but the error still tells users to installdevicekit-ios.Suggested fix
- return nil, fmt.Errorf("DeviceKit main app not found. Please install devicekit-ios on the device") + return nil, fmt.Errorf("DeviceKit H.264 app not found. Please install devicekit-h264 on the device")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devices/ios.go` around lines 1452 - 1460, The missing-app error in the DeviceKit bundle lookup is stale and still references devicekit-ios even though the search now matches the devicekit-h264 suffix. Update the error text in the devicekitMainAppBundleId lookup path to tell users to install the correct bundle/app name that matches the suffix check, keeping it aligned with the logic in the same block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devices/avc_control.go`:
- Around line 15-48: The AVC control registry currently allows overlapping
captures to overwrite each other, so stale cleanup can remove the active writer.
Update registerAvcControl and unregisterAvcControl to track ownership per
capture instance (not just by deviceID) and reject or detect duplicate
registration in SetAvcBitrate’s lifecycle path. If registerAvcControl finds an
existing writer for the same deviceID, have the caller treat that as a conflict
and stop the new capture/close its pipe instead of replacing the entry. Ensure
unregisterAvcControl only removes the writer it originally registered so a
previous deferred cleanup cannot clear a newer capture.
In `@devices/ios.go`:
- Line 1002: The iOS DeviceKit H.264 start path in StartDeviceKit264 currently
creates a stream but never registers it with the AVC bitrate-control path, so
SetAvcBitrate has no active target to update. Update the ios.go flow around
deviceKitInfo and StartDeviceKit264 to either attach the started stream to the
same writer/control endpoint used by devices.SetAvcBitrate or explicitly
fail/skip AVC bitrate control for iOS streams. Make sure the active iOS AVC
capture is discoverable by the new RPC handler path.
- Line 1451: Remove the stray stdout debug print in the app scan path: the
fmt.Println call in the iOS device logic is emitting a line for every app and
printing the format string literally. Update the code near the app-processing
flow in devices/ios.go to stop writing directly to stdout, and if this
diagnostic output is still needed, route it through utils.Verbose instead.
---
Outside diff comments:
In `@devices/ios.go`:
- Around line 1452-1460: The missing-app error in the DeviceKit bundle lookup is
stale and still references devicekit-ios even though the search now matches the
devicekit-h264 suffix. Update the error text in the devicekitMainAppBundleId
lookup path to tell users to install the correct bundle/app name that matches
the suffix check, keeping it aligned with the logic in the same block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b55e849-6599-4a81-a852-a402fa4fefb7
📒 Files selected for processing (6)
devices/android.godevices/avc_control.godevices/ios.goserver/dispatch.goserver/screencapture_setconfig_test.goserver/server.go
Comment on lines
+15
to
+48
| var ( | ||
| avcControlsMu sync.Mutex | ||
| avcControls = map[string]io.Writer{} | ||
| ) | ||
|
|
||
| func registerAvcControl(deviceID string, w io.Writer) { | ||
| avcControlsMu.Lock() | ||
| defer avcControlsMu.Unlock() | ||
| avcControls[deviceID] = w | ||
| } | ||
|
|
||
| func unregisterAvcControl(deviceID string) { | ||
| avcControlsMu.Lock() | ||
| defer avcControlsMu.Unlock() | ||
| delete(avcControls, deviceID) | ||
| } | ||
|
|
||
| // SetAvcBitrate pushes a live bitrate command to the running AvcServer for the | ||
| // given device. Returns an error if no AVC capture is currently active. | ||
| func SetAvcBitrate(deviceID string, bitrate int) error { | ||
| avcControlsMu.Lock() | ||
| w := avcControls[deviceID] | ||
| avcControlsMu.Unlock() | ||
|
|
||
| if w == nil { | ||
| return fmt.Errorf("no active AVC capture for device %s", deviceID) | ||
| } | ||
|
|
||
| // AvcServer reads newline-delimited commands from stdin. | ||
| if _, err := fmt.Fprintf(w, "bitrate %d\n", bitrate); err != nil { | ||
| return fmt.Errorf("failed to send bitrate command: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the registry against overlapping capture lifetimes.
registerAvcControl silently overwrites an existing writer, and unregisterAvcControl deletes by deviceID only. If a second AVC capture starts before the first deferred cleanup runs, the first cleanup can remove the new writer and make SetAvcBitrate fail with “no active AVC capture”.
Suggested direction
+type avcControl struct {
+ w io.Writer
+}
+
var (
avcControlsMu sync.Mutex
- avcControls = map[string]io.Writer{}
+ avcControls = map[string]*avcControl{}
)
-func registerAvcControl(deviceID string, w io.Writer) {
+func registerAvcControl(deviceID string, w io.Writer) (*avcControl, error) {
avcControlsMu.Lock()
defer avcControlsMu.Unlock()
- avcControls[deviceID] = w
+ if _, exists := avcControls[deviceID]; exists {
+ return nil, fmt.Errorf("AVC capture already active for device %s", deviceID)
+ }
+ control := &avcControl{w: w}
+ avcControls[deviceID] = control
+ return control, nil
}
-func unregisterAvcControl(deviceID string) {
+func unregisterAvcControl(deviceID string, control *avcControl) {
avcControlsMu.Lock()
defer avcControlsMu.Unlock()
- delete(avcControls, deviceID)
+ if avcControls[deviceID] == control {
+ delete(avcControls, deviceID)
+ }
}
func SetAvcBitrate(deviceID string, bitrate int) error {
avcControlsMu.Lock()
- w := avcControls[deviceID]
+ control := avcControls[deviceID]
avcControlsMu.Unlock()
- if w == nil {
+ if control == nil {
return fmt.Errorf("no active AVC capture for device %s", deviceID)
}
- if _, err := fmt.Fprintf(w, "bitrate %d\n", bitrate); err != nil {
+ if _, err := fmt.Fprintf(control.w, "bitrate %d\n", bitrate); err != nil {
return fmt.Errorf("failed to send bitrate command: %w", err)
}Callers should then handle duplicate registration by closing the new pipe and stopping the newly started process.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var ( | |
| avcControlsMu sync.Mutex | |
| avcControls = map[string]io.Writer{} | |
| ) | |
| func registerAvcControl(deviceID string, w io.Writer) { | |
| avcControlsMu.Lock() | |
| defer avcControlsMu.Unlock() | |
| avcControls[deviceID] = w | |
| } | |
| func unregisterAvcControl(deviceID string) { | |
| avcControlsMu.Lock() | |
| defer avcControlsMu.Unlock() | |
| delete(avcControls, deviceID) | |
| } | |
| // SetAvcBitrate pushes a live bitrate command to the running AvcServer for the | |
| // given device. Returns an error if no AVC capture is currently active. | |
| func SetAvcBitrate(deviceID string, bitrate int) error { | |
| avcControlsMu.Lock() | |
| w := avcControls[deviceID] | |
| avcControlsMu.Unlock() | |
| if w == nil { | |
| return fmt.Errorf("no active AVC capture for device %s", deviceID) | |
| } | |
| // AvcServer reads newline-delimited commands from stdin. | |
| if _, err := fmt.Fprintf(w, "bitrate %d\n", bitrate); err != nil { | |
| return fmt.Errorf("failed to send bitrate command: %w", err) | |
| } | |
| return nil | |
| } | |
| type avcControl struct { | |
| w io.Writer | |
| } | |
| var ( | |
| avcControlsMu sync.Mutex | |
| avcControls = map[string]*avcControl{} | |
| ) | |
| func registerAvcControl(deviceID string, w io.Writer) (*avcControl, error) { | |
| avcControlsMu.Lock() | |
| defer avcControlsMu.Unlock() | |
| if _, exists := avcControls[deviceID]; exists { | |
| return nil, fmt.Errorf("AVC capture already active for device %s", deviceID) | |
| } | |
| control := &avcControl{w: w} | |
| avcControls[deviceID] = control | |
| return control, nil | |
| } | |
| func unregisterAvcControl(deviceID string, control *avcControl) { | |
| avcControlsMu.Lock() | |
| defer avcControlsMu.Unlock() | |
| if avcControls[deviceID] == control { | |
| delete(avcControls, deviceID) | |
| } | |
| } | |
| // SetAvcBitrate pushes a live bitrate command to the running AvcServer for the | |
| // given device. Returns an error if no AVC capture is currently active. | |
| func SetAvcBitrate(deviceID string, bitrate int) error { | |
| avcControlsMu.Lock() | |
| control := avcControls[deviceID] | |
| avcControlsMu.Unlock() | |
| if control == nil { | |
| return fmt.Errorf("no active AVC capture for device %s", deviceID) | |
| } | |
| // AvcServer reads newline-delimited commands from stdin. | |
| if _, err := fmt.Fprintf(control.w, "bitrate %d\n", bitrate); err != nil { | |
| return fmt.Errorf("failed to send bitrate command: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devices/avc_control.go` around lines 15 - 48, The AVC control registry
currently allows overlapping captures to overwrite each other, so stale cleanup
can remove the active writer. Update registerAvcControl and unregisterAvcControl
to track ownership per capture instance (not just by deviceID) and reject or
detect duplicate registration in SetAvcBitrate’s lifecycle path. If
registerAvcControl finds an existing writer for the same deviceID, have the
caller treat that as a conflict and stop the new capture/close its pipe instead
of replacing the entry. Ensure unregisterAvcControl only removes the writer it
originally registered so a previous deferred cleanup cannot clear a newer
capture.
| // Note: passing nil registry since this is internal call from StartScreenCapture | ||
| // ScreenCapture callers should have already registered the device via StartAgent | ||
| deviceKitInfo, err = d.StartDeviceKit(nil) | ||
| deviceKitInfo, err = d.StartDeviceKit264(nil) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wire iOS AVC streams into the bitrate-control path, or reject them explicitly.
This branch starts DeviceKit H.264 streaming, but no iOS code here registers a writer/control endpoint for SetAvcBitrate. The new RPC handler calls devices.SetAvcBitrate(targetDevice.ID(), req.Bitrate), so active iOS AVC streams will still return “no active AVC capture”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devices/ios.go` at line 1002, The iOS DeviceKit H.264 start path in
StartDeviceKit264 currently creates a stream but never registers it with the AVC
bitrate-control path, so SetAvcBitrate has no active target to update. Update
the ios.go flow around deviceKitInfo and StartDeviceKit264 to either attach the
started stream to the same writer/control endpoint used by devices.SetAvcBitrate
or explicitly fail/skip AVC bitrate control for iOS streams. Make sure the
active iOS AVC capture is discoverable by the new RPC handler path.
| // signing/team prefix when re-signed (e.g. com.megidish1.devicekit-ios), so | ||
| // match on suffix rather than substring. the runner ends with | ||
| // "...devicekit-iosUITests.xctrunner" and is excluded by this suffix. | ||
| fmt.Println("-> %s", app.PackageName) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- devices/ios.go context ---'
nl -ba devices/ios.go | sed -n '1420,1475p'
echo
echo '--- search for utils.Verbose and fmt.Println usage in devices/ios.go ---'
rg -n 'utils\.Verbose|fmt\.Print(ln|f)\(' devices/ios.go
echo
echo '--- search for utils.Verbose in repo ---'
rg -n 'utils\.Verbose' .
echo
echo '--- search for startup/app bundle logging in devices/ios.go ---'
rg -n 'PackageName|bundle|startup|app bundle|debug' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- devices/ios.go context ---'
sed -n '1420,1475p' devices/ios.go | cat -n
echo
echo '--- search for utils.Verbose and fmt.Println usage in devices/ios.go ---'
rg -n 'utils\.Verbose|fmt\.Print(ln|f)\(' devices/ios.go || true
echo
echo '--- search for utils.Verbose in repo ---'
rg -n 'utils\.Verbose' . || true
echo
echo '--- search for startup/app bundle logging in devices/ios.go ---'
rg -n 'PackageName|bundle|startup|app bundle|debug' devices/ios.go || trueRepository: mobile-next/mobilecli
Length of output: 27752
Remove the stray stdout debug print.
fmt.Println("-> %s", app.PackageName) prints the format string literally and writes a line for every app scanned during startup. Use utils.Verbose if this output is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devices/ios.go` at line 1451, Remove the stray stdout debug print in the app
scan path: the fmt.Println call in the iOS device logic is emitting a line for
every app and printing the format string literally. Update the code near the
app-processing flow in devices/ios.go to stop writing directly to stdout, and if
this diagnostic output is still needed, route it through utils.Verbose instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
device.screencapture.setConfiguration— a new RPC method that applies live encoder settings to an in-flight AVC screen capture session without restarting the stream.Changes
server/server.go—handleScreenCaptureSetConfigurationhandler: validatesdeviceId+bitrate, finds the device, callsSetAvcBitrateserver/dispatch.go— registersdevice.screencapture.setConfigurationin the method registrydevices/avc_control.go—SetAvcBitrateimplementation (platform-specific)devices/android.go,devices/ios.go— platform wiring for live bitrate adjustmentserver/screencapture_setconfig_test.go— tests for the new handlerAPI
Testing
screencapture_setconfig_test.go