rs2v shot infer: stream save video, support deploy worker, va_control…#934
rs2v shot infer: stream save video, support deploy worker, va_control…#934helloyongyang merged 1 commit intomainfrom
Conversation
…ler fix div 0 error
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to the video generation pipeline, focusing on streaming capabilities, refactoring video/audio saving, and enabling worker deployment for the RS2V inference pipeline. However, critical security vulnerabilities have been identified, as the VAController exposes the application to GStreamer pipeline injection and arbitrary file write vulnerabilities due to unvalidated user-supplied paths and URLs in system commands. These issues are particularly critical given the intended deployment as a worker service. Furthermore, the fix for a division-by-zero error in va_controller.py is flawed and can still result in a crash if record_fps is zero. There is also a potential issue with passing None to pub_livestream for the gen_video argument, which could lead to unexpected behavior.
| # run input encoder only once, get the target shape, init video recorder | ||
| rs2v.input_info = clip_input_info | ||
| rs2v.inputs = rs2v.run_input_encoder() | ||
| self.va_controller = VAController(rs2v) |
There was a problem hiding this comment.
The instantiation of VAController with rs2v (which contains unvalidated user input from clip_input_info) leads to several security vulnerabilities in the underlying components. Since this tool is intended to be deployed as a worker, these inputs are potentially attacker-controlled.
- GStreamer Pipeline Injection: In
VAReader.start_ffmpeg_process_whep(inva_reader.py), thestream_url(derived fromaudio_path) is interpolated directly into a GStreamer pipeline string:f"whep-endpoint={self.stream_url}". An attacker could provide a malicious URL containing GStreamer elements (e.g., using!separators) to manipulate the pipeline, potentially leading to arbitrary file writes (viafilesink) or other unintended behaviors. - Arbitrary File Write / Argument Injection: In
VARecorder.start_ffmpeg_process_local(inva_recorder.py), thelivestream_url(derived fromsave_result_path) is passed as an output argument toffmpeg. This allows an attacker to overwrite arbitrary files on the system or inject additionalffmpegarguments if the path starts with a dash.
It is recommended to implement strict validation and sanitization for all file paths and URLs provided by users before passing them to system commands or complex parsers.
| elif self.va_controller.recorder is not None: | ||
| video_seg = torch.clamp(video_seg, -1, 1).to(torch.float).cpu() | ||
| video_seg = vae_to_comfyui_image_inplace(video_seg) | ||
| self.va_controller.pub_livestream(video_seg, audio_seg, None) |
There was a problem hiding this comment.
Passing None as the gen_video argument to self.va_controller.pub_livestream can lead to issues. The VARecorder.buffer_stream method (which pub_livestream calls when realtime is true) stores this gen_video in its stream_buffer. Later, VARecorder.truncate_stream_buffer might attempt to return this gen value, which would be None, potentially causing NoneType errors or unexpected behavior if the caller expects a tensor. It appears gen_latents (from line 92) would be the appropriate tensor to pass here.
| self.va_controller.pub_livestream(video_seg, audio_seg, None) | |
| self.va_controller.pub_livestream(video_seg, audio_seg, gen_latents) |
| self.slice_frame = config.get("slice_frame", self.prev_frame_length) | ||
| # estimate the max infer seconds, for immediate switch with local omni | ||
| slice_interval = self.slice_frame / self.record_fps | ||
| slice_interval = max(1, self.slice_frame / self.record_fps) |
There was a problem hiding this comment.
The fix for the division by zero error is incorrect. The expression self.slice_frame / self.record_fps is evaluated before the max(1, ...) function is called. If self.record_fps is zero, a ZeroDivisionError will still be raised. To properly fix this, the max function (or another check) should be applied to the divisor to ensure it is non-zero before the division occurs.
| slice_interval = max(1, self.slice_frame / self.record_fps) | |
| slice_interval = max(1, self.slice_frame / (self.record_fps if self.record_fps > 0 else 1)) |
2 participants
rs2v shot infer: