GNOME Bugzilla – Bug 788418
Add brotli encoding support
Last modified: 2018-09-21 16:32:37 UTC
Libsoup could/should support Brotli-encoded content, indicating the support through the Accept-Encoding header and managing such content through a Brotli decoder in SoupContentDecoder.
Created attachment 360746 [details] [review] RFC patch This patch depends on the Brotli decoder library, libbrotlidec, from Google's project: https://github.com/google/brotli.git The implementation is functional, but it serves more as an RFC to get feedback on whether this approach is appropriate.
I think this is fine. I think the brotli support should be auto by default, the code needs to be cleaned up and we need to add tests, but other than that I think this is fine. Dan?
In particular, the indentation in this patch is really broken, as you can see in the code review tool. Make sure you set tab width to eight characters. The new file should of include the libsoup modeline (and a copyright header).
Review of attachment 360746 [details] [review]: ::: libsoup/soup-brotli-decompressor.c @@ +17,3 @@ + GObject parent_instance; + + BrotliDecoderState* brotli_decoder; Star is always on the name, not the type, in GNOME. @@ +21,3 @@ + +#define G_TYPE_SOUP_BROTLI_DECOMPRESSOR (soup_brotli_decompressor_get_type ()) +#define SOUP_BROTLI_DECOMPRESSOR(o) (G_TYPE_CHECK_INSTANCE_CAST ((o), G_TYPE_SOUP_BROTLI_DECOMPRESSOR, SoupBrotliDecompressor)) This goo belongs in the header file. @@ +44,3 @@ + +static void* +soup_brotli_decompressor_alloc (void* opaque, size_t size) What's the first parameter supposed to be? For placement? Isn't it bad to ignore it?
(In reply to Michael Catanzaro from comment #4) > @@ +44,3 @@ > + > +static void* > +soup_brotli_decompressor_alloc (void* opaque, size_t size) > > What's the first parameter supposed to be? For placement? Isn't it bad to > ignore it? It's an "opaque custom memory manager handle provided by client". In this case it's the `opaque` parameter that's passed to BrotliDecoderCreateInstance(), which is intentionally NULL and as such safe to ignore. BrotliDecoderCreateInstance() declaration: https://github.com/google/brotli/blob/master/c/include/brotli/decode.h#L153 alloc and free function declarations in brotli: https://github.com/google/brotli/blob/master/c/include/brotli/types.h#L62
Comment on attachment 360746 [details] [review] RFC patch I was going to say "this doesn't need to be in libsoup, WebKitGTK can just implement it itself"... > /* This is constant for now */ >+#ifdef LIBSOUP_HAVE_BROTLI >+#define ACCEPT_ENCODING_HEADER "gzip, deflate, br" >+#else > #define ACCEPT_ENCODING_HEADER "gzip, deflate" >+#endif But I guess that's not true. But it *should* be true. I think I just didn't bother to implement that because (a) everyone wants gzip and deflate, and (b) at the time, there were no other encodings in existence. So anyway, SoupContentDecoder should implement the add_feature/remove_feature/has_feature methods of SoupSessionFeature, like SoupAuthManager does, allowing you add GConverters... oh, except there needs to be something specifying the name to include in Accept-Encoding... so, I guess there has to be a SoupContentEncoding interface or something, with GConverter as a prereq, and a get_encoding_name() method. (I don't think this is going to be an important feature to anyone except WebKitGTK, and I don't know that we want to force all other users to pull in libbrotli.)
(In reply to Dan Winship from comment #6) > (I don't think this is going to be an important feature to anyone except > WebKitGTK, That depends on your definition of "important". Its not mandatory in 2017 to have brotli support to talk to most servers. But it is something that all libsoup users could benefit from in order to get faster data transfers for free. (free in this context means that support is implemented in the libsoup library rather than left to the user as an exercise). > and I don't know that we want to force all other users to pull in > libbrotli.) libbrotli is coming to all distributions sooner than later. Specially after WebKitGTK+ will stop bundling it on 2.20 for WOFF2 usage. Example: Debian, Fedora, Arch, OpenSuse.. are all already packaging and shipping it.
If brotli is likely to gain wider adoption than HTTP compression, it could go down to gio, but I personally have no idea if this will be the case -- nor how likely it is to be accepted there.
(In reply to Carlos Alberto Lopez Perez from comment #7) > But it is something that all libsoup users could benefit from in order to > get faster data transfers for free. (free in this context means that support > is implemented in the libsoup library rather than left to the user as an > exercise). I shared some numbers here https://bugs.webkit.org/show_bug.cgi?id=178122#c13 about how many of the Alexa top N sites support brotli for http content-encoding as of today.
(In reply to Carlos Alberto Lopez Perez from comment #7) > But it is something that all libsoup users could benefit from in order to > get faster data transfers for free. As I understand it, brotli is specifically designed for compressing *web pages*, and so is primarily of use to libsoup users who are downloading web pages (ie, WebKitGTK) and less useful to people connecting to random JSON APIs (that probably only return small responses anyway). > Example: Debian, Fedora, Arch, OpenSuse.. are all already packaging and > shipping it. The problem isn't whether distros ship it, it's whether people want to pull it in as a dependency. Eg, NetworkManager used to use libsoup to do captive portal detection, but it was eventually rewritten to use libcurl instead, because some people complained that the libsoup dep was causing NM to require things like sqlite as well. OTOH maybe that's just a sign that libsoup should completely give up on caring about minimizing dependencies, since anyone who cares about that is probably already lost.
(In reply to Dan Winship from comment #6) > Comment on attachment 360746 [details] [review] [review] > RFC patch > > I was going to say "this doesn't need to be in libsoup, WebKitGTK can just > implement it itself"... > > > /* This is constant for now */ > >+#ifdef LIBSOUP_HAVE_BROTLI > >+#define ACCEPT_ENCODING_HEADER "gzip, deflate, br" > >+#else > > #define ACCEPT_ENCODING_HEADER "gzip, deflate" > >+#endif > > But I guess that's not true. But it *should* be true. I think I just didn't > bother to implement that because (a) everyone wants gzip and deflate, and > (b) at the time, there were no other encodings in existence. > > So anyway, SoupContentDecoder should implement the > add_feature/remove_feature/has_feature methods of SoupSessionFeature, like > SoupAuthManager does, allowing you add GConverters... oh, except there needs > to be something specifying the name to include in Accept-Encoding... so, I > guess there has to be a SoupContentEncoding interface or something, with > GConverter as a prereq, and a get_encoding_name() method. > I agree with this approach.
(In reply to Zan Dobersek from comment #1) > Created attachment 360746 [details] [review] [review] > RFC patch > This used to work back in early September. As it stands, it's not functioning properly.
Is there any reason why brotli couldn't be made an optional dependency for those uninterested in it?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libsoup/issues/106.