After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788418 - Add brotli encoding support
Add brotli encoding support
Status: RESOLVED OBSOLETE
Product: libsoup
Classification: Core
Component: Misc
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2017-10-02 09:02 UTC by Zan Dobersek
Modified: 2018-09-21 16:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
RFC patch (8.18 KB, patch)
2017-10-02 09:08 UTC, Zan Dobersek
needs-work Details | Review

Description Zan Dobersek 2017-10-02 09:02:07 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.
Comment 1 Zan Dobersek 2017-10-02 09:08:29 UTC
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.
Comment 2 Carlos Garcia Campos 2017-10-02 09:18:59 UTC
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?
Comment 3 Michael Catanzaro 2017-10-02 12:20:22 UTC
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).
Comment 4 Michael Catanzaro 2017-10-02 12:23:40 UTC
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?
Comment 5 Zan Dobersek 2017-10-03 09:31:35 UTC
(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 6 Dan Winship 2017-10-03 16:12:56 UTC
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.)
Comment 7 Carlos Alberto Lopez Perez 2017-10-11 10:49:52 UTC
(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.
Comment 8 Claudio Saavedra 2017-10-11 14:22:36 UTC
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.
Comment 9 Carlos Alberto Lopez Perez 2017-10-11 15:04:34 UTC
(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.
Comment 10 Dan Winship 2017-10-11 20:56:40 UTC
(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.
Comment 11 Zan Dobersek 2017-10-13 17:07:05 UTC
(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.
Comment 12 Zan Dobersek 2017-10-13 17:11:47 UTC
(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.
Comment 13 Claudio Saavedra 2017-10-30 16:13:17 UTC
Is there any reason why brotli couldn't be made an optional dependency for those uninterested in it?
Comment 14 GNOME Infrastructure Team 2018-09-21 16:32:37 UTC
-- 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.