Skip to content

mtmd : add const in various places - #28307

Merged
ngxson merged 3 commits into
ggml-org:masterfrom
nobodywho-ooo:mtmd-const
Sep 3, 2026
Merged

mtmd : add const in various places#28307
ngxson merged 3 commits into
ggml-org:masterfrom
nobodywho-ooo:mtmd-const

Conversation

@madsmtm

@madsmtm madsmtm commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Overview

Mark mtmd_context as const in:

  • mtmd_bitmap_init_lazy
  • mtmd_tokenize
  • mtmd_tokenize_from_parts
  • mtmd_helper_support_video
  • mtmd_helper_bitmap_init_from_file
  • mtmd_helper_bitmap_init_from_buf
  • mtmd_helper_video_init
  • mtmd_helper_video_init_from_buf
  • mtmd_helper_model_can_chat

And mark the bitmaps and parts array pointers in mtmd_tokenize and mtmd_tokenize_from_parts as const.

The tokenization functions in particular are useful to have marked const, as that allows more easily telling the compiler that we can safely tokenize from multiple threads (mtmd_tokenize is already documented as thread-safe, this just reifies that in the signature). Having the array pointers be const allows more easily calling these with immutable arrays / vectors.

CC @ngxson

Additional information

This would be useful for the llama.cpp Rust bindings, as it would allow us to make various methods use the correct Rust &T or &mut T references as necessary.

Requirements

Mark `mtmd_context` as `const` in:
- mtmd_bitmap_init_lazy
- mtmd_tokenize
- mtmd_tokenize_from_parts
- mtmd_helper_support_video
- mtmd_helper_bitmap_init_from_file
- mtmd_helper_bitmap_init_from_buf
- mtmd_helper_video_init
- mtmd_helper_video_init_from_buf
- mtmd_helper_model_can_chat

The tokenization functions in particular are useful to have marked
`const`, as that allows more easily telling the compiler that we can
safely tokenize from multiple threads (`mtmd_tokenize` is already
documented as thread-safe, this just reifies that in the signature).
Mark the `bitmaps` and `parts` pointers in `mtmd_tokenize` and
`mtmd_tokenize_from_parts` as `const`. This allows more easily calling
these with immutable arrays / vectors.
@madsmtm
madsmtm requested a review from a team as a code owner September 3, 2026 09:15
@github-actions github-actions Bot added the mtmd Related to multimodal functionality (video/image/audio) label Sep 3, 2026
@ngxson

ngxson commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown
Automated code review

Review of PR #28307 (mtmd: add const in various places)

Scope: small, single-purpose API hygiene pass over tools/mtmd/mtmd.h, mtmd-helper.h and their implementations. I verified the changes statically (including g++ -fsyntax-only passes on mtmd.cpp, mtmd-helper.cpp, mtmd-cli.cpp, server-common.cpp and compiling mtmd.h as C) - all clean.

No blocking findings. The change is correct and well-scoped:

  • Every mtmd_context access inside the tokenizer path is genuinely read-only: proj_type_v() / n_embd_out() are const methods, vocab / ctx_v / ctx_a / token-marker members are only read, and the preprocessors are reached through unique_ptr members (const access still yields a non-const pointee), so everything compiles without casts.
  • ABI is unaffected: both headers are extern "C", and parameter qualifiers (const mtmd_context *, const mtmd_bitmap * const *) do not change symbol names or the calling convention.
  • Source-compatible for existing callers: const T ** to const T * const * is a valid qualification conversion in both C and C++, so all current call sites (server, mtmd-cli, tts) still compile unchanged; in-repo callers passing non-const mtmd_context * also convert implicitly.
  • The remaining non-const mtmd_context * parameters (encode/eval/batch/output-embd functions) correctly stay mutable, so the const-marking is scoped exactly right.

Will slow the review / worth a look:

  • (point 1) The PR description frames the const as reifying thread-safety in the signature. Careful: it does not. The tokenize path calls ctx->image_preproc->preprocess(...) and ctx->audio_preproc->preprocess(...), which are non-const virtuals whose pointee is not const even through a const mtmd_context * (member pointers/unique_ptrs don't propagate const). So const here is an API-documentation gesture, not a compiler-enforced guarantee that concurrent mtmd_tokenize on a shared ctx is race-free. The existing "this function is thread-safe (shared ctx)" doc claim predates this PR, so nothing to fix here, but worth keeping in mind if preprocessor implementations ever gain mutable state.
  • (point 2) mtmd_helper_model_can_chat(llama_context * lctx, ...) (mtmd-helper.cpp:1019): lctx is only passed to llama_get_model(), which already takes const llama_context *. Since this function is on the PR's own list, marking lctx const too would make the pass complete and consistent.
  • (point 3) mtmd_helper_video::mctx (mtmd-helper.cpp:523) is stored but never read anywhere in the struct - only assigned in mtmd_helper_video_init* and decode_webp_with_ffmpeg. Since you are touching this member anyway, consider either dropping it or leaving a one-line comment that it's reserved for future use; right now it looks like a dangling lifetime risk (mtmd_context outliving the mtmd_helper_video or vice versa is never enforced, and the field does nothing).

Nits:

  • (point 4) The header now mixes const mtmd_context * (bitmap helpers) and const struct mtmd_context * (video helpers, mtmd_helper_model_can_chat). The struct keyword inconsistency is pre-existing, so no action required, but pick one style if you touch these declarations again.

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

Comment on lines 522 to +523
struct mtmd_helper_video {
mtmd_context * mctx;
const mtmd_context * mctx;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually unused, so we could also just remove it. I kept it for now in case the code wants to use it in the future.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok I think, mtmd_helper_video should be thread-safe (multiple video helper can use the same mctx)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I misread it, let's remove it then

@ngxson ngxson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable. I was a bit worry if I will need to modify something inside mctx in the future, but it's true that having const make it clear which APIs are thread-safe

let's wait for bot review to see if we miss anything

@ngxson

ngxson commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

bot review:

point 1 is optional, can be a follow-up fix if you want (preprocessor must be thread-safe by design)

let's address point 2 here

@madsmtm

madsmtm commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Coolio, I've marked llama_context as const in mtmd_helper_model_can_chat as well.

@ngxson
ngxson merged commit f45576a into ggml-org:master Sep 3, 2026
17 of 26 checks passed
@madsmtm
madsmtm deleted the mtmd-const branch September 3, 2026 10:19
Te-eMster pushed a commit to Te-eMster/mx-llama.cpp that referenced this pull request Sep 4, 2026
* mtmd : mark context as const in more methods

Mark `mtmd_context` as `const` in:
- mtmd_bitmap_init_lazy
- mtmd_tokenize
- mtmd_tokenize_from_parts
- mtmd_helper_support_video
- mtmd_helper_bitmap_init_from_file
- mtmd_helper_bitmap_init_from_buf
- mtmd_helper_video_init
- mtmd_helper_video_init_from_buf
- mtmd_helper_model_can_chat

The tokenization functions in particular are useful to have marked
`const`, as that allows more easily telling the compiler that we can
safely tokenize from multiple threads (`mtmd_tokenize` is already
documented as thread-safe, this just reifies that in the signature).

* mtmd : mark tokenization input pointer as const

Mark the `bitmaps` and `parts` pointers in `mtmd_tokenize` and
`mtmd_tokenize_from_parts` as `const`. This allows more easily calling
these with immutable arrays / vectors.

* mtmd : mark llama_context as const in mtmd_helper_model_can_chat
fewtarius pushed a commit to fewtarius/CachyLLama that referenced this pull request Sep 5, 2026
* mtmd : mark context as const in more methods

Mark `mtmd_context` as `const` in:
- mtmd_bitmap_init_lazy
- mtmd_tokenize
- mtmd_tokenize_from_parts
- mtmd_helper_support_video
- mtmd_helper_bitmap_init_from_file
- mtmd_helper_bitmap_init_from_buf
- mtmd_helper_video_init
- mtmd_helper_video_init_from_buf
- mtmd_helper_model_can_chat

The tokenization functions in particular are useful to have marked
`const`, as that allows more easily telling the compiler that we can
safely tokenize from multiple threads (`mtmd_tokenize` is already
documented as thread-safe, this just reifies that in the signature).

* mtmd : mark tokenization input pointer as const

Mark the `bitmaps` and `parts` pointers in `mtmd_tokenize` and
`mtmd_tokenize_from_parts` as `const`. This allows more easily calling
these with immutable arrays / vectors.

* mtmd : mark llama_context as const in mtmd_helper_model_can_chat
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.

2 participants