search

We-Amp / ngx-pagespeed-alpine

ngx_pagespeed docker image for Alpine. Moved over to https://github.com/apache/incubator-pagespeed-ngx/tree/master/docker
Apache License 2.0
36 stars 26 forks source link

Brotli image compression #10

Open justinmayer opened 6 years ago

justinmayer commented 6 years ago

Would you consider adding the ngx_brotli Brotli image compression module for Nginx to this excellent image? That would allow folks, among other use cases, to pre-compress images via Brotli and use brotli_static to serve pre-compressed files with the .br extension.

There's an existing Nginx + Brotli docker image, but it (1) doesn't support PageSpeed and (2) uses an abandoned ngx_brotli repo. Just thought I'd refer to it here since it could provide guidance on how one could add Brotli support to this image. 😸

nberlee commented 6 years ago

Its actually not so hard since I already added this for non public use. And eustas fork is actually now not causing any issues anymore. But all css/javascript files processed by ngx_pagespeed are only served with Gzip on the fly. HTML can be compressed by Brotli and optimized by PageSpeed on the fly. I am not sure about images, as I never tried to (pre)compress those with gzip/brotli.

Adding Brotli as an extra docker image flavor (extra tags) would probably benefit the target audience and closely supplements PageSpeed capabilities. However I would be hesitant in adding other nginx plugins that aren't that supplementary to the PageSpeed project. As everyone would benefit of clean docker images....

I would value the opinion of @oschaaf on adding the Nginx brotli plugin, as an optional extra docker image tag.

oschaaf commented 6 years ago

@nberlee Adding Brotli support to the PageSpeed modules actually was prioritized quite highly:

The advantage I see of having PageSpeed controlling the encoding is that it will do the compression in the background, rate-limit the amount of compression, and also cache outputs for re-use, both in regular (with .pagespeed. -rewritten urls) and in in-place mode. One could then turn on another brotli module for whatever content didn't get compressed by pagespeed, probably just the html.

However, while the feature is still assigned, I'm pretty sure no progress was made on this for a long time. Also, work is in progress for ironing out some very subtle bugs that entered when gzip support was added, it is probably best that is wrapped up first before complicating things some more.

Having said that, support isn't there yet in PageSpeed, and it is unclear when that will be completed.

As for my thoughts on supporting ngx_brotli in the docker image: Compression is a relatively resource intensive process, especially for Brotli. Dynamic compression should probably be avoided as much as possible (I suspect dynamic Brotli compression may add significant time-to-first-byte latency, especially on lower-end hardware, potentially to the point of causing actual trouble when traffic peaks hit a site).

I see some value in having ngx_brotli supported here, but I also do wonder if we would be encouraging a practice which could at some point cause trouble for a significant fraction of the target audience. (not everyone may be able to assess adequately when ngx_pagespeed + ngx_brotli is a good idea and when it is not). If we 'single out' ngx_brotli, we do seem to back/recommend that combination, on the surface at least. So a related question: I do wonder if it would make sense, and viable to come up with a way to add an escape for adding a third party module in general? Maybe based on dynamic modules? Then we could write an example of how to accomplish adding a couple of random modules including ngx_brotli for documentation, possibly with some disclaimers/warnings added. This would help even more people, probably, and we wouldn't be singling out one module, avoiding the suggestion of recommendation. WDYT?

sachk commented 5 years ago

I've been using my own hacky docker container, and it works fine.