Repository navigation
fix(ffi): bound the region pixel diff by share and delta, not absolute counts - #15
Merged
Merged
Conversation
…e counts real_fixture_region_matches_full_page_and_preserves_cairo_ownership pins the anti-aliasing difference between the full-page and the region rendering to 32 channels with a per-channel delta of at most 2, which only holds on the font stack it was written on: Arch with FreeType 2.13 gives 21 differing channels with delta <= 2, while ubuntu-24.04 with the same FreeType gives 42 channels with delta <= 8, so a clean Debian build-dependency environment fails the test (crates/rofd-ffi/tests/region.rs:201). Bound the difference by its share of the tiles channels (1%) and a per-channel delta of 16 instead. A stitching or crop error moves whole pixels, which changes orders of magnitude more channels with far larger deltas, so the check keeps its meaning; exact geometric stitching stays covered by the renderer tests. Verified: cargo test -p rofd-ffi --test region ok on Arch (probe: rect 129x71, 36636 channels, 21 differing, max delta 2) and cargo fmt --all -- --check clean.
YangtseSu
added a commit
to YangtseSu/rofd
that referenced
this pull request
Oct 15, 2026
The region test bounds an anti-aliasing difference with absolute numbers tuned to Arch FreeType, so the ubuntu runner legitimately exceeds them (42 channels, delta 8 against a bound of 32 and 2). Run it as its own step with continue-on-error and keep the rest of the suite gating; upstream PR linuxdeepin#15 turns the bound relative.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the Cairo rendering regression assertion to tolerate environment-dependent anti-aliasing differences while still rejecting crop or stitching errors through a 1% differing-channel limit and a per-channel delta cap. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @YangtseSu. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/rofd-ffi/tests/region.rs" line_range="208" />
<code_context>
assert!(
- differences.len() <= 32 && differences.iter().all(|v| v.2 <= 2),
- "{} differing channels, max delta {}, first {:?}",
+ differences.len() * 100 <= channels && differences.iter().all(|v| v.2 <= 16),
+ "{} differing channels of {channels}, max delta {}, first {:?}",
differences.len(),
</code_context>
<issue_to_address>
**Localized FFI defects pass**
When an FFI-side region, crop, or stitching regression changes no more than 1% of channels, with each delta at most 16, the `differences.len() * 100 <= channels` check accepts the regression, and `CairoRenderer`'s exact-pixel checks do not exercise this FFI comparison, so the integration test passes and the defect goes undetected.
Restore a tighter changed-channel and per-channel-delta bound so localized regressions fail the FFI test.
Also at `crates/rofd-ffi/tests/region.rs:209`.
</issue_to_address>| assert!( | ||
| differences.len() <= 32 && differences.iter().all(|v| v.2 <= 2), | ||
| "{} differing channels, max delta {}, first {:?}", | ||
| differences.len() * 100 <= channels && differences.iter().all(|v| v.2 <= 16), |
There was a problem hiding this comment.
🟡 Medium · Localized FFI defects pass
When an FFI-side region, crop, or stitching regression changes no more than 1% of channels, with each delta at most 16, the differences.len() * 100 <= channels check accepts the regression, and CairoRenderer's exact-pixel checks do not exercise this FFI comparison, so the integration test passes and the defect goes undetected.
Restore a tighter changed-channel and per-channel-delta bound so localized regressions fail the FFI test.
Also at crates/rofd-ffi/tests/region.rs:209.
Prompt for AI agents
In `crates/rofd-ffi/tests/region.rs` at line 208:
**Localized FFI defects pass**
When an FFI-side region, crop, or stitching regression changes no more than 1% of channels, with each delta at most 16, the `differences.len() * 100 <= channels` check accepts the regression, and `CairoRenderer`'s exact-pixel checks do not exercise this FFI comparison, so the integration test passes and the defect goes undetected.
Restore a tighter changed-channel and per-channel-delta bound so localized regressions fail the FFI test.
Also at `crates/rofd-ffi/tests/region.rs:209`.
YangtseSu
added a commit
to YangtseSu/rofd
that referenced
this pull request
Oct 15, 2026
Merging the workflow onto the release branch left its trigger on ci/fork-checks, so nothing ran on the branch that actually ships (0 runs for release/0.4.3-fork.1). Point push at release/* and make a release ref run strict: no --skip and no continue-on-error steps, because a release branch carries the fixes (PRs linuxdeepin#14, linuxdeepin#15) that make both upstream-red tests green, so they gate like everything else. Non-release refs, reachable only through workflow_dispatch, keep the loose mode: the two tests run as visible, non-gating steps so a branch's own regressions still fail the run.
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA. |
lzwind
approved these changes
Oct 19, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, YangtseSu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
YangtseSu
added a commit
to YangtseSu/rofd
that referenced
this pull request
Oct 19, 2026
…merged Upstream `main` carries the decoder budget and the region tolerance (PRs linuxdeepin#14 and linuxdeepin#15), so neither previously red test is red anywhere any more and the dev-only distinction between strict and loose checking has no purpose left. The ref matrix loses its `strict` field, the two `continue-on-error` steps go, and FORK.md describes the single mode.
YangtseSu
added a commit
to YangtseSu/rofd
that referenced
this pull request
Oct 19, 2026
…merged Upstream `main` carries the decoder budget and the region tolerance (PRs linuxdeepin#14 and linuxdeepin#15), so neither previously red test is red anywhere any more and the dev-only distinction between strict and loose checking has no purpose left. The ref matrix loses its `strict` field, the two `continue-on-error` steps go, and FORK.md describes the single mode.
YangtseSu
added a commit
to YangtseSu/rofd
that referenced
this pull request
Oct 20, 2026
…merged Upstream `main` carries the decoder budget and the region tolerance (PRs linuxdeepin#14 and linuxdeepin#15), so neither previously red test is red anywhere any more and the dev-only distinction between strict and loose checking has no purpose left. The ref matrix loses its `strict` field, the two `continue-on-error` steps go, and FORK.md describes the single mode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
real_fixture_region_matches_full_page_and_preserves_cairo_ownership(
crates/rofd-ffi/tests/region.rs) pins the anti-aliasing difference between thefull-page and the region rendering to 32 channels with a per-channel delta of at most
2. That bound only holds on the font stack it was written on:
So the test fails on a clean Debian build-dependency environment:
The cause is the one the comment already names — Cairo changes curve edge coverage when
the target extents change — so the difference is anti-aliasing only, and its magnitude
depends on the font stack (
fontconfigpicks different faces, whose outlines change howmuch coverage differs between the two target extents).
Change. Bound the difference by its share of the tile's channels (1 %) and a
per-channel delta of 16, instead of absolute numbers that only hold on one font stack. A
stitching or crop error moves whole pixels, which changes orders of magnitude more
channels with far larger deltas, so the check keeps its meaning; the exact geometric
stitching assertions stay in the renderer tests.
Verification
cargo test -p rofd-ffi --test region→ ok on Arch; instrumented probe: rect 129×71,36 636 channels, 21 differing, max delta 2
ubuntu-24.04runner with the new bound: 42 differingchannels of 36 636 (0.11 %) and max delta 8 — passes, where the old bound failed
cargo fmt --all -- --checkcleanSummary by Sourcery
Adjust region rendering diff thresholds to validate meaningful image errors without depending on font-stack-specific anti-aliasing counts.
Bug Fixes:
Enhancements: