Update Brotli library to 0.6.0 version

VERIFIED FIXED in Firefox 57

Status

()

enhancement
P3
major
VERIFIED FIXED
2 years ago
4 days ago

People

(Reporter: Virtual, Assigned: jfkthame)

Tracking

({nightly-community})

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [gfx-noted], )

Attachments

(3 attachments, 2 obsolete attachments)

https://github.com/google/brotli/releases

> Brotli 0.5.2 changelog:
>   Extracted common parts: constants, dictionary, etc.
>   Converted encoder to plain C
>   Supported build systems: Bazel, CMake, Premake
Flags: needinfo?(fred.wang)
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
I don't have plan to upgrade to 0.5.2, but feel free to submit a patch.

In next versions (https://github.com/google/brotli/issues/483#issuecomment-280666860), it will be possible to create shared libraries. I'm interested to check these as that gives an option for distro maintainers to use system packages.
Flags: needinfo?(fred.wang)
Whiteboard: [gfx-noted]

Comment 2

2 years ago
Note that the v0.5.2 release seems to be in a separate branch so the current update script would not work unless brotli merged that branch to master.
I'm interested in this for bug 1352595 because I want to import the encoder and the python module, and the python module has a nicer interface in 0.5.2.
Assignee: nobody → mh+mozilla
Blocks: 1352595
Err, actually, the nicer python module interface is on master, not in any released version...
Depends on: 1353990
https://github.com/google/brotli/issues/483 suggests there might be a 0.6.0 release soon, so I guess I'll work on master, because 0.5.2 doesn't actually fit the bill for what I need.
Summary: Update Brotli library to 0.5.2 version → Update Brotli library to feature 0.6.0/1.0.0 version
Summary: Update Brotli library to feature 0.6.0/1.0.0 version → Update Brotli library to future 0.6.0/1.0.0 version
In the end, I can manage without an update to brotli. Building the python module doesn't pan out on Windows (needs Visual Studio 10), and while creating a shared library, and loading it with ctypes would work, and the new APIs would be useful, but that would require deep changes to the build system (or a set of awful hacks on top of awful hacks that I don't want to do).

Anyways, long story short, I won't need the brotli update immediately. However, I already went through updating everything that uses brotli to the new API, so I'll attach my patches here after bug 1352595 is finished, and rebase them on top of that.
No longer blocks: 1352595
Brotli 0.6.0 was released
https://github.com/google/brotli/releases/tag/v0.6.0


>Brotli v0.6.0 changelog
>   better compression on 1MiB+ files
>   update "common" API to make dictionary fetching more flexible
>   fix decoder bug
>   faster compression on mid-low quality levels
>   update Python wrapper, etc.
>   fix encoder q10-11 slowdown after long copy
>   introduce Brotli*TakeOutput API
> 
>C API is expected to be final. The only difference in v1.0.0 will be the removal of methods (and constants) marked as deprecated.
Assignee

Comment 9

2 years ago
Doesn't look like 1.0.0 has been released at this point, but there's been a stable 0.6.0 for quite a while, and the next WOFF2 update is going to depend on the updated API here so I'd like to get it into the tree. There are some minor API changes from 0.4.0, hence the updates at various Gecko call sites.
Attachment #8891516 - Flags: review?(gps)
Assignee

Updated

2 years ago
Assignee: mh+mozilla → jfkthame
Comment on attachment 8891516 [details] [diff] [review]
Update Brotli to release 0.6.0 from upstream

Review of attachment 8891516 [details] [diff] [review]:
-----------------------------------------------------------------

Normally I'd rubber stamp this. But I spotted a few changes to Firefox C++ in there and I'm not a peer on any C++ modules.

Since glandium mucked with the brotli code recently for jar support, I'm sending review his way.
Attachment #8891516 - Flags: review?(gps) → review?(mh+mozilla)
Summary: Update Brotli library to future 0.6.0/1.0.0 version → Update Brotli library to 0.6.0 version
Assignee

Updated

2 years ago
Blocks: 1384862
Comment on attachment 8891516 [details] [diff] [review]
Update Brotli to release 0.6.0 from upstream

Review of attachment 8891516 [details] [diff] [review]:
-----------------------------------------------------------------

Please update this patch with hg addremove with the -s option so that the file renames are marked. Also please separate the pure upstream changes from the gecko changes to make the review easier.

::: modules/brotli/moz.build
@@ +6,5 @@
>  
>  with Files('**'):
>      BUG_COMPONENT = ('Core', 'General')
>  
> +EXPORTS.brotli += [

I'm not totally a fan of this, but this is definitely less bad now that the headers are meant to be #included as brotli/*.h.

::: modules/brotli/update.sh
@@ +18,5 @@
>  	mv ${MY_TEMP_DIR}/brotli/$d $d
>  done
>  rm -rf ${MY_TEMP_DIR}
>  
> +#patch -p1 < clang-cl-exceptions.patch

we have version control. If we need this in the future, we can dig it. Just remove the comment and the file if they're not used.
Attachment #8891516 - Flags: review?(mh+mozilla)
Assignee

Updated

2 years ago
Attachment #8893338 - Attachment is obsolete: true
Attachment #8893338 - Flags: review?(mh+mozilla)
Assignee

Updated

2 years ago
Attachment #8891516 - Attachment is obsolete: true
Assignee

Comment 16

2 years ago
Note that the tree will fail to build with pt 2 but not pt 3 applied, so I would normally expect to fold these together before landing.
Assignee

Comment 17

2 years ago
glandium: r? ping...
Flags: needinfo?(mh+mozilla)
Attachment #8893339 - Flags: review?(mh+mozilla) → review+
Attachment #8893341 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8893342 [details] [diff] [review]
pt 3 - Gecko updates required to work with Brotli 0.6.0 API changes

Review of attachment 8893342 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/woff2/src/woff2_dec.cc
@@ -750,5 @@
>  
>  bool Woff2Uncompress(uint8_t* dst_buf, size_t dst_size,
>    const uint8_t* src_buf, size_t src_size) {
>    size_t uncompressed_size = dst_size;
> -  int ok = BrotliDecompressBuffer(src_size, src_buf,

Note that woff2 is third party code, so that change should be documented in a patch and in the README.mozilla in that directory.
Attachment #8893342 - Flags: review?(mh+mozilla) → review+
Flags: needinfo?(mh+mozilla)
Assignee

Comment 19

2 years ago
(In reply to Mike Hommey [:glandium] from comment #18)
> Comment on attachment 8893342 [details] [diff] [review]
> pt 3 - Gecko updates required to work with Brotli 0.6.0 API changes
> 
> Review of attachment 8893342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/woff2/src/woff2_dec.cc
> @@ -750,5 @@
> >  
> >  bool Woff2Uncompress(uint8_t* dst_buf, size_t dst_size,
> >    const uint8_t* src_buf, size_t src_size) {
> >    size_t uncompressed_size = dst_size;
> > -  int ok = BrotliDecompressBuffer(src_size, src_buf,
> 
> Note that woff2 is third party code, so that change should be documented in
> a patch and in the README.mozilla in that directory.

FWIW, this is a temporary adjustment that will immediately be overwritten by the WOFF2 update in bug 1384862, which expects the new Brotli API anyway.
Assignee

Comment 20

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b71ebbcb56f29702e124bf1afbabce9c9228ccc6
Bug 1340910 - pt 1 - Update our Brotli import script to pull release 0.6.0 from upstream. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/704c58d8803e7a0388111af34325dc1964cd4a3b
Bug 1340910 - pt 2+3 - Import Brotli 0.6.0 from upstream, and Gecko updates required to work with Brotli 0.6.0 API changes. r=glandium

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b71ebbcb56f2
https://hg.mozilla.org/mozilla-central/rev/704c58d8803e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Many small improvements from Brotli update:

== Change summary for alert #8670 (as of August 09 2017 10:46 UTC) ==

Improvements:

  3%  compiler warnings summary windows2012-64 pgo      150.00 -> 145.00
  3%  compiler warnings summary windows2012-64 opt      150.00 -> 145.00
  3%  compiler warnings summary windows2012-64-noopt debug 156.00 -> 151.00
  3%  compiler warnings summary windows2012-64 debug    156.00 -> 151.00
  3%  compiler warnings summary windows2012-32 pgo      131.00 -> 127.00
  3%  compiler warnings summary windows2012-32 opt      131.00 -> 127.00
  3%  compiler warnings summary windows2012-32-noopt debug 136.00 -> 132.00
  3%  compiler warnings summary windows2012-32 debug    136.00 -> 132.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8670
Awesome! \o/
Thank you very much!
I'm marking this bug as VERIFIED.
Status: RESOLVED → VERIFIED

Updated

2 years ago
Blocks: 1402350
You need to log in before you can comment on or make changes to this bug.