-
Notifications
You must be signed in to change notification settings - Fork 0
Implement scrcpy screenshot backend in adb_vision #44
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,7 @@ | ||
| [project] | ||
| name = "adb-vision" | ||
| version = "0.1.0" | ||
| dependencies = [ | ||
| "av>=16.1.0", | ||
| "pillow>=10.0.0", | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,164 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| import asyncio | ||||||||||||||||||||||||||||||||||||||||||||
| import struct | ||||||||||||||||||||||||||||||||||||||||||||
| import base64 | ||||||||||||||||||||||||||||||||||||||||||||
| import io | ||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2
to
+5
|
||||||||||||||||||||||||||||||||||||||||||||
| import struct | |
| import base64 | |
| import io | |
| import os | |
| import base64 | |
| import io |
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: 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
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: 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
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 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()}" | |
| ) |
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.
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, |
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.
Guard scrcpy socket reads with timeouts
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 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
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: 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
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: 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
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,130 @@ | ||||||||||||
| import pytest | ||||||||||||
| import asyncio | ||||||||||||
| import base64 | ||||||||||||
| import io | ||||||||||||
| from unittest.mock import MagicMock, patch, AsyncMock, ANY | ||||||||||||
| from adb_vision.screenshot import _capture_scrcpy | ||||||||||||
|
Comment on lines
+1
to
+6
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.asyncio | ||||||||||||
| async def test_capture_scrcpy_happy_path(): | ||||||||||||
| # Mock adb_run | ||||||||||||
| adb_run = AsyncMock() | ||||||||||||
| adb_run.side_effect = [ | ||||||||||||
| b"pushed", # push jar | ||||||||||||
| b"21504", # forward tcp:0 | ||||||||||||
| b"removed" # forward --remove | ||||||||||||
| ] | ||||||||||||
|
|
||||||||||||
| # Mock asyncio.create_subprocess_exec | ||||||||||||
| mock_process = AsyncMock() | ||||||||||||
| mock_process.returncode = None | ||||||||||||
| mock_process.terminate = MagicMock() | ||||||||||||
| mock_process.wait = AsyncMock() | ||||||||||||
|
|
||||||||||||
| # Mock asyncio.open_connection | ||||||||||||
| mock_reader = AsyncMock() | ||||||||||||
| mock_writer = AsyncMock() | ||||||||||||
| mock_writer.close = MagicMock() | ||||||||||||
| mock_writer.wait_closed = AsyncMock() | ||||||||||||
|
|
||||||||||||
| # Header: 69 bytes | ||||||||||||
| mock_reader.readexactly.return_value = b"\x00" + b"x" * 64 + b"\x05\x00\x02\xd0" | ||||||||||||
|
|
||||||||||||
| # Mock frame.to_image() to return a real PIL image so img.save works | ||||||||||||
| from PIL import Image | ||||||||||||
| real_img = Image.new("RGB", (1280, 720), color="red") | ||||||||||||
|
|
||||||||||||
| with patch("asyncio.create_subprocess_exec", return_value=mock_process), \ | ||||||||||||
| patch("asyncio.open_connection", return_value=(mock_reader, mock_writer)), \ | ||||||||||||
| patch("av.codec.CodecContext") as mock_codec_context_class: | ||||||||||||
|
|
||||||||||||
| mock_codec = MagicMock() | ||||||||||||
| mock_codec_context_class.create.return_value = mock_codec | ||||||||||||
|
|
||||||||||||
| mock_packet = MagicMock() | ||||||||||||
| mock_codec.parse.return_value = [mock_packet] | ||||||||||||
|
|
||||||||||||
| mock_frame = MagicMock() | ||||||||||||
| mock_codec.decode.return_value = [mock_frame] | ||||||||||||
| mock_frame.to_image.return_value = real_img | ||||||||||||
|
|
||||||||||||
| mock_reader.read.side_effect = [b"some h264 data", b""] | ||||||||||||
|
|
||||||||||||
| serial = "127.0.0.1:5555" | ||||||||||||
| result = await _capture_scrcpy( | ||||||||||||
| adb_run=adb_run, | ||||||||||||
| serial=serial, | ||||||||||||
| adb_exe="adb" | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| assert result is not None | ||||||||||||
| assert len(result) > 0 | ||||||||||||
| # Verify it's valid base64 | ||||||||||||
| base64.b64decode(result) | ||||||||||||
|
|
||||||||||||
| # Verify ADB calls | ||||||||||||
| assert adb_run.call_count >= 3 | ||||||||||||
|
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: 🩹 The Fix:
Suggested change
📏 Severity: suggestion |
||||||||||||
| 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) | ||||||||||||
|
|
||||||||||||
| @pytest.mark.asyncio | ||||||||||||
| async def test_capture_scrcpy_server_fail(): | ||||||||||||
| adb_run = AsyncMock() | ||||||||||||
| adb_run.side_effect = [ | ||||||||||||
| b"pushed", # push jar | ||||||||||||
| b"21504", # forward tcp:0 | ||||||||||||
| b"removed" # forward --remove | ||||||||||||
| ] | ||||||||||||
|
|
||||||||||||
| mock_process = AsyncMock() | ||||||||||||
| mock_process.returncode = 1 | ||||||||||||
| mock_process.stderr.read.return_value = b"some error" | ||||||||||||
| mock_process.terminate = MagicMock() | ||||||||||||
| mock_process.wait = AsyncMock() | ||||||||||||
|
|
||||||||||||
| with patch("asyncio.create_subprocess_exec", return_value=mock_process), \ | ||||||||||||
| patch("asyncio.open_connection", side_effect=ConnectionRefusedError()): | ||||||||||||
|
|
||||||||||||
| with pytest.raises(RuntimeError, match="scrcpy-server exited: some error"): | ||||||||||||
| await _capture_scrcpy( | ||||||||||||
| adb_run=adb_run, | ||||||||||||
| serial="127.0.0.1:5555", | ||||||||||||
| adb_exe="adb" | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| @pytest.mark.asyncio | ||||||||||||
| async def test_capture_scrcpy_decode_fail(): | ||||||||||||
| adb_run = AsyncMock() | ||||||||||||
| adb_run.side_effect = [ | ||||||||||||
| b"pushed", # push jar | ||||||||||||
| b"21504", # forward tcp:0 | ||||||||||||
| b"removed" # forward --remove | ||||||||||||
| ] | ||||||||||||
|
|
||||||||||||
| mock_process = AsyncMock() | ||||||||||||
| mock_process.returncode = None | ||||||||||||
| mock_process.terminate = MagicMock() | ||||||||||||
| mock_process.wait = AsyncMock() | ||||||||||||
|
|
||||||||||||
| mock_reader = AsyncMock() | ||||||||||||
| mock_writer = AsyncMock() | ||||||||||||
| mock_writer.close = MagicMock() | ||||||||||||
| mock_writer.wait_closed = AsyncMock() | ||||||||||||
| mock_reader.readexactly.return_value = b"\x00" + b"x" * 64 + b"\x05\x00\x02\xd0" | ||||||||||||
| mock_reader.read.return_value = b"" # No data | ||||||||||||
|
|
||||||||||||
| with patch("asyncio.create_subprocess_exec", return_value=mock_process), \ | ||||||||||||
| patch("asyncio.open_connection", return_value=(mock_reader, mock_writer)), \ | ||||||||||||
| patch("av.codec.CodecContext") as mock_codec_context_class: | ||||||||||||
|
|
||||||||||||
| mock_codec = MagicMock() | ||||||||||||
| mock_codec_context_class.create.return_value = mock_codec | ||||||||||||
| mock_codec.parse.return_value = [] | ||||||||||||
|
|
||||||||||||
| with pytest.raises(RuntimeError, match="Failed to decode a frame from scrcpy stream"): | ||||||||||||
| await _capture_scrcpy( | ||||||||||||
| adb_run=adb_run, | ||||||||||||
| serial="127.0.0.1:5555", | ||||||||||||
| adb_exe="adb" | ||||||||||||
| ) | ||||||||||||
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: Importing
structlike 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:
📏 Severity: nitpick