refactor: move gen logic to frontend#40
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @Tohrusky, 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 introduces a significant architectural refactoring by shifting the complex logic for generating video processing scripts and commands from the Electron main process to the renderer process. This change centralizes configuration management and command generation within the frontend, making the application more modular and maintainable. Additionally, new features such as pause/resume functionality for video processing and more detailed control over output encoder settings have been implemented. The update also includes a version bump and various code cleanups.
Highlights
- Architectural Refactoring: The core logic for generating VapourSynth (.vpy) scripts and FFmpeg commands has been moved from the Electron main process to the renderer process. This improves separation of concerns and simplifies the main process's role.
- Standardized Configuration Management: A new shared
TaskConfiginterface and related utility functions (buildTaskConfig,getVpy,getFFmpeg) have been introduced to standardize configuration data transfer and generation. - Pause/Resume Functionality: The application now supports pausing and resuming video processing tasks, providing more control over long-running operations. This is implemented by tracking process IDs and using
pssuspend.exe. - Improved Process Execution: The method for executing external commands has been enhanced by switching from a single shell command to separate
spawnprocesses with pipedstdin/stdoutforvspipeandffmpeg, improving robustness and security. - Enhanced Output Settings UI: The UI for output settings has been refined to offer more granular control over encoder quality parameters, including new options for NVENC, AMF, and QSV hardware encoders.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request is a major refactoring that moves the generation logic for VapourSynth scripts and FFmpeg commands from the main process (backend) to the renderer process (frontend). This is a great architectural improvement that enhances separation of concerns and type safety. The changes are extensive, touching many files across the main and renderer processes, including introducing new utility files and reorganizing the state management with Pinia stores.
My review has identified a few issues: a potential crash due to a missing check for an empty file list, a resource leak in the process manager, and a couple of bugs in the VapourSynth script generation logic that would lead to incorrect behavior or invalid scripts. I've provided suggestions to fix these issues.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that moves the generation logic for Vapoursynth scripts and FFmpeg commands from the backend to the frontend. This change greatly improves the architecture by decoupling the core processing logic from the UI configuration, using a new shared TaskConfig object. The frontend state management and code organization are substantially improved, and the backend command execution is now more robust and secure. My review identifies a critical syntax error, a few potential bugs, and some areas for improving code consistency and clarity. Overall, this is an excellent enhancement to the project's maintainability and structure.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that moves the generation logic for VPY scripts and FFmpeg commands to the frontend. This is an excellent architectural improvement for an Electron application, making the main process lighter and centralizing configuration management in the renderer process using Pinia stores. The introduction of a shared TaskConfig type for IPC communication is a major enhancement for type safety. Additionally, replacing spawn with shell: true with a safer piping mechanism is a critical security and stability improvement, and the new pause/resume feature is a great addition. My review focuses on a few areas for improvement, including some minor inconsistencies, a potential security enhancement, and a couple of bugs I found in the new logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that moves the core logic for generating VapourSynth scripts and FFmpeg commands from the main process to the renderer process. This is a great architectural change that improves modularity and separation of concerns. The introduction of a shared TaskConfig type enhances type safety across IPC boundaries. Additionally, the new pause/resume functionality is a valuable feature. I've identified a few areas for improvement, mainly concerning UI consistency, code clarity, and platform compatibility, which are detailed in the comments below.
| <div class="slider-demo-block"> | ||
| <span class="demonstration">码率(M)</span> | ||
| <el-slider v-model="bitValue" :min="1" :max="100" show-input style="max-width: 500px;" /> | ||
| </div> |
There was a problem hiding this comment.
The bitrate slider is always visible, but its value is only used for lib... encoders when CRF is disabled. For hardware encoders (nvenc, amf, qsv), the generated command always uses a quality-based setting (-cq or -qp), and the bitrate value is ignored. This is misleading for the user. The bitrate slider should only be visible when it's actually applicable to the selected encoder and settings.
| /** | ||
| * 获取 VSET 生成的设置文件路径 | ||
| * 暂时存放在 config_json.outputfolder 目录下 | ||
| * @param config_json | ||
| * 暂时存放在 outputfolder 目录下 | ||
| */ |
| /** | ||
| * 获取 VSET 生成的 vpy 文件路径 | ||
| * 暂时存放在 config_json.outputfolder 目录下 | ||
| * @param config_json | ||
| * @param base_name 生成的 vpy 文件名(不含扩展名) | ||
| * 暂时存放在 outputfolder 目录下 | ||
| */ |
| export async function PauseCommand(event: IpcMainEvent, data: { isPause: boolean, vspipePID: number }): Promise<void> { | ||
| const pssuspendPath = path.join(getCorePath(), 'pssuspend.exe') | ||
| const { isPause, vspipePID } = data | ||
| const action = isPause ? '' : '-r' | ||
|
|
||
| const vspipeProc = spawn(pssuspendPath, ['-accepteula', action, vspipePID.toString()], { shell: true }) | ||
| vspipeProc.on('close', () => { | ||
| event.sender.send('ffmpeg-output', `${isPause ? '暂停' : '恢复'}完成\n`) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This new PauseCommand function uses pssuspend.exe, which is a Windows-specific utility. This will cause the application to crash or behave unexpectedly on other platforms like macOS or Linux. If cross-platform support is intended, you should add a platform check and either provide an alternative implementation for other operating systems or gracefully disable the feature.
| export async function PauseCommand(event: IpcMainEvent, data: { isPause: boolean, vspipePID: number }): Promise<void> { | |
| const pssuspendPath = path.join(getCorePath(), 'pssuspend.exe') | |
| const { isPause, vspipePID } = data | |
| const action = isPause ? '' : '-r' | |
| const vspipeProc = spawn(pssuspendPath, ['-accepteula', action, vspipePID.toString()], { shell: true }) | |
| vspipeProc.on('close', () => { | |
| event.sender.send('ffmpeg-output', `${isPause ? '暂停' : '恢复'}完成\n`) | |
| }) | |
| } | |
| export async function PauseCommand(event: IpcMainEvent, data: { isPause: boolean, vspipePID: number }): Promise<void> { | |
| if (process.platform !== 'win32') { | |
| event.sender.send('ffmpeg-output', '暂停/恢复功能仅在 Windows 上受支持。\n') | |
| return | |
| } | |
| const pssuspendPath = path.join(getCorePath(), 'pssuspend.exe') | |
| const { isPause, vspipePID } = data | |
| const action = isPause ? '' : '-r' | |
| const vspipeProc = spawn(pssuspendPath, ['-accepteula', action, vspipePID.toString()], { shell: true }) | |
| vspipeProc.on('close', () => { | |
| event.sender.send('ffmpeg-output', `${isPause ? '暂停' : '恢复'}完成\n`) | |
| }) | |
| } |
| const HevcqsvQualityValue = ref('medium') | ||
| const H264qsvQualityValue = ref('medium') | ||
|
|
||
| const videoContainer = ref('MP4') |
There was a problem hiding this comment.
The initial value for videoContainer is 'MP4', but the values in VideoContainer_options are file extensions like '.mp4'. This inconsistency will prevent the correct initial value from being displayed in the dropdown. The initial value should match one of the option values.
| const videoContainer = ref('MP4') | |
| const videoContainer = ref('.mp4') |
| } | ||
| if (RealcuganInferenceValue.value === 'TensorRt') { | ||
| vpyContent += 'device_sr=Backend.TRT()\n' | ||
| vpyContent += `device_sr.use_cuda_graph = ${String(Sr_cudagraph.value)[0].toUpperCase() + String(Sr_cudagraph.value).slice(1)}\n` |
There was a problem hiding this comment.
This method of converting a boolean to a capitalized string ('True' or 'False') is a bit complex and could be simplified for better readability and performance. A simple ternary operator would be more idiomatic here.
| vpyContent += `device_sr.use_cuda_graph = ${String(Sr_cudagraph.value)[0].toUpperCase() + String(Sr_cudagraph.value).slice(1)}\n` | |
| vpyContent += `device_sr.use_cuda_graph = ${Sr_cudagraph.value ? 'True' : 'False'}\n` |
#28
#39