Closed Bug 1229475 Opened 4 years ago Closed 4 years ago

Update libopus to 1.1.1

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed
Blocking Flags:

People

(Reporter: ionnv, Assigned: rillian)

References

()

Details

Attachments

(5 files, 6 obsolete files)

1.06 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.24 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
799.07 KB, patch
jmvalin
: review+
Details | Diff | Splinter Review
13.85 KB, patch
jmvalin
: review+
Details | Diff | Splinter Review
2.99 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
https://www.opus-codec.org/

"This Opus 1.1.1 release brings many optimizations to the encoder and decoder, including:

    x86 SSE, SSE2 and SSE4.1 intrinsics optimizations with run-time CPU detection contributed by Cisco Systems,
    MIPS intrinsics optimizations contributed by Imagination Technologies,
    ARM Neon optimizations contributed by Linaro and ARM,
    many architecture-independent optimizations and memory footprint reductions that should improve performance on all platforms, and
    several minor bug fixes.

The quality of the encoder should be mostly unchanged compared to version 1.1. The code is available from the downloads page."
Depends on: 945419
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Component: Audio/Video → WebRTC: Audio/Video
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
In Firefox 45 (bug 1229605), jya allowed Opus MSE to be enabled independently of VP9 MSE. YouTube prefers Opus over AAC, so they automatically began serving Opus to Firefox 45+. We are now working with them to A/B test Firefox's Opus vs AAC. They will dial back Opus until we're ready to ship it.

Since libopus 1.1.1 has added many decoder optimizations since 1.1 was release two years ago (!), we should probably update to 1.1.1 before we ask YouTube to serve Opus to Firefox release users. Since they are already serving Opus to Firefox users with libopus 1.1, they will be able to compare user stats for 1.1 vs 1.1.1.
See Also: → 1229605
Ralph, can you update libopus? Anthony volunteered you. :)
Flags: needinfo?(giles)
We should double check whether enabling the new intrinsics in 1.1.1 requires new #defines or Makefile magic be added to moz.build.
Duplicate of this bug: 1233772
WIP. This doensn't work, and I won't be able to work on it again until Dec 29.

 If someone wants to rewrite mshal's parser so filtering is easier in the meantime, feel free to take the bug.
Assignee: nobody → giles
Flags: needinfo?(giles)
1.1.1 changed this (private) API. Make it more like a public api.
Attached patch Part 2 - update to libopus 1.1.1 (obsolete) — Splinter Review
Attachment #8700273 - Attachment is obsolete: true
Attachment #8702748 - Flags: review?(jmvalin)
Patch out the new arch parameter added to run_analysis() in 1.1.1, and replace it with tonality_analysis_init() which can be called to intern the same value inside the state struct.

This makes it sane for the webrtc code to call in the next patch.
Attachment #8702750 - Flags: review?(jmvalin)
Call the new initializer function to set asm capability flags before calling run_analysis() in the webrtc code.
Attachment #8702751 - Flags: review?(rjesup)
Bug 1146204 put this in without fixing update.sh to maintain it. In the interest of landing the update, this just patches the changes back in, so at least we won't lose them next time.
Attachment #8702752 - Flags: review?(cpearce)
Corresponding punt for gen_sources.py.
Attachment #8700272 - Attachment is obsolete: true
Attachment #8702753 - Flags: review?(cpearce)
Attachment #8702751 - Flags: review?(rjesup) → review+
Update run_analysis patch to handle decoder reset properly.

Thanks to Jean-Marc and Mark Harris for review comments. See also https://review.xiph.org/1185/
Attachment #8702750 - Attachment is obsolete: true
Attachment #8702750 - Flags: review?(jmvalin)
Attachment #8702991 - Flags: review?(jmvalin)
Add new files missing from the previous patch.
Attachment #8702748 - Attachment is obsolete: true
Attachment #8702748 - Flags: review?(jmvalin)
Attachment #8703092 - Flags: review?(jmvalin)
run_analysis patch backported from the changes which landed upstream.
Attachment #8702991 - Attachment is obsolete: true
Attachment #8702991 - Flags: review?(jmvalin)
Attachment #8703096 - Flags: review?(jmvalin)
Update unified build patch to add two more conflicting files from 1.1.1.

I only found these because of a redefined cpp symbol warning, which left me nervous that there could be other collisions affecting compression. How trustworthy do you think the warning are to catch everything like this?
Attachment #8702752 - Attachment is obsolete: true
Attachment #8702752 - Flags: review?(cpearce)
Attachment #8703099 - Flags: review?(cpearce)
Comment on attachment 8703092 [details] [diff] [review]
Part 2 - Update libopus source to 1.1.1 v2

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

r+ on the assumption that this gigantic patch basically brings libopus to 1.1.1.
Attachment #8703092 - Flags: review?(jmvalin) → review+
Comment on attachment 8703096 [details] [diff] [review]
Part 3 - Patch out run_analysis arch parameter v3

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

Why does the patch include a patch in addition to changing the files? Also, are these changes identical to what you just landed in Opus master?
(In reply to Jean-Marc Valin (:jmspeex) from comment #17)

> Why does the patch include a patch in addition to changing the files?

We use the update.sh script to pull new releases and apply any patches we're carrying relative to official opus repo. Including a copy of the patch makes this reproducible. We'll be able to remove it when we upgrade to a release with the changes included.

> are these changes identical to what you just landed in Opus master?

Yes. This is a squashed backport of the two patches I landed on master which applies to 1.1.1.
Comment on attachment 8703096 [details] [diff] [review]
Part 3 - Patch out run_analysis arch parameter v3

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

r+
Attachment #8703096 - Flags: review?(jmvalin) → review+
Attachment #8702753 - Flags: review?(cpearce) → review+
Attachment #8703099 - Flags: review?(cpearce) → review+
You need to log in before you can comment on or make changes to this bug.