- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 514
 
[Canary] Transcoding support using NetVips #3978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
Conversation
| 
           FYI: @majora2007, @DieselTech Additional Notes: Docker this time was not changed since new format support are embedded into the new netvips packages.  | 
    
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| 
           Canary docker with latest changes in this PR can be found in maxpiva/kavita:nightly  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a preliminary review from just the code, I will pull this down as well and do some shakeout testing.
| private readonly PngEncoder _pngEncoder = new PngEncoder(); | ||
| private readonly WebpEncoder _webPEncoder = new WebpEncoder(); | ||
| private readonly IImageFactory _imageFactory; | ||
| private const string SourceImage = "C:/Users/josep/Pictures/obey_by_grrsa-d6llkaa_colored_by_me.png"; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever test this is, we probably need to recreate or remove it. I dont even have this file anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just ported it over, should remove the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
| public void ImageFactory_ExtractImage_PNG() | ||
| { | ||
| var outputDirectory = "C:/Users/josep/Pictures/imagesharp/"; | ||
| var outputDirectory = "C:/Users/josep/Pictures/netvips/"; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to refactor this to use the TestData folder within the project and not paths on my dev machine.  (../../../Services/Test Data/ArchiveService/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do.
| var thumbnail = imageService.ImageFactory.Create(Path.Join(testDirectory, expectedOutputFile)); | ||
| int width = 320; | ||
| int height =(int)(thumbnail.Height * (width / (double)thumbnail.Width)); | ||
| thumbnail = thumbnail.Thumbnail(width, height); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not wrap this in the Image Interface so it's not so manual across the tests? Looks like a duplication of logic to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also refactor this to use var dims = CoverImageSize.Default.GetDimensions();. It never got updated when I introduced the concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, I don't see the use case for this, archiveService.GetCoverImage would internally make the same calls anyway.
| // Load and compare similarity | ||
| 
               | 
          ||
| var similarity = expectedFaviconPath.CalculateSimilarity(actualFaviconPath); // Assuming you have this extension | ||
| var similarity = _imageService.ImageFactory.CalculateSimilarity(expectedFaviconPath, actualFaviconPath); // Assuming you have this extension | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why the imageService.CalculateSimularity doesn't internally call the Image factory leading to a much cleaner API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can go one way or the other, depending of SRP, will change
| } | ||
| 
               | 
          ||
| // Ensure band count is 4 (RGBA) | ||
| if (image.Bands == 1) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to maintain all this logic now even when with NetVips still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, The colourspace and similarity algorithms, uses rawpixel data.
| using var res = im.Resize(percent / 100f); | ||
| float[] pixels = NetVipsImage.GetRGBAFloatImageDataFromImage(res); | ||
| if (pixels == null) | ||
| return new List<Vector3>(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bring to same line. Can we use Array.Empty instead of a new instance?
| { | ||
| var existingPath = Path.Combine(_directoryService.CoverImageDirectory, person.CoverImage); | ||
| var betterImage = existingPath.GetBetterImage(tempFullPath)!; | ||
| var betterImage = _imageService.ImageFactory.GetBetterImage(existingPath,tempFullPath)!; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also moved to IImageService.
| public const string MacOsMetadataFileStartsWith = @"._"; | ||
| 
               | 
          ||
| public const string SupportedExtensions = | ||
| public static string SupportedExtensions = | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this static and not const?
SupportedExtensions is what we support from the Disk, not from the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit tricky to explain, but here we go, there are two distinct concerns here.
I had to switch to a static field because the base value is no longer a constant. It's now a constructed string that depends on the supported formats. Otherwise, we’d have to duplicate the list of extensions here, and I’d rather keep a single source of truth to avoid human error during edits.
You're absolutely right: what the browser or client supports is not the same as what we support from disk.
So, we split the disk-supported formats into two collections, one for formats universally supported by clients, and another for those that aren't.
| 
           Just pushed the additional changes. Didn't change the Benchmark methods, or the intent of the test method. until have more clarity. also, the IOC part I didn't understand the context.  | 
    
| List<string> supportedExtensions = Parser.UniversalFileImageExtensionArray.ToList(); | ||
| //Early eject if the browser or api do not provide an Accept header. | ||
| if (!request.Headers.ContainsKey("Accept")) | ||
| return supportedExtensions; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be on the same line as the if
| 
           I just got back from a holiday, I'm planning to do one more once over and get this into a canary release for dedicated user testing.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces transcoding support for modern image formats (JXL, JPEG 2000, HEIF, AVIF, TIFF) using NetVips library and abstracts Kavita's image processing through a clean interface pattern. The changes decouple the application from specific graphics libraries while enabling automatic format conversion based on browser compatibility.
- Replaces SixLabors.ImageSharp dependency with NetVips (default) and optional ImageMagick backend
 - Introduces 
IImageFactoryandIImageinterfaces to abstract image processing operations - Adds automatic image format transcoding with browser compatibility detection via Accept headers
 
Reviewed Changes
Copilot reviewed 32 out of 54 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description | 
|---|---|
| API/Services/Tasks/Scanner/Parser/Parser.cs | Adds support for non-universal image formats and browser compatibility mappings | 
| API/Services/ImageServices/*.cs | New abstraction layer with NetVips and ImageMagick implementations | 
| API/Services/ImageService.cs | Refactored to use new image abstraction with format conversion capabilities | 
| API/Extensions/ImageExtensions.cs | Updated image utility methods to work with new abstraction layer | 
| API/Controllers/ReaderController.cs | Adds automatic image format replacement based on browser support | 
| var filename = ImageService.GetWebLinkFormat(baseUrl, encodeFormat); | ||
| 
               | 
          ||
| image.WriteToFile(Path.Combine(_directoryService.FaviconDirectory, filename)); | ||
| await image.SaveAsync(Path.Combine(_directoryService.FaviconDirectory, filename),encodeFormat); | 
    
      
    
      Copilot
AI
    
    
    
      Aug 20, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma. Should be: await image.SaveAsync(Path.Combine(_directoryService.FaviconDirectory, filename), encodeFormat);
| { | ||
| var existingPath = Path.Combine(_directoryService.CoverImageDirectory, series.CoverImage); | ||
| var betterImage = existingPath.GetBetterImage(tempFullPath)!; | ||
| var betterImage =_imageService.GetBetterImage(existingPath, tempFullPath)!; | 
    
      
    
      Copilot
AI
    
    
    
      Aug 20, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after the '=' operator. Should be: var betterImage = _imageService.GetBetterImage(existingPath, tempFullPath)!;
| { | ||
| var existingPath = Path.Combine(_directoryService.CoverImageDirectory, chapter.CoverImage); | ||
| var betterImage = existingPath.GetBetterImage(tempFullPath)!; | ||
| var betterImage = _imageService.GetBetterImage(existingPath,tempFullPath)!; | 
    
      
    
      Copilot
AI
    
    
    
      Aug 20, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma. Should be: var betterImage = _imageService.GetBetterImage(existingPath, tempFullPath)!;
| { | ||
| if (supportedImageFormats == null) return false; | ||
| 
               | 
          ||
| string ext = Path.GetExtension(filename).ToLowerInvariant().Substring(1); | 
    
      
    
      Copilot
AI
    
    
    
      Aug 20, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: Calling Substring(1) on an empty string will throw an exception if the filename has no extension. Add a null/empty check before calling Substring.
| string ext = Path.GetExtension(filename).ToLowerInvariant().Substring(1); | |
| string extension = Path.GetExtension(filename); | |
| if (string.IsNullOrEmpty(extension) || extension.Length < 2) return false; | |
| string ext = extension.ToLowerInvariant().Substring(1); | 
| } | ||
| 
               | 
          ||
| image = image.Insert(tile, x, y); | ||
| image.Composite(tile, (int)x, (int)y); | 
    
      
    
      Copilot
AI
    
    
    
      Aug 20, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast from double to int may cause precision loss. Consider using Math.Round or ensure x and y are already integers.
| image.Composite(tile, (int)x, (int)y); | |
| image.Composite(tile, (int)Math.Round(x), (int)Math.Round(y)); | 
| if (!extensions.Contains(extension)) | ||
| { | ||
| extensions.Add(extension); | ||
| } | 
    
      
    
      Copilot
AI
    
    
    
      Aug 20, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method performs a linear search with Contains() for each extension. Consider using a HashSet for better performance when dealing with many extensions.
| } | |
| private static void AddExtension(HashSet<string> extensions, string extension) | |
| { | |
| if (string.IsNullOrEmpty(extension)) | |
| return; | |
| extensions.Add(extension); | 
| 
           I'm back from holiday and going to get this canary released for user testing.  | 
    
| 
           I haven't forgotten about this, just been heads down on the new annotation system. Once I get that stabilized, I'll pull this in and get it to some users.  | 
    
| 
           I took care of the merge for you and pushed up the updated code into  I see that 2 unit tests are broken (JXL/JP2), I would appreciate a quick look @maxpiva Let's leave this PR open, but please target any changes towards my new branch.  | 
    
| 
           letme check :)  | 
    
Added
Transcoding support via NetVips
Adds support for transcoding/converting and serving image formats such as JXL, JPEG 2000, HEIF, AVIF, and TIFF.
Also switches JPEG encoding/decoding to use JPEGLI instead of the native JPEG implementation, providing faster performance and improved quality-to-size ratio.
Removed all SixLabors dependencies.
Decoupled image processing into two interfaces:
IImageFactoryIImageKavita is now decoupled from any specific graphics library.
Added two implementations:
NetVips(default)ImageMagick(can be enabled via conditional compilation)Extended
ImageService:ReplaceImageFileFormat, which auto-converts unsupported formats (e.g., AVIF, JXL) to JPEG based on browser compatibility.Note: Only one backend is supported at a time.
Refactored
ImageExtensionsto remove SixLabors usage and delegate to the active backend.Added local
packages/directory containing customnetvips.nativeNuGet packages with support for JXL, JPEG 2000, and JPEGLI.These may be published to NuGet.org pending review from Kleisauke.
Changed
ImageServiceto instance methods to support dependency injection of the graphics backend.WillScaleWellandIsLikelyWideImageintoImageExtensions.Notes
Tested successfully on Windows, Linux x64, and Docker over a 2-day period.
Further testing is recommended for macOS, ARM, and other platforms.
Building libvips with JPEGLI, JXL, and JPEG 2000 support required patching and custom builds.
Special thanks and hat tip to Kleisauke for his foundational work, without it, this integration would have been extremely difficult.
Custom forks of
netvips,libvips-packaging, andbuild-win64-mxeare available in my GitHub account and can be reused if needed, just be aware I just pushed until it works, some changes might be unrequired.