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

Features/brotlimagick #2589

Closed

Conversation

staccDOTsol
Copy link

@staccDOTsol staccDOTsol commented Oct 25, 2023

  1. it brotlifies text encoded messages, so long as they are more than 10 chars (as brotli doesn't do anything at all for small strings anyways)
  2. the server shows brotlified messages
  3. the tests run
  4. it uses a new content-encoding header, tag 8
    Screenshot from 2023-10-25 18-21-40
    http://localhost/preview/b882ff711cfb777e491d89aa13effe51a45f952189df85e2db821c1746677289i0 (testnet)

@lifofifoX
Copy link
Collaborator

  • Would it be better to start with gzip instead of brotli for better browser support? Certainly worth giving people the choice.
  • IMO, this should be opt-in via a flag, at least to begin with. This would be useful for non text categories as well, such as image/svg+xml or application/json. So, it could be something like: ord wallet inscribe --encoding gzip --file art.svg

@staccDOTsol
Copy link
Author

staccDOTsol commented Oct 26, 2023

Gm @devords !

The previous few attempts were for brotli given the challenges made here

#1699

And also in previous tickets there was feedback from others to make it default to brotli where appropriate.

#1713

The browser doesn't need to decode in this instance as the ord server does.

I can add gzip compression as well that's not a terribly complicated adjustment from here, but I would benefit more personally from brotli as it applies better to the case of text compression whereas I'm doing work to incentivize immortalized web2 data on the BTC chain.

Gzip I'm happy to do in a 2nd pr, as well as adding the cli optionality for users.

bitcoin/bips#1408 (comment)

@lifofifoX
Copy link
Collaborator

If brotli is supported everywhere, it'd make sense to start with just that. Still think it should be opt-in. Inscriptions that require content encoding header will break on all the previous versions of ord, which maynot always be desirable.

@elocremarc
Copy link
Contributor

I think they plan to do compression on everything and then decode it at the server level to be uncompressed in the content route.

@lifofifoX
Copy link
Collaborator

@elocremarc Just setting the Content-Encoding header and serving compressed content should be sufficient for the browser to automatically decompress.

@lifofifoX
Copy link
Collaborator

Just noticed that the PR has the server serving decompressed content. IMO, the server should to be serving compressed content with the appropriate Content-encoding header based on the accept encoding header. If the request doesn't have a compatible accept encoding header, then the server can send a decompressed response.

This approach would provide a pretty significant speed/performance boost as well.

@casey
Copy link
Collaborator

casey commented Nov 16, 2023

Closing in favor of #1713.

@casey casey closed this Nov 16, 2023
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.

5 participants