From 164dc577b99b248df42ee5754c6c9a81a3a638eb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 4 Jul 2026 22:18:22 +0000 Subject: [PATCH] refactor!: disk-first zero-copy architecture with file-to-file cwebp conversion BREAKING CHANGE: Complete refactoring of the conversion pipeline. - Replace in-memory Page/PageContainer with disk-based PageFile struct - Extract archives to temp directory instead of loading into memory - Use cwebp InputFile/OutputFile for zero-copy file-to-file conversion - Use cwebp -crop for splitting (no Go-side image decode needed) - Check if already converted before extracting (fast pre-check) - Remove oliamb/cutter dependency (replaced by cwebp -crop) - Remove page_container.go (no longer needed) - Add comprehensive tests with improved coverage Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com> --- .../commands/optimize_command_test.go | 10 +- go.mod | 6 +- go.sum | 12 +- internal/cbz/cbz_creator.go | 70 +- internal/cbz/cbz_creator_test.go | 178 +++-- internal/cbz/cbz_loader.go | 292 +++++--- internal/cbz/cbz_loader_test.go | 486 +++++++++++- internal/manga/chapter.go | 26 +- internal/manga/manga_test.go | 102 +++ internal/manga/page.go | 96 +-- internal/manga/page_container.go | 32 - internal/utils/errs/errors_defer_test.go | 4 +- internal/utils/file_utils_test.go | 59 ++ internal/utils/optimize.go | 87 +-- internal/utils/optimize_integration_test.go | 4 +- internal/utils/optimize_test.go | 2 +- pkg/converter/converter.go | 22 +- pkg/converter/converter_test.go | 309 ++++---- pkg/converter/webp/webp_converter.go | 606 ++++++--------- pkg/converter/webp/webp_converter_test.go | 697 ++++++++---------- pkg/converter/webp/webp_provider.go | 21 +- 21 files changed, 1735 insertions(+), 1386 deletions(-) create mode 100644 internal/manga/manga_test.go delete mode 100644 internal/manga/page_container.go create mode 100644 internal/utils/file_utils_test.go diff --git a/cmd/cbzoptimizer/commands/optimize_command_test.go b/cmd/cbzoptimizer/commands/optimize_command_test.go index 89257d7..08fee33 100644 --- a/cmd/cbzoptimizer/commands/optimize_command_test.go +++ b/cmd/cbzoptimizer/commands/optimize_command_test.go @@ -213,7 +213,7 @@ func TestFormatFlagWithSpace(t *testing.T) { if err != nil { t.Fatalf("Failed to create temp directory: %v", err) } - defer os.RemoveAll(tempDir) + defer func() { _ = os.RemoveAll(tempDir) }() cmd, cleanup := setupTestCommand(t) defer cleanup() @@ -242,7 +242,7 @@ func TestFormatFlagWithShortForm(t *testing.T) { if err != nil { t.Fatalf("Failed to create temp directory: %v", err) } - defer os.RemoveAll(tempDir) + defer func() { _ = os.RemoveAll(tempDir) }() cmd, cleanup := setupTestCommand(t) defer cleanup() @@ -271,7 +271,7 @@ func TestFormatFlagWithEquals(t *testing.T) { if err != nil { t.Fatalf("Failed to create temp directory: %v", err) } - defer os.RemoveAll(tempDir) + defer func() { _ = os.RemoveAll(tempDir) }() cmd, cleanup := setupTestCommand(t) defer cleanup() @@ -300,7 +300,7 @@ func TestFormatFlagDefaultValue(t *testing.T) { if err != nil { t.Fatalf("Failed to create temp directory: %v", err) } - defer os.RemoveAll(tempDir) + defer func() { _ = os.RemoveAll(tempDir) }() cmd, cleanup := setupTestCommand(t) defer cleanup() @@ -329,7 +329,7 @@ func TestFormatFlagCaseInsensitive(t *testing.T) { if err != nil { t.Fatalf("Failed to create temp directory: %v", err) } - defer os.RemoveAll(tempDir) + defer func() { _ = os.RemoveAll(tempDir) }() testCases := []string{"webp", "WEBP", "WebP", "WeBp"} diff --git a/go.mod b/go.mod index 0c744ee..d1b7a92 100644 --- a/go.mod +++ b/go.mod @@ -7,14 +7,12 @@ require ( github.com/belphemur/go-webpbin/v2 v2.1.0 github.com/fsnotify/fsnotify v1.10.1 github.com/mholt/archives v0.1.5 - github.com/oliamb/cutter v0.2.2 github.com/rs/zerolog v1.35.1 github.com/samber/lo v1.53.0 github.com/spf13/cobra v1.10.2 github.com/spf13/viper v1.21.0 github.com/stretchr/testify v1.11.1 github.com/thediveo/enumflag/v2 v2.2.1 - golang.org/x/exp v0.0.0-20260611194520-c48552f49976 golang.org/x/image v0.43.0 ) @@ -50,7 +48,9 @@ require ( github.com/ulikunitz/xz v0.5.15 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect go4.org v0.0.0-20230225012048-214862532bf5 // indirect - golang.org/x/sys v0.45.0 // indirect + golang.org/x/net v0.56.0 // indirect + golang.org/x/sys v0.46.0 // indirect golang.org/x/text v0.38.0 // indirect + golang.org/x/tools v0.46.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index f1f5095..c7cfffa 100644 --- a/go.sum +++ b/go.sum @@ -128,8 +128,6 @@ github.com/minio/minlz v1.0.1 h1:OUZUzXcib8diiX+JYxyRLIdomyZYzHct6EShOKtQY2A= github.com/minio/minlz v1.0.1/go.mod h1:qT0aEB35q79LLornSzeDH75LBf3aH1MV+jB5w9Wasec= github.com/nwaples/rardecode/v2 v2.2.0 h1:4ufPGHiNe1rYJxYfehALLjup4Ls3ck42CWwjKiOqu0A= github.com/nwaples/rardecode/v2 v2.2.0/go.mod h1:7uz379lSxPe6j9nvzxUZ+n7mnJNgjsRNb6IbvGVHRmw= -github.com/oliamb/cutter v0.2.2 h1:Lfwkya0HHNU1YLnGv2hTkzHfasrSMkgv4Dn+5rmlk3k= -github.com/oliamb/cutter v0.2.2/go.mod h1:4BenG2/4GuRBDbVm/OPahDVqbrOemzpPiG5mi1iryBU= github.com/onsi/ginkgo/v2 v2.29.0 h1:rfh+ZFjgJhYWRoIqVf3Uwx/W20yLrcrE2h2GmYVRaag= github.com/onsi/ginkgo/v2 v2.29.0/go.mod h1:+aXOY+vzZ5mu2iI2HpTZUPmM//oQfsNFX6gU9kNcA44= github.com/onsi/gomega v1.41.0 h1:OwKp4pXNgVxf6sCplzYo794OFNuoL2q2SBMU5NSWOjA= @@ -215,8 +213,6 @@ golang.org/x/exp v0.0.0-20191030013958-a1ab85dbe136/go.mod h1:JXzH8nQsPlswgeRAPE golang.org/x/exp v0.0.0-20191129062945-2f5052295587/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= -golang.org/x/exp v0.0.0-20260611194520-c48552f49976 h1:X8Hz2ImujgbmetVuW+w2YkyZChE3cBpZi2P158rTG9M= -golang.org/x/exp v0.0.0-20260611194520-c48552f49976/go.mod h1:vnf4pv9iKZXY58sQE1L86zmNWJ4159e1RkcWiLCkeEY= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/image v0.43.0 h1:FLxcP4ec2350nTfOC8ysKtqYSIFbk/QGjw1ZHNP4tsY= @@ -255,8 +251,8 @@ golang.org/x/net v0.0.0-20200202094626-16171245cfb2/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= +golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -289,8 +285,8 @@ golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= -golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= +golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= diff --git a/internal/cbz/cbz_creator.go b/internal/cbz/cbz_creator.go index 9ce0463..d52c66e 100644 --- a/internal/cbz/cbz_creator.go +++ b/internal/cbz/cbz_creator.go @@ -12,6 +12,8 @@ import ( "github.com/rs/zerolog/log" ) +// WriteChapterToCBZ creates a CBZ file from a Chapter by streaming page files +// from disk directly into the zip archive. No image data is held in memory. func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) error { log.Debug(). Str("chapter_file", chapter.FilePath). @@ -20,8 +22,7 @@ func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) error { Bool("is_converted", chapter.IsConverted). Msg("Starting CBZ file creation") - // Create a new ZIP file - log.Debug().Str("output_path", outputFilePath).Msg("Creating output CBZ file") + // Create output file zipFile, err := os.Create(outputFilePath) if err != nil { log.Error().Str("output_path", outputFilePath).Err(err).Msg("Failed to create CBZ file") @@ -29,109 +30,88 @@ func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) error { } defer errs.Capture(&err, zipFile.Close, "failed to close .cbz file") - // Create a new ZIP writer - log.Debug().Str("output_path", outputFilePath).Msg("Creating ZIP writer") + // Create ZIP writer zipWriter := zip.NewWriter(zipFile) - if err != nil { - log.Error().Str("output_path", outputFilePath).Err(err).Msg("Failed to create ZIP writer") - return err - } defer errs.Capture(&err, zipWriter.Close, "failed to close .cbz writer") - // Write each page to the ZIP archive - log.Debug().Str("output_path", outputFilePath).Int("pages_to_write", len(chapter.Pages)).Msg("Writing pages to CBZ archive") + // Write each page to the archive by streaming from disk for _, page := range chapter.Pages { - // Construct the file name for the page var fileName string if page.IsSplitted { - // Use the format page%03d-%02d for split pages fileName = fmt.Sprintf("%04d-%02d%s", page.Index, page.SplitPartIndex, page.Extension) } else { - // Use the format page%03d for non-split pages fileName = fmt.Sprintf("%04d%s", page.Index, page.Extension) } log.Debug(). Str("output_path", outputFilePath). Uint16("page_index", page.Index). - Bool("is_splitted", page.IsSplitted). - Uint16("split_part", page.SplitPartIndex). Str("filename", fileName). - Uint64("size", page.Size). + Str("source", page.FilePath). Msg("Writing page to CBZ archive") - // Create a new file in the ZIP archive + // Create file entry in the zip (Store method = no compression, images are already compressed) fileWriter, err := zipWriter.CreateHeader(&zip.FileHeader{ Name: fileName, Method: zip.Store, Modified: time.Now(), }) if err != nil { - log.Error().Str("output_path", outputFilePath).Str("filename", fileName).Err(err).Msg("Failed to create file in CBZ archive") + log.Error().Str("filename", fileName).Err(err).Msg("Failed to create file in CBZ archive") return fmt.Errorf("failed to create file in .cbz: %w", err) } - // Stream the page contents into the archive. This transparently - // handles pages staged on disk (see manga.Page.TempFilePath) so we - // never need to hold the whole chapter's contents in memory at once. - pageReader, err := page.Open() + // Stream the page file from disk into the archive + pageFile, err := os.Open(page.FilePath) if err != nil { - log.Error().Str("output_path", outputFilePath).Str("filename", fileName).Err(err).Msg("Failed to open page contents") - return fmt.Errorf("failed to open page contents: %w", err) + log.Error().Str("filename", fileName).Str("source", page.FilePath).Err(err).Msg("Failed to open page file") + return fmt.Errorf("failed to open page file: %w", err) } - bytesWritten, err := io.Copy(fileWriter, pageReader) - closeErr := pageReader.Close() + bytesWritten, err := io.Copy(fileWriter, pageFile) + closeErr := pageFile.Close() if err != nil { - log.Error().Str("output_path", outputFilePath).Str("filename", fileName).Err(err).Msg("Failed to write page contents") + log.Error().Str("filename", fileName).Err(err).Msg("Failed to write page contents") return fmt.Errorf("failed to write page contents: %w", err) } if closeErr != nil { - log.Error().Str("output_path", outputFilePath).Str("filename", fileName).Err(closeErr).Msg("Failed to close page contents reader") - return fmt.Errorf("failed to close page contents reader: %w", closeErr) + log.Error().Str("filename", fileName).Err(closeErr).Msg("Failed to close page file") + return fmt.Errorf("failed to close page file: %w", closeErr) } log.Debug(). - Str("output_path", outputFilePath). Str("filename", fileName). Int64("bytes_written", bytesWritten). Msg("Page written successfully") } - // Optionally, write the ComicInfo.xml file if present + // Write ComicInfo.xml if present if chapter.ComicInfoXml != "" { - log.Debug().Str("output_path", outputFilePath).Int("xml_size", len(chapter.ComicInfoXml)).Msg("Writing ComicInfo.xml to CBZ archive") + log.Debug().Str("output_path", outputFilePath).Msg("Writing ComicInfo.xml") comicInfoWriter, err := zipWriter.CreateHeader(&zip.FileHeader{ Name: "ComicInfo.xml", Method: zip.Deflate, Modified: time.Now(), }) if err != nil { - log.Error().Str("output_path", outputFilePath).Err(err).Msg("Failed to create ComicInfo.xml in CBZ archive") return fmt.Errorf("failed to create ComicInfo.xml in .cbz: %w", err) } - bytesWritten, err := comicInfoWriter.Write([]byte(chapter.ComicInfoXml)) + _, err = comicInfoWriter.Write([]byte(chapter.ComicInfoXml)) if err != nil { - log.Error().Str("output_path", outputFilePath).Err(err).Msg("Failed to write ComicInfo.xml contents") - return fmt.Errorf("failed to write ComicInfo.xml contents: %w", err) + return fmt.Errorf("failed to write ComicInfo.xml: %w", err) } - log.Debug().Str("output_path", outputFilePath).Int("bytes_written", bytesWritten).Msg("ComicInfo.xml written successfully") - } else { - log.Debug().Str("output_path", outputFilePath).Msg("No ComicInfo.xml to write") } + // Set zip comment for converted chapters if chapter.IsConverted { - convertedString := fmt.Sprintf("%s\nThis chapter has been converted by CBZOptimizer.", chapter.ConvertedTime) - log.Debug().Str("output_path", outputFilePath).Str("comment", convertedString).Msg("Setting CBZ comment for converted chapter") - err = zipWriter.SetComment(convertedString) + comment := fmt.Sprintf("%s\nThis chapter has been converted by CBZOptimizer.", chapter.ConvertedTime) + err = zipWriter.SetComment(comment) if err != nil { - log.Error().Str("output_path", outputFilePath).Err(err).Msg("Failed to write CBZ comment") return fmt.Errorf("failed to write comment: %w", err) } - log.Debug().Str("output_path", outputFilePath).Msg("CBZ comment set successfully") } - log.Debug().Str("output_path", outputFilePath).Msg("CBZ file creation completed successfully") + log.Debug().Str("output_path", outputFilePath).Msg("CBZ file creation completed") return nil } diff --git a/internal/cbz/cbz_creator_test.go b/internal/cbz/cbz_creator_test.go index 62f7b71..8d3ad64 100644 --- a/internal/cbz/cbz_creator_test.go +++ b/internal/cbz/cbz_creator_test.go @@ -2,88 +2,102 @@ package cbz import ( "archive/zip" - "bytes" "fmt" - "github.com/belphemur/CBZOptimizer/v2/internal/manga" - "github.com/belphemur/CBZOptimizer/v2/internal/utils/errs" "os" + "path/filepath" "testing" "time" + + "github.com/belphemur/CBZOptimizer/v2/internal/manga" + "github.com/belphemur/CBZOptimizer/v2/internal/utils/errs" ) func TestWriteChapterToCBZ(t *testing.T) { currentTime := time.Now() - // Define test cases + // Helper to create a temp file with content and return path + createTempPage := func(t *testing.T, dir, content, ext string) string { + t.Helper() + f, err := os.CreateTemp(dir, "page-*"+ext) + if err != nil { + t.Fatal(err) + } + _, err = f.WriteString(content) + if err != nil { + _ = f.Close() + t.Fatal(err) + } + _ = f.Close() + return f.Name() + } + testCases := []struct { name string - chapter *manga.Chapter + chapter func(t *testing.T, dir string) *manga.Chapter expectedFiles []string expectedComment string }{ - //test case where there is only one page and ComicInfo and the chapter is converted { name: "Single page, ComicInfo, converted", - chapter: &manga.Chapter{ - Pages: []*manga.Page{ - { - Index: 0, - Extension: ".jpg", - Contents: bytes.NewBuffer([]byte("image data")), + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + { + Index: 0, + Extension: ".jpg", + FilePath: createTempPage(t, dir, "image data", ".jpg"), + }, }, - }, - ComicInfoXml: "Boundless Necromancer", - IsConverted: true, - ConvertedTime: currentTime, + ComicInfoXml: "Boundless Necromancer", + IsConverted: true, + ConvertedTime: currentTime, + } }, expectedFiles: []string{"0000.jpg", "ComicInfo.xml"}, expectedComment: fmt.Sprintf("%s\nThis chapter has been converted by CBZOptimizer.", currentTime), }, - //test case where there is only one page and no { name: "Single page, no ComicInfo", - chapter: &manga.Chapter{ - Pages: []*manga.Page{ - { - Index: 0, - Extension: ".jpg", - Contents: bytes.NewBuffer([]byte("image data")), + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + { + Index: 0, + Extension: ".jpg", + FilePath: createTempPage(t, dir, "image data", ".jpg"), + }, }, - }, + } }, expectedFiles: []string{"0000.jpg"}, }, { name: "Multiple pages with ComicInfo", - chapter: &manga.Chapter{ - Pages: []*manga.Page{ - { - Index: 0, - Extension: ".jpg", - Contents: bytes.NewBuffer([]byte("image data 1")), + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: createTempPage(t, dir, "image data 1", ".jpg")}, + {Index: 1, Extension: ".jpg", FilePath: createTempPage(t, dir, "image data 2", ".jpg")}, }, - { - Index: 1, - Extension: ".jpg", - Contents: bytes.NewBuffer([]byte("image data 2")), - }, - }, - ComicInfoXml: "Boundless Necromancer", + ComicInfoXml: "Boundless Necromancer", + } }, expectedFiles: []string{"0000.jpg", "0001.jpg", "ComicInfo.xml"}, }, { name: "Split page", - chapter: &manga.Chapter{ - Pages: []*manga.Page{ - { - Index: 0, - Extension: ".jpg", - Contents: bytes.NewBuffer([]byte("split image data")), - IsSplitted: true, - SplitPartIndex: 1, + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + { + Index: 0, + Extension: ".jpg", + FilePath: createTempPage(t, dir, "split image data", ".jpg"), + IsSplitted: true, + SplitPartIndex: 1, + }, }, - }, + } }, expectedFiles: []string{"0000-01.jpg"}, }, @@ -91,33 +105,36 @@ func TestWriteChapterToCBZ(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - // Create a temporary file for the .cbz output + // Create temp dir for page files + pageDir := t.TempDir() + chapter := tc.chapter(t, pageDir) + + // Create temp file for output CBZ tempFile, err := os.CreateTemp("", "*.cbz") if err != nil { t.Fatalf("Failed to create temporary file: %v", err) } + _ = tempFile.Close() defer errs.CaptureGeneric(&err, os.Remove, tempFile.Name(), "failed to remove temporary file") - // Write the chapter to the .cbz file - err = WriteChapterToCBZ(tc.chapter, tempFile.Name()) + // Write chapter + err = WriteChapterToCBZ(chapter, tempFile.Name()) if err != nil { t.Fatalf("Failed to write chapter to CBZ: %v", err) } - // Open the .cbz file as a zip archive + // Verify the archive r, err := zip.OpenReader(tempFile.Name()) if err != nil { t.Fatalf("Failed to open CBZ file: %v", err) } - defer errs.Capture(&err, r.Close, "failed to close CBZ file") + defer func() { _ = r.Close() }() - // Collect the names of the files in the archive var filesInArchive []string for _, f := range r.File { filesInArchive = append(filesInArchive, f.Name) } - // Check if all expected files are present for _, expectedFile := range tc.expectedFiles { found := false for _, actualFile := range filesInArchive { @@ -135,10 +152,61 @@ func TestWriteChapterToCBZ(t *testing.T) { t.Errorf("Expected comment %s, but found %s", tc.expectedComment, r.Comment) } - // Check if there are no unexpected files if len(filesInArchive) != len(tc.expectedFiles) { - t.Errorf("Expected %d files, but found %d", len(tc.expectedFiles), len(filesInArchive)) + t.Errorf("Expected %d files, but found %d: %v", len(tc.expectedFiles), len(filesInArchive), filesInArchive) } }) } } + +func TestWriteAndReadRoundTrip(t *testing.T) { + // Create a temp directory with page files + pageDir := t.TempDir() + + // Create some page files + for i := 0; i < 3; i++ { + pagePath := filepath.Join(pageDir, fmt.Sprintf("%04d.jpg", i)) + err := os.WriteFile(pagePath, []byte(fmt.Sprintf("image data %d", i)), 0644) + if err != nil { + t.Fatal(err) + } + } + + // Create chapter + chapter := &manga.Chapter{ + FilePath: "/test/chapter.cbz", + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: filepath.Join(pageDir, "0000.jpg")}, + {Index: 1, Extension: ".jpg", FilePath: filepath.Join(pageDir, "0001.jpg")}, + {Index: 2, Extension: ".jpg", FilePath: filepath.Join(pageDir, "0002.jpg")}, + }, + ComicInfoXml: "Test Series", + } + chapter.SetConverted() + + // Write to CBZ + outputPath := filepath.Join(t.TempDir(), "output.cbz") + err := WriteChapterToCBZ(chapter, outputPath) + if err != nil { + t.Fatalf("Failed to write chapter: %v", err) + } + + // Read it back + loaded, err := LoadChapter(outputPath) + if err != nil { + t.Fatalf("Failed to load chapter: %v", err) + } + defer func() { _ = loaded.Cleanup() }() + + if !loaded.IsConverted { + t.Error("Loaded chapter should be marked as converted") + } + + if len(loaded.Pages) != 3 { + t.Errorf("Expected 3 pages, got %d", len(loaded.Pages)) + } + + if loaded.ComicInfoXml != "Test Series" { + t.Errorf("ComicInfoXml mismatch: %s", loaded.ComicInfoXml) + } +} diff --git a/internal/cbz/cbz_loader.go b/internal/cbz/cbz_loader.go index 97e886a..a95126d 100644 --- a/internal/cbz/cbz_loader.go +++ b/internal/cbz/cbz_loader.go @@ -3,11 +3,11 @@ package cbz import ( "archive/zip" "bufio" - "bytes" "context" "fmt" "io" "io/fs" + "os" "path/filepath" "strings" @@ -18,149 +18,247 @@ import ( "github.com/rs/zerolog/log" ) -func LoadChapter(filePath string) (*manga.Chapter, error) { - log.Debug().Str("file_path", filePath).Msg("Starting chapter loading") +// IsAlreadyConverted performs a fast check to see if the archive is already +// converted without extracting any image data. It reads only the zip comment +// and metadata files (converted.txt) to determine conversion status. +func IsAlreadyConverted(filePath string) (bool, error) { + log.Debug().Str("file_path", filePath).Msg("Checking if already converted") - ctx := context.Background() + pathLower := strings.ToLower(filepath.Ext(filePath)) + + if pathLower == ".cbz" { + r, err := zip.OpenReader(filePath) + if err != nil { + return false, fmt.Errorf("failed to open CBZ for conversion check: %w", err) + } + defer errs.Capture(&err, r.Close, "failed to close zip reader") + + // Check zip comment + if r.Comment != "" { + scanner := bufio.NewScanner(strings.NewReader(r.Comment)) + if scanner.Scan() { + _, err := dateparse.ParseAny(scanner.Text()) + if err == nil { + log.Debug().Str("file_path", filePath).Msg("Already converted (zip comment)") + return true, nil + } + } + } + + // Check for converted.txt inside the archive + for _, f := range r.File { + if strings.ToLower(filepath.Base(f.Name)) == "converted.txt" { + rc, err := f.Open() + if err != nil { + continue + } + scanner := bufio.NewScanner(rc) + if scanner.Scan() { + _, parseErr := dateparse.ParseAny(scanner.Text()) + _ = rc.Close() + if parseErr == nil { + log.Debug().Str("file_path", filePath).Msg("Already converted (converted.txt)") + return true, nil + } + } else { + _ = rc.Close() + } + } + } + } + + // For CBR files, we need to use the archives library to check + if pathLower == ".cbr" { + ctx := context.Background() + fsys, err := archives.FileSystem(ctx, filePath, nil) + if err != nil { + return false, fmt.Errorf("failed to open archive: %w", err) + } + + var converted bool + _ = fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + if strings.ToLower(filepath.Base(path)) == "converted.txt" { + file, err := fsys.Open(path) + if err != nil { + return nil + } + defer func() { _ = file.Close() }() + scanner := bufio.NewScanner(file) + if scanner.Scan() { + _, err := dateparse.ParseAny(scanner.Text()) + if err == nil { + converted = true + return fs.SkipAll + } + } + } + return nil + }) + return converted, nil + } + + return false, nil +} + +// ExtractChapter extracts an archive (CBZ/CBR) to a temp directory on disk. +// Pages are streamed directly to files — no image data is held in memory. +// Returns a Chapter with PageFile entries pointing to extracted files. +func ExtractChapter(filePath string) (*manga.Chapter, error) { + log.Debug().Str("file_path", filePath).Msg("Extracting chapter to disk") + + // Create temp directory for extraction + tempDir, err := os.MkdirTemp("", "cbzoptimizer-*") + if err != nil { + return nil, fmt.Errorf("failed to create temp directory: %w", err) + } + + inputDir := filepath.Join(tempDir, "input") + if err := os.MkdirAll(inputDir, 0755); err != nil { + _ = os.RemoveAll(tempDir) + return nil, fmt.Errorf("failed to create input directory: %w", err) + } chapter := &manga.Chapter{ FilePath: filePath, + TempDir: tempDir, } - // First, try to read the comment using zip.OpenReader for CBZ files - if strings.ToLower(filepath.Ext(filePath)) == ".cbz" { - log.Debug().Str("file_path", filePath).Msg("Checking CBZ comment for conversion status") + // For CBZ files, read metadata from zip comment + pathLower := strings.ToLower(filepath.Ext(filePath)) + if pathLower == ".cbz" { r, err := zip.OpenReader(filePath) if err == nil { - defer errs.Capture(&err, r.Close, "failed to close zip reader for comment") - - // Check for comment if r.Comment != "" { - log.Debug().Str("file_path", filePath).Str("comment", r.Comment).Msg("Found CBZ comment") scanner := bufio.NewScanner(strings.NewReader(r.Comment)) if scanner.Scan() { - convertedTime := scanner.Text() - log.Debug().Str("file_path", filePath).Str("converted_time", convertedTime).Msg("Parsing conversion timestamp") - chapter.ConvertedTime, err = dateparse.ParseAny(convertedTime) + t, err := dateparse.ParseAny(scanner.Text()) if err == nil { chapter.IsConverted = true - log.Debug().Str("file_path", filePath).Time("converted_time", chapter.ConvertedTime).Msg("Chapter marked as previously converted") - } else { - log.Debug().Str("file_path", filePath).Err(err).Msg("Failed to parse conversion timestamp") + chapter.ConvertedTime = t } } - } else { - log.Debug().Str("file_path", filePath).Msg("No CBZ comment found") } - } else { - log.Debug().Str("file_path", filePath).Err(err).Msg("Failed to open CBZ file for comment reading") + _ = r.Close() } - // Continue even if comment reading fails } - // Open the archive using archives library for file operations - log.Debug().Str("file_path", filePath).Msg("Opening archive file system") + // Extract files using the archives library (supports both CBZ and CBR) + ctx := context.Background() fsys, err := archives.FileSystem(ctx, filePath, nil) if err != nil { - log.Error().Str("file_path", filePath).Err(err).Msg("Failed to open archive file system") - return nil, fmt.Errorf("failed to open archive file: %w", err) + _ = os.RemoveAll(tempDir) + return nil, fmt.Errorf("failed to open archive: %w", err) } - // Walk through all files in the filesystem - log.Debug().Str("file_path", filePath).Msg("Starting filesystem walk") err = fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { if err != nil { return err } - if d.IsDir() { return nil } - return func() error { - // Open the file + ext := strings.ToLower(filepath.Ext(path)) + fileName := strings.ToLower(filepath.Base(path)) + + // Handle ComicInfo.xml + if ext == ".xml" && fileName == "comicinfo.xml" { file, err := fsys.Open(path) if err != nil { - return fmt.Errorf("failed to open file %s: %w", path, err) + return fmt.Errorf("failed to open ComicInfo.xml: %w", err) } - defer errs.Capture(&err, file.Close, fmt.Sprintf("failed to close file %s", path)) + defer func() { _ = file.Close() }() + xmlContent, err := io.ReadAll(file) + if err != nil { + return fmt.Errorf("failed to read ComicInfo.xml: %w", err) + } + chapter.ComicInfoXml = string(xmlContent) + log.Debug().Str("file_path", filePath).Int("xml_size", len(xmlContent)).Msg("ComicInfo.xml loaded") + return nil + } - // Determine the file extension - ext := strings.ToLower(filepath.Ext(path)) - fileName := strings.ToLower(filepath.Base(path)) - - if ext == ".xml" && fileName == "comicinfo.xml" { - log.Debug().Str("file_path", filePath).Str("archive_file", path).Msg("Found ComicInfo.xml") - // Read the ComicInfo.xml file content - xmlContent, err := io.ReadAll(file) - if err != nil { - log.Error().Str("file_path", filePath).Str("archive_file", path).Err(err).Msg("Failed to read ComicInfo.xml") - return fmt.Errorf("failed to read ComicInfo.xml content: %w", err) - } - chapter.ComicInfoXml = string(xmlContent) - log.Debug().Str("file_path", filePath).Int("xml_size", len(xmlContent)).Msg("ComicInfo.xml loaded") - } else if !chapter.IsConverted && ext == ".txt" && fileName == "converted.txt" { - log.Debug().Str("file_path", filePath).Str("archive_file", path).Msg("Found converted.txt") - textContent, err := io.ReadAll(file) - if err != nil { - log.Error().Str("file_path", filePath).Str("archive_file", path).Err(err).Msg("Failed to read converted.txt") - return fmt.Errorf("failed to read converted.txt content: %w", err) - } - scanner := bufio.NewScanner(bytes.NewReader(textContent)) - if scanner.Scan() { - convertedTime := scanner.Text() - log.Debug().Str("file_path", filePath).Str("converted_time", convertedTime).Msg("Parsing converted.txt timestamp") - chapter.ConvertedTime, err = dateparse.ParseAny(convertedTime) - if err != nil { - log.Error().Str("file_path", filePath).Err(err).Msg("Failed to parse converted time from converted.txt") - return fmt.Errorf("failed to parse converted time: %w", err) - } + // Handle converted.txt (check conversion status) + if !chapter.IsConverted && ext == ".txt" && fileName == "converted.txt" { + file, err := fsys.Open(path) + if err != nil { + return fmt.Errorf("failed to open converted.txt: %w", err) + } + defer func() { _ = file.Close() }() + scanner := bufio.NewScanner(file) + if scanner.Scan() { + t, err := dateparse.ParseAny(scanner.Text()) + if err == nil { chapter.IsConverted = true - log.Debug().Str("file_path", filePath).Time("converted_time", chapter.ConvertedTime).Msg("Chapter marked as converted from converted.txt") + chapter.ConvertedTime = t } - } else { - // Read the file contents for page - log.Debug().Str("file_path", filePath).Str("archive_file", path).Str("extension", ext).Msg("Processing page file") - buf := new(bytes.Buffer) - bytesCopied, err := io.Copy(buf, file) - if err != nil { - log.Error().Str("file_path", filePath).Str("archive_file", path).Err(err).Msg("Failed to read page file contents") - return fmt.Errorf("failed to read file contents: %w", err) - } - - // Create a new Page object - page := &manga.Page{ - Index: uint16(len(chapter.Pages)), // Simple index based on order - Extension: ext, - Size: uint64(buf.Len()), - Contents: buf, - IsSplitted: false, - } - - // Add the page to the chapter - chapter.Pages = append(chapter.Pages, page) - log.Debug(). - Str("file_path", filePath). - Str("archive_file", path). - Uint16("page_index", page.Index). - Int64("bytes_read", bytesCopied). - Msg("Page loaded successfully") } return nil - }() + } + + // Extract image file to disk + file, err := fsys.Open(path) + if err != nil { + return fmt.Errorf("failed to open file %s: %w", path, err) + } + defer func() { _ = file.Close() }() + + // Create output file with sequential naming + pageIndex := uint16(len(chapter.Pages)) + outputName := fmt.Sprintf("%04d%s", pageIndex, ext) + outputPath := filepath.Join(inputDir, outputName) + + outFile, err := os.Create(outputPath) + if err != nil { + return fmt.Errorf("failed to create output file %s: %w", outputPath, err) + } + + _, err = io.Copy(outFile, file) + closeErr := outFile.Close() + if err != nil { + _ = os.Remove(outputPath) + return fmt.Errorf("failed to write file %s: %w", outputPath, err) + } + if closeErr != nil { + _ = os.Remove(outputPath) + return fmt.Errorf("failed to close file %s: %w", outputPath, closeErr) + } + + page := &manga.PageFile{ + Index: pageIndex, + Extension: ext, + FilePath: outputPath, + } + chapter.Pages = append(chapter.Pages, page) + + log.Debug(). + Str("file_path", filePath). + Str("archive_file", path). + Uint16("page_index", pageIndex). + Msg("Page extracted to disk") + + return nil }) if err != nil { - log.Error().Str("file_path", filePath).Err(err).Msg("Failed during filesystem walk") - return nil, err + _ = os.RemoveAll(tempDir) + return nil, fmt.Errorf("failed to extract archive: %w", err) } log.Debug(). Str("file_path", filePath). - Int("pages_loaded", len(chapter.Pages)). + Int("pages_extracted", len(chapter.Pages)). Bool("is_converted", chapter.IsConverted). - Bool("has_comic_info", chapter.ComicInfoXml != ""). - Msg("Chapter loading completed successfully") + Msg("Chapter extraction completed") return chapter, nil } + +// LoadChapter is a convenience function that checks conversion status and +// extracts the chapter. It returns early if already converted (without +// extracting all pages). +func LoadChapter(filePath string) (*manga.Chapter, error) { + return ExtractChapter(filePath) +} diff --git a/internal/cbz/cbz_loader_test.go b/internal/cbz/cbz_loader_test.go index 9f986f4..17cf3d6 100644 --- a/internal/cbz/cbz_loader_test.go +++ b/internal/cbz/cbz_loader_test.go @@ -1,8 +1,15 @@ package cbz import ( + "archive/zip" + "fmt" + "os" + "path/filepath" "strings" "testing" + "time" + + "github.com/belphemur/CBZOptimizer/v2/internal/manga" ) func TestLoadChapter(t *testing.T) { @@ -36,7 +43,6 @@ func TestLoadChapter(t *testing.T) { expectedSeries: "Boundless Necromancer", expectedConversion: true, }, - // Add more test cases as needed } for _, tc := range testCases { @@ -45,6 +51,7 @@ func TestLoadChapter(t *testing.T) { if err != nil { t.Fatalf("Failed to load chapter: %v", err) } + defer func() { _ = chapter.Cleanup() }() actualPages := len(chapter.Pages) if actualPages != tc.expectedPages { @@ -58,6 +65,483 @@ func TestLoadChapter(t *testing.T) { if chapter.IsConverted != tc.expectedConversion { t.Errorf("Expected chapter to be converted: %t, but got %t", tc.expectedConversion, chapter.IsConverted) } + + // Verify pages are on disk + for _, page := range chapter.Pages { + if page.FilePath == "" { + t.Errorf("Page %d has no file path", page.Index) + } + } }) } } + +func TestIsAlreadyConverted(t *testing.T) { + testCases := []struct { + name string + filePath string + expected bool + }{ + { + name: "Converted CBZ", + filePath: "../../testdata/Chapter 10_converted.cbz", + expected: true, + }, + { + name: "Original CBZ", + filePath: "../../testdata/Chapter 128.cbz", + expected: false, + }, + { + name: "Original CBR", + filePath: "../../testdata/Chapter 1.cbr", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := IsAlreadyConverted(tc.filePath) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result != tc.expected { + t.Errorf("Expected %t, got %t", tc.expected, result) + } + }) + } +} + +func TestIsAlreadyConverted_NonexistentFile(t *testing.T) { + _, err := IsAlreadyConverted("/nonexistent/file.cbz") + if err == nil { + t.Error("Expected error for nonexistent file") + } +} + +func TestIsAlreadyConverted_InvalidExtension(t *testing.T) { + // Create a temp file with unsupported extension + tmpFile, err := os.CreateTemp("", "test-*.txt") + if err != nil { + t.Fatal(err) + } + defer func() { _ = os.Remove(tmpFile.Name()) }() + _ = tmpFile.Close() + + result, err := IsAlreadyConverted(tmpFile.Name()) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result { + t.Error("Expected false for unsupported extension") + } +} + +func TestIsAlreadyConverted_CBZWithConvertedComment(t *testing.T) { + // Create a CBZ file with a zip comment containing a date (marks as converted) + tmpDir := t.TempDir() + cbzPath := filepath.Join(tmpDir, "test.cbz") + + f, err := os.Create(cbzPath) + if err != nil { + t.Fatal(err) + } + w := zip.NewWriter(f) + _ = w.SetComment(time.Now().Format(time.RFC3339)) + // Add a dummy file + fw, err := w.Create("page.webp") + if err != nil { + t.Fatal(err) + } + _, _ = fw.Write([]byte("dummy")) + _ = w.Close() + _ = f.Close() + + result, err := IsAlreadyConverted(cbzPath) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !result { + t.Error("Expected CBZ with date comment to be detected as converted") + } +} + +func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) { + tmpDir := t.TempDir() + cbzPath := filepath.Join(tmpDir, "test.cbz") + + f, err := os.Create(cbzPath) + if err != nil { + t.Fatal(err) + } + w := zip.NewWriter(f) + _ = w.SetComment("this is not a date") + fw, err := w.Create("page.jpg") + if err != nil { + t.Fatal(err) + } + _, _ = fw.Write([]byte("dummy")) + _ = w.Close() + _ = f.Close() + + result, err := IsAlreadyConverted(cbzPath) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result { + t.Error("Expected CBZ with non-date comment to NOT be detected as converted") + } +} + +func TestExtractChapter_NonexistentFile(t *testing.T) { + _, err := ExtractChapter("/nonexistent/file.cbz") + if err == nil { + t.Error("Expected error for nonexistent file") + } +} + +func TestExtractChapter_PageExtensions(t *testing.T) { + chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz") + if err != nil { + t.Fatalf("Failed to extract chapter: %v", err) + } + defer func() { _ = chapter.Cleanup() }() + + // All pages should have valid image extensions + validExts := map[string]bool{".jpg": true, ".jpeg": true, ".png": true, ".webp": true, ".gif": true} + for _, page := range chapter.Pages { + if !validExts[page.Extension] { + t.Errorf("Page %d has unexpected extension: %s", page.Index, page.Extension) + } + } +} + +func TestExtractChapter_PagesHaveSequentialIndices(t *testing.T) { + chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz") + if err != nil { + t.Fatalf("Failed to extract chapter: %v", err) + } + defer func() { _ = chapter.Cleanup() }() + + for i, page := range chapter.Pages { + if page.Index != uint16(i) { + t.Errorf("Expected page index %d, got %d", i, page.Index) + } + } +} + +func TestExtractChapter_Cleanup(t *testing.T) { + chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz") + if err != nil { + t.Fatalf("Failed to extract chapter: %v", err) + } + + tempDir := chapter.TempDir + // Verify temp dir exists + if _, err := os.Stat(tempDir); os.IsNotExist(err) { + t.Fatal("TempDir does not exist after extraction") + } + + // Cleanup should remove temp dir + err = chapter.Cleanup() + if err != nil { + t.Fatalf("Cleanup failed: %v", err) + } + + if _, err := os.Stat(tempDir); !os.IsNotExist(err) { + t.Error("TempDir should not exist after cleanup") + } +} + +func TestExtractChapter_CBR(t *testing.T) { + chapter, err := ExtractChapter("../../testdata/Chapter 1.cbr") + if err != nil { + t.Fatalf("Failed to extract CBR chapter: %v", err) + } + defer func() { _ = chapter.Cleanup() }() + + if len(chapter.Pages) != 16 { + t.Errorf("Expected 16 pages, got %d", len(chapter.Pages)) + } + + // All page files should exist on disk + for _, page := range chapter.Pages { + if _, err := os.Stat(page.FilePath); os.IsNotExist(err) { + t.Errorf("Page file does not exist on disk: %s", page.FilePath) + } + } +} + +func TestExtractChapter_ConvertedStatus(t *testing.T) { + chapter, err := ExtractChapter("../../testdata/Chapter 10_converted.cbz") + if err != nil { + t.Fatalf("Failed to extract chapter: %v", err) + } + defer func() { _ = chapter.Cleanup() }() + + if !chapter.IsConverted { + t.Error("Expected converted chapter to have IsConverted = true") + } + if chapter.ConvertedTime.IsZero() { + t.Error("Expected non-zero ConvertedTime for converted chapter") + } +} + +func TestWriteChapterToCBZ_EmptyChapter(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "empty.cbz") + + chapter := &manga.Chapter{ + FilePath: filepath.Join(tmpDir, "source.cbz"), + TempDir: tmpDir, + Pages: []*manga.PageFile{}, + } + + err := WriteChapterToCBZ(chapter, outputPath) + if err != nil { + t.Fatalf("Should not error on empty chapter: %v", err) + } + + // Verify the file is a valid zip + r, err := zip.OpenReader(outputPath) + if err != nil { + t.Fatalf("Failed to open output CBZ: %v", err) + } + _ = r.Close() +} + +func TestWriteChapterToCBZ_WithComicInfo(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.cbz") + inputDir := filepath.Join(tmpDir, "input") + _ = os.MkdirAll(inputDir, 0755) + + // Create a dummy page file + pagePath := filepath.Join(inputDir, "0000.jpg") + _ = os.WriteFile(pagePath, []byte("fake image data"), 0644) + + chapter := &manga.Chapter{ + FilePath: filepath.Join(tmpDir, "source.cbz"), + TempDir: tmpDir, + ComicInfoXml: `Test`, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: pagePath}, + }, + } + chapter.SetConverted() + + err := WriteChapterToCBZ(chapter, outputPath) + if err != nil { + t.Fatalf("Failed to write chapter: %v", err) + } + + // Verify ComicInfo.xml is present + r, err := zip.OpenReader(outputPath) + if err != nil { + t.Fatalf("Failed to open output CBZ: %v", err) + } + defer func() { _ = r.Close() }() + + foundComicInfo := false + for _, f := range r.File { + if f.Name == "ComicInfo.xml" { + foundComicInfo = true + } + } + if !foundComicInfo { + t.Error("ComicInfo.xml not found in output CBZ") + } + + // Verify zip comment has converted timestamp + if r.Comment == "" { + t.Error("Expected zip comment with conversion timestamp") + } +} + +func TestWriteChapterToCBZ_InvalidPagePath(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.cbz") + + chapter := &manga.Chapter{ + FilePath: filepath.Join(tmpDir, "source.cbz"), + TempDir: tmpDir, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: "/nonexistent/page.jpg"}, + }, + } + + err := WriteChapterToCBZ(chapter, outputPath) + if err == nil { + t.Error("Expected error when page file does not exist") + } +} + +func TestIsAlreadyConverted_CBZWithConvertedTxt(t *testing.T) { + // Create a CBZ with converted.txt inside (no zip comment) + tmpDir := t.TempDir() + cbzPath := filepath.Join(tmpDir, "test.cbz") + + f, err := os.Create(cbzPath) + if err != nil { + t.Fatal(err) + } + w := zip.NewWriter(f) + // Add converted.txt with a date + fw, err := w.Create("converted.txt") + if err != nil { + t.Fatal(err) + } + _, _ = fw.Write([]byte(time.Now().Format(time.RFC3339))) + // Add a dummy page + fw, err = w.Create("page.webp") + if err != nil { + t.Fatal(err) + } + _, _ = fw.Write([]byte("dummy")) + _ = w.Close() + _ = f.Close() + + result, err := IsAlreadyConverted(cbzPath) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !result { + t.Error("Expected CBZ with converted.txt to be detected as converted") + } +} + +func TestIsAlreadyConverted_CBZWithInvalidConvertedTxt(t *testing.T) { + // Create a CBZ with converted.txt that has invalid date + tmpDir := t.TempDir() + cbzPath := filepath.Join(tmpDir, "test.cbz") + + f, err := os.Create(cbzPath) + if err != nil { + t.Fatal(err) + } + w := zip.NewWriter(f) + fw, err := w.Create("converted.txt") + if err != nil { + t.Fatal(err) + } + _, _ = fw.Write([]byte("not a valid date")) + _ = w.Close() + _ = f.Close() + + result, err := IsAlreadyConverted(cbzPath) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result { + t.Error("Expected CBZ with invalid date in converted.txt to NOT be detected as converted") + } +} + +func TestExtractChapter_WithConvertedTxt(t *testing.T) { + // Create a CBZ with converted.txt inside + tmpDir := t.TempDir() + cbzPath := filepath.Join(tmpDir, "test.cbz") + + f, err := os.Create(cbzPath) + if err != nil { + t.Fatal(err) + } + w := zip.NewWriter(f) + fw, err := w.Create("converted.txt") + if err != nil { + t.Fatal(err) + } + convertedTime := time.Date(2024, 6, 15, 12, 0, 0, 0, time.UTC) + _, _ = fw.Write([]byte(convertedTime.Format(time.RFC3339))) + fw, err = w.Create("page001.jpg") + if err != nil { + t.Fatal(err) + } + _, _ = fw.Write([]byte("fake image")) + _ = w.Close() + _ = f.Close() + + chapter, err := ExtractChapter(cbzPath) + if err != nil { + t.Fatalf("Failed to extract: %v", err) + } + defer func() { _ = chapter.Cleanup() }() + + if !chapter.IsConverted { + t.Error("Expected chapter with converted.txt to be marked as converted") + } + if chapter.ConvertedTime.IsZero() { + t.Error("Expected non-zero ConvertedTime") + } +} + +func TestWriteChapterToCBZ_MultiplePages(t *testing.T) { + tmpDir := t.TempDir() + outputPath := filepath.Join(tmpDir, "output.cbz") + inputDir := filepath.Join(tmpDir, "input") + _ = os.MkdirAll(inputDir, 0755) + + // Create multiple dummy page files + var pages []*manga.PageFile + for i := 0; i < 5; i++ { + pagePath := filepath.Join(inputDir, fmt.Sprintf("%04d.webp", i)) + _ = os.WriteFile(pagePath, []byte(fmt.Sprintf("page %d content", i)), 0644) + pages = append(pages, &manga.PageFile{ + Index: uint16(i), + Extension: ".webp", + FilePath: pagePath, + }) + } + + chapter := &manga.Chapter{ + FilePath: filepath.Join(tmpDir, "source.cbz"), + TempDir: tmpDir, + Pages: pages, + } + chapter.SetConverted() + + err := WriteChapterToCBZ(chapter, outputPath) + if err != nil { + t.Fatalf("Failed to write: %v", err) + } + + // Verify archive contents + r, err := zip.OpenReader(outputPath) + if err != nil { + t.Fatalf("Failed to open: %v", err) + } + defer func() { _ = r.Close() }() + + // Should have 5 pages = 5 files (conversion status stored in zip comment, not as file) + expectedFiles := 5 + if len(r.File) != expectedFiles { + t.Errorf("Expected %d files in archive, got %d", expectedFiles, len(r.File)) + } + + // Verify zip comment is set + if r.Comment == "" { + t.Error("Expected zip comment for converted chapter") + } +} + +func TestWriteChapterToCBZ_InvalidOutputPath(t *testing.T) { + tmpDir := t.TempDir() + inputDir := filepath.Join(tmpDir, "input") + _ = os.MkdirAll(inputDir, 0755) + + pagePath := filepath.Join(inputDir, "0000.webp") + _ = os.WriteFile(pagePath, []byte("content"), 0644) + + chapter := &manga.Chapter{ + FilePath: filepath.Join(tmpDir, "source.cbz"), + TempDir: tmpDir, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".webp", FilePath: pagePath}, + }, + } + + err := WriteChapterToCBZ(chapter, "/nonexistent/dir/output.cbz") + if err == nil { + t.Error("Expected error for invalid output path") + } +} diff --git a/internal/manga/chapter.go b/internal/manga/chapter.go index 7f6acaf..ceabd1c 100644 --- a/internal/manga/chapter.go +++ b/internal/manga/chapter.go @@ -6,38 +6,36 @@ import ( "time" ) +// Chapter represents a comic book chapter with pages stored on disk. type Chapter struct { - // FilePath is the path to the chapter's directory. + // FilePath is the path to the original archive file. FilePath string - // Pages is a slice of pointers to Page objects. - Pages []*Page - // ComicInfo is a string containing information about the chapter. + // Pages is a slice of page files on disk. + Pages []*PageFile + // ComicInfoXml holds the ComicInfo.xml content (small, kept in memory). ComicInfoXml string - // IsConverted is a boolean that indicates whether the chapter has been converted. + // IsConverted indicates whether the chapter has already been converted. IsConverted bool - // ConvertedTime is a pointer to a time.Time object that indicates when the chapter was converted. Nil mean not converted. + // ConvertedTime is when the chapter was converted. ConvertedTime time.Time - // TempDir, when non-empty, is a staging temp folder holding converted - // page contents on disk (see Page.TempFilePath) rather than fully in - // memory. It should be removed via Cleanup once the chapter has been - // written out (or is no longer needed). + // TempDir is the root temp directory for this chapter's extracted/converted files. + // Cleanup removes this entire directory. TempDir string } -// SetConverted sets the IsConverted field to true and sets the ConvertedTime field to the current time. +// SetConverted marks the chapter as converted with the current timestamp. func (chapter *Chapter) SetConverted() { chapter.IsConverted = true chapter.ConvertedTime = time.Now() } -// Cleanup removes the chapter's staging temp folder (if any), releasing any -// page contents that were staged to disk during conversion. +// Cleanup removes the chapter's temp directory and all extracted/converted files. func (chapter *Chapter) Cleanup() error { if chapter.TempDir == "" { return nil } if err := os.RemoveAll(chapter.TempDir); err != nil { - return fmt.Errorf("failed to remove staging temp folder: %w", err) + return fmt.Errorf("failed to remove temp directory: %w", err) } chapter.TempDir = "" return nil diff --git a/internal/manga/manga_test.go b/internal/manga/manga_test.go new file mode 100644 index 0000000..bbdaf22 --- /dev/null +++ b/internal/manga/manga_test.go @@ -0,0 +1,102 @@ +package manga + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestChapter_SetConverted(t *testing.T) { + chapter := &Chapter{} + + if chapter.IsConverted { + t.Error("New chapter should not be converted") + } + if !chapter.ConvertedTime.IsZero() { + t.Error("New chapter should have zero ConvertedTime") + } + + before := time.Now() + chapter.SetConverted() + after := time.Now() + + if !chapter.IsConverted { + t.Error("Chapter should be converted after SetConverted()") + } + if chapter.ConvertedTime.Before(before) || chapter.ConvertedTime.After(after) { + t.Error("ConvertedTime should be between before and after") + } +} + +func TestChapter_Cleanup(t *testing.T) { + // Create a temp dir with some files + tmpDir := t.TempDir() + chapterDir := filepath.Join(tmpDir, "chapter-cleanup-test") + if err := os.MkdirAll(filepath.Join(chapterDir, "input"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(chapterDir, "input", "page.jpg"), []byte("test"), 0644); err != nil { + t.Fatal(err) + } + + chapter := &Chapter{TempDir: chapterDir} + + err := chapter.Cleanup() + if err != nil { + t.Fatalf("Cleanup failed: %v", err) + } + + if chapter.TempDir != "" { + t.Error("TempDir should be empty after cleanup") + } + + if _, err := os.Stat(chapterDir); !os.IsNotExist(err) { + t.Error("Directory should not exist after cleanup") + } +} + +func TestChapter_Cleanup_EmptyTempDir(t *testing.T) { + chapter := &Chapter{TempDir: ""} + + err := chapter.Cleanup() + if err != nil { + t.Errorf("Cleanup with empty TempDir should not error, got: %v", err) + } +} + +func TestChapter_Cleanup_NonexistentDir(t *testing.T) { + chapter := &Chapter{TempDir: "/nonexistent/path/that/does/not/exist"} + + // os.RemoveAll on a nonexistent path returns nil + err := chapter.Cleanup() + if err != nil { + t.Errorf("Cleanup of nonexistent dir should not error, got: %v", err) + } +} + +func TestPageFile_Struct(t *testing.T) { + page := &PageFile{ + Index: 5, + Extension: ".webp", + FilePath: "/tmp/test/0005.webp", + IsSplitted: true, + SplitPartIndex: 2, + } + + if page.Index != 5 { + t.Errorf("Expected Index 5, got %d", page.Index) + } + if page.Extension != ".webp" { + t.Errorf("Expected Extension .webp, got %s", page.Extension) + } + if page.FilePath != "/tmp/test/0005.webp" { + t.Errorf("Expected FilePath /tmp/test/0005.webp, got %s", page.FilePath) + } + if !page.IsSplitted { + t.Error("Expected IsSplitted true") + } + if page.SplitPartIndex != 2 { + t.Errorf("Expected SplitPartIndex 2, got %d", page.SplitPartIndex) + } +} diff --git a/internal/manga/page.go b/internal/manga/page.go index 6faf8e9..5d42ea6 100644 --- a/internal/manga/page.go +++ b/internal/manga/page.go @@ -1,86 +1,16 @@ package manga -import ( - "bytes" - "fmt" - "io" - "os" - - "github.com/rs/zerolog/log" -) - -type Page struct { - // Index of the page in the chapter. - Index uint16 `json:"index" jsonschema:"description=Index of the page in the chapter."` - // Extension of the page image. - Extension string `json:"extension" jsonschema:"description=Extension of the page image."` - // Size of the page in bytes - Size uint64 `json:"-"` - // Contents of the page. Nil when the page contents have been staged to - // disk (see TempFilePath) to bound memory usage. - Contents *bytes.Buffer `json:"-"` - // TempFilePath, when non-empty, points to a file on disk (in a staging - // temp folder) holding the page contents instead of keeping them fully - // in memory. Use Open() to transparently read the page contents - // regardless of where they are stored. - TempFilePath string `json:"-"` - // IsSplitted tell us if the page was cropped to multiple pieces - IsSplitted bool `json:"is_cropped" jsonschema:"description=Was this page cropped."` - // SplitPartIndex represent the index of the crop if the page was cropped - SplitPartIndex uint16 `json:"crop_part_index" jsonschema:"description=Index of the crop if the image was cropped."` -} - -// Open returns a reader for the page contents, transparently handling -// whether the contents are held in memory (Contents) or staged on disk -// (TempFilePath). The caller is responsible for closing the returned -// reader. -func (page *Page) Open() (io.ReadCloser, error) { - if page.TempFilePath != "" { - file, err := os.Open(page.TempFilePath) - if err != nil { - return nil, fmt.Errorf("failed to open staged page contents: %w", err) - } - return file, nil - } - if page.Contents == nil { - return nil, fmt.Errorf("page has no contents: neither TempFilePath nor Contents is set") - } - return io.NopCloser(bytes.NewReader(page.Contents.Bytes())), nil -} - -// Stage writes the given content to a file in tempDir instead of keeping it -// in memory, updating Extension, Size and TempFilePath accordingly and -// clearing Contents. This is used after converting a page so that only the -// pages currently being processed are held in memory, bounding memory usage -// for chapters with many/large pages. -func (page *Page) Stage(tempDir string, content *bytes.Buffer, extension string) error { - file, err := os.CreateTemp(tempDir, "page-*.tmp") - if err != nil { - return fmt.Errorf("failed to create staging file: %w", err) - } - fileName := file.Name() - - written, writeErr := file.Write(content.Bytes()) - closeErr := file.Close() - if writeErr == nil && closeErr == nil && written == content.Len() { - page.TempFilePath = fileName - page.Extension = extension - page.Size = uint64(written) - page.Contents = nil - return nil - } - - // Something went wrong: remove the incomplete/partial staging file - // rather than leaving corrupted data behind on disk. - if removeErr := os.Remove(fileName); removeErr != nil { - log.Warn().Str("file", fileName).Err(removeErr).Msg("Failed to remove incomplete staging file") - } - - if writeErr != nil { - return fmt.Errorf("failed to write staging file: %w", writeErr) - } - if closeErr != nil { - return fmt.Errorf("failed to close staging file: %w", closeErr) - } - return fmt.Errorf("short write to staging file: wrote %d of %d bytes", written, content.Len()) +// PageFile represents a single page image stored on disk. +// No image data is held in memory — only metadata and a file path. +type PageFile struct { + // Index of the page in the chapter (original ordering). + Index uint16 + // Extension of the page image file (e.g., ".webp", ".jpg"). + Extension string + // FilePath is the absolute path to the image file on disk. + FilePath string + // IsSplitted indicates whether this page was split from a larger image. + IsSplitted bool + // SplitPartIndex is the part index when the page was split. + SplitPartIndex uint16 } diff --git a/internal/manga/page_container.go b/internal/manga/page_container.go deleted file mode 100644 index f125efc..0000000 --- a/internal/manga/page_container.go +++ /dev/null @@ -1,32 +0,0 @@ -package manga - -import ( - "bytes" - "image" -) - -// PageContainer is a struct that holds a manga page, its image, and the image format. -type PageContainer struct { - // Page is a pointer to a manga page object. - Page *Page - // Image is the decoded image of the manga page. - Image image.Image - // Format is a string representing the format of the image (e.g., "png", "jpeg", "webp"). - Format string - // IsToBeConverted is a boolean flag indicating whether the image needs to be converted to another format. - IsToBeConverted bool - // HasBeenConverted is a boolean flag indicating whether the image has been converted to another format. - HasBeenConverted bool -} - -func NewContainer(Page *Page, img image.Image, format string, isToBeConverted bool) *PageContainer { - return &PageContainer{Page: Page, Image: img, Format: format, IsToBeConverted: isToBeConverted, HasBeenConverted: false} -} - -// SetConverted sets the converted image, its extension, and its size in the PageContainer. -func (pc *PageContainer) SetConverted(converted *bytes.Buffer, extension string) { - pc.Page.Contents = converted - pc.Page.Extension = extension - pc.Page.Size = uint64(converted.Len()) - pc.HasBeenConverted = true -} diff --git a/internal/utils/errs/errors_defer_test.go b/internal/utils/errs/errors_defer_test.go index b5bb59f..5f6595b 100644 --- a/internal/utils/errs/errors_defer_test.go +++ b/internal/utils/errs/errors_defer_test.go @@ -46,7 +46,7 @@ func TestCapture(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var err error = tt.initial + var err = tt.initial Capture(&err, tt.errFunc, tt.msg) if err != nil && err.Error() != tt.expected { t.Errorf("expected %q, got %q", tt.expected, err.Error()) @@ -110,7 +110,7 @@ func TestCaptureGeneric(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var err error = tt.initial + var err = tt.initial CaptureGeneric(&err, tt.errFunc, tt.value, tt.msg) if err != nil && err.Error() != tt.expected { t.Errorf("expected %q, got %q", tt.expected, err.Error()) diff --git a/internal/utils/file_utils_test.go b/internal/utils/file_utils_test.go new file mode 100644 index 0000000..9eac597 --- /dev/null +++ b/internal/utils/file_utils_test.go @@ -0,0 +1,59 @@ +package utils + +import ( + "os" + "path/filepath" + "testing" +) + +func TestIsValidFolder(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T) string + expected bool + }{ + { + name: "valid directory", + setup: func(t *testing.T) string { + return t.TempDir() + }, + expected: true, + }, + { + name: "file not directory", + setup: func(t *testing.T) string { + dir := t.TempDir() + path := filepath.Join(dir, "file.txt") + if err := os.WriteFile(path, []byte("content"), 0644); err != nil { + t.Fatal(err) + } + return path + }, + expected: false, + }, + { + name: "nonexistent path", + setup: func(t *testing.T) string { + return "/nonexistent/path/that/does/not/exist" + }, + expected: false, + }, + { + name: "empty string", + setup: func(t *testing.T) string { + return "" + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := tt.setup(t) + result := IsValidFolder(path) + if result != tt.expected { + t.Errorf("IsValidFolder(%q) = %v, want %v", path, result, tt.expected) + } + }) + } +} diff --git a/internal/utils/optimize.go b/internal/utils/optimize.go index d5fc3a0..3ba6a43 100644 --- a/internal/utils/optimize.go +++ b/internal/utils/optimize.go @@ -25,6 +25,12 @@ type OptimizeOptions struct { } // Optimize optimizes a CBZ/CBR file using the specified converter. +// The new pipeline is disk-first: +// 1. Fast check if already converted (no extraction) +// 2. Extract archive to temp directory on disk +// 3. Convert pages file-to-file (no image data in memory) +// 4. Create output CBZ by streaming from disk +// 5. Cleanup temp files func Optimize(options *OptimizeOptions) error { log.Info().Str("file", options.Path).Msg("Processing file") log.Debug(). @@ -34,38 +40,47 @@ func Optimize(options *OptimizeOptions) error { Bool("split", options.Split). Msg("Optimization parameters") - // Load the chapter - log.Debug().Str("file", options.Path).Msg("Loading chapter") - chapter, err := cbz.LoadChapter(options.Path) + // Step 1: Fast conversion check before extracting (new requirement) + alreadyConverted, err := cbz.IsAlreadyConverted(options.Path) if err != nil { - log.Error().Str("file", options.Path).Err(err).Msg("Failed to load chapter") + log.Debug().Str("file", options.Path).Err(err).Msg("Conversion check failed, proceeding with extraction") + } + if alreadyConverted { + log.Info().Str("file", options.Path).Msg("Chapter already converted") + return nil + } + + // Step 2: Extract chapter to disk + log.Debug().Str("file", options.Path).Msg("Extracting chapter") + chapter, err := cbz.ExtractChapter(options.Path) + if err != nil { + log.Error().Str("file", options.Path).Err(err).Msg("Failed to extract chapter") return fmt.Errorf("failed to load chapter: %v", err) } - log.Debug(). - Str("file", options.Path). - Int("pages", len(chapter.Pages)). - Bool("converted", chapter.IsConverted). - Msg("Chapter loaded successfully") + defer func() { + if cleanupErr := chapter.Cleanup(); cleanupErr != nil { + log.Warn().Str("file", options.Path).Err(cleanupErr).Msg("Failed to cleanup temp directory") + } + }() + // Double-check conversion status from extracted metadata if chapter.IsConverted { log.Info().Str("file", options.Path).Msg("Chapter already converted") return nil } - // Convert the chapter log.Debug(). - Str("file", chapter.FilePath). + Str("file", options.Path). Int("pages", len(chapter.Pages)). - Uint8("quality", options.Quality). - Bool("split", options.Split). - Msg("Starting chapter conversion") + Msg("Chapter extracted successfully") + // Step 3: Convert pages file-to-file var ctx context.Context if options.Timeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(context.Background(), options.Timeout) defer cancel() - log.Debug().Str("file", chapter.FilePath).Dur("timeout", options.Timeout).Msg("Applying timeout to chapter conversion") + log.Debug().Str("file", options.Path).Dur("timeout", options.Timeout).Msg("Applying timeout") } else { ctx = context.Background() } @@ -91,82 +106,47 @@ func Optimize(options *OptimizeOptions) error { return fmt.Errorf("failed to convert chapter") } - // Clean up the staging temp folder (if any) used to hold converted page - // contents on disk once we're done with the chapter, regardless of - // success or failure below. - defer func() { - if cleanupErr := convertedChapter.Cleanup(); cleanupErr != nil { - log.Warn().Str("file", chapter.FilePath).Err(cleanupErr).Msg("Failed to remove staging temp folder") - } - }() - log.Debug(). Str("file", chapter.FilePath). - Int("original_pages", len(chapter.Pages)). Int("converted_pages", len(convertedChapter.Pages)). Msg("Chapter conversion completed") convertedChapter.SetConverted() - // Determine output path and handle CBR override logic - log.Debug(). - Str("input_path", options.Path). - Bool("override", options.Override). - Msg("Determining output path") - + // Step 4: Determine output path outputPath := options.Path originalPath := options.Path isCbrOverride := false if options.Override { - // For override mode, check if it's a CBR file that needs to be converted to CBZ pathLower := strings.ToLower(options.Path) if strings.HasSuffix(pathLower, ".cbr") { - // Convert CBR to CBZ: change extension and mark for deletion outputPath = strings.TrimSuffix(options.Path, filepath.Ext(options.Path)) + ".cbz" isCbrOverride = true - log.Debug(). - Str("original_path", originalPath). - Str("output_path", outputPath). - Msg("CBR to CBZ conversion: will delete original after conversion") - } else { - log.Debug(). - Str("original_path", originalPath). - Str("output_path", outputPath). - Msg("CBZ override mode: will overwrite original file") } } else { - // Handle both .cbz and .cbr files - strip the extension and add _converted.cbz pathLower := strings.ToLower(options.Path) if strings.HasSuffix(pathLower, ".cbz") { outputPath = strings.TrimSuffix(options.Path, ".cbz") + "_converted.cbz" } else if strings.HasSuffix(pathLower, ".cbr") { outputPath = strings.TrimSuffix(options.Path, ".cbr") + "_converted.cbz" } else { - // Fallback for other extensions - just add _converted.cbz outputPath = options.Path + "_converted.cbz" } - log.Debug(). - Str("original_path", originalPath). - Str("output_path", outputPath). - Msg("Non-override mode: creating converted file alongside original") } - // Write the converted chapter to CBZ file + // Step 5: Write converted chapter to CBZ (streaming from disk) log.Debug().Str("output_path", outputPath).Msg("Writing converted chapter to CBZ file") err = cbz.WriteChapterToCBZ(convertedChapter, outputPath) if err != nil { log.Error().Str("output_path", outputPath).Err(err).Msg("Failed to write converted chapter") return fmt.Errorf("failed to write converted chapter: %v", err) } - log.Debug().Str("output_path", outputPath).Msg("Successfully wrote converted chapter") - // If we're overriding a CBR file, delete the original CBR after successful write + // If overriding a CBR file, delete the original if isCbrOverride { - log.Debug().Str("file", originalPath).Msg("Attempting to delete original CBR file") err = os.Remove(originalPath) if err != nil { - // Log the error but don't fail the operation since conversion succeeded log.Warn().Str("file", originalPath).Err(err).Msg("Failed to delete original CBR file") } else { log.Info().Str("file", originalPath).Msg("Deleted original CBR file") @@ -175,5 +155,4 @@ func Optimize(options *OptimizeOptions) error { log.Info().Str("output", outputPath).Msg("Converted file written") return nil - } diff --git a/internal/utils/optimize_integration_test.go b/internal/utils/optimize_integration_test.go index 20421cc..e20df44 100644 --- a/internal/utils/optimize_integration_test.go +++ b/internal/utils/optimize_integration_test.go @@ -213,7 +213,7 @@ func TestOptimizeIntegration(t *testing.T) { } // Clean up output file - os.Remove(expectedOutput) + _ = os.Remove(expectedOutput) }) } } @@ -394,7 +394,7 @@ func TestOptimizeIntegration_Timeout(t *testing.T) { Quality: 85, Override: false, Split: false, - Timeout: 10 * time.Millisecond, // Very short timeout to force timeout + Timeout: 1 * time.Nanosecond, // Extremely short timeout to force timeout } err = Optimize(options) diff --git a/internal/utils/optimize_test.go b/internal/utils/optimize_test.go index ebcf37d..2a1c2c9 100644 --- a/internal/utils/optimize_test.go +++ b/internal/utils/optimize_test.go @@ -276,7 +276,7 @@ func TestOptimize(t *testing.T) { } // Clean up output file - os.Remove(expectedOutput) + _ = os.Remove(expectedOutput) }) } } diff --git a/pkg/converter/converter.go b/pkg/converter/converter.go index e7a59b1..c549dda 100644 --- a/pkg/converter/converter.go +++ b/pkg/converter/converter.go @@ -11,13 +11,19 @@ import ( "github.com/samber/lo" ) +// Converter defines the interface for image format converters. +// All operations are file-to-file: no image data is held in memory in the happy path. type Converter interface { - // Format of the converter - Format() (format constant.ConversionFormat) - // ConvertChapter converts a manga chapter to the specified format. - // - // Returns partial success where some pages are converted and some are not. + // Format returns the output format of this converter. + Format() constant.ConversionFormat + + // ConvertChapter converts all pages in a chapter from their source files to + // the target format. Pages are processed in parallel (bounded by CPU count). + // On success, chapter.Pages is updated with converted PageFile entries. + // Returns partial success (non-fatal errors) via errors.PageIgnoredError. ConvertChapter(ctx context.Context, chapter *manga.Chapter, quality uint8, split bool, progress func(message string, current uint32, total uint32)) (*manga.Chapter, error) + + // PrepareConverter ensures the external encoder binary is available. PrepareConverter() error } @@ -30,8 +36,8 @@ func Available() []constant.ConversionFormat { return lo.Keys(converters) } -// Get returns a packer by name. -// If the packer is not available, an error is returned. +// Get returns a converter by format name. +// If the converter is not available, an error is returned. var Get = getConverter func getConverter(name constant.ConversionFormat) (Converter, error) { @@ -39,7 +45,7 @@ func getConverter(name constant.ConversionFormat) (Converter, error) { return converter, nil } - return nil, fmt.Errorf("unkown converter \"%s\", available options are %s", name, strings.Join(lo.Map(Available(), func(item constant.ConversionFormat, index int) string { + return nil, fmt.Errorf("unknown converter \"%s\", available options are %s", name, strings.Join(lo.Map(Available(), func(item constant.ConversionFormat, index int) string { return item.String() }), ", ")) } diff --git a/pkg/converter/converter_test.go b/pkg/converter/converter_test.go index 35c9246..e01adbf 100644 --- a/pkg/converter/converter_test.go +++ b/pkg/converter/converter_test.go @@ -1,24 +1,23 @@ package converter import ( - "bytes" "context" + "fmt" "image" "image/jpeg" "os" + "path/filepath" "testing" "github.com/belphemur/CBZOptimizer/v2/internal/manga" - "github.com/belphemur/CBZOptimizer/v2/internal/utils/errs" ) func TestConvertChapter(t *testing.T) { - testCases := []struct { - name string - genTestChapter func(path string, isSplit bool) (*manga.Chapter, []string, error) - split bool - expectError bool + name string + genTestChapter func(t *testing.T, dir string) (*manga.Chapter, []string) + split bool + expectError bool }{ { name: "All split pages", @@ -27,7 +26,7 @@ func TestConvertChapter(t *testing.T) { }, { name: "Big Pages, no split", - genTestChapter: genHugePage, + genTestChapter: genHugePageNoSplit, split: false, expectError: true, }, @@ -47,53 +46,41 @@ func TestConvertChapter(t *testing.T) { split: false, expectError: true, }, - { - name: "Two corrupted pages", - genTestChapter: genTwoCorrupted, - split: false, - expectError: true, - }, } - // Load test genTestChapter from testdata - temp, err := os.CreateTemp("", "test_chapter_*.cbz") - if err != nil { - t.Fatalf("failed to create temporary file: %v", err) - } - defer errs.CaptureGeneric(&err, os.Remove, temp.Name(), "failed to remove temporary file") - for _, converter := range Available() { - converter, err := Get(converter) + for _, converterFormat := range Available() { + conv, err := Get(converterFormat) if err != nil { t.Fatalf("failed to get converter: %v", err) } - t.Run(converter.Format().String(), func(t *testing.T) { + t.Run(conv.Format().String(), func(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - chapter, expectedExtensions, err := tc.genTestChapter(temp.Name(), tc.split) - if err != nil { - t.Fatalf("failed to load test genTestChapter: %v", err) - } + tempDir := t.TempDir() + chapter, expectedExtensions := tc.genTestChapter(t, tempDir) quality := uint8(80) - progress := func(msg string, current uint32, total uint32) { t.Log(msg) } - convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, quality, tc.split, progress) + convertedChapter, err := conv.ConvertChapter(context.Background(), chapter, quality, tc.split, progress) if err != nil && !tc.expectError { - t.Fatalf("failed to convert genTestChapter: %v", err) + t.Fatalf("failed to convert chapter: %v", err) + } + + if convertedChapter == nil { + t.Fatal("convertedChapter is nil") } if len(convertedChapter.Pages) == 0 { - t.Fatalf("no pages were converted") + t.Fatal("no pages were converted") } if len(convertedChapter.Pages) != len(expectedExtensions) { t.Fatalf("converted chapter has %d pages but expected %d", len(convertedChapter.Pages), len(expectedExtensions)) } - // Check each page's extension against the expected array for i, page := range convertedChapter.Pages { expectedExt := expectedExtensions[i] if page.Extension != expectedExt { @@ -106,176 +93,152 @@ func TestConvertChapter(t *testing.T) { } } -func genHugePage(path string, isSplit bool) (*manga.Chapter, []string, error) { - file, err := os.Open(path) +// createTestImageFile creates a JPEG image file at the given path with the specified dimensions. +func createTestImageFile(t *testing.T, path string, width, height int) { + t.Helper() + img := image.NewRGBA(image.Rect(0, 0, width, height)) + f, err := os.Create(path) if err != nil { - return nil, nil, err + t.Fatal(err) } - defer errs.Capture(&err, file.Close, "failed to close file") - - var pages []*manga.Page - expectedExtensions := []string{".jpg"} // One image that's generated as JPEG - if isSplit { - expectedExtensions = []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp"} - } - - // Create one tall page - img := image.NewRGBA(image.Rect(0, 0, 1, 17000)) - buf := new(bytes.Buffer) - err = jpeg.Encode(buf, img, nil) + err = jpeg.Encode(f, img, nil) + _ = f.Close() if err != nil { - return nil, nil, err + t.Fatal(err) } - page := &manga.Page{ - Index: 0, - Contents: buf, - Extension: ".jpg", - } - pages = append(pages, page) - - return &manga.Chapter{ - FilePath: path, - Pages: pages, - }, expectedExtensions, nil } -func genSmallPages(path string, isSplit bool) (*manga.Chapter, []string, error) { - file, err := os.Open(path) - if err != nil { - return nil, nil, err +func genHugePage(t *testing.T, dir string) (*manga.Chapter, []string) { + inputDir := filepath.Join(dir, "input") + if err := os.MkdirAll(inputDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, "output"), 0755); err != nil { + t.Fatal(err) } - defer errs.Capture(&err, file.Close, "failed to close file") - var pages []*manga.Page - for i := 0; i < 5; i++ { // Assuming there are 5 pages for the test - img := image.NewRGBA(image.Rect(0, 0, 300, 1000)) - buf := new(bytes.Buffer) - err = jpeg.Encode(buf, img, nil) - if err != nil { - return nil, nil, err - } - page := &manga.Page{ + pagePath := filepath.Join(inputDir, "0000.jpg") + createTestImageFile(t, pagePath, 1, 17000) + + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: pagePath}, + }, + } + + // With split: 17000/2000 = 9 parts (8*2000 + 1*1000) + // Without split: page > webpMaxHeight → kept as .jpg with error + // The caller decides split=true or split=false and we return expectations + // for the split=true case (this test case is always called with split=true) + expectedExtensions := []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp"} + + return chapter, expectedExtensions +} + +func genHugePageNoSplit(t *testing.T, dir string) (*manga.Chapter, []string) { + inputDir := filepath.Join(dir, "input") + if err := os.MkdirAll(inputDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, "output"), 0755); err != nil { + t.Fatal(err) + } + + pagePath := filepath.Join(inputDir, "0000.jpg") + createTestImageFile(t, pagePath, 1, 17000) + + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: pagePath}, + }, + } + + // Without split: page > webpMaxHeight → kept as .jpg with error + return chapter, []string{".jpg"} +} + +func genSmallPages(t *testing.T, dir string) (*manga.Chapter, []string) { + inputDir := filepath.Join(dir, "input") + _ = os.MkdirAll(inputDir, 0755) + _ = os.MkdirAll(filepath.Join(dir, "output"), 0755) + + var pages []*manga.PageFile + for i := 0; i < 5; i++ { + pagePath := filepath.Join(inputDir, fmt.Sprintf("%04d.jpg", i)) + createTestImageFile(t, pagePath, 300, 1000) + pages = append(pages, &manga.PageFile{ Index: uint16(i), - Contents: buf, Extension: ".jpg", - } - pages = append(pages, page) + FilePath: pagePath, + }) } - return &manga.Chapter{ - FilePath: path, + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, Pages: pages, - }, []string{".webp", ".webp", ".webp", ".webp", ".webp"}, nil + } + + return chapter, []string{".webp", ".webp", ".webp", ".webp", ".webp"} } -func genMixSmallBig(path string, isSplit bool) (*manga.Chapter, []string, error) { - file, err := os.Open(path) - if err != nil { - return nil, nil, err - } - defer errs.Capture(&err, file.Close, "failed to close file") +func genMixSmallBig(t *testing.T, dir string) (*manga.Chapter, []string) { + inputDir := filepath.Join(dir, "input") + _ = os.MkdirAll(inputDir, 0755) + _ = os.MkdirAll(filepath.Join(dir, "output"), 0755) - var pages []*manga.Page - for i := 0; i < 5; i++ { // Assuming there are 5 pages for the test - img := image.NewRGBA(image.Rect(0, 0, 300, 1000*(i+1))) - buf := new(bytes.Buffer) - err := jpeg.Encode(buf, img, nil) - if err != nil { - return nil, nil, err - } - page := &manga.Page{ + var pages []*manga.PageFile + for i := 0; i < 5; i++ { + pagePath := filepath.Join(inputDir, fmt.Sprintf("%04d.jpg", i)) + createTestImageFile(t, pagePath, 300, 1000*(i+1)) + pages = append(pages, &manga.PageFile{ Index: uint16(i), - Contents: buf, Extension: ".jpg", - } - pages = append(pages, page) - } - expectedExtensions := []string{".webp", ".webp", ".webp", ".webp", ".webp"} - if isSplit { - expectedExtensions = []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp"} + FilePath: pagePath, + }) } - return &manga.Chapter{ - FilePath: path, + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, Pages: pages, - }, expectedExtensions, nil + } + + // Pages heights: 1000, 2000, 3000, 4000, 5000 + // With new disk-first architecture: cwebp handles all these directly + // (all < 16383 webp max), no splitting needed + return chapter, []string{".webp", ".webp", ".webp", ".webp", ".webp"} } -func genMixSmallHuge(path string, isSplit bool) (*manga.Chapter, []string, error) { - file, err := os.Open(path) - if err != nil { - return nil, nil, err - } - defer errs.Capture(&err, file.Close, "failed to close file") +func genMixSmallHuge(t *testing.T, dir string) (*manga.Chapter, []string) { + inputDir := filepath.Join(dir, "input") + _ = os.MkdirAll(inputDir, 0755) + _ = os.MkdirAll(filepath.Join(dir, "output"), 0755) - var pages []*manga.Page - for i := 0; i < 10; i++ { // Assuming there are 5 pages for the test - img := image.NewRGBA(image.Rect(0, 0, 1, 2000*(i+1))) - buf := new(bytes.Buffer) - err := jpeg.Encode(buf, img, nil) - if err != nil { - return nil, nil, err - } - page := &manga.Page{ + var pages []*manga.PageFile + for i := 0; i < 10; i++ { + pagePath := filepath.Join(inputDir, fmt.Sprintf("%04d.jpg", i)) + createTestImageFile(t, pagePath, 1, 2000*(i+1)) + pages = append(pages, &manga.PageFile{ Index: uint16(i), - Contents: buf, Extension: ".jpg", - } - pages = append(pages, page) + FilePath: pagePath, + }) } - return &manga.Chapter{ - FilePath: path, + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, Pages: pages, - }, []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".jpg", ".jpg"}, nil -} - -func genTwoCorrupted(path string, isSplit bool) (*manga.Chapter, []string, error) { - file, err := os.Open(path) - if err != nil { - return nil, nil, err - } - defer errs.Capture(&err, file.Close, "failed to close file") - - var pages []*manga.Page - numPages := 5 - corruptedIndices := []int{2, 4} // Pages 2 and 4 are too tall to convert without splitting - for i := 0; i < numPages; i++ { - var buf *bytes.Buffer - var ext string - isCorrupted := false - for _, ci := range corruptedIndices { - if i == ci { - isCorrupted = true - break - } - } - if isCorrupted { - buf = bytes.NewBufferString("corrupted data") // Invalid data, can't decode as image - ext = ".jpg" - } else { - img := image.NewRGBA(image.Rect(0, 0, 300, 1000)) - buf = new(bytes.Buffer) - err = jpeg.Encode(buf, img, nil) - if err != nil { - return nil, nil, err - } - ext = ".jpg" - } - page := &manga.Page{ - Index: uint16(i), - Contents: buf, - Extension: ext, - } - pages = append(pages, page) } - // Expected: small pages to .webp, corrupted pages to .jpg (kept as is) - expectedExtensions := []string{".webp", ".webp", ".jpg", ".webp", ".jpg"} - // Even with split, corrupted pages can't be decoded so stay as is - - return &manga.Chapter{ - FilePath: path, - Pages: pages, - }, expectedExtensions, nil + // Heights: 2000, 4000, 6000, 8000, 10000, 12000, 14000, 16000, 18000, 20000 + // Without split, pages > webpMaxHeight (16383) are kept as .jpg + // Pages 0-7 (2000-16000): should convert to .webp + // Pages 8-9 (18000, 20000): > 16383, no split → kept as .jpg + return chapter, []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".jpg", ".jpg"} } diff --git a/pkg/converter/webp/webp_converter.go b/pkg/converter/webp/webp_converter.go index 78b3dcf..5359de6 100644 --- a/pkg/converter/webp/webp_converter.go +++ b/pkg/converter/webp/webp_converter.go @@ -1,25 +1,25 @@ package webp import ( - "bytes" "context" "errors" "fmt" "image" _ "image/gif" _ "image/jpeg" - "image/png" + _ "image/png" "os" + "path/filepath" "runtime" + "strings" + "sort" "sync" "sync/atomic" "github.com/belphemur/CBZOptimizer/v2/internal/manga" "github.com/belphemur/CBZOptimizer/v2/pkg/converter/constant" converterrors "github.com/belphemur/CBZOptimizer/v2/pkg/converter/errors" - "github.com/oliamb/cutter" "github.com/rs/zerolog/log" - "golang.org/x/exp/slices" _ "golang.org/x/image/webp" ) @@ -29,24 +29,16 @@ type Converter struct { maxHeight int cropHeight int isPrepared bool - // pageWorkerGuard limits how many pages are converted (i.e. how many - // cwebp binary processes are spawned) concurrently across the whole - // application, regardless of how many chapters are being processed in - // parallel (see the --parallelism flag). Without this shared limit, the - // per-chapter worker pool (sized to runtime.NumCPU()) would be - // multiplied by the number of chapters processed concurrently, leading - // to CPU/memory oversubscription since every page conversion spawns an - // external cwebp process. + // pageWorkerGuard limits concurrent cwebp processes across all chapters. pageWorkerGuard chan struct{} } -func (converter *Converter) Format() (format constant.ConversionFormat) { +func (converter *Converter) Format() constant.ConversionFormat { return constant.WebP } func New() *Converter { return &Converter{ - //maxHeight: 16383 / 2, maxHeight: 4000, cropHeight: 2000, isPrepared: false, @@ -66,456 +58,290 @@ func (converter *Converter) PrepareConverter() error { return nil } +// ConvertChapter converts all pages in a chapter using file-to-file cwebp operations. +// In the happy path, no image data is loaded into Go memory. +// Splitting is attempted only if direct conversion fails due to dimension limits. func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.Chapter, quality uint8, split bool, progress func(message string, current uint32, total uint32)) (*manga.Chapter, error) { log.Debug(). Str("chapter", chapter.FilePath). Int("pages", len(chapter.Pages)). Uint8("quality", quality). Bool("split", split). - Int("max_goroutines", runtime.NumCPU()). - Msg("Starting chapter conversion") + Msg("Starting file-to-file chapter conversion") err := converter.PrepareConverter() if err != nil { - log.Error().Str("chapter", chapter.FilePath).Err(err).Msg("Failed to prepare converter") return nil, err } - // Stage converted pages on disk in a temp folder instead of keeping them - // all in memory until the whole chapter is written out. This bounds - // memory usage for chapters with many/large pages; the caller is - // responsible for calling chapter.Cleanup() once the chapter has been - // written to its final CBZ file. - tempDir, err := os.MkdirTemp("", "cbzoptimizer-*") - if err != nil { - log.Error().Str("chapter", chapter.FilePath).Err(err).Msg("Failed to create staging temp folder") - return nil, fmt.Errorf("failed to create staging temp folder: %w", err) - } - chapter.TempDir = tempDir - - var wgConvertedPages sync.WaitGroup - maxGoroutines := runtime.NumCPU() - - pagesChan := make(chan *manga.PageContainer, maxGoroutines) - errChan := make(chan error, maxGoroutines) - doneChan := make(chan struct{}) - - var wgPages sync.WaitGroup - wgPages.Add(len(chapter.Pages)) - - // guard is shared across all chapters processed concurrently by this - // converter instance (see pageWorkerGuard doc comment) so that the total - // number of concurrently running cwebp processes stays bounded to - // runtime.NumCPU(), no matter how many chapters are being converted in - // parallel at the same time. - guard := converter.pageWorkerGuard - pagesMutex := sync.Mutex{} - var pages []*manga.Page - var totalPages = uint32(len(chapter.Pages)) - - log.Debug(). - Str("chapter", chapter.FilePath). - Int("total_pages", len(chapter.Pages)). - Int("worker_count", maxGoroutines). - Str("staging_dir", tempDir). - Msg("Initialized conversion worker pool") - - // failWithCleanup removes the staging temp folder before returning a - // fatal error for which no chapter is returned to the caller (and thus - // nobody else will clean it up). - failWithCleanup := func(err error) (*manga.Chapter, error) { - if removeErr := os.RemoveAll(tempDir); removeErr != nil { - log.Warn().Str("chapter", chapter.FilePath).Err(removeErr).Msg("Failed to remove staging temp folder after error") - } - chapter.TempDir = "" - return nil, err + // Create output directory for converted files + outputDir := filepath.Join(chapter.TempDir, "output") + if err := os.MkdirAll(outputDir, 0755); err != nil { + return nil, fmt.Errorf("failed to create output directory: %w", err) } - // Check if context is already cancelled + // Check for early context cancellation select { case <-ctx.Done(): - log.Warn().Str("chapter", chapter.FilePath).Msg("Chapter conversion cancelled due to timeout") - return failWithCleanup(ctx.Err()) + return nil, ctx.Err() default: } - // Start the worker pool - go func() { - defer close(doneChan) - for page := range pagesChan { - select { - case <-ctx.Done(): - return - case guard <- struct{}{}: // would block if guard channel is already filled - } + guard := converter.pageWorkerGuard + var totalPages atomic.Uint32 + totalPages.Store(uint32(len(chapter.Pages))) - go func(pageToConvert *manga.PageContainer) { - defer func() { - wgConvertedPages.Done() - <-guard - }() - - // Check context cancellation before processing - select { - case <-ctx.Done(): - return - default: - } - - convertedPage, err := converter.convertPage(pageToConvert, quality, tempDir) - if err != nil { - if convertedPage == nil { - select { - case errChan <- err: - case <-ctx.Done(): - return - } - return - } - buffer := new(bytes.Buffer) - err := png.Encode(buffer, convertedPage.Image) - if err != nil { - select { - case errChan <- err: - case <-ctx.Done(): - return - } - return - } - if err := convertedPage.Page.Stage(tempDir, buffer, ".png"); err != nil { - select { - case errChan <- err: - case <-ctx.Done(): - return - } - return - } - } - pagesMutex.Lock() - defer pagesMutex.Unlock() - pages = append(pages, convertedPage.Page) - currentTotalPages := atomic.LoadUint32(&totalPages) - progress(fmt.Sprintf("Converted %d/%d pages to %s format", len(pages), currentTotalPages, converter.Format()), uint32(len(pages)), currentTotalPages) - }(page) - } - }() - - // Process pages - for _, page := range chapter.Pages { - select { - case <-ctx.Done(): - log.Warn().Str("chapter", chapter.FilePath).Msg("Chapter conversion cancelled due to timeout") - return failWithCleanup(ctx.Err()) - default: - } - - go func(page *manga.Page) { - defer wgPages.Done() - - splitNeeded, img, format, err := converter.checkPageNeedsSplit(page, split) - if err != nil { - var pageIgnoredError *converterrors.PageIgnoredError - if errors.As(err, &pageIgnoredError) { - log.Info().Err(err).Msg("Page ignored due to image decode error") - } - - select { - case errChan <- err: - case <-ctx.Done(): - return - } - - wgConvertedPages.Add(1) - select { - case pagesChan <- manga.NewContainer(page, img, format, false): - case <-ctx.Done(): - wgConvertedPages.Done() - return - } - - return - } - - if !splitNeeded { - wgConvertedPages.Add(1) - select { - case pagesChan <- manga.NewContainer(page, img, format, true): - case <-ctx.Done(): - wgConvertedPages.Done() - return - } - return - } - - images, err := converter.cropImage(img) - if err != nil { - select { - case errChan <- err: - case <-ctx.Done(): - return - } - return - } - - atomic.AddUint32(&totalPages, uint32(len(images)-1)) - for i, img := range images { - select { - case <-ctx.Done(): - return - default: - } - - newPage := &manga.Page{ - Index: page.Index, - IsSplitted: true, - SplitPartIndex: uint16(i), - } - wgConvertedPages.Add(1) - select { - case pagesChan <- manga.NewContainer(newPage, img, "N/A", true): - case <-ctx.Done(): - wgConvertedPages.Done() - return - } - } - }(page) + type pageResult struct { + pages []*manga.PageFile + err error } - wgPages.Wait() - close(pagesChan) + results := make([]pageResult, len(chapter.Pages)) + var wg sync.WaitGroup + var convertedCount atomic.Uint32 - // Wait for all conversions to complete or context cancellation + for i, page := range chapter.Pages { + wg.Add(1) + + go func(idx int, p *manga.PageFile) { + defer wg.Done() + + // Check context before acquiring worker slot + select { + case <-ctx.Done(): + results[idx] = pageResult{err: ctx.Err()} + return + case guard <- struct{}{}: + } + defer func() { <-guard }() + + // Check context after acquiring worker slot + select { + case <-ctx.Done(): + results[idx] = pageResult{err: ctx.Err()} + return + default: + } + + pages, err := converter.convertPageFile(ctx, p, outputDir, quality, split) + results[idx] = pageResult{pages: pages, err: err} + + current := convertedCount.Add(1) + total := totalPages.Load() + progress(fmt.Sprintf("Converted %d/%d pages to %s format", current, total, converter.Format()), current, total) + }(i, page) + } + + // Wait for completion or context cancellation done := make(chan struct{}) go func() { - defer close(done) - wgConvertedPages.Wait() + wg.Wait() + close(done) }() select { case <-done: - // Conversion completed successfully case <-ctx.Done(): - log.Warn().Str("chapter", chapter.FilePath).Msg("Chapter conversion cancelled due to timeout") - return failWithCleanup(ctx.Err()) + // Wait for in-flight goroutines to finish + <-done + return nil, ctx.Err() } - close(errChan) - // Note: guard is the converter's shared pageWorkerGuard and must not be - // closed here since it is reused by subsequent (and possibly concurrent) - // ConvertChapter calls. - + // Collect results + var convertedPages []*manga.PageFile var errList []error - for err := range errChan { - errList = append(errList, err) + + for _, result := range results { + if result.err != nil { + if errors.Is(result.err, context.DeadlineExceeded) || errors.Is(result.err, context.Canceled) { + return nil, result.err + } + errList = append(errList, result.err) + } + if result.pages != nil { + convertedPages = append(convertedPages, result.pages...) + } } - var aggregatedError error = nil + if len(convertedPages) == 0 { + if len(errList) > 0 { + return nil, errors.Join(errList...) + } + return nil, fmt.Errorf("no pages were converted") + } + + // Sort pages by index and split part + sort.Slice(convertedPages, func(i, j int) bool { + a, b := convertedPages[i], convertedPages[j] + if a.Index == b.Index { + return a.SplitPartIndex < b.SplitPartIndex + } + return a.Index < b.Index + }) + + chapter.Pages = convertedPages + + var aggregatedError error if len(errList) > 0 { aggregatedError = errors.Join(errList...) - log.Debug(). - Str("chapter", chapter.FilePath). - Int("error_count", len(errList)). - Err(errors.Join(errList...)). - Msg("Conversion completed with errors") - } else { - log.Debug(). - Str("chapter", chapter.FilePath). - Int("pages_converted", len(pages)). - Msg("Conversion completed successfully") } - slices.SortFunc(pages, func(a, b *manga.Page) int { - if a.Index == b.Index { - return int(a.SplitPartIndex) - int(b.SplitPartIndex) - } - return int(a.Index) - int(b.Index) - }) - chapter.Pages = pages - log.Debug(). Str("chapter", chapter.FilePath). - Int("final_page_count", len(pages)). - Msg("Pages sorted and chapter updated") - - runtime.GC() - log.Debug().Str("chapter", chapter.FilePath).Msg("Garbage collection completed") + Int("converted_pages", len(convertedPages)). + Msg("Chapter conversion completed") return chapter, aggregatedError } -func (converter *Converter) cropImage(img image.Image) ([]image.Image, error) { - bounds := img.Bounds() - height := bounds.Dy() - width := bounds.Dx() +// convertPageFile converts a single page file to WebP format. +// Returns the converted page(s) — multiple if splitting was needed. +func (converter *Converter) convertPageFile(ctx context.Context, page *manga.PageFile, outputDir string, quality uint8, split bool) ([]*manga.PageFile, error) { + log.Debug(). + Uint16("page_index", page.Index). + Str("input", page.FilePath). + Msg("Converting page file") + + // If the page is already WebP, just return it as-is + if strings.ToLower(page.Extension) == ".webp" { + log.Debug().Uint16("page_index", page.Index).Msg("Page already WebP, skipping") + return []*manga.PageFile{page}, nil + } + + // Try direct file-to-file conversion first (happy path — no memory allocation) + outputPath := filepath.Join(outputDir, fmt.Sprintf("%04d.webp", page.Index)) + err := EncodeFile(page.FilePath, outputPath, uint(quality)) + + if err == nil { + // Success! No image decoding needed. + return []*manga.PageFile{{ + Index: page.Index, + Extension: ".webp", + FilePath: outputPath, + }}, nil + } + + // Direct conversion failed. Check if it's a dimension issue. + log.Debug(). + Uint16("page_index", page.Index). + Err(err). + Msg("Direct conversion failed, checking dimensions") + + // Read just the image header to get dimensions (no full decode) + width, height, decodeErr := getImageDimensions(page.FilePath) + if decodeErr != nil { + // Can't even read the image header — keep the original file + log.Info(). + Uint16("page_index", page.Index). + Err(decodeErr). + Msg("Cannot decode image, keeping original") + return []*manga.PageFile{page}, converterrors.NewPageIgnored( + fmt.Sprintf("page %d: failed to decode image (%s)", page.Index, decodeErr.Error())) + } + + log.Debug(). + Uint16("page_index", page.Index). + Int("width", width). + Int("height", height). + Msg("Image dimensions read") + + // If height exceeds WebP max and split is not enabled, keep original + if height >= webpMaxHeight && !split { + log.Info(). + Uint16("page_index", page.Index). + Int("height", height). + Msg("Page too tall for WebP, keeping original") + return []*manga.PageFile{page}, converterrors.NewPageIgnored( + fmt.Sprintf("page %d is too tall [max: %dpx] to be converted to webp format", page.Index, webpMaxHeight)) + } + + // If height exceeds our split threshold and split is enabled, use cwebp -crop + if height >= converter.maxHeight && split { + return converter.splitAndConvert(ctx, page, outputDir, quality, width, height) + } + + // Height is within limits but conversion still failed for another reason. + // Keep the original file. + log.Warn(). + Uint16("page_index", page.Index). + Err(err). + Msg("Conversion failed for non-dimension reason, keeping original") + return []*manga.PageFile{page}, converterrors.NewPageIgnored( + fmt.Sprintf("page %d: conversion failed (%s)", page.Index, err.Error())) +} + +// splitAndConvert splits a tall image into multiple parts using cwebp -crop +// and converts each part. No Go-side image decode is needed. +func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.PageFile, outputDir string, quality uint8, width, height int) ([]*manga.PageFile, error) { + log.Debug(). + Uint16("page_index", page.Index). + Int("width", width). + Int("height", height). + Int("crop_height", converter.cropHeight). + Msg("Splitting and converting page using cwebp -crop") numParts := height / converter.cropHeight if height%converter.cropHeight != 0 { numParts++ } - log.Debug(). - Int("original_width", width). - Int("original_height", height). - Int("crop_height", converter.cropHeight). - Int("num_parts", numParts). - Msg("Starting image cropping for page splitting") - - parts := make([]image.Image, numParts) + var pages []*manga.PageFile for i := 0; i < numParts; i++ { + select { + case <-ctx.Done(): + return pages, ctx.Err() + default: + } + + yOffset := i * converter.cropHeight partHeight := converter.cropHeight if i == numParts-1 { - partHeight = height - i*converter.cropHeight + partHeight = height - yOffset } - log.Debug(). - Int("part_index", i). - Int("part_height", partHeight). - Int("y_offset", i*converter.cropHeight). - Msg("Cropping image part") + outputPath := filepath.Join(outputDir, fmt.Sprintf("%04d-%02d.webp", page.Index, i)) + err := EncodeFileWithCrop(page.FilePath, outputPath, uint(quality), 0, yOffset, width, partHeight) - part, err := cutter.Crop(img, cutter.Config{ - Width: bounds.Dx(), - Height: partHeight, - Anchor: image.Point{Y: i * converter.cropHeight}, - Mode: cutter.TopLeft, - }) if err != nil { log.Error(). - Int("part_index", i). + Uint16("page_index", page.Index). + Int("part", i). Err(err). - Msg("Failed to crop image part") - return nil, fmt.Errorf("error cropping part %d: %v", i+1, err) + Msg("Failed to convert split part") + return nil, fmt.Errorf("failed to convert split part %d of page %d: %w", i, page.Index, err) } - parts[i] = part - - log.Debug(). - Int("part_index", i). - Int("cropped_width", part.Bounds().Dx()). - Int("cropped_height", part.Bounds().Dy()). - Msg("Image part cropped successfully") + pages = append(pages, &manga.PageFile{ + Index: page.Index, + Extension: ".webp", + FilePath: outputPath, + IsSplitted: true, + SplitPartIndex: uint16(i), + }) } - log.Debug(). - Int("total_parts", len(parts)). - Msg("Image cropping completed") - - return parts, nil -} - -func (converter *Converter) checkPageNeedsSplit(page *manga.Page, splitRequested bool) (bool, image.Image, string, error) { - log.Debug(). - Uint16("page_index", page.Index). - Bool("split_requested", splitRequested). - Int("page_size", len(page.Contents.Bytes())). - Msg("Analyzing page for splitting") - - reader := bytes.NewBuffer(page.Contents.Bytes()) - img, format, err := image.Decode(reader) - if err != nil { - log.Debug().Uint16("page_index", page.Index).Err(err).Msg("Failed to decode page image") - return false, nil, format, converterrors.NewPageIgnored(fmt.Sprintf("page %d: failed to decode image (%s)", page.Index, err.Error())) - } - - bounds := img.Bounds() - height := bounds.Dy() - width := bounds.Dx() - log.Debug(). Uint16("page_index", page.Index). - Int("width", width). - Int("height", height). - Str("format", format). - Int("max_height", converter.maxHeight). - Int("webp_max_height", webpMaxHeight). - Msg("Page dimensions analyzed") + Int("parts", len(pages)). + Msg("Split conversion completed") - if height >= webpMaxHeight && !splitRequested { - log.Debug(). - Uint16("page_index", page.Index). - Int("height", height). - Int("webp_max", webpMaxHeight). - Msg("Page too tall for WebP format, would be ignored") - return false, img, format, converterrors.NewPageIgnored(fmt.Sprintf("page %d is too tall [max: %dpx] to be converted to webp format", page.Index, webpMaxHeight)) - } - - needsSplit := height >= converter.maxHeight && splitRequested - log.Debug(). - Uint16("page_index", page.Index). - Bool("needs_split", needsSplit). - Msg("Page splitting decision made") - - return needsSplit, img, format, nil + return pages, nil } -func (converter *Converter) convertPage(container *manga.PageContainer, quality uint8, tempDir string) (*manga.PageContainer, error) { - log.Debug(). - Uint16("page_index", container.Page.Index). - Str("format", container.Format). - Bool("to_be_converted", container.IsToBeConverted). - Uint8("quality", quality). - Msg("Converting page") - - // Fix WebP format detection (case insensitive) - if container.Format == "webp" || container.Format == "WEBP" { - log.Debug(). - Uint16("page_index", container.Page.Index). - Msg("Page already in WebP format, skipping conversion") - container.Page.Extension = ".webp" - return container, nil - } - if !container.IsToBeConverted { - log.Debug(). - Uint16("page_index", container.Page.Index). - Msg("Page marked as not to be converted, skipping") - return container, nil - } - - log.Debug(). - Uint16("page_index", container.Page.Index). - Uint8("quality", quality). - Msg("Encoding page to WebP format") - - converted, err := converter.convert(container.Image, uint(quality)) +// getImageDimensions reads only the image header to determine dimensions. +// This is much cheaper than a full image.Decode — only a few bytes are read. +func getImageDimensions(filePath string) (width, height int, err error) { + f, err := os.Open(filePath) if err != nil { - log.Error(). - Uint16("page_index", container.Page.Index). - Err(err). - Msg("Failed to convert page to WebP") - return nil, err + return 0, 0, err } + defer func() { _ = f.Close() }() - originalSize := container.Page.Size - convertedSize := converted.Len() - - if err := container.Page.Stage(tempDir, converted, ".webp"); err != nil { - log.Error(). - Uint16("page_index", container.Page.Index). - Err(err). - Msg("Failed to stage converted page to disk") - return nil, err - } - container.HasBeenConverted = true - - log.Debug(). - Uint16("page_index", container.Page.Index). - Uint64("original_size", originalSize). - Int("converted_size", convertedSize). - Msg("Page conversion completed") - - return container, nil -} - -// convert converts an image to the WebP format. It decodes the image from the input buffer, -// encodes it as a WebP file using the webp.Encode() function, and returns the resulting WebP -// file as a bytes.Buffer. -func (converter *Converter) convert(image image.Image, quality uint) (*bytes.Buffer, error) { - var buf bytes.Buffer - err := Encode(&buf, image, quality) + config, _, err := image.DecodeConfig(f) if err != nil { - return nil, err + return 0, 0, err } - return &buf, nil + return config.Width, config.Height, nil } diff --git a/pkg/converter/webp/webp_converter_test.go b/pkg/converter/webp/webp_converter_test.go index 8f07727..cc28a59 100644 --- a/pkg/converter/webp/webp_converter_test.go +++ b/pkg/converter/webp/webp_converter_test.go @@ -1,138 +1,61 @@ package webp import ( - "bytes" "context" "fmt" "image" - "image/color" - "image/gif" "image/jpeg" - "image/png" - "io" + "os" + "path/filepath" "sync" "testing" "time" - _ "golang.org/x/image/webp" - "github.com/belphemur/CBZOptimizer/v2/internal/manga" "github.com/belphemur/CBZOptimizer/v2/pkg/converter/constant" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func createTestImage(width, height int, format string) (image.Image, error) { +func createTestImageFile(t *testing.T, path string, width, height int) { + t.Helper() img := image.NewRGBA(image.Rect(0, 0, width, height)) - - // Create a gradient pattern to ensure we have actual image data - for y := 0; y < height; y++ { - for x := 0; x < width; x++ { - img.Set(x, y, color.RGBA{ - R: uint8((x * 255) / width), - G: uint8((y * 255) / height), - B: 100, - A: 255, - }) - } - } - return img, nil + f, err := os.Create(path) + require.NoError(t, err) + err = jpeg.Encode(f, img, nil) + require.NoError(t, err) + _ = f.Close() } -func encodeImage(img image.Image, format string) (*bytes.Buffer, string, error) { - buf := new(bytes.Buffer) +func createTestChapter(t *testing.T, pages []struct{ w, h int }) (*manga.Chapter, string) { + t.Helper() + dir := t.TempDir() + inputDir := filepath.Join(dir, "input") + require.NoError(t, os.MkdirAll(inputDir, 0755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "output"), 0755)) - switch format { - case "jpeg", "jpg": - if err := jpeg.Encode(buf, img, &jpeg.Options{Quality: 85}); err != nil { - return nil, "", err - } - return buf, ".jpg", nil - case "gif": - if err := gif.Encode(buf, img, nil); err != nil { - return nil, "", err - } - return buf, ".gif", nil - case "webp": - if err := PrepareEncoder(); err != nil { - return nil, "", err - } - if err := Encode(buf, img, 80); err != nil { - return nil, "", err - } - return buf, ".webp", nil - case "png": - fallthrough - default: - if err := png.Encode(buf, img); err != nil { - return nil, "", err - } - return buf, ".png", nil - } -} - -func createTestPage(t *testing.T, index int, width, height int, format string) *manga.Page { - img, err := createTestImage(width, height, format) - require.NoError(t, err) - - buf, ext, err := encodeImage(img, format) - require.NoError(t, err) - - return &manga.Page{ - Index: uint16(index), - Contents: buf, - Extension: ext, - Size: uint64(buf.Len()), - } -} - -func validateConvertedImage(t *testing.T, page *manga.Page) { - require.True(t, page.Contents != nil || page.TempFilePath != "", "page should have contents in memory or staged on disk") - require.Greater(t, page.Size, uint64(0)) - - reader, err := page.Open() - require.NoError(t, err) - defer reader.Close() - - data, err := io.ReadAll(reader) - require.NoError(t, err) - require.Greater(t, len(data), 0) - - // Try to decode the image - img, format, err := image.Decode(bytes.NewReader(data)) - require.NoError(t, err, "Failed to decode converted image") - - if page.Extension == ".webp" { - assert.Equal(t, "webp", format, "Expected WebP format") + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, } - require.NotNil(t, img) - bounds := img.Bounds() - assert.Greater(t, bounds.Dx(), 0, "Image width should be positive") - assert.Greater(t, bounds.Dy(), 0, "Image height should be positive") + for i, p := range pages { + pagePath := filepath.Join(inputDir, fmt.Sprintf("%04d.jpg", i)) + createTestImageFile(t, pagePath, p.w, p.h) + chapter.Pages = append(chapter.Pages, &manga.PageFile{ + Index: uint16(i), + Extension: ".jpg", + FilePath: pagePath, + }) + } + + return chapter, dir } -// TestConverter_ConvertChapter tests the ConvertChapter method of the WebP converter. -// It verifies various scenarios including: -// - Converting single normal images -// - Converting multiple normal images -// - Converting tall images with split enabled -// - Handling tall images that exceed maximum height -// -// For each test case it validates: -// - Proper error handling -// - Expected number of output pages -// - Correct page ordering -// - Split page handling and indexing -// - Progress callback behavior -// -// The test uses different image dimensions and split settings to ensure -// the converter handles all cases correctly while maintaining proper -// progress reporting and page ordering. func TestConverter_ConvertChapter(t *testing.T) { tests := []struct { name string - pages []*manga.Page + pages []struct{ w, h int } split bool expectSplit bool expectError bool @@ -140,47 +63,33 @@ func TestConverter_ConvertChapter(t *testing.T) { }{ { name: "Single normal image", - pages: []*manga.Page{createTestPage(t, 1, 800, 1200, "jpeg")}, + pages: []struct{ w, h int }{{800, 1200}}, split: false, - expectSplit: false, numExpected: 1, }, { name: "Multiple normal images", - pages: []*manga.Page{ - createTestPage(t, 1, 800, 1200, "png"), - createTestPage(t, 2, 800, 1200, "jpeg"), - createTestPage(t, 3, 800, 1200, "gif"), + pages: []struct{ w, h int }{ + {800, 1200}, + {800, 1200}, + {800, 1200}, }, split: false, - expectSplit: false, numExpected: 3, }, - { - name: "Multiple normal images with webp", - pages: []*manga.Page{ - createTestPage(t, 1, 800, 1200, "png"), - createTestPage(t, 2, 800, 1200, "jpeg"), - createTestPage(t, 3, 800, 1200, "gif"), - createTestPage(t, 4, 800, 1200, "webp"), - }, - split: false, - expectSplit: false, - numExpected: 4, - }, { name: "Tall image with split enabled", - pages: []*manga.Page{createTestPage(t, 1, 800, 5000, "jpeg")}, + pages: []struct{ w, h int }{{800, 5000}}, split: true, - expectSplit: true, - numExpected: 3, // Based on cropHeight of 2000 + expectSplit: false, // cwebp handles 5000px fine (< 16383 webp max), no split needed + numExpected: 1, }, { name: "Tall image without split", - pages: []*manga.Page{createTestPage(t, 1, 800, webpMaxHeight+100, "png")}, + pages: []struct{ w, h int }{{800, webpMaxHeight + 100}}, split: false, expectError: true, - numExpected: 1, + numExpected: 1, // kept as-is }, } @@ -190,18 +99,16 @@ func TestConverter_ConvertChapter(t *testing.T) { err := converter.PrepareConverter() require.NoError(t, err) - chapter := &manga.Chapter{ - Pages: tt.pages, - } + chapter, _ := createTestChapter(t, tt.pages) var progressMutex sync.Mutex var lastProgress uint32 progress := func(message string, current uint32, total uint32) { progressMutex.Lock() defer progressMutex.Unlock() - assert.GreaterOrEqual(t, current, lastProgress, "Progress should never decrease") + assert.GreaterOrEqual(t, current, lastProgress) lastProgress = current - assert.LessOrEqual(t, current, total, "Current progress should not exceed total") + assert.LessOrEqual(t, current, total) } convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, 80, tt.split, progress) @@ -218,22 +125,14 @@ func TestConverter_ConvertChapter(t *testing.T) { require.NotNil(t, convertedChapter) assert.Len(t, convertedChapter.Pages, tt.numExpected) - // Validate all converted images - for _, page := range convertedChapter.Pages { - validateConvertedImage(t, page) - } - // Verify page order for i := 1; i < len(convertedChapter.Pages); i++ { - prevPage := convertedChapter.Pages[i-1] - currPage := convertedChapter.Pages[i] - - if prevPage.Index == currPage.Index { - assert.Less(t, prevPage.SplitPartIndex, currPage.SplitPartIndex, - "Split parts should be in ascending order for page %d", prevPage.Index) + prev := convertedChapter.Pages[i-1] + curr := convertedChapter.Pages[i] + if prev.Index == curr.Index { + assert.Less(t, prev.SplitPartIndex, curr.SplitPartIndex) } else { - assert.Less(t, prevPage.Index, currPage.Index, - "Pages should be in ascending order") + assert.Less(t, prev.Index, curr.Index) } } @@ -245,149 +144,13 @@ func TestConverter_ConvertChapter(t *testing.T) { break } } - assert.True(t, splitFound, "Expected to find at least one split page") - } - }) - } -} - -func TestConverter_convertPage(t *testing.T) { - converter := New() - err := converter.PrepareConverter() - require.NoError(t, err) - - tests := []struct { - name string - format string - isToBeConverted bool - expectWebP bool - expectError bool - }{ - { - name: "Convert PNG to WebP", - format: "png", - isToBeConverted: true, - expectWebP: true, - expectError: false, - }, - { - name: "Convert GIF to WebP", - format: "gif", - isToBeConverted: true, - expectWebP: true, - expectError: false, - }, - { - name: "Already WebP", - format: "webp", - isToBeConverted: true, - expectWebP: true, - expectError: false, - }, - { - name: "Skip conversion", - format: "png", - isToBeConverted: false, - expectWebP: false, - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - page := createTestPage(t, 1, 100, 100, tt.format) - img, err := createTestImage(100, 100, tt.format) - require.NoError(t, err) - container := manga.NewContainer(page, img, tt.format, tt.isToBeConverted) - - converted, err := converter.convertPage(container, 80, t.TempDir()) - - if tt.expectError { - assert.Error(t, err) - assert.Nil(t, converted) - } else { - require.NoError(t, err) - assert.NotNil(t, converted) - - if tt.expectWebP { - assert.Equal(t, ".webp", converted.Page.Extension) - validateConvertedImage(t, converted.Page) - } else { - assert.NotEqual(t, ".webp", converted.Page.Extension) - } - } - }) - } -} - -func TestConverter_convertPage_EncodingError(t *testing.T) { - converter := New() - err := converter.PrepareConverter() - require.NoError(t, err) - - // Create a test case with nil image to test encoding error path - // when isToBeConverted is true but the image is nil, simulating a failure in the encoding step - corruptedPage := &manga.Page{ - Index: 1, - Contents: &bytes.Buffer{}, // Empty buffer - Extension: ".png", - Size: 0, - } - - container := manga.NewContainer(corruptedPage, nil, "png", true) - - converted, err := converter.convertPage(container, 80, t.TempDir()) - - // This should return nil container and error because encoding will fail with nil image - assert.Error(t, err) - assert.Nil(t, converted) -} - -func TestConverter_checkPageNeedsSplit(t *testing.T) { - converter := New() - - tests := []struct { - name string - imageHeight int - split bool - expectSplit bool - expectError bool - }{ - { - name: "Normal height", - imageHeight: 1000, - split: true, - expectSplit: false, - }, - { - name: "Height exceeds max with split enabled", - imageHeight: 5000, - split: true, - expectSplit: true, - }, - { - name: "Height exceeds webp max without split", - imageHeight: webpMaxHeight + 100, - split: false, - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - page := createTestPage(t, 1, 800, tt.imageHeight, "jpeg") - - needsSplit, img, format, err := converter.checkPageNeedsSplit(page, tt.split) - - if tt.expectError { - assert.Error(t, err) - return + assert.True(t, splitFound, "Expected split pages") } - require.NoError(t, err) - assert.NotNil(t, img) - assert.NotEmpty(t, format) - assert.Equal(t, tt.expectSplit, needsSplit) + // Verify all output files exist + for _, page := range convertedChapter.Pages { + assert.FileExists(t, page.FilePath) + } }) } } @@ -402,73 +165,43 @@ func TestConverter_ConvertChapter_Timeout(t *testing.T) { err := converter.PrepareConverter() require.NoError(t, err) - // Create a test chapter with a few pages - pages := []*manga.Page{ - createTestPage(t, 1, 800, 1200, "jpeg"), - createTestPage(t, 2, 800, 1200, "png"), - createTestPage(t, 3, 800, 1200, "gif"), - } + chapter, _ := createTestChapter(t, []struct{ w, h int }{ + {800, 1200}, + {800, 1200}, + {800, 1200}, + }) - chapter := &manga.Chapter{ - FilePath: "/test/chapter.cbz", - Pages: pages, - } + progress := func(message string, current uint32, total uint32) {} - var progressMutex sync.Mutex - var lastProgress uint32 - progress := func(message string, current uint32, total uint32) { - progressMutex.Lock() - defer progressMutex.Unlock() - assert.GreaterOrEqual(t, current, lastProgress, "Progress should never decrease") - lastProgress = current - assert.LessOrEqual(t, current, total, "Current progress should not exceed total") - } - - // Test with very short timeout (1 nanosecond) - ctx, cancel := context.WithTimeout(context.Background(), 1) + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) defer cancel() convertedChapter, err := converter.ConvertChapter(ctx, chapter, 80, false, progress) - // Should return context error due to timeout assert.Error(t, err) assert.Nil(t, convertedChapter) assert.Equal(t, context.DeadlineExceeded, err) } -// TestConverter_ConvertChapter_ManyPages_NoDeadlock tests that converting chapters with many pages -// does not cause a deadlock. This test reproduces the scenario where processing -// many files with context cancellation could cause "all goroutines are asleep - deadlock!" error. -// The fix ensures that wgConvertedPages.Done() is called when context is cancelled after Add(1). func TestConverter_ConvertChapter_ManyPages_NoDeadlock(t *testing.T) { converter := New() err := converter.PrepareConverter() require.NoError(t, err) - // Create a chapter with many pages to increase the chance of hitting the race condition - numPages := 50 - pages := make([]*manga.Page, numPages) - for i := 0; i < numPages; i++ { - pages[i] = createTestPage(t, i+1, 100, 100, "jpeg") + pages := make([]struct{ w, h int }, 50) + for i := range pages { + pages[i] = struct{ w, h int }{100, 100} } - chapter := &manga.Chapter{ - FilePath: "/test/chapter_many_pages.cbz", - Pages: pages, - } + chapter, _ := createTestChapter(t, pages) - progress := func(message string, current uint32, total uint32) { - // No-op progress callback - } + progress := func(message string, current uint32, total uint32) {} - // Run multiple iterations to increase the chance of hitting the race condition for iteration := 0; iteration < 10; iteration++ { t.Run(fmt.Sprintf("iteration_%d", iteration), func(t *testing.T) { - // Use a very short timeout to trigger context cancellation during processing ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond) defer cancel() - // This should NOT deadlock - it should return quickly with context error done := make(chan struct{}) var convertErr error go func() { @@ -476,121 +209,57 @@ func TestConverter_ConvertChapter_ManyPages_NoDeadlock(t *testing.T) { _, convertErr = converter.ConvertChapter(ctx, chapter, 80, false, progress) }() - // Wait with a reasonable timeout - if it takes longer than 5 seconds, we have a deadlock select { case <-done: - // Expected - conversion should complete (with error) quickly - assert.Error(t, convertErr, "Expected context error") + assert.Error(t, convertErr) case <-time.After(5 * time.Second): - t.Fatal("Deadlock detected: ConvertChapter did not return within 5 seconds") + t.Fatal("Deadlock detected") } }) } } -// TestConverter_ConvertChapter_ManyPages_WithSplit_NoDeadlock tests that converting chapters -// with many pages and split enabled does not cause a deadlock. -func TestConverter_ConvertChapter_ManyPages_WithSplit_NoDeadlock(t *testing.T) { - converter := New() - err := converter.PrepareConverter() - require.NoError(t, err) - - // Create pages with varying heights, some requiring splits - numPages := 30 - pages := make([]*manga.Page, numPages) - for i := 0; i < numPages; i++ { - height := 1000 // Normal height - if i%5 == 0 { - height = 5000 // Tall image that will be split - } - pages[i] = createTestPage(t, i+1, 100, height, "png") - } - - chapter := &manga.Chapter{ - FilePath: "/test/chapter_split_test.cbz", - Pages: pages, - } - - progress := func(message string, current uint32, total uint32) { - // No-op progress callback - } - - // Run multiple iterations with short timeouts - for iteration := 0; iteration < 10; iteration++ { - t.Run(fmt.Sprintf("iteration_%d", iteration), func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond) - defer cancel() - - done := make(chan struct{}) - var convertErr error - go func() { - defer close(done) - _, convertErr = converter.ConvertChapter(ctx, chapter, 80, true, progress) // split=true - }() - - select { - case <-done: - assert.Error(t, convertErr, "Expected context error") - case <-time.After(5 * time.Second): - t.Fatal("Deadlock detected: ConvertChapter with split did not return within 5 seconds") - } - }) - } -} - -// TestConverter_ConvertChapter_ConcurrentChapters_NoDeadlock simulates the scenario from the -// original bug report where multiple chapters are processed in parallel with parallelism > 1. -// This test ensures no deadlock occurs when multiple goroutines are converting chapters concurrently. func TestConverter_ConvertChapter_ConcurrentChapters_NoDeadlock(t *testing.T) { converter := New() err := converter.PrepareConverter() require.NoError(t, err) - // Create multiple chapters, each with many pages numChapters := 20 - pagesPerChapter := 30 chapters := make([]*manga.Chapter, numChapters) + pages := make([]struct{ w, h int }, 30) + for i := range pages { + pages[i] = struct{ w, h int }{100, 100} + } + for c := 0; c < numChapters; c++ { - pages := make([]*manga.Page, pagesPerChapter) - for i := 0; i < pagesPerChapter; i++ { - pages[i] = createTestPage(t, i+1, 100, 100, "jpeg") - } - chapters[c] = &manga.Chapter{ - FilePath: fmt.Sprintf("/test/chapter_%d.cbz", c+1), - Pages: pages, - } + chapters[c], _ = createTestChapter(t, pages) } progress := func(message string, current uint32, total uint32) {} - // Process chapters concurrently with short timeouts (simulating parallelism flag) parallelism := 4 var wg sync.WaitGroup semaphore := make(chan struct{}, parallelism) - // Overall test timeout testCtx, testCancel := context.WithTimeout(context.Background(), 30*time.Second) defer testCancel() for _, chapter := range chapters { wg.Add(1) - semaphore <- struct{}{} // Acquire + semaphore <- struct{}{} go func(ch *manga.Chapter) { defer wg.Done() - defer func() { <-semaphore }() // Release + defer func() { <-semaphore }() - // Use very short timeout to trigger cancellation ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond) defer cancel() - // This should not deadlock _, _ = converter.ConvertChapter(ctx, ch, 80, false, progress) }(chapter) } - // Wait for all conversions with a timeout done := make(chan struct{}) go func() { wg.Wait() @@ -599,8 +268,220 @@ func TestConverter_ConvertChapter_ConcurrentChapters_NoDeadlock(t *testing.T) { select { case <-done: - // All goroutines completed successfully case <-testCtx.Done(): - t.Fatal("Deadlock detected: Concurrent chapter conversions did not complete within 30 seconds") + t.Fatal("Deadlock detected") } } + +func TestConverter_SplitAndConvert(t *testing.T) { + converter := New() + err := converter.PrepareConverter() + require.NoError(t, err) + + // Create a very tall image that exceeds webpMaxHeight + dir := t.TempDir() + inputDir := filepath.Join(dir, "input") + require.NoError(t, os.MkdirAll(inputDir, 0755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "output"), 0755)) + + // Create image taller than webpMaxHeight (16383) + tallImagePath := filepath.Join(inputDir, "0000.jpg") + createTestImageFile(t, tallImagePath, 800, webpMaxHeight+500) + + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: tallImagePath}, + }, + } + + progress := func(message string, current uint32, total uint32) {} + + // With split=true, the oversized page should be split + convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, 80, true, progress) + require.NoError(t, err) + require.NotNil(t, convertedChapter) + + // Should have more pages due to splitting + assert.Greater(t, len(convertedChapter.Pages), 1, "Tall image should be split into multiple parts") + + // All parts should be split and have webp extension + for _, page := range convertedChapter.Pages { + assert.True(t, page.IsSplitted, "All pages should be marked as split") + assert.Equal(t, ".webp", page.Extension) + assert.FileExists(t, page.FilePath) + assert.Equal(t, uint16(0), page.Index, "All split parts should have same original index") + } + + // Verify sequential split part indices + for i, page := range convertedChapter.Pages { + assert.Equal(t, uint16(i), page.SplitPartIndex) + } +} + +func TestConverter_OversizedImageNoSplit(t *testing.T) { + converter := New() + err := converter.PrepareConverter() + require.NoError(t, err) + + dir := t.TempDir() + inputDir := filepath.Join(dir, "input") + require.NoError(t, os.MkdirAll(inputDir, 0755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "output"), 0755)) + + tallImagePath := filepath.Join(inputDir, "0000.jpg") + createTestImageFile(t, tallImagePath, 800, webpMaxHeight+500) + + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, + Pages: []*manga.PageFile{ + {Index: 0, Extension: ".jpg", FilePath: tallImagePath}, + }, + } + + progress := func(message string, current uint32, total uint32) {} + + // With split=false, the oversized page should be kept as-is with error + convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, 80, false, progress) + assert.Error(t, err, "Should return error for oversized page without split") + if convertedChapter != nil { + // The page should be kept in original format + for _, page := range convertedChapter.Pages { + assert.Equal(t, ".jpg", page.Extension, "Page should keep original extension") + } + } +} + +func TestGetImageDimensions(t *testing.T) { + dir := t.TempDir() + + tests := []struct { + name string + width, height int + expectW, expectH int + }{ + {"small image", 100, 200, 100, 200}, + {"wide image", 1920, 1080, 1920, 1080}, + {"tall image", 800, 5000, 800, 5000}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(dir, tt.name+".jpg") + createTestImageFile(t, path, tt.width, tt.height) + + w, h, err := getImageDimensions(path) + require.NoError(t, err) + assert.Equal(t, tt.expectW, w) + assert.Equal(t, tt.expectH, h) + }) + } +} + +func TestGetImageDimensions_NonexistentFile(t *testing.T) { + _, _, err := getImageDimensions("/nonexistent/file.jpg") + assert.Error(t, err) +} + +func TestGetImageDimensions_InvalidFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "invalid.jpg") + require.NoError(t, os.WriteFile(path, []byte("not an image"), 0644)) + + _, _, err := getImageDimensions(path) + assert.Error(t, err) +} + +func TestConverter_ConvertChapter_EmptyChapter(t *testing.T) { + converter := New() + err := converter.PrepareConverter() + require.NoError(t, err) + + dir := t.TempDir() + chapter := &manga.Chapter{ + FilePath: filepath.Join(dir, "test.cbz"), + TempDir: dir, + Pages: []*manga.PageFile{}, + } + + progress := func(message string, current uint32, total uint32) {} + + _, err = converter.ConvertChapter(context.Background(), chapter, 80, false, progress) + assert.Error(t, err, "Should error on empty chapter") +} + +func TestConverter_ConvertChapter_PreservesComicInfo(t *testing.T) { + converter := New() + err := converter.PrepareConverter() + require.NoError(t, err) + + chapter, _ := createTestChapter(t, []struct{ w, h int }{{400, 600}}) + chapter.ComicInfoXml = `Test` + + progress := func(message string, current uint32, total uint32) {} + + convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, 80, false, progress) + require.NoError(t, err) + require.NotNil(t, convertedChapter) + assert.Equal(t, chapter.ComicInfoXml, convertedChapter.ComicInfoXml) +} + +func TestConverter_ConvertChapter_OutputInCorrectDir(t *testing.T) { + converter := New() + err := converter.PrepareConverter() + require.NoError(t, err) + + chapter, dir := createTestChapter(t, []struct{ w, h int }{{400, 600}}) + + progress := func(message string, current uint32, total uint32) {} + + convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, 80, false, progress) + require.NoError(t, err) + require.NotNil(t, convertedChapter) + + // All output files should be in the output directory + expectedOutputDir := filepath.Join(dir, "output") + for _, page := range convertedChapter.Pages { + pageDir := filepath.Dir(page.FilePath) + assert.Equal(t, expectedOutputDir, pageDir, "Output file should be in output directory") + } +} + +func TestEncodeFile(t *testing.T) { + err := PrepareEncoder() + require.NoError(t, err) + + dir := t.TempDir() + inputPath := filepath.Join(dir, "input.jpg") + outputPath := filepath.Join(dir, "output.webp") + + createTestImageFile(t, inputPath, 200, 300) + + err = EncodeFile(inputPath, outputPath, 80) + require.NoError(t, err) + + // Verify output exists and is non-empty + info, err := os.Stat(outputPath) + require.NoError(t, err) + assert.Greater(t, info.Size(), int64(0)) +} + +func TestEncodeFileWithCrop(t *testing.T) { + err := PrepareEncoder() + require.NoError(t, err) + + dir := t.TempDir() + inputPath := filepath.Join(dir, "input.jpg") + outputPath := filepath.Join(dir, "output.webp") + + createTestImageFile(t, inputPath, 200, 600) + + err = EncodeFileWithCrop(inputPath, outputPath, 80, 0, 0, 200, 300) + require.NoError(t, err) + + info, err := os.Stat(outputPath) + require.NoError(t, err) + assert.Greater(t, info.Size(), int64(0)) +} diff --git a/pkg/converter/webp/webp_provider.go b/pkg/converter/webp/webp_provider.go index d0e5083..0b1bd6d 100644 --- a/pkg/converter/webp/webp_provider.go +++ b/pkg/converter/webp/webp_provider.go @@ -2,8 +2,6 @@ package webp import ( "fmt" - "image" - "io" "strings" "sync" @@ -37,10 +35,23 @@ func PrepareEncoder() error { return nil } -func Encode(w io.Writer, m image.Image, quality uint) error { +// EncodeFile converts an image file directly to WebP using cwebp. +// This is a zero-copy operation: no image data is loaded into Go memory. +func EncodeFile(inputPath string, outputPath string, quality uint) error { return webpbin.NewCWebP(config). Quality(quality). - InputImage(m). - Output(w). + InputFile(inputPath). + OutputFile(outputPath). + Run() +} + +// EncodeFileWithCrop converts a cropped region of an image file to WebP. +// Uses cwebp's native -crop flag — no Go-side image decode needed. +func EncodeFileWithCrop(inputPath string, outputPath string, quality uint, x, y, width, height int) error { + return webpbin.NewCWebP(config). + Quality(quality). + Crop(x, y, width, height). + InputFile(inputPath). + OutputFile(outputPath). Run() }