Arlive#1233
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces streaming audio support for the seko_talk_ar model, implementing the SekoAROmniVAReader for audio chunking and alignment, and refactoring WanAudioRunner to support a streaming main loop. The code review feedback identifies several critical issues and improvement opportunities: handling missing control actions (such as image switching) in the streaming loop, adding a delay on audio fetch failures to prevent rapid retry crashes, using torch.from_numpy to avoid unnecessary memory copies, removing redundant global seed modifications, preventing potential crashes from negative slicing in audio preparation, and replacing unsafe assert statements with explicit exceptions for external data validation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if control.action == "blank_to_voice": | ||
| self.prev_video = control.data | ||
| elif control.action == "wait": | ||
| time.sleep(0.01) | ||
| continue |
There was a problem hiding this comment.
The loop retrieves control actions from self.va_controller.next_control(), but only handles "blank_to_voice" and "wait". Other actions like "switch_image" and "perform_action" (which are supported by the controller and handled in WanAudioRunner.run_main) are silently ignored. This creates a functional gap where image or action switching requests will not take effect.
| if control.action == "blank_to_voice": | |
| self.prev_video = control.data | |
| elif control.action == "wait": | |
| time.sleep(0.01) | |
| continue | |
| if control.action == "blank_to_voice": | |
| self.prev_video = control.data | |
| elif control.action == "switch_image": | |
| self.input_info.image_path = control.data | |
| self.inputs = self.run_input_encoder() | |
| if self.config.get("f2v_process", False): | |
| self.prev_video = self.ref_img.unsqueeze(2) | |
| else: | |
| self.prev_video = None | |
| elif control.action == "perform_action": | |
| logger.warning("perform_action is not fully supported in seko_talk_ar stream yet") | |
| elif control.action == "wait": | |
| time.sleep(0.01) | |
| continue |
| if origin_audio is None or latent_audio is None: | ||
| fail_count += 1 | ||
| logger.warning(f"Failed to get audio chunk {fail_count} times") | ||
| if fail_count > max_fail_count: | ||
| raise Exception(f"Failed to get audio chunk {fail_count} times, stop reader") | ||
| continue |
There was a problem hiding this comment.
If get_audio_segment() fails and returns None (due to a transient network or reader error), the loop immediately logs a warning and executes continue without any delay. This will cause the loop to retry 10 times in a fraction of a millisecond, exhausting max_fail_count almost instantly and crashing the runner. Adding a small sleep (e.g., time.sleep(0.1)) before continuing gives the reader/stream a chance to recover and prevents instant crashes.
| if origin_audio is None or latent_audio is None: | |
| fail_count += 1 | |
| logger.warning(f"Failed to get audio chunk {fail_count} times") | |
| if fail_count > max_fail_count: | |
| raise Exception(f"Failed to get audio chunk {fail_count} times, stop reader") | |
| continue | |
| if origin_audio is None or latent_audio is None: | |
| fail_count += 1 | |
| logger.warning(f"Failed to get audio chunk {fail_count} times") | |
| if fail_count > max_fail_count: | |
| raise Exception(f"Failed to get audio chunk {fail_count} times, stop reader") | |
| time.sleep(0.1) | |
| continue |
| origin_audio_tensor = torch.Tensor(origin_audio).float().unsqueeze(0) | ||
| self.segment = AudioSegment(origin_audio_tensor, 0, end_idx) | ||
|
|
||
| latent_audio_tensor = torch.Tensor(latent_audio).float().to(AI_DEVICE) |
There was a problem hiding this comment.
Using torch.Tensor on a NumPy array copies the underlying data. Using torch.from_numpy is more idiomatic, preserves the array's dtype, and avoids copying memory by sharing the same storage, which is highly beneficial for performance in real-time loops.
| origin_audio_tensor = torch.Tensor(origin_audio).float().unsqueeze(0) | |
| self.segment = AudioSegment(origin_audio_tensor, 0, end_idx) | |
| latent_audio_tensor = torch.Tensor(latent_audio).float().to(AI_DEVICE) | |
| origin_audio_tensor = torch.from_numpy(origin_audio).float().unsqueeze(0) | |
| self.segment = AudioSegment(origin_audio_tensor, 0, end_idx) | |
| latent_audio_tensor = torch.from_numpy(latent_audio).float().to(AI_DEVICE) |
| torch.manual_seed(seed) | ||
| generator = torch.Generator("cpu").manual_seed(seed) |
There was a problem hiding this comment.
Modifying the global PyTorch random seed with torch.manual_seed(seed) is redundant here because a local torch.Generator is already created and explicitly passed to torch.randn. Modifying the global seed can negatively impact the reproducibility of other concurrent or subsequent operations in the application.
| torch.manual_seed(seed) | |
| generator = torch.Generator("cpu").manual_seed(seed) | |
| generator = torch.Generator("cpu").manual_seed(seed) |
| start_idx, end_idx = self.get_chunk_origin_idxs(base_idx) | ||
| pad_idx = max(-start_idx, 0) | ||
| real_start = max(start_idx, 0) | ||
| real_len = min(end_idx, audio_length) - real_start | ||
| # logger.debug(f"origin audios pad_idx: {pad_idx} real_start: {real_start} real_len: {real_len}") | ||
| origin_audios[pad_idx : pad_idx + real_len] = merged_audio[real_start : real_start + real_len] |
There was a problem hiding this comment.
In prepare_ar_audios, real_len is calculated as min(end_idx, audio_length) - real_start. If audio_length is less than real_start (e.g., due to an unexpected stream truncation or short audio chunk), real_len will be negative. In Python, a negative slice index like pad_idx + real_len slices from the end of the array rather than being empty, which will lead to a ValueError when trying to assign an empty array to a non-empty slice. Clamping real_len to a minimum of 0 using max(..., 0) prevents this potential crash.
| start_idx, end_idx = self.get_chunk_origin_idxs(base_idx) | |
| pad_idx = max(-start_idx, 0) | |
| real_start = max(start_idx, 0) | |
| real_len = min(end_idx, audio_length) - real_start | |
| # logger.debug(f"origin audios pad_idx: {pad_idx} real_start: {real_start} real_len: {real_len}") | |
| origin_audios[pad_idx : pad_idx + real_len] = merged_audio[real_start : real_start + real_len] | |
| start_idx, end_idx = self.get_chunk_origin_idxs(base_idx) | |
| pad_idx = max(-start_idx, 0) | |
| real_start = max(start_idx, 0) | |
| real_len = max(min(end_idx, audio_length) - real_start, 0) | |
| # logger.debug(f"origin audios pad_idx: {pad_idx} real_start: {real_start} real_len: {real_len}") | |
| origin_audios[pad_idx : pad_idx + real_len] = merged_audio[real_start : real_start + real_len] |
| expect_count = int(self.segment_duration * self.sample_rate) | ||
| else: | ||
| expect_count = int(self.other_fetch_duration * self.sample_rate) | ||
| assert sample_count <= expect_count, f"audio length {sample_count} > expect_count {expect_count}" |
There was a problem hiding this comment.
Using assert statements for validating external stream data (like sample_count <= expect_count) is unsafe because assertions can be globally disabled in Python when run with optimization flags (e.g., python -O). If disabled, a malformed or unexpectedly long audio stream could bypass this check and cause downstream indexing or buffer overflow issues. It is safer to raise a ValueError explicitly.
| assert sample_count <= expect_count, f"audio length {sample_count} > expect_count {expect_count}" | |
| if sample_count > expect_count: | |
| raise ValueError(f"audio length {sample_count} > expect_count {expect_count}") |
No description provided.