Skip to content

args: add --video-* CLI arguments - #24318

Merged
ngxson merged 5 commits into
masterfrom
xsn/video_args
Aug 27, 2026
Merged

args: add --video-* CLI arguments#24318
ngxson merged 5 commits into
masterfrom
xsn/video_args

Conversation

@ngxson

@ngxson ngxson commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Overview

As discussed in #24269 , add these new arguments to CLI:

  • --video-fps
  • --video-timestamp-interval
  • --video-ffmpeg-dir

Also ran llama-gen-docs to sync the docs

Requirements

@ngxson
ngxson requested review from a team as code owners June 8, 2026 20:12
Comment thread tools/mtmd/mtmd-helper.h Outdated
Comment on lines +129 to +131
// note: setter is NOT thread-safe, should only be called on application startup
MTMD_API struct mtmd_helper_video_init_params mtmd_helper_video_get_default_params(void);
MTMD_API void mtmd_helper_video_set_default_params(struct mtmd_helper_video_init_params params);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO it would be cleaner if we pass the video init params to the mtmd_helper_bitmap_init_from_buf function explicitly instead of having a global set of parameters.

@ngxson ngxson Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

that would require changing quite a lot of code on llama-server, but on second thought, I think it may worth the effort

temporary changing this to draft now

@ngxson
ngxson marked this pull request as draft June 9, 2026 10:19
@github-actions github-actions Bot added documentation Improvements or additions to documentation mtmd Related to multimodal functionality (video/image/audio) labels Aug 27, 2026
@ngxson
ngxson marked this pull request as ready for review August 27, 2026 10:01
@ngxson

ngxson commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Aug 27, 2026

Copy link
Copy Markdown
Automated code review

I reviewed the diff, the surrounding arg/server/mtmd-helper code, and verified all call sites are updated. Here is my review.

Scope / quick-reject gate

  • Prior discussion exists: the PR links mtmd : add video input support #24269, which is where these args were agreed. Good.
  • Change is single-purpose and self-contained: add three --video-* CLI args, store them in common_params, and plumb a new mtmd_helper_init_opt through the mtmd helper + server tokenization paths. No unrelated changes bundled in.
  • No ggml/backend changes, no new ggml_type, no new model arch.
  • The helper header (tools/mtmd/mtmd-helper.h) already carries an explicit "not guaranteed to be stable, BREAKING CHANGES are expected" disclaimer, so the signature break on mtmd_helper_bitmap_init_from_file / mtmd_helper_bitmap_init_from_buf is acceptable for this surface.
  • All in-tree callers updated (mtmd-cli, tts, server-common, server-context). grep confirms no missed call sites.

No blocking scope issues.

Correctness

  • Lifetime of the ffmpeg_bin_dir raw pointer is sound in both consumers:
    • server_context_impl: init_opt is populated from params_base.video_ffmpeg_bin_dir.c_str() after the final params_base = params; assignment, and params_base is not reassigned afterward. The comment at server-context.cpp:797 documents this. Both are members of the same struct, so the pointer stays valid for the struct's lifetime.
    • mtmd_cli_context: it deliberately copies into a std::string video_ffmpeg_bin_dir member rather than aliasing the caller's params, so the pointer does not depend on the caller's params lifetime. Good choice.
  • decode_webp_with_ffmpeg now receives opt.video_params instead of a fresh default. For the webp (single-frame) path this is safe and is actually an improvement: fps_target is forced to 0.0f inside, timestamp_interval_ms is unused for a single frame, and ffmpeg_bin_dir is now respected for webp too. Consistent with the new --video-ffmpeg-dir semantics.
  • --video-fps uses std::stof, matching the established pattern for other float args (temp, top_p, etc.). <= 0 meaning "use native fps" is documented on the struct, so negative/zero inputs are well-defined, not a bug.
  • --video-timestamp-interval uses the int lambda variant, consistent with --mtmd-batch-max-tokens.
  • Env var names in arg.cpp match the synced docs in tools/cli/README.md and tools/server/README.md.
  • mmproj_examples (MTMD, SERVER, CLI, TTS) is the right example set for these args; SERVER is included, so the server docs entry is correct.

Security

  • No client-supplied data reaches ffmpeg_bin_dir or the video params; they come only from CLI args / env, i.e. operator-controlled config. The constructed binary path in video_resolve_bin is spawned server-side from trusted config. No attacker-controlled vector introduced.
  • No new size/count parsing from GGUF or request bodies. The init_opt is passed through but only consumed by the existing video decoding path.

Style / conventions

  • ASCII only in code and comments; no emdash/unicode arrows. Comments are concise and explain why (e.g. the "defined at the top, as it is part of mtmd_helper_init_opt" note).
  • snake_case names, 4-space indent, aligned = in common_params matches the surrounding block. Header formatting (wrapped signatures) is consistent with the rest of mtmd-helper.h.
  • mtmd_helper_init_opt is a thin extension struct currently holding only video_params, and mtmd_helper_bitmap_init_from_buf marks it GGML_UNUSED on non-video paths. This is a reasonable forward-looking extension point and the header's breaking-changes disclaimer covers it; no action needed.

Nits (optional)

(point 1) tools/server/server-context.cpp:797 - the comment says params_base "outlives this struct", but init_opt and params_base are both members of server_context_impl, so they share the same lifetime. The real invariant is that the pointer is captured after the last write to params_base and params_base is never reassigned afterward. Slightly misleading wording; not worth a respin on its own.

(point 2) tools/mtmd/mtmd-cli.cpp:91 - init_opt is declared before video_ffmpeg_bin_dir. Order is fine (assignment happens before the pointer is taken), but declaring the owning std::string first would make the ownership relationship read more naturally. Cosmetic only.

Overall: no blocking or slowing findings. The change is correct, complete, and consistent with project conventions.

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

@ngxson
ngxson merged commit f295512 into master Aug 27, 2026
28 of 30 checks passed
thecodacus pushed a commit to thecodacus/llama.cpp that referenced this pull request Sep 7, 2026
* args: add --video-* CLI arguments

* gen docs

* nits

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

Labels

documentation Improvements or additions to documentation examples mtmd Related to multimodal functionality (video/image/audio) server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants