Skip to content
New issue

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

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

Transcoding Support + More Image Format Support (JPEGXL) #3250

Open
wants to merge 37 commits into
base: develop
Choose a base branch
from

Conversation

maxpiva
Copy link

@maxpiva maxpiva commented Oct 4, 2024

Added/Modified.

Support for JPEG-XL, JPEG 2000, HEIF and AVIF.
Dynamic transcoding to JPEG in the cache, when the browser does not have support for the image types.
Direct serving when the browser supports the new image type. (IE, MAC, IOS )
Transcoding of the thumbnails when needed.
Based on ImageMagick. (Magick.NET)
Created IIMage and IIMageFactory interfaces with their ImageMagick Implementation, decoupling image manipulations implementations from the ImageService.
Edited Dockefile, adding required library support for reading the new image formats in Linux versions. [Bumped ubuntu to oracular, because of the JPEG-XL image support]
Removed Netvips.
Removed SixLabors ImageSharp.
Change the Context Aware Thumbnail, from NetVips to SmartCrop.js C# port.
[Not Related] Update nugets to latest, Flurl Http changed the way its configured, edited accordingly.

Missing

Additional Testing methods.

Dockerfile

maxpiva/kavita:nightly

maxpiva and others added 30 commits August 15, 2024 08:25
Fixed test and benchmark DI.
Point docker-build to another hub
Edit dockerfile and include jxl package.
1. Supported Image Formats came in the accept flag from the browser.
2. If jxl (or future formats, ie browser not supported AVIF, HEIF, etc) are not supported, Kavita will convert it to a suitable display format via conversion providers.
3. if it supported it will return the image as is, no conversion needed.
4. Thumbnails are always converted.
1. Supported Image Formats came in the accept flag from the browser.
2. If jxl (or future formats, ie browser not supported AVIF, HEIF, etc) are not supported, Kavita will convert it to a suitable display format via conversion providers.
3. if it supported it will return the image as is, no conversion needed.
4. Thumbnails are always converted.
Fix GetCoverImage, trying to create a temporary image in current directory, and rename the parameter field name, that generates a wrong asumption.
Optimize Cover Creation
Removing ImageConvertors (No Longer Needed)
Refactored mime types and extensions constants
Enable back docker build/push
@majora2007 majora2007 changed the title Remove six labors and net vips, add dynamic transcoding when needed on HEIF, AVIF, JPEGXL and JPG2000 Transcoding Support + More Image Format Support (JPEGXL) Oct 4, 2024
@majora2007 majora2007 added the accepted A feature request that is accepted label Oct 4, 2024
@adamradocz
Copy link

HEIF and AVIF are superior formats compared to JpegXL and Jpeg2000. Why the hassle to support them?

@maxpiva
Copy link
Author

maxpiva commented Oct 4, 2024

HEIF and AVIF are superior formats compared to JpegXL and Jpeg2000. Why the hassle to support them?

Apple might want to talk to you, while I don't want to start a format war, I want to know how much the google chrome AVIF team is paying you, I want the same deal! :)

Support came from changing netvips to ImageMagick we don't need to add specific support/mantain for JPEG-XL. There is a secondary why, @majora2007 probably can explain it better, but netvips limits the possible CPU targets.

What is added, is, we identify if your browser supports a new format A, B, C or D, and if not with transcode the image to a format that your browser/app supports.

While is true that the original intent was adding JPEG-XL, let's say because my own bias, and current is in vogue.

In the end, we end supporting AVIF, HEIF, JPEG-XL and JPEG2000 comics/manga, regardless your browser/app supports it or not.

Copy link
Member

@majora2007 majora2007 left a comment

Choose a reason for hiding this comment

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

I went through the whole PR. Please address comments and style changes.

I'm worried about EncodeFormat not being taken into consideration and I want to research a way to remove JPEG as the default format as we can use PNG instead.

API/Entities/Enums/EncodeFormat.cs Outdated Show resolved Hide resolved
API/Extensions/HttpExtensions.cs Show resolved Hide resolved
API/Extensions/HttpExtensions.cs Outdated Show resolved Hide resolved
stream.Seek(0, SeekOrigin.Begin);
image.SaveAsPng(stream);
image.Save(stream, EncodeFormat.PNG, 100);
Copy link
Member

Choose a reason for hiding this comment

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

You can leave off the 100 since it's default quality.

Copy link
Author

Choose a reason for hiding this comment

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

New IImage interface, do not have default value, we can add it if you want.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see ever a need to not generate at 100 quality, so we should probably just leave it off entirely. If there is ever a need, it's easy to refactor the signature.

Copy link
Author

Choose a reason for hiding this comment

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

Well, if I remove the 100, I cannot control JPEG image quality required below, #3250 (comment)

if you want, I can add a default value to the interface.

FYI (ImageMagick)

100 PNG = Maximum Compression. loseless
0 PNG = No Compression. loseless
100 JPG = Loseless JPG
99 - 1 = Lossy JPG
AVIF and WEBP I think they're similar to JPG.

API/Services/CacheService.cs Show resolved Hide resolved
API/Services/ImageServices/ImageMagick/ImageMagickImage.cs Outdated Show resolved Hide resolved
API/Services/ImageServices/ImageMagick/ImageMagickImage.cs Outdated Show resolved Hide resolved
API/Services/Tasks/Scanner/Parser/Parser.cs Show resolved Hide resolved
@majora2007
Copy link
Member

I saw a comment about what IDE do I use last night, but wasn't able to find it. I use Rider. We shouldn't have BOM. It's likely due to PRs submitted by the community where it might have been added.

@therobbiedavis
Copy link
Collaborator

I saw a comment about what IDE do I use last night, but wasn't able to find it. I use Rider. We shouldn't have BOM. It's likely due to PRs submitted by the community where it might have been added.

By default VS will save with BOM. I had this issue as well and I believe you can change this default behavior to save without BOM.

Added Per Image Lock on conversion, to prevent multiple threads from processing the same image at the same time.
@maxpiva
Copy link
Author

maxpiva commented Oct 10, 2024

I saw a comment about what IDE do I use last night, but wasn't able to find it. I use Rider. We shouldn't have BOM. It's likely due to PRs submitted by the community where it might have been added.

By default VS will save with BOM. I had this issue as well and I believe you can change this default behavior to save without BOM.

I did just that, then some other files, got BOM when they original did NOT.

Swapped to Rider, for this, Rider does not touch the BOM (Nor add, nor remove)

@majora2007
Copy link
Member

@maxpiva if you're on discord, can you reach out to me? I want to discuss about the quality a bit more and get a better understanding.

My biggest questions are about the quality (100, 0, 99, etc) and how to know when to use what and the time delta you provided for PNG vs JPEG for transcoding, what type of image did you use and what is the quality delta between (is it noticeable to the user)?

Added Quality From EncodedFormat Extension
@majora2007
Copy link
Member

Just remembering that this impacts the docker image and that might mean we need to check with unraid/lsio/qnap where they make customizations.

@DieselTech can you confirm?

If this is true, we will need to pair this with a release that isn't primarily bugfixes (which is what the upcoming one is).

@maxpiva
Copy link
Author

maxpiva commented Oct 15, 2024

Working in unraid as is, using (original unraid template) but changed the image path to my docker repo.

@majora2007
Copy link
Member

Note to self: #3164 should get merged in with this Transcoding PR so any Colorscape effects can happen together.

@majora2007
Copy link
Member

Just an update, I'm wrapping the current canary testing. #3025 gets first priority to the canary testing (as it's likely to be a shorter test phase) then this is up. If I can, I will get this into this v0.8.4 release (as otherwise it's a bugfix release).

Thanks for your patience. I think this will be a nice enhancement to Kavita and allow for much better format support in the future.

@maxpiva
Copy link
Author

maxpiva commented Oct 23, 2024

Nice :)
Hope the Panels guys add image/jxl in the accept header, since IOS supports it. Safari on iOS was working on all the new image types without transcoding.

@majora2007
Copy link
Member

I reached out to Dani from Panels and they will add Accept headers to their requests so we don't need to always transcode.

@majora2007
Copy link
Member

@maxpiva So what was the consensus of the NetVips stuff? Wondering if I can slot this in the canary (since the Koreader one needs some more work).

@maxpiva
Copy link
Author

maxpiva commented Oct 29, 2024

@majora2007 IDK. Asked the guy how to make it possible, but have no answer, is he/she in the discord channel?

Ref:
#3141 (comment)

if we make it work at OS level. I could code also the netvips implementation, and we can choose at build time which one to use.

@majora2007
Copy link
Member

@majora2007 IDK. Asked the guy how to make it possible, but have no answer, is he/she in the discord channel?

Ref: #3141 (comment)

if we make it work at OS level. I could code also the netvips implementation, and we can choose at build time which one to use.

That is the maintainer of NetVips actually. They aren't in the discord. I'm not sure if it can be possible as well have native Linux builds as well. Let's move forward with canary testing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted A feature request that is accepted
Projects
Status: Done, Not Pushed
Development

Successfully merging this pull request may close these issues.

4 participants