Replace ffmpeg-kit with libopus and libopusenc #6208

jmartinesp · 2022-05-31T13:32:02Z

Type of change

Feature
Bugfix
Technical
Other :

Content

This replaces ffmpeg-kit, which was used to convert audio files to and from ogg files with opus codec with libopus and a JNI wrapper. This change is only noticeable on devices with Android version < 10, as >= 10 can use Android's own implementation. This also removes the need for converting these files in the first place, as received audio files will already be in ogg format and the ones recorded will be encoded on-the-fly, with no need for post-processing.

Motivation and context

Fixes #6203. It's done mainly to get rid of a dependency and licensing issues for customers.

Tests

Ideally, we should test this on phones or emulators in versions < 10 and >= 10, both recording and playing and on both ARM and x86 archs.

Open a room with someone else.
Record and send a voice message.
Play that voice message.
Play some other voice message received from the other user (ideally, from a version without this change).

Tested devices

Physical
Emulator
OS version(s): 12 (emulator), 9 (phone).

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

bmarty · 2022-05-31T13:44:47Z

library/opusencoder/src/main/cpp/codec/CodecOggOpus.cpp

@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2022 New Vector Ltd


You should keep the original copyright.

Also, maybe create a doc to explain where does this code come from?

Since I wrote this file I guess that's the original copyright? I just changed if from 'written by Jorge Martín on dd/MM' to the vector copyright, since it wasn't applied automatically and I didn't notice before uploading it.
Docs are a good idea, thanks.

Ah, yes, sorry. If you are the author, this is the correct copyright.

github-actions · 2022-05-31T15:04:37Z

Unit Test Results

146 files ±0 146 suites ±0 2m 10s ⏱️ -30s
236 tests ±0 236 ✔️ ±0 0 💤 ±0 0 ❌ ±0
788 runs ±0 788 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 2c56b21. ± Comparison against base commit 9cf9e10.

jmartinesp · 2022-06-01T06:12:38Z

Please note that there's no need to review the files in the opus/include directory as they're just included headers from the opus library. This should make the review go from 4400 LoC to ~500.

onurays · 2022-06-07T13:43:53Z

vector/src/main/java/im/vector/app/features/voice/VoiceRecorderL.kt

+        recordingJob = recorderScope.launch {
+            while (audioRecorder?.state != AudioRecord.STATE_INITIALIZED) {
+                // If the recorder is not ready let's give it some extra time
+                delay(10L)


Do we have a reported problem about this or can you reproduce an issue?

I think I ran into this before, but I can't seem to trigger this situation again. It should always be STATE_INITIALIZED at this point, so I'll just remove it.

onurays · 2022-06-07T13:45:48Z

library/opusencoder/src/main/java/im/vector/opusencoder/configuration/SampleRate.kt

+ * Sampling rate of the input signal in Hz.
+ */
+sealed class SampleRate private constructor(val value: Int) {
+    object _8khz : SampleRate(8000)


Starting with _ prefix is not allowed by lint.

I honestly can't find a better way to name these and keep the numbers. Any suggestions?

In the end I went with a Rate prefix. It's a bit weird, but it should fix the issue.

Claire1817 · 2022-06-07T15:03:50Z

Code Quality Checks failed, did you check it ? :)

jmartinesp · 2022-06-07T15:04:29Z

Code Quality Checks failed, did you check it ? :)

Yes, it's related to this: #6208 (comment)

bmarty · 2022-06-07T21:33:21Z

What is the impact on the APK size? I think it will be good, can you check please? Thanks!

jmartinesp · 2022-06-08T06:42:46Z

What is the impact on the APK size? I think it will be good, can you check please? Thanks!

With FFMPEG-kit:

 53M  8 jun 07:45 vector-gplay-arm64-v8a-release-unsigned.apk
 60M  8 jun 07:45 vector-gplay-armeabi-v7a-release-unsigned.apk
136M  8 jun 07:45 vector-gplay-universal-release-unsigned.apk
 54M  8 jun 07:45 vector-gplay-x86-release-unsigned.apk
 56M  8 jun 07:45 vector-gplay-x86_64-release-unsigned.apk

Without FFMPEG-kit, using libopus:

 45M  8 jun 08:30 vector-gplay-arm64-v8a-release-unsigned.apk
 43M  8 jun 08:30 vector-gplay-armeabi-v7a-release-unsigned.apk
 94M  8 jun 08:30 vector-gplay-universal-release-unsigned.apk
 46M  8 jun 08:30 vector-gplay-x86-release-unsigned.apk
 47M  8 jun 08:30 vector-gplay-x86_64-release-unsigned.apk

Claire1817

Tested with:
Emulator API 31 and Element Web : OK 👍
Emulator API 30 and Emulator API 30: Ok 👍

onurays

Nice improvement. LGTM, there are merge conflicts to resolve.

bmarty · 2022-06-15T09:12:43Z

dependencies.gradle

@@ -29,7 +29,7 @@ def jjwt = "0.11.5"
 def vanniktechEmoji = "0.15.0"

 // Testing
-def mockk = "1.12.4"
+def mockk = "1.12.3"


Is there a reason to downgrade here? If yes, please add a comment above this line, else Dependabot will change this line again and we will have some trouble.

bmarty · 2022-06-15T09:15:53Z

vector/build.gradle

@@ -337,6 +337,9 @@ android {
 }

 dependencies {
+    implementation fileTree(dir: "libs", include: ["*.aar", "*.jar"])


Is it necessary to add this line? I prefer to add explicit dependencies one by one if any

No, sorry, this was added to test some pre-built libs and I completely forgot to delete it.

…no codec

sonarcloud · 2022-06-15T10:26:37Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

68.0% Coverage
0.0% Duplication

jmartinesp requested review from a team, Florian14 and Claire1817 and removed request for a team May 31, 2022 13:32

jmartinesp self-assigned this May 31, 2022

bmarty reviewed May 31, 2022

View reviewed changes

jmartinesp force-pushed the feature/jorgem/6203-remove-usage-of-ffmpeg-kit branch from 837640b to 2c56b21 Compare May 31, 2022 13:53

jmartinesp requested review from onurays and removed request for Florian14 June 7, 2022 10:06

onurays reviewed Jun 7, 2022

View reviewed changes

jmartinesp force-pushed the feature/jorgem/6203-remove-usage-of-ffmpeg-kit branch from 2c56b21 to 1162700 Compare June 7, 2022 15:15

Claire1817 approved these changes Jun 8, 2022

View reviewed changes

jmartinesp requested a review from onurays June 8, 2022 07:28

onurays approved these changes Jun 9, 2022

View reviewed changes

jmartinesp force-pushed the feature/jorgem/6203-remove-usage-of-ffmpeg-kit branch from 1162700 to fd4276f Compare June 9, 2022 11:32

jmartinesp enabled auto-merge June 9, 2022 11:32

jmartinesp force-pushed the feature/jorgem/6203-remove-usage-of-ffmpeg-kit branch 2 times, most recently from 3aa7d7d to d1f9c0f Compare June 14, 2022 10:44

bmarty reviewed Jun 15, 2022

View reviewed changes

jmartinesp added 3 commits June 15, 2022 11:34

Replace ffmpeg-kit with libopus and libopusenc.

6fc278e

Fix copyright in C++ files

fa2d9e9

Fix crash when asking for Mic permissions, stopRecord is called with …

b993bd9

…no codec

jmartinesp added 4 commits June 15, 2022 11:34

Add some doc comments

155842f

Address review comments, fix quality check issues.

2fbec13

Add some tests

64334c3

Try to fix tests, address review comments.

c204f41

jmartinesp force-pushed the feature/jorgem/6203-remove-usage-of-ffmpeg-kit branch from d1f9c0f to c204f41 Compare June 15, 2022 09:39

bmarty disabled auto-merge June 15, 2022 13:43

bmarty merged commit cbfe0d6 into develop Jun 15, 2022

bmarty deleted the feature/jorgem/6203-remove-usage-of-ffmpeg-kit branch June 15, 2022 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ffmpeg-kit with libopus and libopusenc #6208

Replace ffmpeg-kit with libopus and libopusenc #6208

jmartinesp commented May 31, 2022

bmarty May 31, 2022

jmartinesp May 31, 2022 •

edited

Loading

bmarty May 31, 2022

github-actions bot commented May 31, 2022

jmartinesp commented Jun 1, 2022

onurays Jun 7, 2022

jmartinesp Jun 7, 2022

onurays Jun 7, 2022

jmartinesp Jun 7, 2022

jmartinesp Jun 7, 2022

Claire1817 commented Jun 7, 2022

jmartinesp commented Jun 7, 2022

bmarty commented Jun 7, 2022

jmartinesp commented Jun 8, 2022

Claire1817 left a comment

onurays left a comment

bmarty Jun 15, 2022

bmarty Jun 15, 2022

jmartinesp Jun 15, 2022

sonarcloud bot commented Jun 15, 2022

Replace ffmpeg-kit with libopus and libopusenc #6208

Replace ffmpeg-kit with libopus and libopusenc #6208

Conversation

jmartinesp commented May 31, 2022

Type of change

Content

Motivation and context

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

jmartinesp May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2022

Unit Test Results

jmartinesp commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Claire1817 commented Jun 7, 2022

jmartinesp commented Jun 7, 2022

bmarty commented Jun 7, 2022

jmartinesp commented Jun 8, 2022

Claire1817 left a comment

Choose a reason for hiding this comment

onurays left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 15, 2022

jmartinesp May 31, 2022 •

edited

Loading