feat(cpu): add Qwen3.5 0.8B video support - #697
Conversation
📝 WalkthroughWalkthroughAdds bounded short-video inference to Qwen3.5. The change includes portable H.264/MP4 decoding, video preprocessing, multimodal token and embedding support, CLI options, CMake configuration, documentation, smoke tests, and focused CPU tests. ChangesQwen3.5 video inference
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Qwen3.5 runner
participant Decoder as Portable video decoder
participant Preprocessor as Qwen3_5VideoPreprocessor
participant Tokenizer as Qwen3_5Tokenizer
participant Model as Qwen3_5Model
CLI->>Decoder: Decode bounded MP4 input
Decoder-->>CLI: Return selected RGB frames and metadata
CLI->>Preprocessor: Resize and patchify frames
Preprocessor-->>Tokenizer: Return video tensors and grid metadata
Tokenizer->>Model: Provide video tokens and multimodal inputs
Model-->>CLI: Run multimodal inference
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c1966ec to
aa8b2eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
mllm/models/qwen3_5/multimodal_qwen3_5.hpp (2)
253-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider routing
makeQwen3_5ImagePositionIdsthrough the new multimodal walker.
makeQwen3_5MultimodalPositionIdsis a superset ofmakeQwen3_5ImagePositionIdsat Lines 187-251. Both walk contiguous same-type spans, assign sequential text positions, map grid rows tot/h/woffsets, and advancecurrent_positionbystd::max(grid_h, grid_w) / spatial_merge_size. The image-only version now differs only by rejecting type 2 and by its error strings.Two near-identical walkers mean a future fix to the span or offset logic must land twice.
makeQwen3_5ImagePositionIdscan delegate to the multimodal walker with a nilvideo_grid_thw, which preserves its current behavior including the rejection of type-2 tokens.Keep this deferrable. The existing image tests at
tests/cpu/Qwen35MultimodalTest.cppLines 355-398 and 448-458 pin the image behavior and would catch a regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/qwen3_5/multimodal_qwen3_5.hpp` around lines 253 - 259, Refactor makeQwen3_5ImagePositionIds to delegate to makeQwen3_5MultimodalPositionIds with a nil video_grid_thw tensor, preserving image-only validation, type-2 rejection, position calculations, and existing error behavior. Keep the current image tests passing and avoid maintaining a second span/offset walker.
56-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one expansion helper between the image and video paths.
expandQwen3_5VideoPlaceholdersduplicatesexpandQwen3_5ImagePlaceholdersat Lines 18-48. The bodies differ only in the assigned token type (2versus1) and three message strings.This PR already generalized the injection path into
injectQwen3_5ModalityEmbeddingswith thininjectQwen3_5ImageEmbeddingsandinjectQwen3_5VideoEmbeddingswrappers. Apply the same pattern to expansion so a future fix to the expansion logic lands in one place.♻️ Proposed shared expansion helper
+inline auto expandQwen3_5ModalityPlaceholders(const std::vector<int64_t>& token_ids, int64_t modality_token_id, + const std::vector<int32_t>& modality_token_counts, int32_t modality_type, + const char* over_supply_message, const char* under_supply_message) + -> std::pair<std::vector<int64_t>, std::vector<int32_t>> { + if (modality_token_counts.empty() + || std::any_of(modality_token_counts.begin(), modality_token_counts.end(), [](int32_t count) { return count <= 0; })) { + throw std::invalid_argument("Qwen3.5 modality token counts must be non-empty and positive"); + } + std::vector<int64_t> expanded; + std::vector<int32_t> token_types; + expanded.reserve(token_ids.size() + std::accumulate(modality_token_counts.begin(), modality_token_counts.end(), 0) + - modality_token_counts.size()); + token_types.reserve(expanded.capacity()); + size_t modality_index = 0; + for (const auto token_id : token_ids) { + if (token_id != modality_token_id) { + expanded.push_back(token_id); + token_types.push_back(0); + continue; + } + if (modality_index >= modality_token_counts.size()) { throw std::invalid_argument(over_supply_message); } + expanded.insert(expanded.end(), modality_token_counts[modality_index], modality_token_id); + token_types.insert(token_types.end(), modality_token_counts[modality_index], modality_type); + ++modality_index; + } + if (modality_index != modality_token_counts.size()) { throw std::invalid_argument(under_supply_message); } + return {expanded, token_types}; +} + inline auto expandQwen3_5VideoPlaceholders(const std::vector<int64_t>& token_ids, int64_t video_token_id, const std::vector<int32_t>& video_token_counts) -> std::pair<std::vector<int64_t>, std::vector<int32_t>> { - if (video_token_counts.empty() - || std::any_of(video_token_counts.begin(), video_token_counts.end(), [](int32_t count) { return count <= 0; })) { - throw std::invalid_argument("Qwen3.5 video token counts must be non-empty and positive"); - } - std::vector<int64_t> expanded; - std::vector<int32_t> token_types; - expanded.reserve(token_ids.size() + std::accumulate(video_token_counts.begin(), video_token_counts.end(), 0) - - video_token_counts.size()); - token_types.reserve(expanded.capacity()); - size_t video_index = 0; - for (const auto token_id : token_ids) { - if (token_id != video_token_id) { - expanded.push_back(token_id); - token_types.push_back(0); - continue; - } - if (video_index >= video_token_counts.size()) { - throw std::invalid_argument("Qwen3.5 template contains more video placeholders than temporal patches"); - } - expanded.insert(expanded.end(), video_token_counts[video_index], video_token_id); - token_types.insert(token_types.end(), video_token_counts[video_index], 2); - ++video_index; - } - if (video_index != video_token_counts.size()) { - throw std::invalid_argument("Qwen3.5 template contains fewer video placeholders than temporal patches"); - } - return {expanded, token_types}; + return expandQwen3_5ModalityPlaceholders( + token_ids, video_token_id, video_token_counts, 2, + "Qwen3.5 template contains more video placeholders than temporal patches", + "Qwen3.5 template contains fewer video placeholders than temporal patches"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/qwen3_5/multimodal_qwen3_5.hpp` around lines 56 - 86, Refactor expandQwen3_5VideoPlaceholders and expandQwen3_5ImagePlaceholders to use one shared modality-placeholder expansion helper, parameterized by token type and modality-specific validation/error text. Keep the existing image and video wrappers as thin adapters, matching the shared-helper pattern used by injectQwen3_5ModalityEmbeddings and its modality wrappers.tests/cpu/Qwen35MultimodalTest.cpp (2)
400-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a video position case with an asymmetric grid.
Both video position tests use the grid
{2, 4, 4}, wheregrid_h == grid_w. That makesstd::max(grid_h, grid_w) / spatial_merge_sizeatmllm/models/qwen3_5/multimodal_qwen3_5.hppLine 339 indistinguishable from using either dimension alone.The image test at Lines 378-398 does cover an asymmetric grid, but it runs through
makeQwen3_5ImagePositionIds, not the video branch. The video branch has separate temporal-slice bookkeeping at Lines 306-316 of that file, so it needs its own asymmetric case.Add one video case with
grid_h != grid_w, for example{2, 2, 4}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cpu/Qwen35MultimodalTest.cpp` around lines 400 - 446, Add an asymmetric-grid video position test alongside BuildsTimestampSeparatedVideoPositions or MatchesPinnedTransformersVideoMropeOracle, using video_grid dimensions such as {2, 2, 4}. Update the token layout and expected temporal, height, and width position arrays to validate the video branch’s temporal-slice bookkeeping and max-dimension spatial offset when grid_h differs from grid_w.
135-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases that exercise the pixel-budget correction branches.
The three positive cases at Lines 137-139 all produce a
rounded_pixelsvalue inside[min_pixels_, max_pixels_], so none of them reaches the shrink branch or the grow branch ofsmartResize. Those branches contain the correction arithmetic I flagged inmllm/models/qwen3_5/video_preprocessor_qwen3_5.hppat Lines 241-252.Add one case above
max_pixels_and one belowmin_pixels_, and use an oddnum_framesfor at least one of them. An odd frame count is what makespadded_framesdiffer fromnum_framesand exposes the asymmetry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cpu/Qwen35MultimodalTest.cpp` around lines 135 - 142, Extend Qwen35MultimodalTest.VideoSmartResizeMatchesOfficialTotalPixelEnvelope with positive cases whose rounded pixel totals trigger both smartResize correction branches: one above max_pixels_ and one below min_pixels_. Use an odd num_frames in at least one case so padded_frames differs from num_frames, and assert the expected resized dimensions for each case.mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd brief docstrings to the new public functions.
This header introduces eight public entry points. Only
flattenNormalizedPatcheshas an explanatory comment at Lines 275-276. The others document neither their units nor their throw conditions, and several have non-obvious contracts:
convertQwen3_5I420ToRgbwrites float RGB in the range[0,255], not[0,1].resizeQwen3_5RgbLikeTorchvisionexpects[0,255]float input and returns quantizeduint8.sampleQwen3_5VideoFramesreturns source frame indices, not timestamps.smartResizetakesnum_framesfirst, thenheight, thenwidth, and returns{height, width}.Each of these functions throws
std::invalid_argumentorstd::runtime_erroron invalid input. State the units, the argument order, and the throw conditions.As per coding guidelines: "Ensure public APIs, classes, and functions have clear docstrings or comments explaining purpose, parameters, returns, and errors."
Also applies to: 47-47, 80-81, 134-136, 165-166, 189-189, 218-218, 256-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp` around lines 21 - 22, Add concise Doxygen-style comments for all eight new public functions, including convertQwen3_5I420ToRgb, resizeQwen3_5RgbLikeTorchvision, sampleQwen3_5VideoFrames, and smartResize. Document each function’s purpose, parameter units and order, return value, relevant value ranges or types, and that invalid input throws std::invalid_argument or std::runtime_error; explicitly note RGB [0,255] output, [0,255] float input with uint8 output, source frame indices, and smartResize’s num_frames/height/width order and {height,width} result.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/qwen3_5/README.md`:
- Line 28: Update the “Video decoder boundary” heading in the README from level
three to level two, unless an appropriate level-two parent heading is added
before it.
In `@examples/qwen3_5/video_decoder.cpp`:
- Around line 113-145: Update collectFrame and its caller to preserve
source-frame identity by tracking input timestamps, mapping each
SBufferInfo::uiOutYuvTimeStamp to its corresponding source index, and selecting
decoded RGB data by that mapped index rather than decoded_frames order.
Materialize selected_rgb in selected_indices order even when H.264 outputs
frames out of order, and add a fixture covering reordered decoder output.
In `@mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp`:
- Around line 241-252: Update the over-budget and under-budget beta calculations
in the surrounding video preprocessing logic to use padded_frames instead of
num_frames. Keep the existing geometry correction formulas unchanged so both
corrections remain consistent with rounded_pixels and enforce the pixel budget
after temporal padding.
- Around line 277-288: Update flattenNormalizedPatches to require
video_thwc.isContiguous() alongside its existing dtype, device, rank, and
channel validation before obtaining the raw pointer. Reject non-contiguous
tensors with the same invalid-argument path, while preserving the current dense
THWC indexing behavior for valid inputs.
- Around line 189-202: Update makeQwen3_5VideoMarkers to imbue its markers
std::ostringstream with std::locale::classic() before formatting timestamps, and
include the <locale> header so decimal output remains locale-independent.
---
Nitpick comments:
In `@mllm/models/qwen3_5/multimodal_qwen3_5.hpp`:
- Around line 253-259: Refactor makeQwen3_5ImagePositionIds to delegate to
makeQwen3_5MultimodalPositionIds with a nil video_grid_thw tensor, preserving
image-only validation, type-2 rejection, position calculations, and existing
error behavior. Keep the current image tests passing and avoid maintaining a
second span/offset walker.
- Around line 56-86: Refactor expandQwen3_5VideoPlaceholders and
expandQwen3_5ImagePlaceholders to use one shared modality-placeholder expansion
helper, parameterized by token type and modality-specific validation/error text.
Keep the existing image and video wrappers as thin adapters, matching the
shared-helper pattern used by injectQwen3_5ModalityEmbeddings and its modality
wrappers.
In `@mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp`:
- Around line 21-22: Add concise Doxygen-style comments for all eight new public
functions, including convertQwen3_5I420ToRgb, resizeQwen3_5RgbLikeTorchvision,
sampleQwen3_5VideoFrames, and smartResize. Document each function’s purpose,
parameter units and order, return value, relevant value ranges or types, and
that invalid input throws std::invalid_argument or std::runtime_error;
explicitly note RGB [0,255] output, [0,255] float input with uint8 output,
source frame indices, and smartResize’s num_frames/height/width order and
{height,width} result.
In `@tests/cpu/Qwen35MultimodalTest.cpp`:
- Around line 400-446: Add an asymmetric-grid video position test alongside
BuildsTimestampSeparatedVideoPositions or
MatchesPinnedTransformersVideoMropeOracle, using video_grid dimensions such as
{2, 2, 4}. Update the token layout and expected temporal, height, and width
position arrays to validate the video branch’s temporal-slice bookkeeping and
max-dimension spatial offset when grid_h differs from grid_w.
- Around line 135-142: Extend
Qwen35MultimodalTest.VideoSmartResizeMatchesOfficialTotalPixelEnvelope with
positive cases whose rounded pixel totals trigger both smartResize correction
branches: one above max_pixels_ and one below min_pixels_. Use an odd num_frames
in at least one case so padded_frames differs from num_frames, and assert the
expected resized dimensions for each case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c391bfe5-e0ce-482b-a5db-6550dd0be11f
⛔ Files ignored due to path filters (2)
bench_assets/qwen35_moving_square_192x128_4fps_4s.mp4is excluded by!**/*.mp4bench_assets/qwen35_moving_square_demo.gifis excluded by!**/*.gif
📒 Files selected for processing (13)
CMakeLists.txtexamples/qwen3_5/CMakeLists.txtexamples/qwen3_5/README.mdexamples/qwen3_5/main.cppexamples/qwen3_5/video_decoder.cppexamples/qwen3_5/video_decoder.hppexamples/qwen3_5/video_decoder_smoke.cppmllm/models/qwen3_5/modeling_qwen3_5.hppmllm/models/qwen3_5/multimodal_qwen3_5.hppmllm/models/qwen3_5/tokenization_qwen3_5.hppmllm/models/qwen3_5/video_preprocessor_qwen3_5.hpptests/cpu/CMakeLists.txttests/cpu/Qwen35MultimodalTest.cpp
| videos, URLs, cameras, streaming, deep-stack visual features, MTP, and | ||
| multimodal benchmark mode are not supported. | ||
|
|
||
| ### Video decoder boundary |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the heading level.
Line 28 changes from the level-one document heading to a level-three heading. Change this heading to level two, or add a level-two parent heading before it.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 28-28: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/qwen3_5/README.md` at line 28, Update the “Video decoder boundary”
heading in the README from level three to level two, unless an appropriate
level-two parent heading is added before it.
Source: Linters/SAST tools
| void collectFrame(unsigned char* const planes[3], const SBufferInfo& info, const std::vector<int32_t>& selected_indices, | ||
| int32_t& decoded_frames, int32_t& width, int32_t& height, std::vector<float>& selected_rgb, | ||
| int64_t max_decoded_pixels, int64_t max_selected_pixels) { | ||
| if (info.iBufferStatus != 1) return; | ||
| const int32_t frame_width = info.UsrData.sSystemBuffer.iWidth; | ||
| const int32_t frame_height = info.UsrData.sSystemBuffer.iHeight; | ||
| if (frame_width <= 0 || frame_height <= 0 || frame_width % 2 != 0 || frame_height % 2 != 0 || planes[0] == nullptr | ||
| || planes[1] == nullptr || planes[2] == nullptr) { | ||
| throw std::runtime_error("OpenH264 produced an invalid I420 frame"); | ||
| } | ||
| if (width == 0) { | ||
| width = frame_width; | ||
| height = frame_height; | ||
| } else if (width != frame_width || height != frame_height) { | ||
| throw std::invalid_argument("mid-stream video dimension changes are outside the bounded contract"); | ||
| } | ||
| const int64_t frame_pixels = static_cast<int64_t>(width) * height; | ||
| if (frame_pixels > max_decoded_pixels / (decoded_frames + 1)) { | ||
| throw std::invalid_argument("decoded video exceeds the configured pixel limit"); | ||
| } | ||
|
|
||
| if (std::binary_search(selected_indices.begin(), selected_indices.end(), decoded_frames)) { | ||
| const int64_t selected_pixels = static_cast<int64_t>(selected_rgb.size() / 3); | ||
| if (selected_pixels > max_selected_pixels || frame_pixels > max_selected_pixels - selected_pixels) { | ||
| throw std::invalid_argument("selected source frames exceed the configured pixel limit"); | ||
| } | ||
| const size_t old_size = selected_rgb.size(); | ||
| selected_rgb.resize(old_size + static_cast<size_t>(width) * height * 3); | ||
| models::qwen3_5::convertQwen3_5I420ToRgb( | ||
| planes[0], info.UsrData.sSystemBuffer.iStride[0], planes[1], info.UsrData.sSystemBuffer.iStride[1], planes[2], | ||
| info.UsrData.sSystemBuffer.iStride[1], width, height, selected_rgb.data() + old_size); | ||
| } | ||
| ++decoded_frames; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether short-video sampling repeats source indices to satisfy min_sampled_frames.
ast-grep outline mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp \
--match sampleQwen3_5VideoFrames --view expanded
rg -n -C 8 \
'sampleQwen3_5VideoFrames|temporal.*pad|min_sampled_frames|source_frame_indices' \
mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp \
examples/qwen3_5/video_decoder.cpp \
tests/cpu/Qwen35MultimodalTest.cppRepository: UbiquitousLearning/mllm
Length of output: 10501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sampler implementation ---'
sed -n '134,175p' mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp
printf '%s\n' '--- decoder setup, timestamp use, and collection ---'
sed -n '200,278p' examples/qwen3_5/video_decoder.cpp
printf '%s\n' '--- OpenH264 timestamp declarations/usages ---'
rg -n -C 5 'uiOutYuvTimeStamp|uiInTimestamp|SBufferInfo|DecodeFrame2|DecodeFrameNoDelay' \
examples include mllm tests 2>/dev/null || true
printf '%s\n' '--- all sampler tests and decoder call sites ---'
rg -n -C 4 'sampleQwen3_5VideoFrames|decodeH264Mp4Portable|source_frame_indices' \
mllm examples testsRepository: UbiquitousLearning/mllm
Length of output: 23850
🌐 Web query:
OpenH264 SBufferInfo uiOutYuvTimeStamp uiInTimestamp DecodeFrame2 timestamp semantics reordered output
💡 Result:
In the OpenH264 decoder API, the relationship between SBufferInfo fields and timestamp semantics during frame reordering is defined as follows: SBufferInfo Semantics - uiInBsTimeStamp: This field is intended to carry the input bitstream (BS) timestamp [1][2]. It is the responsibility of the caller to pass the presentation timestamp (PTS) of the encoded packet into this field when calling DecodeFrame2 or DecodeFrameNoDelay [3][4][5]. - uiOutYuvTimeStamp: This field contains the output YUV timestamp [1][2]. Crucially, this value is only valid and relevant when iBufferStatus is 1 (indicating a frame is ready for output) [1][6][7]. The decoder populates this field with the timestamp corresponding to the decoded YUV frame [4][5]. Timestamp Semantics and Reordering When dealing with video streams containing B-frames, the decoder must perform buffering and reordering because the decoding order of frames does not necessarily match their display (presentation) order. - Reordered Output: Because of this reordering, a single call to the decoder with a specific input packet may not result in an immediate output frame, or it may result in an output frame that corresponds to a previously supplied input timestamp [4][5]. - Tracking: The OpenH264 decoder is designed to propagate timestamps correctly through its internal reordering buffers. When a frame is finally ready to be output (iBufferStatus == 1), the decoder sets uiOutYuvTimeStamp to the correct PTS for that specific frame [3][4]. Consequently, downstream applications must rely on the uiOutYuvTimeStamp returned in the SBufferInfo structure for each output frame, rather than assuming it corresponds to the uiInBsTimeStamp of the most recently provided input packet [3][4]. Usage Recommendation - The recommended practice, as seen in implementations like FFmpeg, is to capture the uiOutYuvTimeStamp returned by the decoder upon successful decoding (iBufferStatus == 1) and assign it to the decoded AVFrame's PTS [4][5]. - In scenarios where an older OpenH264 API version is used or where the library fails to properly propagate timestamps, applications might fall back to using the input timestamp, though this may lead to incorrect presentation timing (jank) in streams with B-frames [3].
Citations:
- 1: https://github.com/cisco/openh264/wiki/TypesAndStructures
- 2: https://docs.rs/openh264-sys2/latest/openh264_sys2/type.SBufferInfo.html
- 3: https://bugzilla.mozilla.org/show_bug.cgi?id=1827703
- 4: https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libopenh264dec.c
- 5: https://ffmpeg.org/doxygen/trunk/libopenh264dec_8c_source.html
- 6: https://github.com/cisco/openh264/wiki/ISVCDecoder
- 7: https://chromium.googlesource.com/external/github.com/cisco/openh264/+/master/codec/api/wels/codec_api.h
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp")
text = path.read_text()
start = text.index("inline auto sampleQwen3_5VideoFrames")
end = text.index("\n}", start) + 2
print(text[start:end])
PY
printf '%s\n' '--- decoder function outline ---'
ast-grep outline examples/qwen3_5/video_decoder.cpp --view expanded | sed -n '1,220p'Repository: UbiquitousLearning/mllm
Length of output: 2626
🌐 Web query:
OpenH264 DecodeFrameNoDelay frame reordering uiOutYuvTimeStamp B frames documentation
💡 Result:
In the OpenH264 library, DecodeFrameNoDelay is the recommended entry point for decoding H.264 streams, specifically to handle B-frame reordering and buffering [1][2][3]. Despite the name "NoDelay," this function performs necessary buffering to manage the presentation order of frames [1][4]. The relationship between frame reordering and timestamps in OpenH264 is managed via the SBufferInfo structure [5][6]. 1. Frame Reordering: OpenH264 manages internal reordering to ensure frames are emitted in the correct presentation order, which is essential for streams containing B-frames [1][4]. Historically, relying on earlier API methods like DecodeFrame2 caused issues with B-frames, where buffers could be overwritten because the decoder did not manage the release of ready frames correctly [3]. DecodeFrameNoDelay addresses this by performing the internal logic required to release non-reference B-frames at the appropriate time [3]. 2. uiOutYuvTimeStamp: This field in SBufferInfo is intended to provide the presentation timestamp (PTS) for the decoded frame [5][6]. When a frame is ready (indicated by iBufferStatus == 1), the decoder fills this field with the corresponding output timestamp [5][6]. Implementation Note: Users of the OpenH264 library are generally expected to use the uiOutYuvTimeStamp provided by the decoder to associate the correct presentation time with the output YUV frame [1][6]. In some older or specific integrations (such as older versions of Firefox's OpenH264 plugin), this timestamp was not correctly propagated, causing video playback issues (jank) because the system defaulted to using the input timestamp instead [7]. Developers integrating OpenH264 should ensure they utilize this output timestamp to maintain proper A/V synchronization, as the decoder's output order may differ from the input (bitstream) order due to B-frame reordering [1][8].
Citations:
- 1: https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libopenh264dec.c
- 2: https://github.com/cisco/openh264/wiki/ISVCDecoder
- 3: Decoded B-frame buffer can be overwritten by next decoding frame cisco/openh264#3079
- 4: https://ffmpeg.org/doxygen/trunk/libopenh264dec_8c_source.html
- 5: https://github.com/cisco/openh264/wiki/TypesAndStructures
- 6: https://docs.rs/openh264-sys2/latest/openh264_sys2/type.SBufferInfo.html
- 7: https://bugzilla.mozilla.org/show_bug.cgi?id=1827703
- 8: Output frames from openh264 in chronological order by DecodeFrameNoDelay cisco/openh264#3601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- local OpenH264 API/header references ---'
rg -n -C 8 'DecodeFrameNoDelay|uiOutYuvTimeStamp|uiInBsTimeStamp|iBufferStatus' . \
-g '!build/**' -g '!dist/**' -g '!node_modules/**' 2>/dev/null || true
printf '%s\n' '--- MP4 timestamp metadata and sample ordering assumptions ---'
sed -n '150,210p' examples/qwen3_5/video_decoder.cpp
rg -n -C 5 'MP4D_frame_offset|timestamp|duration_units|source_timescale' \
examples/qwen3_5 tests mllm 2>/dev/null || trueRepository: UbiquitousLearning/mllm
Length of output: 25823
Preserve decoder output identity when collecting frames.
sampleQwen3_5VideoFrames returns unique indices for clips shorter than four frames, so the short-clip repetition claim does not apply. However, collectFrame selects by decoded output ordinal and ignores SBufferInfo::uiOutYuvTimeStamp. Reordered H.264 output can associate RGB data with the wrong source_frame_indices. Track input timestamps, map each output timestamp to its source index, and materialize selected_rgb in selected_indices order. Add a reordered-output fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/qwen3_5/video_decoder.cpp` around lines 113 - 145, Update
collectFrame and its caller to preserve source-frame identity by tracking input
timestamps, mapping each SBufferInfo::uiOutYuvTimeStamp to its corresponding
source index, and selecting decoded RGB data by that mapped index rather than
decoded_frames order. Materialize selected_rgb in selected_indices order even
when H.264 outputs frames out of order, and add a fixture covering reordered
decoder output.
| inline auto makeQwen3_5VideoMarkers(const std::vector<double>& timestamps) -> std::string { | ||
| if (timestamps.empty()) { throw std::invalid_argument("Qwen3.5 video markers require timestamps"); } | ||
| std::ostringstream markers; | ||
| markers << "<|vision_start|>"; | ||
| markers << std::fixed << std::setprecision(1); | ||
| for (const double timestamp : timestamps) { | ||
| if (!std::isfinite(timestamp) || timestamp < 0.0) { | ||
| throw std::invalid_argument("Qwen3.5 video timestamps must be finite and non-negative"); | ||
| } | ||
| markers << '<' << timestamp << " seconds><|vision_start|><|video_pad|><|vision_end|>"; | ||
| } | ||
| markers << "<|vision_end|>"; | ||
| return markers.str(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find initLocal and check whether it mutates the global C++ locale.
set -euo pipefail
rg -nP --type=cpp --type=cc -C 12 '\binitLocal\s*\(' || true
fd -i 'Local' -e hpp -e cpp -e h -e cc | head -50Repository: UbiquitousLearning/mllm
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file ---'
sed -n '1,240p' mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp
printf '%s\n' '--- initLocal definitions and call sites ---'
rg -n -C 12 '\binitLocal\s*\(' . || true
printf '%s\n' '--- locale mutations ---'
rg -n -C 4 'global\(\)|locale::global|imbue\s*\(' mllm mllm-cli pymllm tests 2>/dev/null || true
printf '%s\n' '--- relevant test ---'
sed -n '150,210p' tests/cpu/Qwen35MultimodalTest.cppRepository: UbiquitousLearning/mllm
Length of output: 45735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Qwen3.5 prompt construction around marker generation ---'
sed -n '250,390p' mllm/models/qwen3_5/tokenization_qwen3_5.hpp
rg -n -C 8 'makeQwen3_5VideoMarkers|video_markers|VideoMarkers' mllm tests
printf '%s\n' '--- available locales ---'
locale -a 2>/dev/null | head -80 || true
printf '%s\n' '--- standalone C++ locale behavior probe ---'
cat <<'CPP' | c++ -std=c++17 -x c++ -o /tmp/qwen_locale_probe -
`#include` <iomanip>
`#include` <iostream>
`#include` <locale>
`#include` <sstream>
`#include` <string>
struct comma_num_put final : std::num_put<char> {
iter_type do_put(iter_type out, std::ios_base& state, char_type fill, double value) const override {
std::ostringstream classic;
classic.imbue(std::locale::classic());
classic << std::fixed << std::setprecision(1) << value;
std::string text = classic.str();
for (char& c : text) {
if (c == '.') c = ',';
}
for (char c : text) *out++ = c;
return out;
}
};
std::string format_without_imbue(double value) {
std::ostringstream stream;
stream << std::fixed << std::setprecision(1) << value;
return stream.str();
}
std::string format_with_classic(double value) {
std::ostringstream stream;
stream.imbue(std::locale::classic());
stream << std::fixed << std::setprecision(1) << value;
return stream.str();
}
int main() {
std::locale comma(std::locale::classic(), new comma_num_put);
std::locale::global(comma);
std::cout << "custom global: " << format_without_imbue(0.25) << '\n';
std::cout << "classic stream: " << format_with_classic(0.25) << '\n';
std::locale::global(std::locale::classic());
for (const char* name : {"en_US.UTF-8", "C.UTF-8"}) {
try {
std::locale selected(name);
std::locale::global(selected);
std::cout << name << ": " << format_without_imbue(0.25) << '\n';
} catch (const std::exception& error) {
std::cout << name << ": unavailable (" << error.what() << ")\n";
}
}
}
CPP
/tmp/qwen_locale_probe
rm -f /tmp/qwen_locale_probeRepository: UbiquitousLearning/mllm
Length of output: 14467
Make timestamp formatting locale-independent. Imbue markers with std::locale::classic() and include <locale>. A caller with a comma-decimal global locale can otherwise produce <0,2 seconds> and change tokenization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp` around lines 189 - 202,
Update makeQwen3_5VideoMarkers to imbue its markers std::ostringstream with
std::locale::classic() before formatting timestamps, and include the <locale>
header so decimal output remains locale-independent.
| const int32_t padded_frames = python_round_to_factor(num_frames, temporal_patch_size_); | ||
| const int64_t rounded_pixels = static_cast<int64_t>(padded_frames) * resized_height * resized_width; | ||
|
|
||
| if (rounded_pixels > max_pixels_) { | ||
| const double beta = std::sqrt(static_cast<double>(num_frames) * height * width / max_pixels_); | ||
| resized_height = std::max(factor, static_cast<int32_t>(std::floor(height / beta / factor)) * factor); | ||
| resized_width = std::max(factor, static_cast<int32_t>(std::floor(width / beta / factor)) * factor); | ||
| } else if (rounded_pixels < min_pixels_) { | ||
| const double beta = std::sqrt(static_cast<double>(min_pixels_) / (static_cast<double>(num_frames) * height * width)); | ||
| resized_height = std::max(factor, static_cast<int32_t>(std::ceil(height * beta / factor)) * factor); | ||
| resized_width = std::max(factor, static_cast<int32_t>(std::ceil(width * beta / factor)) * factor); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use padded_frames in the pixel-budget corrections.
Line 242 computes rounded_pixels from padded_frames, but Line 245 and Line 249 compute beta from num_frames. The two frame counts differ whenever num_frames is not a multiple of temporal_patch_size_, and sampleQwen3_5VideoFrames can produce an odd frame count.
The effect on the over-budget path: rounded_pixels exceeds max_pixels_ while num_frames * height * width does not. Then beta < 1, and std::floor(height / beta / factor) * factor returns a value larger than the current resized_height. The function enlarges the geometry instead of shrinking it, and the returned geometry still exceeds max_pixels_ after temporal padding. resizeFrames allocates from this result without re-checking, so the bounded-pixel limit does not hold for odd frame counts.
Compute both beta values from padded_frames so the correction uses the same quantity as the budget check.
🛠️ Proposed fix to align the budget check and the correction
if (rounded_pixels > max_pixels_) {
- const double beta = std::sqrt(static_cast<double>(num_frames) * height * width / max_pixels_);
+ const double beta = std::sqrt(static_cast<double>(padded_frames) * height * width / max_pixels_);
resized_height = std::max(factor, static_cast<int32_t>(std::floor(height / beta / factor)) * factor);
resized_width = std::max(factor, static_cast<int32_t>(std::floor(width / beta / factor)) * factor);
} else if (rounded_pixels < min_pixels_) {
- const double beta = std::sqrt(static_cast<double>(min_pixels_) / (static_cast<double>(num_frames) * height * width));
+ const double beta = std::sqrt(static_cast<double>(min_pixels_) / (static_cast<double>(padded_frames) * height * width));
resized_height = std::max(factor, static_cast<int32_t>(std::ceil(height * beta / factor)) * factor);
resized_width = std::max(factor, static_cast<int32_t>(std::ceil(width * beta / factor)) * factor);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const int32_t padded_frames = python_round_to_factor(num_frames, temporal_patch_size_); | |
| const int64_t rounded_pixels = static_cast<int64_t>(padded_frames) * resized_height * resized_width; | |
| if (rounded_pixels > max_pixels_) { | |
| const double beta = std::sqrt(static_cast<double>(num_frames) * height * width / max_pixels_); | |
| resized_height = std::max(factor, static_cast<int32_t>(std::floor(height / beta / factor)) * factor); | |
| resized_width = std::max(factor, static_cast<int32_t>(std::floor(width / beta / factor)) * factor); | |
| } else if (rounded_pixels < min_pixels_) { | |
| const double beta = std::sqrt(static_cast<double>(min_pixels_) / (static_cast<double>(num_frames) * height * width)); | |
| resized_height = std::max(factor, static_cast<int32_t>(std::ceil(height * beta / factor)) * factor); | |
| resized_width = std::max(factor, static_cast<int32_t>(std::ceil(width * beta / factor)) * factor); | |
| } | |
| const int32_t padded_frames = python_round_to_factor(num_frames, temporal_patch_size_); | |
| const int64_t rounded_pixels = static_cast<int64_t>(padded_frames) * resized_height * resized_width; | |
| if (rounded_pixels > max_pixels_) { | |
| const double beta = std::sqrt(static_cast<double>(padded_frames) * height * width / max_pixels_); | |
| resized_height = std::max(factor, static_cast<int32_t>(std::floor(height / beta / factor)) * factor); | |
| resized_width = std::max(factor, static_cast<int32_t>(std::floor(width / beta / factor)) * factor); | |
| } else if (rounded_pixels < min_pixels_) { | |
| const double beta = | |
| std::sqrt(static_cast<double>(min_pixels_) / (static_cast<double>(padded_frames) * height * width)); | |
| resized_height = std::max(factor, static_cast<int32_t>(std::ceil(height * beta / factor)) * factor); | |
| resized_width = std::max(factor, static_cast<int32_t>(std::ceil(width * beta / factor)) * factor); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp` around lines 241 - 252,
Update the over-budget and under-budget beta calculations in the surrounding
video preprocessing logic to use padded_frames instead of num_frames. Keep the
existing geometry correction formulas unchanged so both corrections remain
consistent with rounded_pixels and enforce the pixel budget after temporal
padding.
| [[nodiscard]] std::pair<Tensor, Tensor> flattenNormalizedPatches(const Tensor& video_thwc) const { | ||
| const auto& shape = video_thwc.shape(); | ||
| if (video_thwc.dtype() != kFloat32 || video_thwc.device() != kCPU || shape.size() != 4 || shape[3] != 3) { | ||
| throw std::invalid_argument("Qwen3.5 video preprocessor expects a float32 CPU RGB tensor in THWC layout"); | ||
| } | ||
| const int32_t frames = shape[0]; | ||
| const int32_t height = shape[1]; | ||
| const int32_t width = shape[2]; | ||
| const int32_t factor = patch_size_ * merge_size_; | ||
| if (frames <= 0 || height <= 0 || width <= 0 || height % factor != 0 || width % factor != 0) { | ||
| throw std::invalid_argument("Qwen3.5 resized video dimensions must be positive and divisible by patch_size * merge_size"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate contiguity in flattenNormalizedPatches.
Line 279 checks dtype, device, rank, and channel count, but it does not check isContiguous(). resizeFrames at Line 258 does check it. Line 297 takes a raw pointer and Line 313 indexes it with a dense THWC offset. If a caller passes a non-contiguous view, this method reads the wrong elements or reads past the buffer.
The current in-repo caller in mllm/models/qwen3_5/tokenization_qwen3_5.hpp at Line 433 passes the freshly allocated output of resizeFrames, so the fault is not reachable today. The method is public, and the tests call it directly, so add the guard to match resizeFrames.
🛡️ Proposed fix to add the contiguity guard
- if (video_thwc.dtype() != kFloat32 || video_thwc.device() != kCPU || shape.size() != 4 || shape[3] != 3) {
+ if (video_thwc.dtype() != kFloat32 || video_thwc.device() != kCPU || !video_thwc.isContiguous() || shape.size() != 4
+ || shape[3] != 3) {
throw std::invalid_argument("Qwen3.5 video preprocessor expects a float32 CPU RGB tensor in THWC layout");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[nodiscard]] std::pair<Tensor, Tensor> flattenNormalizedPatches(const Tensor& video_thwc) const { | |
| const auto& shape = video_thwc.shape(); | |
| if (video_thwc.dtype() != kFloat32 || video_thwc.device() != kCPU || shape.size() != 4 || shape[3] != 3) { | |
| throw std::invalid_argument("Qwen3.5 video preprocessor expects a float32 CPU RGB tensor in THWC layout"); | |
| } | |
| const int32_t frames = shape[0]; | |
| const int32_t height = shape[1]; | |
| const int32_t width = shape[2]; | |
| const int32_t factor = patch_size_ * merge_size_; | |
| if (frames <= 0 || height <= 0 || width <= 0 || height % factor != 0 || width % factor != 0) { | |
| throw std::invalid_argument("Qwen3.5 resized video dimensions must be positive and divisible by patch_size * merge_size"); | |
| } | |
| [[nodiscard]] std::pair<Tensor, Tensor> flattenNormalizedPatches(const Tensor& video_thwc) const { | |
| const auto& shape = video_thwc.shape(); | |
| if (video_thwc.dtype() != kFloat32 || video_thwc.device() != kCPU || !video_thwc.isContiguous() || shape.size() != 4 | |
| || shape[3] != 3) { | |
| throw std::invalid_argument("Qwen3.5 video preprocessor expects a float32 CPU RGB tensor in THWC layout"); | |
| } | |
| const int32_t frames = shape[0]; | |
| const int32_t height = shape[1]; | |
| const int32_t width = shape[2]; | |
| const int32_t factor = patch_size_ * merge_size_; | |
| if (frames <= 0 || height <= 0 || width <= 0 || height % factor != 0 || width % factor != 0) { | |
| throw std::invalid_argument("Qwen3.5 resized video dimensions must be positive and divisible by patch_size * merge_size"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mllm/models/qwen3_5/video_preprocessor_qwen3_5.hpp` around lines 277 - 288,
Update flattenNormalizedPatches to require video_thwc.isContiguous() alongside
its existing dtype, device, rank, and channel validation before obtaining the
raw pointer. Reject non-contiguous tensors with the same invalid-argument path,
while preserving the current dense THWC indexing behavior for valid inputs.
2 participants
What this PR does
Extends the existing Qwen3.5-0.8B image/multi-image CPU path with bounded
short-video input:
none/portabledecoder-backend boundary; the optionalportable H.264 MP4 reference backend uses externally supplied pinned
minimp4/OpenH264 dependencies
Qwen3-VL-compatible video patchification
and visual-embedding injection
and projected visual tokens
The decoder backend defaults to
noneand no codec source or binary isvendored or downloaded by the build. Qwen3.5 model correctness consumes
decoded RGB frames and remains independent of the optional codec backend. A
future production Android backend can use Media NDK without changing temporal
patching, video token types, or MRoPE.
End-to-end video demo
Download the exact 192x128, 4 fps H.264 MP4 fixture.
Prompt
Qwen3.5-0.8B Multimodal on OnePlus 13T
This exact-head run verified the reused model receipt, tokenizer, runtime
configuration, sampled frames, and the actually loaded runtime libraries.
Review guide
video_decoder.cpp,video_decoder.hpp,main.cppvideo_preprocessor_qwen3_5.hpp,tokenization_qwen3_5.hppmultimodal_qwen3_5.hpp,modeling_qwen3_5.hppQwen35MultimodalTest.cpp,README.mdSuggested review order: decoder/bounds → preprocessing/template → MRoPE and
embedding injection → tests and runner.
Current-head validation
Candidate HEAD:
aa8b2ebbde76a1cd2f6f3f98f0b3b0e49d81fcadMerged-main base:
d1bc93b8a171bc30cf47b134f12b3798b5e9049abuild-android,build-x86, andbuild-macospassednone/portablebuilds passed; focused + portable CTest 2/2 passed; default-path rejection and invalid-backend configure checks passed[0,2,5,7], grid[2,4,4], patch shape[32,1536], timestamps/token types/MRoPE matchedarm64-v8a; AArch64 ELF/dependency audit and 7-artifact bundle hash passedFull validation details
git diff --checkpassed; 15-path PR diff and 1,880-file committed-source manifest verified94f33a39...; GIF72052c91...; both stored underbench_assets/[0,2,5,7]; projected video tokens8ad43f44...1.1920929e-7, PASS atrtol=1e-6, atol=1e-6[0,2,4,6,9,11,13,15]; grid[4,8,12]; patch shape[384,1536]; exact-head response shown abovelibomp.so, and fixture; manifest5f959cb4...libMllmRT.so,libMllmCPUBackend.so, andlibomp.sofrom the exact-head device bundle4244d758...; tokenizer5f9e4d49...; runtime config26b1eb70...Failed predecessor roots were retained while the final H20 run used fixed
offline stdexec/RAPIDS/CPM inputs. The accepted Linux and Android roots both
carry
rc=0plus atomicdonereceipts and match the tested source manifest.How to use it
Build the optional portable/reference backend with separately supplied
dependencies:
cmake -S . -B build-video \ -DMLLM_QWEN35_VIDEO_DECODER_BACKEND=portable \ -DMLLM_QWEN35_MINIMP4_INCLUDE_DIR=/path/to/minimp4 \ -DMLLM_QWEN35_OPENH264_SOURCE_DIR=/path/to/openh264 \ -DMLLM_QWEN35_OPENH264_LIBRARIES=/path/to/libopenh264.a cmake --build build-video --target mllm-qwen3-5-runnerThen run one local video:
mllm-qwen3-5-runner \ --model_path /path/to/qwen3.5-0.8b-multimodal-w4a32-kai.mllm \ --model_version v2 \ --tokenizer_path /path/to/Qwen3.5-0.8B/tokenizer.json \ --config_path examples/qwen3_5/config_0.8B_multimodal_w4a32_kai.json \ --video_path /path/to/video.mp4 \ --prompt "What happens in this video? Answer in one short sentence." \ --max_new_tokens 48Known limits
none; the portable reference backend requires user-supplied pinned dependenciesExtends the ordered multi-image support merged in #696.
Summary by CodeRabbit
New Features
Documentation
Tests