refactor: improve ReMux logic with fallback codec combinations #67
Conversation
…opy audio and subtitle track first
Reviewer's Guide by SourceryThis pull request refactors the video merging logic to improve handling of different audio and subtitle track combinations. It introduces a new function Sequence diagram for improved ReMux video merging processsequenceDiagram
participant MergeVideo
participant ReMuxWithSourceVideo
participant FFmpeg
participant MKVPropedit
MergeVideo->>FFmpeg: Concat input videos
FFmpeg-->>MergeVideo: Return concatenated video
MergeVideo->>ReMuxWithSourceVideo: ReMux with source video
activate ReMuxWithSourceVideo
Note over ReMuxWithSourceVideo: Try codec combinations in order:
Note over ReMuxWithSourceVideo: 1. Copy audio + subtitles
Note over ReMuxWithSourceVideo: 2. FLAC audio + subtitles
Note over ReMuxWithSourceVideo: 3. Copy audio only
Note over ReMuxWithSourceVideo: 4. FLAC audio only
loop Until success or all combinations tried
ReMuxWithSourceVideo->>FFmpeg: Try current codec combination
FFmpeg-->>ReMuxWithSourceVideo: Return result
alt Success
ReMuxWithSourceVideo-->>MergeVideo: Return success
else Failure
Note over ReMuxWithSourceVideo: Try next combination
end
end
deactivate ReMuxWithSourceVideo
MergeVideo->>MKVPropedit: Remove tags
MKVPropedit-->>MergeVideo: Return final video
State diagram for ReMux codec fallback processstateDiagram-v2
[*] --> TryCopyBoth
TryCopyBoth: Try Copy Audio + Subtitles
TryFlacBoth: Try FLAC Audio + Subtitles
TryCopyAudio: Try Copy Audio Only
TryFlacAudio: Try FLAC Audio Only
Failed: All Combinations Failed
Success: ReMux Successful
TryCopyBoth --> Success: Success
TryCopyBoth --> TryFlacBoth: Failure
TryFlacBoth --> Success: Success
TryFlacBoth --> TryCopyAudio: Failure
TryCopyAudio --> Success: Success
TryCopyAudio --> TryFlacAudio: Failure
TryFlacAudio --> Success: Success
TryFlacAudio --> Failed: Failure
Success --> [*]
Failed --> [*]
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:
- There appears to be a typo in the function name 'ClaerTempFile' - should be 'ClearTempFile'
- Consider making the commit message more descriptive of the actual changes, e.g., 'refactor: implement fallback codec combinations for more robust video remuxing'
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
| @@ -108,3 +71,46 @@ func MergeVideo(originFile string, inputFiles []string, outputPath string) error | |||
| log.Logger.Infof("Remove tags output: %s", out) | |||
| return nil | |||
| } | |||
|
|
|||
| // ReMuxWithSourceVideo 使用 ffmpeg 和原始视频进行 remux | |||
| func ReMuxWithSourceVideo(originPath string, outputPath string, concatOutputPath string) error { | |||
There was a problem hiding this comment.
issue: Missing subtitle stream mapping in some codec combinations
When using codec combinations that include '-c:s copy', you should also include '-map 0:s' to ensure subtitles are properly mapped from the source video. Without this, subtitles might be lost even when attempting to copy them.
| @@ -10,7 +10,7 @@ | |||
| ) | |||
|
|
|||
| // MergeVideo 使用 ffmpeg 进行视频合并，使用 mkvpropedit 清除 tags | |||
| func MergeVideo(originFile string, inputFiles []string, outputPath string) error { | |||
| func MergeVideo(originPath string, inputFiles []string, outputPath string) error { | |||
| listPath := "temp_list.txt" // 写入文件列表 | |||
There was a problem hiding this comment.
issue (bug_risk): Use of hardcoded temporary file paths could cause conflicts in concurrent operations
Consider generating unique temporary file paths using ioutil.TempFile or similar to avoid potential conflicts when multiple instances run simultaneously.
| @@ -108,3 +71,46 @@ | |||
| log.Logger.Infof("Remove tags output: %s", out) | |||
| return nil | |||
| } | |||
There was a problem hiding this comment.
issue (typo): Typo in function name 'ClaerTempFile'
Suggested implementation:
_ = util.ClearTempFile(outputPath)Note: You'll also need to ensure that the function is actually named ClearTempFile in the util package. If the function definition also has the typo, you'll need to fix it there as well in the util package.
…opy audio and subtitle track first
|
@sourcery-ai review |
|
@sourcery-ai summary |
There was a problem hiding this comment.
Hey @Tohrusky - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
module/ffmpeg/merge.go
Outdated
| _ = util.ClearTempFile(outputPath) | ||
| } | ||
|
|
||
| return fmt.Errorf("all codec combinations failed") |
There was a problem hiding this comment.
suggestion: Consider collecting and including individual codec errors in the final error message for better debugging
This would help diagnose why all attempts failed instead of just indicating that they did.
Suggested implementation:
// Log the error and try the next combination
log.Logger.Errorf("Merge failed with codec combination %v: %v", codecArgs, err)
// If failed, clean up temp files which may have been created
_ = util.ClearTempFile(outputPath)
mergeErrors = append(mergeErrors, fmt.Sprintf("codec %v: %v", codecArgs, err))
}
return fmt.Errorf("all codec combinations failed:\n%s", strings.Join(mergeErrors, "\n"))func ReMuxWithSourceVideo(originPath string, outputPath string, concatOutputPath string) error {
// Collect errors from each attempt
var mergeErrors []string
// Define the different codec combinations to tryYou'll need to add "strings" to the imports if it's not already imported:
import (
"strings"
// ... other imports
)
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @Tohrusky - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
Try copy audio and subtitle track first.
Summary by Sourcery
Improve video merging logic by adding fallback codec combinations for audio and subtitles. Try copying both audio and subtitle tracks first, then fall back to FLAC for audio with subtitles, then copying only audio, and finally FLAC for audio only.