Merge pull request #109 from Belphemur/feat/gif

Feat/gif

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
+	// Slice to collect errors with mutex for thread safety
-	errorChan := make(chan error, parallelism)
+	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")

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
+		name           string
-		genTestChapter       func(path string) (*manga.Chapter, error)
+		genTestChapter func(path string, isSplit bool) (*manga.Chapter, []string, error)
-		split                bool
+		split          bool
-		expectFailure        []constant.ConversionFormat
+		expectError    bool
-		expectPartialSuccess []constant.ConversionFormat
 	}{
 		{
-			name:                 "All split pages",
+			name:           "All split pages",
-			genTestChapter:       genHugePage,
+			genTestChapter: genHugePage,
-			split:                true,
+			split:          true,
-			expectFailure:        []constant.ConversionFormat{},
-			expectPartialSuccess: []constant.ConversionFormat{},
 		},
 		{
-			name:                 "Big Pages, no split",
+			name:           "Big Pages, no split",
-			genTestChapter:       genHugePage,
+			genTestChapter: genHugePage,
-			split:                false,
+			split:          false,
-			expectFailure:        []constant.ConversionFormat{constant.WebP},
+			expectError:    true,
-			expectPartialSuccess: []constant.ConversionFormat{},
 		},
 		{
-			name:                 "No split pages",
+			name:           "No split pages",
-			genTestChapter:       genSmallPages,
+			genTestChapter: genSmallPages,
-			split:                false,
+			split:          false,
-			expectFailure:        []constant.ConversionFormat{},
-			expectPartialSuccess: []constant.ConversionFormat{},
 		},
 		{
-			name:                 "Mix of split and no split pages",
+			name:           "Mix of split and no split pages",
-			genTestChapter:       genMixSmallBig,
+			genTestChapter: genMixSmallBig,
-			split:                true,
+			split:          true,
-			expectFailure:        []constant.ConversionFormat{},
-			expectPartialSuccess: []constant.ConversionFormat{},
 		},
 		{
-			name:                 "Mix of Huge and small page",
+			name:           "Mix of Huge and small page",
-			genTestChapter:       genMixSmallHuge,
+			genTestChapter: genMixSmallHuge,
-			split:                false,
+			split:          false,
-			expectFailure:        []constant.ConversionFormat{},
+			expectError:    true,
-			expectPartialSuccess: []constant.ConversionFormat{constant.WebP},
+		},
+		{
+			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 err != nil && !tc.expectError {
-						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
-						}
 						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) {
+					if len(convertedChapter.Pages) != len(expectedExtensions) {
-						t.Fatalf("converted chapter has different number of pages")
+						t.Fatalf("converted chapter has %d pages but expected %d", len(convertedChapter.Pages), len(expectedExtensions))
 					}
 
-					for _, page := range convertedChapter.Pages {
+					// Check each page's extension against the expected array
-						if page.Extension != ".webp" {
+					for i, page := range convertedChapter.Pages {
-							t.Errorf("page %d was not converted to webp format", page.Index)
+						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
+	expectedExtensions := []string{".jpg"} // One image that's generated as JPEG
-		img := image.NewRGBA(image.Rect(0, 0, 1, 17000))
+	if isSplit {
-		buf := new(bytes.Buffer)
+		expectedExtensions = []string{".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp", ".webp"}
-		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)
 	}
 
+	// 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
 }

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)
+				wgConvertedPages.Add(1)
-					select {
+				select {
-					case pagesChan <- manga.NewContainer(page, img, format, false):
+				case pagesChan <- manga.NewContainer(page, img, format, false):
-					case <-ctx.Done():
+				case <-ctx.Done():
-						return
+					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()

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 {
+			if tt.expectError {
-				assert.Equal(t, ".webp", converted.Page.Extension)
+				assert.Error(t, err)
-				validateConvertedImage(t, converted.Page)
+				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, 2, 800, 1200, "png"),
-		createTestPage(t, 3, 800, 1200, "jpeg"),
+		createTestPage(t, 3, 800, 1200, "gif"),
 	}
 
 	chapter := &manga.Chapter{