fix(core): bound model image payload by bytes, not only by pixels - #10091
MostimaBridges wants to merge 2 commits into
Conversation
The still-image passthrough in _convert_image_bytes_sync only checked format, EXIF orientation and the longest edge, so a compliant PNG/JPEG was sent byte-exact no matter how large it was. A noisy 1280x1280 PNG already exceeds 6 MiB, and CUA sessions lift the edge cap entirely. Providers then reject the whole request with 413 before the model sees the image, surfacing as an opaque All chat models failed error. Add MODEL_IMAGE_MAX_BYTES (1 MiB) as a per-still byte budget that gates the passthrough. Stills over budget keep their pixel dimensions but are re-encoded, derating the JPEG quality once when the result is still oversized. CUA sandbox sessions pass None to keep compliant stills byte-exact, since that path deliberately trades payload size for pixel fidelity and already warns about oversized images. Refs AstrBotDevs#10089
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/pipeline/process_stage/method/agent_sub_stages/internal.py" line_range="270" />
<code_context>
max_size = 1_000_000
+ # CUA stills stay byte-exact to avoid JPEG color shifts, so the
+ # byte budget applies only elsewhere; that path warns instead.
+ max_bytes = None if cua_pixel_mode else MODEL_IMAGE_MAX_BYTES
quality = (
options.get("quality") if isinstance(options, dict) else None
</code_context>
<issue_to_address>
**issue (broader_impact):** In CUA mode, the second `prepare_request_images` call does not receive the locally computed `max_bytes=None`, so images added or replaced by the request hook after the first preparation pass use the default 1 MiB budget and are re-encoded instead of remaining byte-exact.
**Triggers:** When an `OnLLMRequestEvent` hook adds or replaces an image after the first preparation pass in a CUA session.
**Suggested fix:** Pass `max_bytes=max_bytes` to the second `prepare_request_images` call as well.
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/media_utils.py" line_range="1273-1280" />
<code_context>
if cached is not None:
return cached
encoded = _encode_image_frame_bytes(image, max_size=max_size, quality=quality)
+ if max_bytes is not None and len(encoded) > max_bytes and quality > 1:
+ # Re-encoding keeps the geometry but not necessarily the payload, so
+ # derate once instead of sending a request the provider rejects.
+ derated = max(1, quality * max_bytes // len(encoded))
+ if derated < quality:
+ encoded = _encode_image_frame_bytes(
+ image, max_size=max_size, quality=derated
+ )
_publish_image_cache_atomic(output_path, encoded)
return encoded
</code_context>
<issue_to_address>
**issue (bug_risk):** The one-time quality derating does not guarantee that the encoded result is at or below `max_bytes`; a compliant image with a large preserved ICC profile can remain over 1 MiB because changing JPEG quality does not reduce the embedded profile, so the provider still receives an oversized payload.
**Triggers:** When the source image contains metadata, especially an ICC profile, whose size alone exceeds the byte budget.
**Suggested fix:** Verify the derated result size and either remove or appropriately handle oversized metadata, or continue reducing/re-encoding until the byte budget is met.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: astrbot/core/pipeline/process_stage/method/agent_sub_stages/internal.py:270, astrbot/core/utils/media_utils.py:1280
… every pass Review follow-ups for the byte budget. Pass max_bytes to the second prepare_request_images call as well. Images added or replaced by an OnLLMRequestEvent hook were re-encoded with the default budget, so CUA sessions lost their byte-exact passthrough there. Keep shrinking until the payload actually fits. A single quality step cannot bound a payload that the embedded ICC profile alone keeps large, because the profile survives every quality setting. Retries now drop the metadata first, then derate the quality, and shrink the canvas only once the quality floor is reached. Refs AstrBotDevs#10089
CI note: the macOS pytest failure is an unrelated timing flake
That test touches nothing this PR changes ( Evidence gathered locally on the same commit: The spread from 2.3s to 4.9s for the same five tests shows how load-sensitive that file is, and running it in the same process as the new image tests does not reproduce the failure. I left the test untouched to keep this PR focused. A re-run of the failed job should be enough; if it recurs it is worth a separate fix, for example asserting on |
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Reviewed latest head 940f416 after the Sourcery findings. The second post-hook prepare_request_images call now receives the same max_bytes value, preserving the CUA max_bytes=None exemption for images added or replaced by hooks. Byte fitting now retries within a bounded loop, drops embedded ICC metadata, derates JPEG quality, and only then scales the canvas, with a regression test using a 2 MiB ICC profile plus oversized noisy PNG coverage. The cache key/version includes byte-budget semantics, and the full CI matrix (including Windows/macOS/Linux, CodeQL, dashboard, smoke) is green. I found no remaining correctness blocker.
Fixes #10089.
A compliant still image (PNG/JPEG, upright, within
image_compress_options.max_size) was passed through byte-exact regardless of its file size. The pixel cap bounds dimensions, not compressed size: a noisy 1280×1280 PNG is already 6.26 MiB, and CUA sandbox sessions lift the edge cap to 1e6 px, which makes the passthrough effectively unconditional. A single prepared still could therefore carry tens of megabytes into the request body. Providers reject the payload with 413 before the model ever sees the image, and the failure only surfaces asAll chat models failed: APIStatusError: 413, which hides the actual cause.Modifications / 改动点
astrbot/core/utils/media_utils.py: addMODEL_IMAGE_MAX_BYTES(1 MiB) as a per-still byte budget, and require the source to be within it before_convert_image_bytes_syncreuses a still unchanged. Oversized stills are re-encoded and retried within a bounded attempt budget (MODEL_IMAGE_SHRINK_ATTEMPTS), because no single lever is enough on its own: quality derating cannot shrink a payload that the embedded ICC profile alone keeps large, and neither lever shrinks a canvas whose lowest-quality output still exceeds the budget. The first retry therefore drops the embedded metadata, later retries derate the JPEG quality proportionally, and the canvas is halved only once the quality floor is reached._encode_image_frame_bytesgainedkeep_metadatafor this,prepare_model_imagegainedmax_bytes(defaults to the budget), and the derived-cache version was bumped so stale entries are not reused..../agent_sub_stages/image_input.pyandinternal.py: threadmax_bytesthroughprepare_request_images, following the existingmontage_max_sizepattern, on both call sites so images added or replaced by anOnLLMRequestEventhook are covered too. CUA pixel sessions passNone, keeping their compliant stills byte-exact, because that path deliberately trades payload size for pixel fidelity and already warns about oversized images. The CUA resize lift and its warning are unchanged.docs/{zh,en}/providers/image-formats.md: document the byte budget and the CUA exemption.tests/test_model_image_preparation.py: cover that an oversized compliant still is re-encoded within the budget, that oversized metadata cannot keep a still over budget, and thatmax_bytes=Nonepreserves byte-exact passthrough.Behavior change to be aware of: compliant stills above 1 MiB are now re-encoded instead of passed through byte-exact. Pixel dimensions are preserved whenever quality alone fits the budget; they are scaled down only in the last resort where the alternative is a request the provider rejects. Oversized embedded metadata is dropped. The source file is never modified, and stills at or below 1 MiB are untouched.
Out of scope for this PR (tracked in the issue): history images are still not re-prepared, and a 413 still walks every fallback provider before failing.
Screenshots or Test Results / 运行截图或测试结果
The reported shape — noisy 1280×1280 RGBA PNG, default
max_size=1280:Oversized metadata — a still carrying a 2 MiB ICC profile, where quality derating alone never converges:
Checks, with
ruff 0.15.22(the version pinned inpyproject.toml):New tests:
Checklist / 检查清单
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。/ 我的更改没有引入恶意代码。