Skip to content

Reject an empty mask in get_crop_region instead of returning an inverted box - #14322

Open
ErenAta16 wants to merge 1 commit into
huggingface:mainfrom
ErenAta16:fix-get-crop-region-empty-mask
Open

Reject an empty mask in get_crop_region instead of returning an inverted box#14322
ErenAta16 wants to merge 1 commit into
huggingface:mainfrom
ErenAta16:fix-get-crop-region-empty-mask

Conversation

@ErenAta16

Copy link
Copy Markdown

What does this PR do?

VaeImageProcessor.get_crop_region scans in from each edge to find the masked area. For an all-zero mask nothing ever breaks those loops, so crop_left and crop_right both reach w (and crop_top/crop_bottom reach h) and the returned box is inverted:

normal mask            -> (10, 20, 30, 40)  valid=True   size=20x20
empty mask (all zeros) -> (64, 64,  0,  0)  valid=False  size=-64x-64
empty mask, pad=8      -> (56, 56,  8,  8)  valid=False  size=-48x-48
single pixel           -> ( 5,  5,  6,  6)  valid=True   size=1x1
fully masked           -> ( 0,  0, 64, 64)  valid=True   size=64x64

The aspect-ratio expansion below operates on the inverted box without noticing, and the result reaches the caller:

PIL.Image.crop((64, 64, 0, 0))
ValueError: Coordinate 'right' is less than 'left'

Nothing in that message points back at the mask.

This is reachable from every inpaint pipeline that takes padding_mask_crop:

stable_diffusion/pipeline_stable_diffusion_inpaint.py:1163
stable_diffusion_xl/pipeline_stable_diffusion_xl_inpaint.py:1431
stable_diffusion_3/pipeline_stable_diffusion_3_inpaint.py:1195
flux/pipeline_flux_kontext_inpaint.py:1125
flux2/pipeline_flux2_klein_inpaint.py:1010
pag/pipeline_pag_sd_inpaint.py:1145
pag/pipeline_pag_sd_xl_inpaint.py:1424
pag/pipeline_pag_controlnet_sd_inpaint.py:1184

An empty mask is a plausible input there: a threshold that removed everything, or a programmatically built mask where no region ended up selected.

The change

Check mask.any() before the scan and raise with a message that names the cause and the two ways out. The empty case already failed; this makes it fail where the reason is known, and it does not change any path that previously worked.

Tests

Added to tests/others/test_image_processor.py:

  • test_get_crop_region_normal_mask — pins the ordinary result (10, 20, 30, 40)
  • test_get_crop_region_rejects_empty_mask — with pad=0 and pad=8, since padding shifts the inverted box but does not fix it
  • test_get_crop_region_single_pixel_and_full_mask_still_work — the two cases either side of empty, so the new check cannot be widened into rejecting sparse or full masks

Reverting only image_processor.py:

FAILED tests/others/test_image_processor.py::ImageProcessorTest::test_get_crop_region_rejects_empty_mask
1 failed, 2 passed

and with the change the whole file passes:

$ pytest tests/others/test_image_processor.py
13 passed

make quality (ruff check + ruff format --check) clean on both files.

Who can review?

Anyone in the community is free to review the PR once the tests have passed.

…ted box

`get_crop_region` scans in from each edge to find the masked area. For an
all-zero mask nothing ever breaks those loops, so `crop_left` and `crop_right`
both reach `w` (and `crop_top`/`crop_bottom` reach `h`) and the box comes out
inverted:

    normal mask            -> (10, 20, 30, 40)  valid=True   size=20x20
    empty mask (all zeros) -> (64, 64,  0,  0)  valid=False  size=-64x-64
    empty mask, pad=8      -> (56, 56,  8,  8)  valid=False  size=-48x-48

The aspect-ratio expansion that follows happily operates on the inverted box,
and the result travels all the way to the caller:

    PIL.Image.crop((64, 64, 0, 0))
    ValueError: Coordinate 'right' is less than 'left'

which says nothing about the mask. Eight inpaint pipelines call this whenever
`padding_mask_crop` is set, so an empty mask is a plausible input (a threshold
that removed everything, or a programmatically built mask with no region
selected).

Check for it up front and say so. The empty case already failed; this just
fails at the point that knows why.

Single-pixel and fully-masked inputs are the cases either side of it and both
still return a usable box, so the new check is added with tests pinning all
three.
@github-actions github-actions Bot added tests size/S PR with diff < 50 LOC labels Jul 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi @ErenAta16, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. Fixes #1234) to the PR description so the issue is linked. See the contribution guide for more details. If this PR intentionally does not fix a tracked issue, a maintainer can add the no-issue-needed label to silence this reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR with diff < 50 LOC tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant