Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use mkvmerge to concat and ReMux again #68

Merged
merged 1 commit into from
Jan 6, 2025
Merged

fix: use mkvmerge to concat and ReMux again #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions module/ffmpeg/cut.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ func CutVideo(inputPath string, outputFolder string) ([]string, error) {
var scriptPath string
switch runtime.GOOS {
case OS_WINDOWS:
commandStr = fmt.Sprintf("ffmpeg -i \"%s\" -f segment -segment_format mkv -segment_time 60 -c copy -map 0:v:0 -segment_list \"%s/out.list\" \"%s/%%%%003d.mkv\"", inputPath, outputFolder, outputFolder) //nolint: lll
commandStr = fmt.Sprintf("ffmpeg -i \"%s\" -f segment -segment_format mkv -segment_time 60 -reset_timestamps 1 -c copy -map 0:v:0 -segment_list \"%s/out.list\" \"%s/%%%%003d.mkv\"", inputPath, outputFolder, outputFolder) //nolint: lll
scriptPath = "temp_script.bat"
commandStr = fmt.Sprintf("@echo off%s%s", "\r\n", commandStr)
default:
commandStr = fmt.Sprintf("ffmpeg -i \"%s\" -f segment -segment_format mkv -segment_time 60 -c copy -map 0:v:0 -segment_list \"%s/out.list\" \"%s/%%003d.mkv\"", inputPath, outputFolder, outputFolder) //nolint: lll
commandStr = fmt.Sprintf("ffmpeg -i \"%s\" -f segment -segment_format mkv -segment_time 60 -reset_timestamps 1 -c copy -map 0:v:0 -segment_list \"%s/out.list\" \"%s/%%003d.mkv\"", inputPath, outputFolder, outputFolder) //nolint: lll
scriptPath = "temp_script.sh"
commandStr = fmt.Sprintf("#!/bin/bash%s%s", "\n", commandStr)
}
Expand Down
60 changes: 33 additions & 27 deletions module/ffmpeg/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package ffmpeg

import (
"fmt"
"os"
"os/exec"

"github.com/TensoRaws/FinalRip/module/log"
Expand All @@ -11,52 +10,59 @@ import (

// MergeVideo 使用 ffmpeg 进行视频合并，使用 mkvpropedit 清除 tags
func MergeVideo(originPath string, inputFiles []string, outputPath string) error {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Eliminates temporary files and multiple conversion steps
  2. Removes the need to try different codec combinations
  3. Handles stream mapping and concatenation in a single operation
  4. Maintains the same functionality with less code and complexity

listPath := "temp_list.txt" // 写入文件列表
tempVideoConcatOutputPath := "temp_video_concat_output.mkv"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

tempVideoMergedOutputPath := "temp_video_merged_output.mkv"

// clear temp file
_ = util.ClearTempFile(listPath, tempVideoConcatOutputPath)
_ = util.ClearTempFile(tempVideoConcatOutputPath, tempVideoMergedOutputPath)
defer func(p ...string) {
log.Logger.Infof("Clear temp file %v", p)
_ = util.ClearTempFile(p...)
}(listPath, tempVideoConcatOutputPath)
}(tempVideoConcatOutputPath, tempVideoMergedOutputPath)

var listStr string
for _, file := range inputFiles {
listStr += fmt.Sprintf("file '%s'\n", file)
}
// Concat video
log.Logger.Infof("Concat video with encoded clips: %s", inputFiles)

err := os.WriteFile(listPath, []byte(listStr), 0755)
if err != nil {
log.Logger.Errorf("write list file failed: %v", err)
return err
mkvmergeArgs := []string{"-v", "-o", tempVideoConcatOutputPath}
for i, file := range inputFiles {
if i > 0 {
mkvmergeArgs = append(mkvmergeArgs, "+")
}
mkvmergeArgs = append(mkvmergeArgs, file)
}
Comment on lines +30 to 32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Concat video
log.Logger.Infof("Concat video with list: %s", listPath)
cmd := exec.Command(
"ffmpeg",
"-safe", "0",
"-f", "concat",
"-i", listPath,
"-c", "copy",
tempVideoConcatOutputPath,
)
cmd := exec.Command("mkvmerge", mkvmergeArgs...)
out, err := cmd.CombinedOutput()
if err != nil {
log.Logger.Errorf("Concat video failed: %v", err)
return err
}
log.Logger.Infof("Concat video output: %s", out)

// ReMuxWithSourceVideo
err = ReMuxWithSourceVideo(originPath, outputPath, tempVideoConcatOutputPath)
// ReMux with source video
err = ReMuxWithSourceVideo(originPath, tempVideoMergedOutputPath, tempVideoConcatOutputPath)
if err != nil {
log.Logger.Errorf("ReMux with source video failed: %v", err)
return err
}

// ReMux with mkvmerge
// !mkvmerge -o output.mkv temp_merged.mkv
log.Logger.Infof("ReMux video with mkvmerge...")
cmd = exec.Command(
"mkvmerge",
"-o", outputPath,
tempVideoMergedOutputPath,
)
out, err = cmd.CombinedOutput()
if err != nil {
log.Logger.Errorf("Re-mux video failed: %v", err)
return err
}
log.Logger.Infof("Re-mux output: %s", out)

// Remove tags with mkvpropedit
// !mkvpropedit output.mkv --tags all:
log.Logger.Infof("Remove tags with mkvpropedit...")
cmd = exec.Command(
"mkvpropedit",
Expand All @@ -73,7 +79,7 @@ func MergeVideo(originPath string, inputFiles []string, outputPath string) error
}

// ReMuxWithSourceVideo 使用 ffmpeg 和原始视频进行 remux
func ReMuxWithSourceVideo(originPath string, outputPath string, concatOutputPath string) error {
func ReMuxWithSourceVideo(originPath string, tempVideoMergedOutputPath string, concatOutputPath string) error {
// Define the different codec combinations to try
codecCombinations := [][]string{
// Try copying both audio and subtitles
Expand Down Expand Up @@ -126,7 +132,7 @@ func ReMuxWithSourceVideo(originPath string, outputPath string, concatOutputPath
"-i", concatOutputPath,
}
args = append(args, codecArgs...)
args = append(args, outputPath)
args = append(args, tempVideoMergedOutputPath)

cmd := exec.Command("ffmpeg", args...)
out, err := cmd.CombinedOutput()
Expand All @@ -140,7 +146,7 @@ func ReMuxWithSourceVideo(originPath string, outputPath string, concatOutputPath
// Log the error and try the next combination
log.Logger.Errorf("ffmpeg remux failed with codec combination %v: %v", codecArgs, err)
// If failed, clean up temp files which may have been created
_ = util.ClearTempFile(outputPath)
_ = util.ClearTempFile(tempVideoMergedOutputPath)
}

return fmt.Errorf("ffmpeg remux: all codec combinations failed")
Expand Down
Loading