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 --preferred-encoding (gzip|brotli) to use when tile is not pre-encoded by source #1189

Merged
merged 22 commits into from
Feb 19, 2024

Conversation

sharkAndshark
Copy link
Collaborator

@sharkAndshark sharkAndshark commented Feb 11, 2024

Try to fix #1178

  • Add optional --preferred-encoding to cli, br brotli and gzip are allowed
  • Add optional preferred-encoding to config, br brotli and gzip are allowed
  • Add test
  • Update doc

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

good first step, left a few comments

martin/src/args/srv.rs Outdated Show resolved Hide resolved
martin/src/args/srv.rs Outdated Show resolved Hide resolved
martin/src/srv/server.rs Outdated Show resolved Hide resolved
@nyurik nyurik changed the title Allow seeting tile encoding preference Add --preferred-encoding (gzip|brotli) to use when tile is not pre-encoded by source Feb 11, 2024
@sharkAndshark sharkAndshark marked this pull request as ready for review February 19, 2024 05:41
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

love it! One small nit, and seems it is pretty good to go!

martin/src/srv/tiles.rs Outdated Show resolved Hide resolved
martin/src/srv/tiles.rs Outdated Show resolved Hide resolved
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

nah, not worth it, will fix naming if needed later, good enough. I made a few more minor suggestions

docs/src/config-file.md Outdated Show resolved Hide resolved
docs/src/run-with-cli.md Show resolved Hide resolved
martin/src/args/srv.rs Outdated Show resolved Hide resolved
martin/tests/mb_server_test.rs Outdated Show resolved Hide resolved
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

I'm not too happy with the description, but can't think think too clearly. Will try in the morning. In the mean time, there is one line that i think was left by an accident, but good to go otherwise, so remove it and feel free to merge :)

// only apply compression if the content supports it
if let Some(HeaderEnc::Known(enc)) =
accept_enc.negotiate(SUPPORTED_ENCODINGS.iter())
// accept_enc.negotiate(SUPPORTED_ENCODINGS.iter())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// accept_enc.negotiate(SUPPORTED_ENCODINGS.iter())

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Deleted.

Copy link
Collaborator Author

@sharkAndshark sharkAndshark Feb 19, 2024

Choose a reason for hiding this comment

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

"Preferred tile encoding when martin is negotiating with browser, only works if the tile itself is not pre-compressed. The default Brotli is much smaller(and may be faster with caching) but a little bit slower than gzip."
Well this version maybe much worse..

@sharkAndshark sharkAndshark merged commit 3e30c43 into maplibre:main Feb 19, 2024
19 checks passed
@sharkAndshark sharkAndshark deleted the compress branch February 19, 2024 11:21
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.

Implement --preferred-encoding (gzip|brotli) configuration
2 participants