diff --git a/cmd/cbzoptimizer/commands/optimize_command.go b/cmd/cbzoptimizer/commands/optimize_command.go index ed7cc68..9149f3a 100644 --- a/cmd/cbzoptimizer/commands/optimize_command.go +++ b/cmd/cbzoptimizer/commands/optimize_command.go @@ -113,8 +113,9 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error { // Channel to manage the files to process fileChan := make(chan string) - // Channel to collect errors - errorChan := make(chan error, parallelism) + // Slice to collect errors with mutex for thread safety + var errs []error + var errMutex sync.Mutex // WaitGroup to wait for all goroutines to finish var wg sync.WaitGroup @@ -138,7 +139,9 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error { }) if err != nil { log.Error().Int("worker_id", workerID).Str("file_path", path).Err(err).Msg("Worker encountered error") - errorChan <- fmt.Errorf("error processing file %s: %w", path, err) + errMutex.Lock() + errs = append(errs, fmt.Errorf("error processing file %s: %w", path, err)) + errMutex.Unlock() } else { log.Debug().Int("worker_id", workerID).Str("file_path", path).Msg("Worker completed file successfully") } @@ -177,13 +180,6 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error { log.Debug().Msg("File channel closed, waiting for workers to complete") wg.Wait() // Wait for all workers to finish log.Debug().Msg("All workers completed") - close(errorChan) // Close the error channel - - var errs []error - for err := range errorChan { - errs = append(errs, err) - log.Error().Err(err).Msg("Collected processing error") - } if len(errs) > 0 { log.Error().Int("error_count", len(errs)).Msg("Command completed with errors") diff --git a/pkg/converter/converter_test.go b/pkg/converter/converter_test.go index 6563f86..35c9246 100644 --- a/pkg/converter/converter_test.go +++ b/pkg/converter/converter_test.go @@ -10,53 +10,48 @@ import ( "github.com/belphemur/CBZOptimizer/v2/internal/manga" "github.com/belphemur/CBZOptimizer/v2/internal/utils/errs" - "github.com/belphemur/CBZOptimizer/v2/pkg/converter/constant" - "golang.org/x/exp/slices" ) func TestConvertChapter(t *testing.T) { testCases := []struct { - name string - genTestChapter func(path string) (*manga.Chapter, error) - split bool - expectFailure []constant.ConversionFormat - expectPartialSuccess []constant.ConversionFormat + name string + genTestChapter func(path string, isSplit bool) (*manga.Chapter, []string, error) + split bool + expectError bool }{ { - name: "All split pages", - genTestChapter: genHugePage, - split: true, - expectFailure: []constant.ConversionFormat{}, - expectPartialSuccess: []constant.ConversionFormat{}, + name: "All split pages", + genTestChapter: genHugePage, + split: true, }, { - name: "Big Pages, no split", - genTestChapter: genHugePage, - split: false, - expectFailure: []constant.ConversionFormat{constant.WebP}, - expectPartialSuccess: []constant.ConversionFormat{}, + name: "Big Pages, no split", + genTestChapter: genHugePage, + split: false, + expectError: true, }, { - name: "No split pages", - genTestChapter: genSmallPages, - split: false, - expectFailure: []constant.ConversionFormat{}, - expectPartialSuccess: []constant.ConversionFormat{}, + name: "No split pages", + genTestChapter: genSmallPages, + split: false, }, { - name: "Mix of split and no split pages", - genTestChapter: genMixSmallBig, - split: true, - expectFailure: []constant.ConversionFormat{}, - expectPartialSuccess: []constant.ConversionFormat{}, + name: "Mix of split and no split pages", + genTestChapter: genMixSmallBig, + split: true, }, { - name: "Mix of Huge and small page", - genTestChapter: genMixSmallHuge, - split: false, - expectFailure: []constant.ConversionFormat{}, - expectPartialSuccess: []constant.ConversionFormat{constant.WebP}, + name: "Mix of Huge and small page", + genTestChapter: genMixSmallHuge, + split: false, + expectError: true, + }, + { + name: "Two corrupted pages", + genTestChapter: genTwoCorrupted, + split: false, + expectError: true, }, } // Load test genTestChapter from testdata @@ -74,7 +69,7 @@ func TestConvertChapter(t *testing.T) { t.Run(converter.Format().String(), func(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - chapter, err := tc.genTestChapter(temp.Name()) + chapter, expectedExtensions, err := tc.genTestChapter(temp.Name(), tc.split) if err != nil { t.Fatalf("failed to load test genTestChapter: %v", err) } @@ -86,31 +81,23 @@ func TestConvertChapter(t *testing.T) { } convertedChapter, err := converter.ConvertChapter(context.Background(), chapter, quality, tc.split, progress) - if err != nil { - if convertedChapter != nil && slices.Contains(tc.expectPartialSuccess, converter.Format()) { - t.Logf("Partial success to convert genTestChapter: %v", err) - return - } - if slices.Contains(tc.expectFailure, converter.Format()) { - t.Logf("Expected failure to convert genTestChapter: %v", err) - return - } + if err != nil && !tc.expectError { t.Fatalf("failed to convert genTestChapter: %v", err) - } else if slices.Contains(tc.expectFailure, converter.Format()) { - t.Fatalf("expected failure to convert genTestChapter didn't happen") } if len(convertedChapter.Pages) == 0 { t.Fatalf("no pages were converted") } - if len(convertedChapter.Pages) != len(chapter.Pages) { - t.Fatalf("converted chapter has different number of pages") + if len(convertedChapter.Pages) != len(expectedExtensions) { + t.Fatalf("converted chapter has %d pages but expected %d", len(convertedChapter.Pages), len(expectedExtensions)) } - for _, page := range convertedChapter.Pages { - if page.Extension != ".webp" { - t.Errorf("page %d was not converted to webp format", page.Index) + // Check each page's extension against the expected array + for i, page := range convertedChapter.Pages { + expectedExt := expectedExtensions[i] + if page.Extension != expectedExt { + t.Errorf("page %d has extension %s but expected %s", page.Index, page.Extension, expectedExt) } } }) @@ -119,39 +106,43 @@ func TestConvertChapter(t *testing.T) { } } -func genHugePage(path string) (*manga.Chapter, error) { +func genHugePage(path string, isSplit bool) (*manga.Chapter, []string, error) { file, err := os.Open(path) if err != nil { - return nil, err + return nil, nil, err } defer errs.Capture(&err, file.Close, "failed to close file") var pages []*manga.Page - for i := 0; i < 1; i++ { // Assuming there are 5 pages for the test - img := image.NewRGBA(image.Rect(0, 0, 1, 17000)) - buf := new(bytes.Buffer) - err := jpeg.Encode(buf, img, nil) - if err != nil { - return nil, err - } - page := &manga.Page{ - Index: uint16(i), - Contents: buf, - Extension: ".jpg", - } - pages = append(pages, 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) + if err != nil { + return nil, nil, err + } + page := &manga.Page{ + Index: 0, + Contents: buf, + Extension: ".jpg", + } + pages = append(pages, page) + return &manga.Chapter{ FilePath: path, Pages: pages, - }, nil + }, expectedExtensions, nil } -func genSmallPages(path string) (*manga.Chapter, error) { +func genSmallPages(path string, isSplit bool) (*manga.Chapter, []string, error) { file, err := os.Open(path) if err != nil { - return nil, err + return nil, nil, err } defer errs.Capture(&err, file.Close, "failed to close file") @@ -159,9 +150,9 @@ func genSmallPages(path string) (*manga.Chapter, error) { 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) + err = jpeg.Encode(buf, img, nil) if err != nil { - return nil, err + return nil, nil, err } page := &manga.Page{ Index: uint16(i), @@ -174,13 +165,13 @@ func genSmallPages(path string) (*manga.Chapter, error) { return &manga.Chapter{ FilePath: path, Pages: pages, - }, nil + }, []string{".webp", ".webp", ".webp", ".webp", ".webp"}, nil } -func genMixSmallBig(path string) (*manga.Chapter, error) { +func genMixSmallBig(path string, isSplit bool) (*manga.Chapter, []string, error) { file, err := os.Open(path) if err != nil { - return nil, err + return nil, nil, err } defer errs.Capture(&err, file.Close, "failed to close file") @@ -190,7 +181,7 @@ func genMixSmallBig(path string) (*manga.Chapter, error) { buf := new(bytes.Buffer) err := jpeg.Encode(buf, img, nil) if err != nil { - return nil, err + return nil, nil, err } page := &manga.Page{ Index: uint16(i), @@ -199,17 +190,21 @@ func genMixSmallBig(path string) (*manga.Chapter, error) { } pages = append(pages, page) } + expectedExtensions := []string{".webp", ".webp", ".webp", ".webp", ".webp"} + if isSplit { + expectedExtensions = []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp"} + } return &manga.Chapter{ FilePath: path, Pages: pages, - }, nil + }, expectedExtensions, nil } -func genMixSmallHuge(path string) (*manga.Chapter, error) { +func genMixSmallHuge(path string, isSplit bool) (*manga.Chapter, []string, error) { file, err := os.Open(path) if err != nil { - return nil, err + return nil, nil, err } defer errs.Capture(&err, file.Close, "failed to close file") @@ -219,7 +214,7 @@ func genMixSmallHuge(path string) (*manga.Chapter, error) { buf := new(bytes.Buffer) err := jpeg.Encode(buf, img, nil) if err != nil { - return nil, err + return nil, nil, err } page := &manga.Page{ Index: uint16(i), @@ -232,5 +227,55 @@ func genMixSmallHuge(path string) (*manga.Chapter, error) { return &manga.Chapter{ FilePath: path, Pages: pages, - }, nil + }, []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 } diff --git a/pkg/converter/webp/webp_converter.go b/pkg/converter/webp/webp_converter.go index 174fdd5..1b54033 100644 --- a/pkg/converter/webp/webp_converter.go +++ b/pkg/converter/webp/webp_converter.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "image" + _ "image/gif" _ "image/jpeg" "image/png" "runtime" @@ -167,19 +168,24 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C 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 } - if img != nil { - wgConvertedPages.Add(1) - select { - case pagesChan <- manga.NewContainer(page, img, format, false): - case <-ctx.Done(): - return - } + + wgConvertedPages.Add(1) + select { + case pagesChan <- manga.NewContainer(page, img, format, false): + case <-ctx.Done(): + return } + return } @@ -258,6 +264,7 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C log.Debug(). Str("chapter", chapter.FilePath). Int("error_count", len(errList)). + Err(errors.Join(errList...)). Msg("Conversion completed with errors") } else { log.Debug(). @@ -357,7 +364,7 @@ func (converter *Converter) checkPageNeedsSplit(page *manga.Page, splitRequested 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, err + return false, nil, format, converterrors.NewPageIgnored(fmt.Sprintf("page %d: failed to decode image (%s)", page.Index, err.Error())) } bounds := img.Bounds() diff --git a/pkg/converter/webp/webp_converter_test.go b/pkg/converter/webp/webp_converter_test.go index 30329ba..48ba729 100644 --- a/pkg/converter/webp/webp_converter_test.go +++ b/pkg/converter/webp/webp_converter_test.go @@ -5,6 +5,7 @@ import ( "context" "image" "image/color" + "image/gif" "image/jpeg" "image/png" "sync" @@ -44,6 +45,11 @@ func encodeImage(img image.Image, format string) (*bytes.Buffer, string, error) 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": PrepareEncoder() if err := Encode(buf, img, 80); err != nil { @@ -131,10 +137,11 @@ func TestConverter_ConvertChapter(t *testing.T) { pages: []*manga.Page{ createTestPage(t, 1, 800, 1200, "png"), createTestPage(t, 2, 800, 1200, "jpeg"), + createTestPage(t, 3, 800, 1200, "gif"), }, split: false, expectSplit: false, - numExpected: 2, + numExpected: 3, }, { name: "Tall image with split enabled", @@ -229,24 +236,35 @@ func TestConverter_convertPage(t *testing.T) { 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, }, } @@ -258,19 +276,47 @@ func TestConverter_convertPage(t *testing.T) { container := manga.NewContainer(page, img, tt.format, tt.isToBeConverted) converted, err := converter.convertPage(container, 80) - require.NoError(t, err) - assert.NotNil(t, converted) - if tt.expectWebP { - assert.Equal(t, ".webp", converted.Page.Extension) - validateConvertedImage(t, converted.Page) + if tt.expectError { + assert.Error(t, err) + assert.Nil(t, converted) } else { - assert.NotEqual(t, ".webp", converted.Page.Extension) + 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_WithCorruptedImage(t *testing.T) { + converter := New() + err := converter.PrepareConverter() + require.NoError(t, err) + + // Create a corrupted PNG image by creating a page with invalid data + corruptedPage := &manga.Page{ + Index: 1, + Contents: &bytes.Buffer{}, // Empty buffer - should cause decode error + Extension: ".png", + Size: 0, + } + + container := manga.NewContainer(corruptedPage, nil, "png", true) + + converted, err := converter.convertPage(container, 80) + + // This should return nil container and error because decode will fail with "image: unknown format" + assert.Error(t, err) + assert.Nil(t, converted) +} + func TestConverter_checkPageNeedsSplit(t *testing.T) { converter := New() @@ -333,8 +379,8 @@ func TestConverter_ConvertChapter_Timeout(t *testing.T) { // Create a test chapter with a few pages pages := []*manga.Page{ createTestPage(t, 1, 800, 1200, "jpeg"), - createTestPage(t, 2, 800, 1200, "jpeg"), - createTestPage(t, 3, 800, 1200, "jpeg"), + createTestPage(t, 2, 800, 1200, "png"), + createTestPage(t, 3, 800, 1200, "gif"), } chapter := &manga.Chapter{