Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

ngx_brotli compression support ? #1021

Closed
centminmod opened this issue Oct 14, 2015 · 7 comments
Closed

ngx_brotli compression support ? #1021

centminmod opened this issue Oct 14, 2015 · 7 comments

Comments

@centminmod
Copy link

I just compiled Nginx ngx_brotli module support into my Nginx server https://community.centminmod.com/posts/19601/ with ngx_pagespeed enabled.

Problem:

  • https version of my site https://centminmod.com has non-working ngx_pagespeed when viewed in Firefox 44 nightly web browser which supports Content-Encoding: "br" - non-working as in no resources are pagespeed optimised
  • https version of my site https://centminmod.com works with ngx_pagespeed when viewed in Chrome, Opera and Firefox 43 dev editions which do not support Brotli encoding so fall back to gzip

using following ngx_brotli directives

brotli on;
brotli_static on;
brotli_min_length 1000;
brotli_buffers 32 8k;
brotli_comp_level 5;
brotli_types text/plain text/css text/xml application/javascript application/x-javascript application/xml application/xml+rss application/ecmascript application/json image/svg+xml;
@jeffkaufman
Copy link
Contributor

Verified:

curl -H 'Accept-Encoding: br' -D- -o tmp.br https://centminmod.com
 ./bro --decompress --input ~/tmp2.html  | fgrep .pagespeed.

My guess is that PageSpeed is seeing the html brotli encoded and doesn't know how to decode it. Ideally the brotli decoding would happen after PageSpeed, like gzip encoding.

I need to build this to test it.

@crowell
Copy link
Contributor

crowell commented Oct 14, 2015

does the order of execution of the modules change this behavior?

eg --with-module=/path/to/pagespeed --with-module=/path/to/brotli vs --with-module=/path/to/brotli --with-module=/path/to/pagespeed ?

http://forum.nginx.org/read.php?2,246978,246999

@jeffkaufman
Copy link
Contributor

That would make sense, except the command they used is:

--add-module=../ngx_brotli ... --add-module=../ngx_pagespeed-release-1.9.32.10-beta

I think either their config or our config needs to change to get the order we want. I think it's us? We have:

  # Make pagespeed run immediately before gzip.
  HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES |\
    sed "s/$HTTP_GZIP_FILTER_MODULE/$HTTP_GZIP_FILTER_MODULE $ngx_addon_name/")

And brotli runs before gzip. So I think what we need is:

  1. Tell people to include us last on the ./configure command line, with brotli right before
  2. Change our regexps to run immediately before brotli if present, otherwise immediately before gzip.

I haven't tested this though.

@oschaaf
Copy link
Member

oschaaf commented Oct 15, 2015

@jeffkaufman On trunk-tracking, we also have a8141ea
We might want to consider defaulting to 'HTTP_AUX_FILTER_MODULES`, and let people have more control over the module positioning?

From the nginx forum [1]:

And again: the only place in the filter chain where third party
modules is expected be installed is HTTP_AUX_FILTER_MODULES. You
should have really good reason to install your module anywhere
else, and if you do it - you do it at your own risk.

[1] http://forum.nginx.org/read.php?2,152699,152727#msg-152727

@jeffkaufman
Copy link
Contributor

HTTP_AUX_FILTER_MODULES does sound like the right place for us, but then we need to be very clear in our documentation about where people should put us in the ordering. "After all other modules manipulating content, but before any modules compressing content" is kind of complex to get across.

@PiotrSikora
Copy link
Contributor

@jeffkaufman: maybe inject pagespeed filter right after postpone filter instead of right before gzip?

@centminmod
Copy link
Author

or maybe you could just echo and inspect $HTTP_FILTER_MODULES values and check for ngx_brotli module if it exists, sed replacement after ngx_http_brotli_filter_module module instead of after $HTTP_GZIP_FILTER_MODULE - would just be a simple IF or ELSE ?

basically

  1. Change our regexps to run immediately before brotli if present, otherwise immediately before gzip.

PiotrSikora added a commit to PiotrSikora/ngx_pagespeed that referenced this issue Nov 14, 2015
Reported by George Liu (eva2000) on GitHub (issue apache#1021).

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
PiotrSikora added a commit that referenced this issue Nov 16, 2015
Reported by George Liu (eva2000) on GitHub (issue #1021).

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Conflicts:
	config
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants