Add BH_SCREENSHOT_COMPACT to size screenshots for an LLM - #579
Conversation
An image-aware LLM scales images larger than 1568px on the long edge down to that before the model sees them, and charges about (width * height) / 750 tokens. On a 2x display capture_screenshot() writes a 3024px PNG, so most of those pixels are discarded on arrival. They cost no extra tokens and add no legibility, but they do enlarge the transcript the screenshot is pasted into. Set BH_SCREENSHOT_COMPACT=1 to cap the long edge at 1568 and default the filename to shot.jpg. On a real 3024x1432 capture that is 360 KB to 77 KB for the same token cost and no visible difference when read back. Defaults are unchanged: without the variable the output is the same full-resolution PNG as before. An explicit max_dim overrides the variable either way, which pixel-diff baselines need. Capture stays at native resolution and the resize happens after, because downscaling a supersampled 2x capture is sharper than rendering at deviceScaleFactor 1. Output format follows the path extension, so a caller passing .png keeps PNG.
✅ Skill review passedReviewed 1 file(s) — no findings. |
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/browser_harness/helpers.py">
<violation number="1" location="src/browser_harness/helpers.py:257">
P2: Compact mode can be unintentionally enabled when BH_SCREENSHOT_COMPACT is set to common false-like strings (for example `false`). This happens because `_compact_screenshots()` treats every non-empty value except `"0"` as true; parsing explicit truthy tokens would avoid unexpected lossy JPEG captures.</violation>
<violation number="2" location="src/browser_harness/helpers.py:284">
P2: In compact mode, passing `max_dim=None` does not actually force the lossless, native-resolution capture the PR and docs claim. When `path` is left as default, the compact branch makes the default filename `shot.jpg`, and because `as_jpeg` is true the function still routes through `img.convert("RGB").save(..., "JPEG", ...)` — so the caller gets a lossy JPEG even though resolution is native. The documentation guidance "Pixel-diff baselines need `max_dim=None`" is therefore actively misleading for anyone who has `BH_SCREENSHOT_COMPACT=1` set for their session: a pixel-diff baseline must also pass an explicit `.png` path or it will silently be a lossy JPEG. The new test `test_explicit_max_dim_overrides_the_env` masks this because it always passes `name="shot.png"`. Consider making an explicit `max_dim=None` also flip the default filename back to `shot.png`, or at least document that both `max_dim=None` and a `.png` path are required in compact mode.</violation>
<violation number="3" location="src/browser_harness/helpers.py:302">
P3: PNG screenshots are re-encoded even when no downscale is needed, which adds extra work and can change file bytes for no visual benefit. Keeping raw bytes unless a resize actually happened preserves the fast path and avoids unnecessary recompression.</violation>
</file>
<file name="tests/unit/test_helpers.py">
<violation number="1" location="tests/unit/test_helpers.py:62">
P3: This test doesn't actually verify alpha-channel dropping: fake_png produces an RGB image (no alpha), so convert("RGB") is a no-op and the real RGBA->RGB path is never covered. The comment's claim that PIL would raise is also inaccurate for this implementation, which calls img.convert("RGB") before saving. Consider feeding an RGBA capture here so the alpha-drop behavior is genuinely tested, and drop the 'raises' wording.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| compact = _compact_screenshots() | ||
| if max_dim is _UNSET: | ||
| max_dim = SCREENSHOT_MAX_DIM if compact else None | ||
| path = path or str(ipc._TMP / ("shot.jpg" if compact else "shot.png")) |
There was a problem hiding this comment.
P2: In compact mode, passing max_dim=None does not actually force the lossless, native-resolution capture the PR and docs claim. When path is left as default, the compact branch makes the default filename shot.jpg, and because as_jpeg is true the function still routes through img.convert("RGB").save(..., "JPEG", ...) — so the caller gets a lossy JPEG even though resolution is native. The documentation guidance "Pixel-diff baselines need max_dim=None" is therefore actively misleading for anyone who has BH_SCREENSHOT_COMPACT=1 set for their session: a pixel-diff baseline must also pass an explicit .png path or it will silently be a lossy JPEG. The new test test_explicit_max_dim_overrides_the_env masks this because it always passes name="shot.png". Consider making an explicit max_dim=None also flip the default filename back to shot.png, or at least document that both max_dim=None and a .png path are required in compact mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/helpers.py, line 284:
<comment>In compact mode, passing `max_dim=None` does not actually force the lossless, native-resolution capture the PR and docs claim. When `path` is left as default, the compact branch makes the default filename `shot.jpg`, and because `as_jpeg` is true the function still routes through `img.convert("RGB").save(..., "JPEG", ...)` — so the caller gets a lossy JPEG even though resolution is native. The documentation guidance "Pixel-diff baselines need `max_dim=None`" is therefore actively misleading for anyone who has `BH_SCREENSHOT_COMPACT=1` set for their session: a pixel-diff baseline must also pass an explicit `.png` path or it will silently be a lossy JPEG. The new test `test_explicit_max_dim_overrides_the_env` masks this because it always passes `name="shot.png"`. Consider making an explicit `max_dim=None` also flip the default filename back to `shot.png`, or at least document that both `max_dim=None` and a `.png` path are required in compact mode.</comment>
<file context>
@@ -239,18 +241,66 @@ def scroll(x, y, dy=-300, dx=0):
+ compact = _compact_screenshots()
+ if max_dim is _UNSET:
+ max_dim = SCREENSHOT_MAX_DIM if compact else None
+ path = path or str(ipc._TMP / ("shot.jpg" if compact else "shot.png"))
+
r = cdp("Page.captureScreenshot", format="png", captureBeyondViewport=full)
</file context>
|
|
||
| def _compact_screenshots(): | ||
| """True when BH_SCREENSHOT_COMPACT asks for LLM-sized screenshots.""" | ||
| return os.environ.get("BH_SCREENSHOT_COMPACT", "") not in ("", "0") |
There was a problem hiding this comment.
P2: Compact mode can be unintentionally enabled when BH_SCREENSHOT_COMPACT is set to common false-like strings (for example false). This happens because _compact_screenshots() treats every non-empty value except "0" as true; parsing explicit truthy tokens would avoid unexpected lossy JPEG captures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/helpers.py, line 257:
<comment>Compact mode can be unintentionally enabled when BH_SCREENSHOT_COMPACT is set to common false-like strings (for example `false`). This happens because `_compact_screenshots()` treats every non-empty value except `"0"` as true; parsing explicit truthy tokens would avoid unexpected lossy JPEG captures.</comment>
<file context>
@@ -239,18 +241,66 @@ def scroll(x, y, dy=-300, dx=0):
+
+def _compact_screenshots():
+ """True when BH_SCREENSHOT_COMPACT asks for LLM-sized screenshots."""
+ return os.environ.get("BH_SCREENSHOT_COMPACT", "") not in ("", "0")
+
+
</file context>
| return os.environ.get("BH_SCREENSHOT_COMPACT", "") not in ("", "0") | |
| return os.environ.get("BH_SCREENSHOT_COMPACT", "").strip().lower() in ("1", "true", "yes", "on") |
|
|
||
| def test_jpeg_output_drops_the_alpha_channel(fake_png): | ||
| # JPEG carries no alpha and PIL raises rather than converting silently. | ||
| assert _run(fake_png, 2000, 1000, name="shot.jpg")[1] == "JPEG" |
There was a problem hiding this comment.
P3: This test doesn't actually verify alpha-channel dropping: fake_png produces an RGB image (no alpha), so convert("RGB") is a no-op and the real RGBA->RGB path is never covered. The comment's claim that PIL would raise is also inaccurate for this implementation, which calls img.convert("RGB") before saving. Consider feeding an RGBA capture here so the alpha-drop behavior is genuinely tested, and drop the 'raises' wording.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/test_helpers.py, line 62:
<comment>This test doesn't actually verify alpha-channel dropping: fake_png produces an RGB image (no alpha), so convert("RGB") is a no-op and the real RGBA->RGB path is never covered. The comment's claim that PIL would raise is also inaccurate for this implementation, which calls img.convert("RGB") before saving. Consider feeding an RGBA capture here so the alpha-drop behavior is genuinely tested, and drop the 'raises' wording.</comment>
<file context>
@@ -9,24 +9,73 @@
+
+def test_jpeg_output_drops_the_alpha_channel(fake_png):
+ # JPEG carries no alpha and PIL raises rather than converting silently.
+ assert _run(fake_png, 2000, 1000, name="shot.jpg")[1] == "JPEG"
+
+
</file context>
| if as_jpeg: | ||
| # JPEG carries no alpha channel, and a screenshot never needs one. | ||
| img.convert("RGB").save(path, "JPEG", quality=quality, optimize=True) | ||
| else: |
There was a problem hiding this comment.
P3: PNG screenshots are re-encoded even when no downscale is needed, which adds extra work and can change file bytes for no visual benefit. Keeping raw bytes unless a resize actually happened preserves the fast path and avoids unnecessary recompression.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/helpers.py, line 302:
<comment>PNG screenshots are re-encoded even when no downscale is needed, which adds extra work and can change file bytes for no visual benefit. Keeping raw bytes unless a resize actually happened preserves the fast path and avoids unnecessary recompression.</comment>
<file context>
@@ -239,18 +241,66 @@ def scroll(x, y, dy=-300, dx=0):
+ if as_jpeg:
+ # JPEG carries no alpha channel, and a screenshot never needs one.
+ img.convert("RGB").save(path, "JPEG", quality=quality, optimize=True)
+ else:
+ img.save(path, "PNG", optimize=True)
return path
</file context>
Why
An image-aware LLM scales any image larger than 1568px on the long edge down to that before the model sees it, then charges roughly
(width × height) / 750tokens.On a 2× display
capture_screenshot()writes a 3024px PNG, so most of those pixels are thrown away on arrival. They cost no extra tokens and add no legibility. What they do cost is transcript size, and for a tool whose output is nearly always pasted into a model's context, that accumulates: in one project a single repeatedly-readshot.pnghad built up 64 MB of base64.What
BH_SCREENSHOT_COMPACT=1caps the long edge at 1568 and makes the default filenameshot.jpg.Measured on a real 3024×1432 capture:
Same effective resolution, same token cost, 4.7× smaller. Verified legible by reading the result back: body text, small grey URLs and form labels all survive.
Defaults are unchanged
Without the environment variable the output is the same full-resolution PNG as before, same filename. This is opt-in on purpose, since anyone relying on lossless PNG at native resolution should not be surprised by a new default.
Other details:
max_dimoverrides the variable in both directions. Pixel-diff baselines can forcemax_dim=Noneeven in compact mode..pngkeeps PNG and only picks up the resize.deviceScaleFactor: 1.interaction-skills/screenshots.mddocuments the 1568 reasoning, and the trade-off below it: at 900px body text survives but dimmed labels and exact identifiers do not, and a misread that forces a recapture costs more than one clean shot.Tests
Existing
test_max_dim_default_is_no_resizestill passes unmodified, which is the assertion that matters for backwards compatibility. Seven tests added for compact mode, the env override, extension-driven format and the default filenames. 104 pass.Summary by cubic
Add compact screenshot mode for image-aware models. When
BH_SCREENSHOT_COMPACT=1, screenshots are capped at 1568px on the long edge and default to JPEG, cutting file size with no extra token cost. Defaults are unchanged unless enabled.BH_SCREENSHOT_COMPACT=1caps long edge at 1568 and defaults toshot.jpg(JPEG q75).max_dimalways overrides the env (useNonefor native size)..pngkeeps PNG and only resizes.Written for commit 441280e. Summary will update on new commits.