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

Add JPEG XL support to image processing. #2488

Open
wants to merge 25 commits into
base: next
Choose a base branch
from
Open

Conversation

veluca93
Copy link

@veluca93 veluca93 commented May 1, 2024

As discussed in #2421.

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

Keats and others added 17 commits December 19, 2023 13:22
* Match <!-- more --> without spaces

* Add tests for new <!-- more --> handling, with a note on pulldown-cmark bug
This fixes a bug introduced in getzola#2258

The issue arose when `output_path` was relative. The request being
served would be canonicalized and this would be a string. So, for
example, if you were serving content from `public` the code
[right after](https://github.com/getzola/zola/blob/38199c125501e9ff0e700e96adaca72cc3f25d2b/src/cmd/serve.rs#L144-L147)
the canonicalization checking if
`root.starts_with(original_root)` would always return `false` since
an absolute path, `/some/path/to/content` would never start with a
string like `public`.
…27.0.0.1 (getzola#2395)

* Parse interface as IpAddr, allow IPv6.

* Default base_url to socket address, instead of 127.0.0.1
* Remove ensure_directory_exists since it's identical to create_directory, and misleading

* Don't create directories unless needed; rely on create_dir_all instead of manually iterating over components
* Allow ignoring files when link checking

* cargo fmt

* Fix tests

* Remove mystery duplicate function..?

* Add in some mysterious missing code..?

* Simple tests for link checker file globs in config

* cargo fmt

* Remove comment

* convert expect to error propagation

* Address comments

* cargo fmt
* adding optional `lang` arugment to `get_section` global function

* Add handling of default language passed in `lang` argument of `get_section`

* Remove clones for path.  Change "?" to an explicit check for error

* lint changes

* Clean up error handling for add_lang_to_path call

* fix format

* Add optional parameter "lang" to get_page template function.  Add check for language available in config.

* Modify helper function name from calculate_path to get_path_with_lang.  Modify documentation for get_section and get_page to include equivalent calls without using lang argument to demostrate how lang argument effects pathing.
getzola#2432)

The pulldown_cmark escaping functionality is now shipped in a separate
pulldown_cmark_escape crate
(https://crates.io/crates/pulldown-cmark-escape), starting with v0.10.0.

The markdown.rs module has to be adapted to a few API changes in
pulldown_cmark, and we have to introduce explicit handling of <img> alt
text to ensure it continues to be properly escaped.

There are also a few other behavior changes that are caught by the
tests, but these actually seem to be desired, so I've updated the insta
snapshot files for those tests to incorporate those changes.
Specifically, one footnote-parsing case seems to be handled better now,
and pulldown-cmark's `push_html` now doesn't escape quotes in text nodes
anymore (see pulldown-cmark/pulldown-cmark#836).
…getzola#2311)

* Fix --base-url improper path and protocol handling.

* Fix formatting.
* Fix resizing for images with EXIF orientation

* Added test for asymmetric resize for exif images

---------

Co-authored-by: Tanishq <[email protected]>
* Fix link check report inconsistency

* Fix formatting issue

---------

Co-authored-by: Tanishq <[email protected]>
* Restore trailing slash behaviour in serve command.

* Restore guard in case where base_url is just a slash.
Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

Can you add some tests? They are in imageproc/tests

components/imageproc/src/processor.rs Outdated Show resolved Hide resolved
@veluca93
Copy link
Author

veluca93 commented May 4, 2024

Can you add some tests? They are in imageproc/tests

Absolutely - what do you think I should test? For now I just added a PNG to JXL test.

@Keats
Copy link
Collaborator

Keats commented May 4, 2024

Maybe a JXL output with a JXL input? The CI failures look legit though

@veluca93
Copy link
Author

veluca93 commented May 4, 2024

Maybe a JXL output with a JXL input? The CI failures look legit though

Ah, JXL reading is not supported :D do you think I should add that?

As for the CI... I believe I need to add the system dependency, but I have no idea how to configure that - any pointers?

@Keats
Copy link
Collaborator

Keats commented May 4, 2024

Ah, JXL reading is not supported :D do you think I should add that?

It depends whether it can be added or not. If so then yes

As for the CI... I believe I need to add the system dependency, but I have no idea how to configure that - any pointers?

It looks like there is a vendored feature for statically linking it

@veluca93
Copy link
Author

veluca93 commented May 4, 2024

Ah, JXL reading is not supported :D do you think I should add that?

It depends whether it can be added or not. If so then yes

As for the CI... I believe I need to add the system dependency, but I have no idea how to configure that - any pointers?

It looks like there is a vendored feature for statically linking it

Did both of those. Also, my pr to jpegxl-rs was accepted and a release should be coming soon, so perhaps waiting a day or two and switching to the next version of jpegxl-rs would be the ideal solution.

In case you are wondering why I did not use the "image" feature of jpegxl-rs: it depends on a different version of "image", and trying to upgrade the dependency for zola to 0.25 produced some dependency incompatibility errors.

@Keats
Copy link
Collaborator

Keats commented May 4, 2024

it depends on a different version of "image", and trying to upgrade the dependency for zola to 0.25 produced some dependency incompatibility errors.

I will update to 0.25 soonish, but I haven't looked at the image feature of jpeg-xl-rs. If that's better, I can update it now and you rebase your PR on it?

@veluca93
Copy link
Author

veluca93 commented May 4, 2024

it depends on a different version of "image", and trying to upgrade the dependency for zola to 0.25 produced some dependency incompatibility errors.

I will update to 0.25 soonish, but I haven't looked at the image feature of jpeg-xl-rs. If that's better, I can update it now and you rebase your PR on it?

If you can do that, it'd be great!

Looks like there are some build issues with jpegxl-rs, so it'll be a while before this PR can be merged anyway :-)

totikom and others added 3 commits May 9, 2024 15:45
* Implemented bottom footnotes with backreferences

Fixes getzola#1285

* Added bottom_footnotes option to configuration.md

* Renamed fix_github_style_footnotes()

* Added tests for convert_footnotes_to_github_style()

* Changed test to plain html instead of Vec<Event>

* Added integration test for footnotes

* Applied suggested changes
@Keats
Copy link
Collaborator

Keats commented May 9, 2024

next has been updated with image 0.25

@veluca93
Copy link
Author

veluca93 commented May 9, 2024

Thanks! I updated the PR accordingly (also squashing the commits, since I was going to rebase anyway...)

@Keats
Copy link
Collaborator

Keats commented May 14, 2024

Can you rebase? CI is fixed minus windows which i'll look at later

@veluca93 veluca93 force-pushed the jxl branch 2 times, most recently from dd43221 to 3eda012 Compare May 14, 2024 18:00
@veluca93
Copy link
Author

Can you rebase? CI is fixed minus windows which i'll look at later

I made a mess with rebasing, but it should be done now :-)

components/imageproc/src/format.rs Outdated Show resolved Hide resolved
@Keats
Copy link
Collaborator

Keats commented May 16, 2024

CI failures seem legit

@veluca93
Copy link
Author

CI failures seem legit

Yep, they are, I am hoping they will be resolved on the jpegxl_rs side (inflation/jpegxl-rs#50) and jxl side (inflation/jpegxl-rs#51) as soon as I can get to it :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.