-
Notifications
You must be signed in to change notification settings - Fork 0
Implement DroidCast Screenshot Backend #47
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,70 @@ | ||||||||
| import asyncio | ||||||||
| import base64 | ||||||||
| import logging | ||||||||
| import urllib.request | ||||||||
| from typing import Callable, Awaitable | ||||||||
|
|
||||||||
| # Type alias for the async ADB runner | ||||||||
| AdbRunFn = Callable[..., Awaitable[bytes]] | ||||||||
|
|
||||||||
| async def _capture_droidcast(*, adb_run: AdbRunFn, serial: str, adb_exe: str) -> str: | ||||||||
| """Return base64-encoded PNG screenshot via DroidCast HTTP stream.""" | ||||||||
| port = "53516" | ||||||||
| apk_path = "alas_wrapped/bin/DroidCast/DroidCast-debug-1.2.3.apk" | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 The Roast: Hardcoding a relative path to the APK? Bold strategy. This will work great until someone runs the code from literally any other directory. Then it's a game of 'Where's Waldo?' but with APK files and FileNotFound errors. 🩹 The Fix: Use 📏 Severity: warning |
||||||||
|
|
||||||||
|
Comment on lines
+12
to
+14
|
||||||||
| # 1. Idempotency check: if port forward already exists, try to fetch first | ||||||||
| try: | ||||||||
| forward_list = await adb_run("forward", "--list", timeout=5.0) | ||||||||
| if f"tcp:{port}" in forward_list.decode(): | ||||||||
| try: | ||||||||
| return await _fetch_screenshot(port) | ||||||||
| except Exception: | ||||||||
| # Port forward exists but service might not be responding, continue to start | ||||||||
| pass | ||||||||
| except Exception: | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 The Roast: Ah yes, the classic 🩹 The Fix: At minimum, log the exception with except Exception as e:
logging.debug(f"Forward list check failed: {e}")📏 Severity: warning |
||||||||
| pass | ||||||||
|
|
||||||||
| # 2. Push/Install APK if needed | ||||||||
| # (In a real scenario we'd check if installed, but following the plan's direct install -r) | ||||||||
| try: | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 The Roast: You attempt to install the APK without first checking if the file even exists. That's like ordering pizza before checking if you have money — technically optimistic, but practically setting yourself up for disappointment. 🩹 The Fix: Add a pre-flight check: apk_path_obj = Path(apk_path)
if not apk_path_obj.exists():
raise FileNotFoundError(f"DroidCast APK not found: {apk_path}")📏 Severity: suggestion |
||||||||
| await adb_run("install", "-r", apk_path, timeout=30.0) | ||||||||
| except Exception as e: | ||||||||
| # If the file is missing in the environment, we log it but might proceed if it's already installed | ||||||||
| logging.warning(f"DroidCast install failed: {e}") | ||||||||
|
|
||||||||
|
Comment on lines
+27
to
+34
|
||||||||
| # 3. Start DroidCast service | ||||||||
| await adb_run( | ||||||||
| "shell", "am", "start-foreground-service", | ||||||||
| "-n", "com.rayworks.droidcast/.Main", | ||||||||
| "-a", "android.intent.action.MAIN", | ||||||||
| timeout=10.0 | ||||||||
| ) | ||||||||
|
Comment on lines
+35
to
+41
|
||||||||
|
|
||||||||
| # 4. Forward the port | ||||||||
| await adb_run("forward", f"tcp:{port}", f"tcp:{port}", timeout=5.0) | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 The Roast: You forward the port but never clean it up. This is the networking equivalent of leaving camping chairs at a festival — eventually you'll run out of ports and wonder why nothing works anymore. 🩹 The Fix: Add a cleanup step or document that callers must manage port lifecycle. Consider using 📏 Severity: warning |
||||||||
|
|
||||||||
| # 5. Fetch and return | ||||||||
| # Allow a small retry loop for the service to bind the port | ||||||||
| for i in range(5): | ||||||||
| try: | ||||||||
| return await _fetch_screenshot(port) | ||||||||
| except Exception: | ||||||||
| if i == 4: | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 The Roast: Another 🩹 The Fix: Capture and log the last exception before raising: except Exception as last_err:
if i == 4:
raise RuntimeError(f"Failed to fetch screenshot after 5 retries: {last_err}") from last_err📏 Severity: suggestion |
||||||||
| raise | ||||||||
| await asyncio.sleep(0.5) | ||||||||
|
|
||||||||
|
|
||||||||
| async def _fetch_screenshot(port: str) -> str: | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 The Roast: Wait, the function returns just a string? I thought we established a Tool Contract in ROADMAP.md that requires 🩹 The Fix: Return the standard tool envelope as documented in return {
"success": True,
"data": {"screenshot_b64": base64.b64encode(data).decode("ascii")},
"error": None,
"observed_state": "screenshot_captured",
"expected_state": "screenshot_captured"
}📏 Severity: critical |
||||||||
| """Internal helper to GET screenshot from localhost:port.""" | ||||||||
| url = f"http://localhost:{port}/screenshot?format=png" | ||||||||
|
||||||||
| url = f"http://localhost:{port}/screenshot?format=png" | |
| # DroidCast_raw serves PNG frames on /preview; /screenshot returns raw RGB565. | |
| url = f"http://localhost:{port}/preview" |
Copilot
AI
Mar 3, 2026
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.
urllib.request.urlopen() raises urllib.error.HTTPError for non-2xx responses, so resp.status != 200 will never run for 404/500 in real usage. Catch HTTPError (and possibly URLError) inside _get() and convert it into the intended RuntimeError so callers/tests get consistent behavior.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,98 @@ | ||||||||||
| import asyncio | ||||||||||
| import base64 | ||||||||||
| import io | ||||||||||
|
Comment on lines
+1
to
+3
|
||||||||||
| import asyncio | |
| import base64 | |
| import io | |
| import base64 |
Copilot
AI
Mar 3, 2026
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.
These tests won’t be collected by the repo’s default pytest run because root pytest.ini restricts testpaths to agent_orchestrator/. If adb_vision tests are meant to run in CI, you’ll need to expand testpaths (or add a separate test invocation / move these tests under the collected suite).
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 Roast: Setting both return_value AND side_effect on the same mock? That's like setting your GPS destination AND telling it to take random turns. Once the side_effect list is exhausted, it'll fall back to return_value instead of raising StopIteration. Your test might pass by accident.
🩹 The Fix: Remove adb_run.return_value = b"" — the side_effect list already defines all expected return values.
📏 Severity: suggestion
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 Roast: Same issue here — you set return_value = b"" but then immediately override the behavior with side_effect. Pick a lane! The mock is having an identity crisis.
🩹 The Fix: Remove line 81 (adb_run.return_value = b"") since you're using side_effect.
📏 Severity: nitpick
Copilot
AI
Mar 3, 2026
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 HTTP failure test mocks urlopen() returning a response with status=404, but real urllib.request.urlopen() raises urllib.error.HTTPError on 404 (no response object is returned). Update the test to reflect urllib’s actual behavior (and update _fetch_screenshot accordingly) so this failure mode is exercised realistically.
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 Roast: You accept
serialandadb_exeparameters like you're running a customizable operation, but then completely ignore them in favor of... whateveradb_runclosure was passed in. These parameters are about as useful as a screen door on a submarine — they look like they do something, but they're just for show.🩹 The Fix: Either use these parameters to construct the ADB command (like the MCP server does with
ADB_EXECUTABLEandADB_SERIAL), or remove them from the signature if the caller is expected to handle device targeting viaadb_run.📏 Severity: warning