Implement scrcpy screenshot backend in adb_vision#44
Conversation
- Created `adb_vision` package for standalone ADB vision tools. - Implemented `_capture_scrcpy` in `adb_vision/screenshot.py` using `scrcpy-server-v1.20.jar`. - Added high-performance H.264 stream decoding via PyAV. - Added comprehensive unit tests with mocked ADB and socket interactions in `adb_vision/test_scrcpy.py`. - Ensured proper cleanup of server processes and port forwarding. - Followed strict modularity by avoiding any ALAS internal imports. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| import struct | ||
| import base64 | ||
| import io | ||
| import os |
There was a problem hiding this comment.
🔥 The Roast: Importing struct like it's going to a party, but it never shows up to work. The invite is right there on line 5, but the function body is ghosting it harder than my ex.
🩹 The Fix:
| import os | |
| import asyncio | |
| import base64 | |
| import io | |
| import re | |
| from pathlib import Path | |
| from typing import Protocol |
📏 Severity: nitpick
| import base64 | ||
| import io | ||
| import os | ||
| import re |
There was a problem hiding this comment.
🔥 The Roast: os is the plus-one that didn't even make it into the venue. Imported but never referenced — it's the os in 'gloss over unused imports.'
🩹 The Fix: Remove this import.
📏 Severity: nitpick
| import os | ||
| import re | ||
| from pathlib import Path | ||
| from typing import Protocol, Optional |
There was a problem hiding this comment.
🔥 The Roast: Optional is sitting on the bench with its helmet on, ready to play, but the coach never calls its name. Classic premature optimization for a type hint that never materialized.
🩹 The Fix: Remove Optional from the import.
📏 Severity: nitpick
| if writer: | ||
| writer.close() | ||
| try: | ||
| await writer.wait_closed() |
There was a problem hiding this comment.
🔥 The Roast: This except Exception: pass is the debugging equivalent of a 'nothing to see here' sign after a car crash. Sure, wait_closed() might fail on a half-dead connection, but swallowing ALL exceptions means legitimate bugs go to die silently. I've debugged issues that took hours because someone thought 'eh, what could go wrong?'
🩹 The Fix:
| await writer.wait_closed() | |
| try: | |
| await writer.wait_closed() | |
| except (ConnectionResetError, BrokenPipeError): | |
| pass # Expected on already-closed connections |
📏 Severity: warning
| except Exception: | ||
| pass | ||
|
|
||
| finally: |
There was a problem hiding this comment.
🔥 The Roast: A triple-nested try-except that ends in pass. This cleanup code is like a Russian nesting doll of exception suppression — by the time you get to the center, you've forgotten what error you were trying to handle. If kill() fails, you're truly out of options, but at least log it so we know something went sideways.
🩹 The Fix:
| finally: | |
| finally: | |
| # 7. Cleanup | |
| if server_process: | |
| try: | |
| server_process.terminate() | |
| await asyncio.wait_for(server_process.wait(), timeout=2.0) | |
| except ProcessLookupError: | |
| pass # Already gone | |
| except asyncio.TimeoutError: | |
| try: | |
| server_process.kill() | |
| await server_process.wait() | |
| except ProcessLookupError: | |
| pass # Already gone |
📏 Severity: warning
| pass | ||
|
|
||
| if local_port: | ||
| try: |
There was a problem hiding this comment.
🔥 The Roast: Another except Exception: pass? This is like leaving a port forward dangling on the device because we couldn't be bothered to handle the error. If adb forward --remove fails, the port stays allocated until someone manually cleans it up or the device reboots. Hope you don't hit this 20 times in a row.
🩹 The Fix:
| try: | |
| if local_port: | |
| try: | |
| await adb_run("-s", serial, "forward", "--remove", f"tcp:{local_port}", timeout=5.0) | |
| except Exception as e: | |
| import warnings | |
| warnings.warn(f"Failed to remove ADB forward for port {local_port}: {e}") |
📏 Severity: warning
| base64.b64decode(result) | ||
|
|
||
| # Verify ADB calls | ||
| assert adb_run.call_count >= 3 |
There was a problem hiding this comment.
🔥 The Roast: assert adb_run.call_count >= 3 is the unit test equivalent of 'I think the code ran, probably, maybe.' This assertion is so loose it would pass if the function called ADB 47 times. The subsequent assert_any_call is the only thing saving this test from being completely useless.
🩹 The Fix:
| assert adb_run.call_count >= 3 | |
| assert adb_run.call_count == 3 | |
| adb_run.assert_any_call("-s", serial, "push", ANY, "/data/local/tmp/scrcpy-server.jar", timeout=15.0) | |
| adb_run.assert_any_call("-s", serial, "forward", "tcp:0", "localabstract:scrcpy", timeout=5.0) | |
| adb_run.assert_any_call("-s", serial, "forward", "--remove", "tcp:21504", timeout=5.0) |
📏 Severity: suggestion
Code Review Roast 🔥Verdict: 7 Issues Found | Recommendation: Address warnings before merge Overview
Issue Details (click to expand)
🏆 Best part: The scrcpy integration is actually well-researched — those positional arguments for v1.20 are documented better than most enterprise APIs I've seen. Someone did their homework on the wire protocol. I need to sit down. 💀 Worst part: Three layers of 📊 Overall: Like a house with a gorgeous facade but duct tape holding the pipes together. The core functionality is solid, but the error handling needs to graduate from 'hope for the best' to 'plan for the worst.' Files Reviewed (3 files)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83bfb2a91a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| # 5. Read header (69 bytes) | ||
| # Dummy byte(1), Device name(64), Resolution(4) | ||
| await reader.readexactly(69) |
There was a problem hiding this comment.
This read path has no timeout (readexactly(69) here and later reader.read(...) in the decode loop), so if scrcpy accepts the TCP connection but stalls before emitting bytes, _capture_scrcpy() can hang forever and never reach the cleanup block. In practice that can wedge screenshot-dependent workflows until an external watchdog kills the task; wrapping these awaits in asyncio.wait_for(...) (or equivalent bounded read logic) avoids indefinite hangs on partial server failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new adb_vision standalone screenshot backend that captures a single frame via scrcpy-server and decodes it to a base64 PNG, targeting emulator environments where adb shell screencap produces blank images (Fixes #41).
Changes:
- Implement
_capture_scrcpy()to push/runscrcpy-server, forward the abstract socket, read H.264, decode the first frame via PyAV, and return base64 PNG. - Add mocked async tests for happy path, server failure, and decode failure scenarios.
- Introduce
adb_visionpackaging metadata withav+pillowdependencies.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
adb_vision/screenshot.py |
Implements the scrcpy-based single-frame screenshot capture and cleanup logic. |
adb_vision/test_scrcpy.py |
Adds async unit tests with mocked ADB/subprocess/socket/decoder behavior. |
adb_vision/pyproject.toml |
Declares the new package and runtime dependencies (av, pillow). |
adb_vision/__init__.py |
Initializes the new package (currently empty). |
| import pytest | ||
| import asyncio | ||
| import base64 | ||
| import io | ||
| from unittest.mock import MagicMock, patch, AsyncMock, ANY | ||
| from adb_vision.screenshot import _capture_scrcpy |
There was a problem hiding this comment.
These tests will not run under the repo’s default pytest configuration: pytest.ini restricts testpaths to agent_orchestrator/, so adb_vision/test_scrcpy.py is not collected in CI by default. If this package is meant to be tested automatically, either update the root pytest configuration / CI command, or relocate these tests under the collected test suite.
| import struct | ||
| import base64 | ||
| import io | ||
| import os |
There was a problem hiding this comment.
There are unused imports here (e.g., struct and os), which adds noise and can confuse readers about what the implementation relies on. Please remove unused imports (or start using them if they’re intended for upcoming functionality).
| import struct | |
| import base64 | |
| import io | |
| import os | |
| import base64 | |
| import io |
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE |
There was a problem hiding this comment.
asyncio.create_subprocess_exec() is started with stdout/stderr set to PIPE, but the code never drains server_process.stdout (and only reads stderr on early exit). If scrcpy writes enough logs, the OS pipe buffer can fill and block the scrcpy process, potentially hanging screenshot capture. Consider redirecting output to DEVNULL, lowering log level, or continuously draining the pipes in background tasks while the server is running.
| stdout=asyncio.subprocess.PIPE, | |
| stderr=asyncio.subprocess.PIPE | |
| stdout=asyncio.subprocess.DEVNULL, | |
| stderr=asyncio.subprocess.DEVNULL, |
| local_jar = base_dir / "alas_wrapped" / "bin" / "scrcpy" / "scrcpy-server-v1.20.jar" | ||
|
|
||
| if not local_jar.exists(): | ||
| # Fallback to current working directory relative path | ||
| local_jar = Path("alas_wrapped/bin/scrcpy/scrcpy-server-v1.20.jar") | ||
| if not local_jar.exists(): | ||
| raise FileNotFoundError(f"scrcpy-server-v1.20.jar not found. Checked: {local_jar.absolute()}") |
There was a problem hiding this comment.
The FileNotFoundError message only reports the fallback path (alas_wrapped/bin/scrcpy/...) even though the function first checks a path relative to __file__. This makes debugging harder when the repo layout is unexpected. Consider including both attempted locations (or the originally computed path) in the error message.
| local_jar = base_dir / "alas_wrapped" / "bin" / "scrcpy" / "scrcpy-server-v1.20.jar" | |
| if not local_jar.exists(): | |
| # Fallback to current working directory relative path | |
| local_jar = Path("alas_wrapped/bin/scrcpy/scrcpy-server-v1.20.jar") | |
| if not local_jar.exists(): | |
| raise FileNotFoundError(f"scrcpy-server-v1.20.jar not found. Checked: {local_jar.absolute()}") | |
| primary_jar = base_dir / "alas_wrapped" / "bin" / "scrcpy" / "scrcpy-server-v1.20.jar" | |
| if primary_jar.exists(): | |
| local_jar = primary_jar | |
| else: | |
| # Fallback to current working directory relative path | |
| fallback_jar = Path("alas_wrapped/bin/scrcpy/scrcpy-server-v1.20.jar") | |
| if fallback_jar.exists(): | |
| local_jar = fallback_jar | |
| else: | |
| raise FileNotFoundError( | |
| "scrcpy-server-v1.20.jar not found. " | |
| f"Checked: {primary_jar.resolve()} and {fallback_jar.resolve()}" | |
| ) |
This change implements a new screenshot backend using
scrcpyin theadb_visionpackage. This backend is critical for environments like MEmu or VirtualBox where standardadb shell screencapreturns blank images.The implementation:
scrcpy-server-v1.20.jarto the Android device.app_process.av(PyAV) and converts it to a base64-encoded PNG.The new package
adb_visionis designed to be standalone and does not import any ALAS internals.Tests cover:
Fixes #41
PR created automatically by Jules for task 9657118203258962835 started by @Coldaine