Skip to content

mtmd: add lanczos resize method - #26341

Merged
ngxson merged 1 commit into
masterfrom
xsn/resize_lanczos
Jul 30, 2026
Merged

ngxson merged 1 commit into
masterfrom
xsn/resize_lanczos

Conversation

@ngxson

@ngxson ngxson commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Overview

Ref #13077 (comment)

Some known models will use this, so adding this ahead of times to simplify implementation of these models.

Requirements

@ngxson
ngxson requested a review from a team as a code owner July 30, 2026 19:28
@ngxson

ngxson commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Jul 30, 2026 •

Copy link
Copy Markdown
Automated code review

I've reviewed the diff (enum addition in tools/mtmd/clip-model.h and the lanczos wiring + shared resize_pillow refactor in tools/mtmd/mtmd-image.cpp), and traced the surrounding resampling code. Findings below.

Blocking

None. The implementation correctly matches Pillow's Image.LANCZOS kernel: sinc(x) * sinc(x/3) over [-3, 3), support radius 3, separable two-pass, and the fixed-point rounding matches Pillow's (int)(v +/- 0.5) (truncate toward zero). Both switch (algo) sites in img_tool are covered, so the new enum value cannot fall through to the default throw.

Will slow the review

(point 1) No test or reference exercising the new path. The existing resize_bicubic_pillow path also has no unit test, so this is consistent with the file, but because the whole point of this PR is bit-exact Pillow parity, a tiny standalone test (resize a fixed small image with RESIZE_ALGO_LANCZOS and assert against Pillow's Image.LANCZOS output bytes) would be the only real way for a reviewer to confirm correctness and guard against regressions. Consider adding one.

(point 2) Preemptive with no consumer. No model in clip.cpp sets image_resize_algo = RESIZE_ALGO_LANCZOS yet, so this is dead code in tree until a follow-up model PR lands. The linked issue (#13077) makes this acceptable, but reviewers will want the consuming model PR queued/linked so the code does not sit unused indefinitely.

Nits

(point 3) The lanczos fixed-point branch drops the std::clamp(..., int32_t::min, int32_t::max) that the bicubic branch applies. Normalized lanczos weights are bounded (peak ~1-2, so ~8e6 fxp units, far under 2.1e9), so there is no overflow, but keeping the clamp for symmetry/defensiveness would be cheap.

(point 4) The two branches now differ in rounding, and the comment "std::round would round twice" is correct, but note the bicubic branch still does exactly that: it adds +/- 0.5 and then also calls std::round, which double-rounds (e.g. a fixed-point value of 2.0 becomes 2.5 then std::round(2.5) = 3 instead of 2). That is a pre-existing deviation from Pillow in the bicubic path and is out of scope to change here (it would alter existing model outputs), but it is worth a // REVIEW_NOTE so the intentional divergence is clear: lanczos uses the correct Pillow rounding, bicubic keeps the legacy one for output stability.

(point 5) The sinc lambda is reconstructed on every invocation of resample_filter, which is called once per contributing input pixel. It is trivially cheap, but hoisting it out of resample_filter (or making it a small file-local helper) is marginally cleaner and avoids re-capturing on each call.

(point 6) Using a literal 3.141592653589793238462643383279502884 instead of M_PI is actually a reasonable portability choice here since mtmd-image.cpp does not define _USE_MATH_DEFINES (unlike mtmd-audio.cpp), so M_PI would not be available on MSVC. No change needed; flagging only so it is not "corrected" to M_PI in a later cleanup.

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

@github-actions github-actions Bot added the mtmd Related to multimodal functionality (video/image/audio) label Jul 30, 2026
@ngxson
ngxson merged commit 5f55650 into master Jul 30, 2026
20 of 26 checks passed
huaxel pushed a commit to huaxel/CachyLLama that referenced this pull request Aug 2, 2026
satindergrewal pushed a commit to satindergrewal/llama.cpp that referenced this pull request Aug 12, 2026
thecodacus pushed a commit to thecodacus/llama.cpp that referenced this pull request Sep 7, 2026
zbrad pushed a commit to zbrad/llama.cpp that referenced this pull request Sep 10, 2026
pl752 pushed a commit to pl752/llama.cpp that referenced this pull request Sep 15, 2026
frostyautumnleaf pushed a commit to frostyautumnleaf/llama.cpp that referenced this pull request Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mtmd Related to multimodal functionality (video/image/audio)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant