mtmd: fix idefics3 preproc

[ngxson]

Overview

Fix a problem reported by @aldehir

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: fully AI-generated

[aldehir]

granite-docling-258m comparison

Looks like it works! The granite-docling-258m model is no longer looping on my images that triggered it, and it's generating the proper doctags.

Thank you!

[ngxson]

/bot review

[ggml-gh-bot]

Automated code review

Review: mtmd: fix idefics3 preproc

The change restores the reference two-step resize for idefics3 (aspect-preserving resize to image_longest_edge, then pad-free resize to a multiple of image_size) when no pixel/token limits are in play, and keeps the generic single-step path otherwise. The approach matches the CITE at the top of the function, and the reporter confirmed the loop bug is gone. Findings below.

Bugs

(point 1) tools/mtmd/mtmd-image.cpp:991-1000 - the aspect-resize step can produce a zero-sized dimension and crash. Example: longest_edge = 1456, input image 1500x1 gives (int)(1456 / 1500) = 0, and the % 2 bump keeps it at 0. img_tool::resize then calls resize_pillow, whose precompute_weights has GGML_ASSERT(inSize > 0 && outSize > 0) - so a valid (if extreme) image aborts the process. The old path guarded this (calc_size_preserved_ratio clamps to at least align_size and returns {0,0} for degenerate inputs). Clamp the short side to at least 1 before the % 2 bump, e.g. resized_size.height = std::max(1, (int)(longest_edge / aspect_ratio));.

(point 2) tools/mtmd/mtmd-image.cpp:1002-1005 - no sanity cap on the size math. grid_x * tile_size is computed from GGUF-supplied image_longest_edge and image_size; a crafted GGUF with a huge image_longest_edge makes resized_size.width + tile_size - 1 and grid_x * tile_size overflow int (UB) before any allocation. The previous path never multiplied grid counts. A cheap bound (e.g. reject image_longest_edge above some max, or int64_t for the intermediate math) closes this. Low severity since GGUF values are semi-trusted, but it is a new arithmetic surface the old code did not have.

Will slow the review

(point 3) tools/mtmd/mtmd-image.cpp:1006-1012 - the new branch uses hparams.image_resize_algo (LANCZOS for idefics3) with PAD_NONE for all three resizes, while the fall-through path below goes through slice_image, which uses image_resize_algo_rf/_ov (both default BICUBIC for idefics3). So the same preprocessor now silently switches resampling kernels depending on whether custom token limits are set. If LANCZOS everywhere is intended, either set image_resize_algo_rf/_ov in the PROJECTOR_TYPE_IDEFICS3 case in clip.cpp, or use those fields here - but pick one behavior and state why in the PR description.

(point 4) tools/mtmd/mtmd-image.cpp:1012 - the new branch derives the overview by downscaling the refined image, while the generic path derives it from the original image (slice_image resizes img directly). That is a behavioral difference between the two branches of the same preprocessor. Deriving from the refined image arguably matches the reference pipeline better, but the fall-through path (old GGUFs, custom token limits) keeps the old behavior - worth stating explicitly so a future reader does not "fix" one to match the other.

Nits

(point 5) The comment "old gguf files have no preprocessor longest size, custom token limits also need the generic size below" is dense; consider rewording so it is clear the branch requires KEY_PREPROC_IMAGE_SIZE to be present and both pixel limits to be unset.

(point 6) The whole block duplicates the tail of the generic path (slices, append, grid assignment). If this grows further, consider folding the pre-resize step into the slice_instructions/slice_image machinery instead of a parallel implementation; not required for this PR.

Scope/meta

No scope problems: single-purpose fix, no new files, no public API changes, reporter-confirmed behavior. The AI-disclosure section is filled in ("fully AI-generated") - per CONTRIBUTING.md, please make sure you can explain and defend every line of this block to a reviewer independently, and that you own debugging it if the loop issue reappears in another variant.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.