Repository navigation
Audio from video in jinja - #49382
Audio from video in jinja #49382zucchini-nlp wants to merge 22 commits into
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
vasqu
left a comment
There was a problem hiding this comment.
Wondering whether we should wait for Guarin's PR first but overall looks nice. Its a bit messy on the git diff tho because of just moving things 😢 not on you
| sscp_num_layers = 2 | ||
| sscp_kernel, sscp_stride, sscp_padding = 3, 2, 1 |
There was a problem hiding this comment.
lets add a note that we should change this for future additions not on you but good to raise awareness cc @eustlb
Meaning that we do not hardcode values
| **kwargs, | ||
| ) | ||
|
|
||
| # TODO @guarin: move it to processor level argument. Used by EG2 and qwen-omni series |
There was a problem hiding this comment.
how much does that PR need? Maybe let's be patient and merge that first -> simpler moving here
There was a problem hiding this comment.
Hmm can we not already declare load_audio_from_video as a processor kwarg instead of video? Afaik it doesn't need defaults or anything so should already work.
Also apply_chat_template(..., load_audio_from_video, processor_kwargs,) is now weird because load_audio_from_video should be in processor_kwargs and not on its own.
There was a problem hiding this comment.
that would need us to break qwen models, where it is in video_kwargs atm. Imo it's better to keep TypedDict deprecation separate from this PR
Also apply_chat_template(..., load_audio_from_video, processor_kwargs,)
hmm yeah, but i think some kwargs are okay to be an arg of its own as long as it is used by chat_template, same as return_tensors which is consumed via jinja directly and also by processors
There was a problem hiding this comment.
Hmm but then we add it to video kwargs now just to deprecate it again. And then we have to support bc for the deprecated version.
There was a problem hiding this comment.
yeah 😢 we already have a "bad" pattern in old models so BC cycle is unevitable, no?
|
Thank you for all your work to enable this awesome feature :) Besides the OmniModalProcessorMixin inheritance issue (#49382 (comment)) and the question of where to put special tokens for EG2 (#49382 (comment)), here's a few issues I found:
if (processed_audio_tokens + num_audio_tokens_per_sec) > num_audio_tokens_total:
num_audio_tokens_per_sec = num_audio_tokens_total - processed_audio_tokens
segments.append(f"{audio_ts} {self.audio_token * num_audio_tokens_per_sec}")
processed_audio_tokens += num_audio_tokens_per_secTwo issues come up here:
To solve these, we should round frame timestamps to the nearest integer second (
|
|
@schechterh thanks for review and testing it. I addressed the edge cases you mentioned in my last PR and added BOA/EOA tokens to wrap each audio frame. For |
|
No rush here, @guarin can you make a pass here first? |
guarin
left a comment
There was a problem hiding this comment.
Super nice update, modular inheritance with OmniModelProcessorMixin works now well 👍🏼 Left some small comments, will have a closer look tomorrow
|
|
||
| def replace_video_with_audio_token( | ||
| self, video_inputs: dict, video_idx: int, audio_inputs: dict, audio_idx: int, **kwargs: VideosKwargs | ||
| ) -> str: | ||
| raise NotImplementedError() | ||
|
|
There was a problem hiding this comment.
If I understand correctly this method is not called by default from anywhere. The idea is that users override it and then call it from within replace_video_token?
There was a problem hiding this comment.
ideally we could call one of two depending on load-audio-from-video but then it forces EG2 to override all. It has additional args to check - dep on that it chooses how to add special tokens. 😿
I think since feat is yet not used widely we can keep it optional, maybe move into base class later on
| "load_audio_from_video": load_audio_from_video, | ||
| } |
There was a problem hiding this comment.
Imo no need to set load_audio_from_video here as it is already known by the caller of the method
There was a problem hiding this comment.
oh it is to pass the arg down to processor, since it is not in kwargs by default 🫠
after you pr we can try to put it in kwargs by default and deprecate the standalone arg - currently it is coming from main as standalone
|
[For maintainers] Suggested jobs to run (before merge) run-slow: embedding_gemma2, gemma4, gemma4_unified, qwen2_5_omni, qwen3_omni_moe |
CI recapDashboard: View test results in Grafana |
|
run-slow: embedding_gemma2 |
|
@vasqu should be ready - added the slow test as well. Lets see if expectations match with runners |
CI (Nvidia + AMD)Nvidia CICommit Info
Model CI Report❌ 1 new failed tests from this PR 😭
AMD CICommit Info
Model CI Report❌ 1 new failed tests from this PR 😭
|
| frame_seconds = [round(s) for s in metadata.timestamps] | ||
| num_frames = len(frame_seconds) | ||
|
|
||
| # Mirrors `max(1, int(metadata.duration * fps))` in `sample_frames` for fps=1 | ||
| total_seconds = max(1, int(metadata.duration)) | ||
|
|
||
| num_audio_tokens_per_sec = self.audio_frame_boundary(seconds=1) | ||
|
|
||
| mapped_audio_idx = video_to_audio_indices[video_idx] | ||
| audio_mask = processed_audio["input_features_mask"][mapped_audio_idx] | ||
| num_audio_tokens_total = self.compute_sscp(audio_mask) | ||
|
|
||
| # Each 1s chunk of video/audio is wrapped in special tokens: | ||
| # "00:00 <BOI> [frame image tokens] <EOI> 00:00 <BOA> [audio tokens] <EOA> ..." | ||
| segments = [] | ||
| processed_audio_tokens = 0 | ||
| for i, frame_sec in enumerate(frame_seconds): | ||
| # vision frame block, labeled with its own (rounded) timestamp | ||
| segments.append( | ||
| f"{self.timestamp_fmt(frame_sec)} {self.boi_token}{self.video_token * num_soft_tokens}{self.eoi_token}" | ||
| ) | ||
|
|
||
| # 1s audio chunks that follow this frame before the next one | ||
| if i < num_frames - 1: | ||
| audio_seconds = range(frame_sec, frame_seconds[i + 1]) | ||
| else: | ||
| audio_seconds = range(frame_sec, total_seconds) | ||
|
|
||
| for sec in audio_seconds: | ||
| n_tokens = num_audio_tokens_per_sec | ||
|
|
||
| # Clamp to account for accumulated rounding/truncation error | ||
| n_tokens = min(n_tokens, num_audio_tokens_total - processed_audio_tokens) | ||
|
|
||
| # The very last audio chunk absorbs any leftover tokens from rounding/sscp | ||
| if i == num_frames - 1 and sec == audio_seconds[-1]: | ||
| n_tokens += max(0, num_audio_tokens_total - processed_audio_tokens - n_tokens) | ||
|
|
||
| if n_tokens > 0: | ||
| segments.append( | ||
| f"{self.timestamp_fmt(sec)} {self.boa_token}{self.audio_token * n_tokens}{self.eoa_token}" | ||
| ) | ||
| processed_audio_tokens += n_tokens | ||
|
|
There was a problem hiding this comment.
I explored a bit this part and there might be an issue, LMK if I'm wrong. since we round, say at fps=2 on a 10s video the last range will be (9.5, 10) rounded so (10, 10) which is empty. in this case at the end there will be some leftover audio tokens since their range is none.
so they would need to be added before the segments are joined, like
leftover_tokens = num_audio_tokens_total - processed_audio_tokens
if leftover_tokens > 0:
# ...compute last second timestamp and add as many audio tokens as needed in segments
Models that don't have explicit format for video+audio or aren't trained for it can use pass
audioin conversation to indicate where to addaudio tokens. This works in plug and play mode for any omni modal modelOmni models with strict formatting and interleaving of audio and video per frame/chunk can inherit from
OmniModalProcessorMixinand indicate how toreplace_video_audio_token. For those models the conversation holds only a video entry and the rest is handled by processors, since model has a strict format that it's trained with!