fix: use mkvmerge to concat and ReMux again #68
Conversation
Reviewer's Guide by SourceryThis pull request refactors the video merging process to use Sequence diagram for updated video merging processsequenceDiagram
participant Client
participant MergeVideo
participant mkvmerge
participant ffmpeg
participant mkvpropedit
Client->>MergeVideo: MergeVideo(originPath, inputFiles, outputPath)
Note over MergeVideo: Clear temp files
MergeVideo->>mkvmerge: Concat video clips
mkvmerge-->>MergeVideo: tempVideoConcatOutputPath
MergeVideo->>ffmpeg: ReMuxWithSourceVideo
ffmpeg-->>MergeVideo: tempVideoMergedOutputPath
MergeVideo->>mkvmerge: Final ReMux
mkvmerge-->>MergeVideo: outputPath
MergeVideo->>mkvpropedit: Remove tags
mkvpropedit-->>MergeVideo: Clean output
MergeVideo-->>Client: Return result
Sequence diagram for video cutting processsequenceDiagram
participant Client
participant CutVideo
participant ffmpeg
Client->>CutVideo: CutVideo(inputPath, outputFolder)
Note over CutVideo: Create script based on OS
CutVideo->>ffmpeg: Execute segment command with reset_timestamps
ffmpeg-->>CutVideo: Generate segments and list
CutVideo-->>Client: Return segment files
State diagram for video processing workflowstateDiagram-v2
[*] --> CutVideo
CutVideo --> Segments: ffmpeg with reset_timestamps
Segments --> MergeVideo: Process segments
MergeVideo --> Concatenated: mkvmerge concat
Concatenated --> ReMuxed: ffmpeg remux
ReMuxed --> FinalReMux: mkvmerge remux
FinalReMux --> CleanOutput: mkvpropedit remove tags
CleanOutput --> [*]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Tohrusky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using unique temporary filenames (e.g. with UUID suffixes) to avoid potential conflicts during concurrent operations
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -11,52 +10,59 @@ import ( | |||
|
|
|||
| // MergeVideo 使用 ffmpeg 进行视频合并，使用 mkvpropedit 清除 tags | |||
| func MergeVideo(originPath string, inputFiles []string, outputPath string) error { | |||
| listPath := "temp_list.txt" // 写入文件列表 | |||
| tempVideoConcatOutputPath := "temp_video_concat_output.mkv" | |||
There was a problem hiding this comment.
🚨 suggestion (security): Consider using os.CreateTemp for temporary file paths to avoid conflicts
Hardcoded temporary file paths could cause issues with concurrent operations or in environments with restricted working directory permissions.
Suggested implementation:
import (
"fmt"
"os"
"os/exec"
"github.com/TensoRaws/FinalRip/module/log"func MergeVideo(originPath string, inputFiles []string, outputPath string) error {
// Create temporary files
tempVideoConcatOutput, err := os.CreateTemp("", "video_concat_*.mkv")
if err != nil {
return fmt.Errorf("failed to create temporary concat output file: %w", err)
}
defer os.Remove(tempVideoConcatOutput.Name())
tempVideoConcatOutputPath := tempVideoConcatOutput.Name()
tempVideoMergedOutput, err := os.CreateTemp("", "video_merged_*.mkv")
if err != nil {
return fmt.Errorf("failed to create temporary merged output file: %w", err)
}
defer os.Remove(tempVideoMergedOutput.Name())
tempVideoMergedOutputPath := tempVideoMergedOutput.Name()The rest of the function should continue using tempVideoConcatOutputPath and tempVideoMergedOutputPath as before, but now they'll point to unique temporary files that will be automatically cleaned up when the function returns (due to the defer statements).
Make sure the function properly closes the temporary file handles after creating them if they're not needed to remain open. If the rest of the code needs to write to these files, you might need to keep the file handles open longer.
| } | ||
| mkvmergeArgs = append(mkvmergeArgs, file) | ||
| } |
There was a problem hiding this comment.
suggestion: Consider checking mkvmerge exit codes more carefully as non-zero doesn't always indicate failure
mkvmerge can return non-zero exit codes for warnings while still producing valid output. Consider parsing the output to determine if it's a real error.
Suggested implementation:
mkvmergeArgs := []string{"--output", tempVideoConcatOutputPath}
for i, file := range inputFiles {
if i > 0 {
mkvmergeArgs = append(mkvmergeArgs, "+")
}
mkvmergeArgs = append(mkvmergeArgs, file)
}
cmd := exec.Command("mkvmerge", mkvmergeArgs...)
output, err := cmd.CombinedOutput()
if err != nil {
// mkvmerge exit code 1 indicates warnings but successful merge
// exit code 2 indicates fatal errors
if exitErr, ok := err.(*exec.ExitError); ok {
if exitErr.ExitCode() == 1 {
// Warning occurred but output should be valid
log.Logger.Warnf("mkvmerge completed with warnings: %s", string(output))
return nil
}
}
log.Logger.Errorf("mkvmerge failed: %v\nOutput: %s", err, string(output))
return fmt.Errorf("mkvmerge failed: %v", err)You'll need to ensure the following imports are available:
- "os/exec"
- "fmt"
Also, this assumes the code continues with executing mkvmerge after the shown snippet. If there's existing mkvmerge execution code, you'll need to modify that instead of adding the new execution code shown here.
| @@ -11,52 +10,59 @@ | |||
|
|
|||
| // MergeVideo 使用 ffmpeg 进行视频合并，使用 mkvpropedit 清除 tags | |||
| func MergeVideo(originPath string, inputFiles []string, outputPath string) error { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying video merging by using mkvmerge for both concatenation and stream mapping in a single command.
The current implementation uses three different tools and multiple conversion steps, making the code harder to maintain. The same result can be achieved using just mkvmerge for both concatenation and stream mapping, eliminating the need for ffmpeg and temporary files.
Here's a simplified approach:
func MergeVideo(originPath string, inputFiles []string, outputPath string) error {
// Build mkvmerge command to handle both concatenation and stream mapping
args := []string{
"-o", outputPath,
"--no-global-tags", // Removes tags directly
originPath, // Add original file first for audio/subtitle streams
"--no-video", // Don't take video from original file
"--audio-tracks", "all", // Keep all audio tracks
"--subtitle-tracks", "all", // Keep all subtitle tracks
}
// Add input files for concatenated video
for i, file := range inputFiles {
if i > 0 {
args = append(args, "+")
}
args = append(args, "--no-audio", "--no-subtitles", file)
}
cmd := exec.Command("mkvmerge", args...)
out, err := cmd.CombinedOutput()
if err != nil {
log.Logger.Errorf("Video merge failed: %v", err)
return err
}
log.Logger.Infof("Merge output: %s", out)
return nil
}This solution:
- Eliminates temporary files and multiple conversion steps
- Removes the need to try different codec combinations
- Handles stream mapping and concatenation in a single operation
- Maintains the same functionality with less code and complexity
Summary by Sourcery
Switch from ffmpeg to mkvmerge for video concatenation and remuxing.
Bug Fixes:
Enhancements: