Closed
Bug 1340910
Opened 7 years ago
Closed 7 years ago
Update Brotli library to 0.6.0 version
Categories
(Core :: Graphics: Text, enhancement, P3)
Core
Graphics: Text
Tracking
()
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: Virtual, Assigned: jfkthame)
References
()
Details
(Keywords: nightly-community, Whiteboard: [gfx-noted])
Attachments
(3 files, 2 obsolete files)
2.48 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
4.73 MB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
19.84 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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)
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Comment 1•7 years ago
|
||
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)
Updated•7 years ago
|
Whiteboard: [gfx-noted]
Comment 2•7 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.
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Keywords: nightly-community
Comment 3•7 years ago
|
||
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
Comment 4•7 years ago
|
||
Err, actually, the nicer python module interface is on master, not in any released version...
Comment 5•7 years ago
|
||
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.
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Summary: Update Brotli library to 0.5.2 version → Update Brotli library to feature 0.6.0/1.0.0 version
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
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
Comment 6•7 years ago
|
||
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
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 7•7 years ago
|
||
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.
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 8•7 years ago
|
||
https://github.com/google/brotli/issues/483#issuecomment-292912658 >and v1.0.0 is coming soon
Updated•7 years ago
|
Priority: -- → P3
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
QA Contact: Virtual
Assignee | ||
Comment 9•7 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•7 years ago
|
Assignee: mh+mozilla → jfkthame
Comment 10•7 years ago
|
||
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)
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•7 years ago
|
Summary: Update Brotli library to future 0.6.0/1.0.0 version → Update Brotli library to 0.6.0 version
Comment 11•7 years ago
|
||
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 | ||
Comment 12•7 years ago
|
||
Attachment #8893338 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8893339 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•7 years ago
|
Attachment #8893338 -
Attachment is obsolete: true
Attachment #8893338 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8893341 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8893342 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•7 years ago
|
Attachment #8891516 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 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.
Updated•7 years ago
|
Attachment #8893339 -
Flags: review?(mh+mozilla) → review+
Updated•7 years ago
|
Attachment #8893341 -
Flags: review?(mh+mozilla) → review+
Comment 18•7 years ago
|
||
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+
Updated•7 years ago
|
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 19•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b71ebbcb56f2 https://hg.mozilla.org/mozilla-central/rev/704c58d8803e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 22•7 years ago
|
||
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
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Comment 23•7 years ago
|
||
Awesome! \o/ Thank you very much! I'm marking this bug as VERIFIED.
Status: RESOLVED → VERIFIED
Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
Reporter
|
||
Updated•5 years ago
|
Type: defect → enhancement
You need to log in
before you can comment on or make changes to this bug.
Description
•