Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEAT(client): Add opus as output format for recording #5251

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

evris99
Copy link
Contributor

@evris99 evris99 commented Sep 1, 2021

This commits adds opus as an output format for voice recordings as requested in #5065. It uses an OGG container like the already implemented Vorbis codec but has much better compression. For clarity the .opus file extension is used.

Implements #5065

Checks

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Sep 1, 2021

Changes LGTM now but we will have to figure out why this doesn't work on Linux. Maybe we'll have to introduce an option for this that is OFF by default on Linux 🤔
But before doing that I would like to find out why it seems that the Linux version of libsndfile seems to ship without Opus support.

@evris99
Copy link
Contributor Author

evris99 commented Sep 1, 2021

I see that the support for opus files was added in libsndfile version 1.0.29 from here. But Ubuntu versions less than 21.04 use a previous version of the library (source).

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Sep 1, 2021

Ah yes that could explain it.

It seems that Ubuntu 20.04 ships version 1.0.28. The cmake output agrees with this observation.

Because of this I would suggest to following: Check whether the libsndfile version is >= 1.0.29 here:

find_pkg("SndFile;LibSndFile;sndfile" REQUIRED)
# Look for various targets as they are named differently on different platforms
if(static AND TARGET sndfile-static)
target_link_libraries(mumble PRIVATE sndfile-static)
elseif(TARGET SndFile::sndfile)
target_link_libraries(mumble PRIVATE SndFile::sndfile)
elseif(TARGET sndfile)
target_link_libraries(mumble PRIVATE sndfile)
else()
target_link_libraries(mumble PRIVATE ${sndfile_LIBRARIES})
endif()

I assume that the found version is written to SndFile_VERSION so you should be able to use that to check for the version.

If the version is >= 1.0.29, then add a global macro definition USE_SNDFILE_OPUS and then encapsulate all of your changes with #ifdef USE_SNDFILE_OPUS blocks.

That should include this feature if the used sndfile-library is recent enough to support it.

@evris99
Copy link
Contributor Author

evris99 commented Sep 1, 2021

Any ideas as to why the macOS workflows fail?

src/mumble/CMakeLists.txt Outdated Show resolved Hide resolved
src/mumble/CMakeLists.txt Outdated Show resolved Hide resolved
src/mumble/CMakeLists.txt Show resolved Hide resolved
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Sep 1, 2021

Any ideas as to why the macOS workflows fail?

Because macOS is weird? 😅

Don't worry though - it just does this sometimes (we don't know why or when). It should be unrelated to your changes though. The important bit (building) succeeded.

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay the changes LGTM now 👍

Please squash all of your commits into the first one and once that is done, please run scripts/updatetranslations.py in order to bring the translation files up-to-date.
This will create a new, separate commit.

Then force-push your branch with these two new commits.

@Krzmbrzl Krzmbrzl linked an issue Sep 2, 2021 that may be closed by this pull request
@Krzmbrzl Krzmbrzl added client feature-request This issue or PR deals with a new feature labels Sep 2, 2021
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Sep 2, 2021

It seems like in your first commit the description of what was done went missing. Please use the same description for the commit as you did for this PR (including the Fixes xxx line) :)

evris99 and others added 2 commits September 2, 2021 21:19
This commits adds opus as an output format for voice recordings as requested in mumble-voip#5065. It uses an OGG container like the already implemented Vorbis codec but has much better compression. For clarity the .opus file extension is used.

Implements mumble-voip#5065

Co-authored-by: Robert Adam <dev@robert-adam.de>
@Krzmbrzl Krzmbrzl merged commit b47e712 into mumble-voip:master Sep 3, 2021
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Sep 3, 2021

Thank you very much for your contribution 👍

@evris99
Copy link
Contributor Author

evris99 commented Sep 3, 2021

Thank you for your help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Opus as recording output format
2 participants