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

Opus encoder #1386

Merged
merged 11 commits into from
Jan 9, 2019
Merged

Opus encoder #1386

merged 11 commits into from
Jan 9, 2019

Conversation

Palakis
Copy link
Contributor

@Palakis Palakis commented Nov 21, 2017

Features:

  • Works for Recording and/or Live Broadcasting
  • Track Metadata for Recording (LB metadata is handled differently)
  • CBR and VBR modes

Started as an internal PR on my fork. Original conversation here: https://github.com/Palakis/mixxx/pull/3

@Palakis
Copy link
Contributor Author

Palakis commented Nov 21, 2017

Among other small bugs, here's an interesting comment by @daschuer from the original pull request:

The encoding type options are getting overwhelmed. Could you add a tooltip for each option, with a > hint for what it should be used:

"Mp3, old but plays everywhere"
"Ogg Vobis, the free an better codec"
"Opus, the Vobis successor"
"AAC ...

We can optimize these strings now or later

@daschuer
Copy link
Member

Thank you for this PR. Is it a candidate for 2.1?
Is it ready for reviewing?

@Palakis
Copy link
Contributor Author

Palakis commented Nov 21, 2017

@daschuer It is a candidate for 2.1, ready for reviewing.

@JosepMaJAZ
Copy link
Contributor

We currently build against opusfile 0.7 and opus 1.1.2, which does not contain some of the functions required. We need to update (windows) buildserver libs.

@Palakis
Copy link
Contributor Author

Palakis commented Dec 2, 2017

Hmm, SampleBuffer causes no trouble on my machine, but seems like Travis can't find it...

@Sa-Ja-Di
Copy link

Sa-Ja-Di commented Dec 3, 2017

So there is a chance that in the next version streaming in Opus is a possibility? I would love that very much as even 48 kbps in Opus is a damn fine quality. Hope you can make it :)

@daschuer
Copy link
Member

daschuer commented Dec 3, 2017

Travis build the merge with master. I can reproduce the build failure on my stem after merging master. So I assume it is a kind of merge conflict.

@Be-ing Be-ing added this to the 2.2.0 milestone Dec 27, 2017
@daschuer
Copy link
Member

@Palakis: will you have time to finish this for Mixxx 2.2?

@Palakis
Copy link
Contributor Author

Palakis commented Apr 22, 2018

@daschuer Sure! I'll have a look at the build failure next week and see what's left to fix.

@Palakis
Copy link
Contributor Author

Palakis commented May 21, 2018

I fixed the merge conflict by rebasing this branch on the latest master. Linux CI passes without issues, however Windows and macOS fail with "undefined symbol" errors, each platform showing a different set of missing symbols.

Windows:

   Creating library win64_build\mixxx.lib and object win64_build\mixxx.exp
opusfile.lib(enc_API.obj) : error LNK2019: unresolved external symbol silk_encode_do_VAD_FLP referenced in function silk_Encode
opusfile.lib(enc_API.obj) : error LNK2019: unresolved external symbol silk_encode_frame_FLP referenced in function silk_Encode
win64_build\mixxx.exe : fatal error LNK1120: 2 unresolved externals

macOS:

Undefined symbols for architecture x86_64:
  "_opus_encode_float", referenced from:
      EncoderOpus::processFIFO() in encoderopus.o

src/encoder/encoderopus.h Outdated Show resolved Hide resolved
src/encoder/encoderopus.h Outdated Show resolved Hide resolved
src/encoder/encoderopus.h Outdated Show resolved Hide resolved
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I added some comments for some minor issues that should be fixed before merging.

There are even more minor code style issue (formatting, missing curly braces, const naming convention, C-style casts, ...) that you may leave as is if no one else complains ;)

@Palakis
Copy link
Contributor Author

Palakis commented Jan 8, 2019

@uklotzde Thanks for your review! Fixed these issues.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thanks @Palakis for getting this in shape quickly. This is a nice feature for 2.3, although I'm not able to test it myself. LGTM.

@uklotzde
Copy link
Contributor

uklotzde commented Jan 9, 2019

Merge?

@daschuer
Copy link
Member

daschuer commented Jan 9, 2019

Yes.
I think we should wait for Jenkins had all 2.3 build with this included and than announce this feature at the forums and mailing list asking for testers.

@daschuer daschuer merged commit 28f902e into mixxxdj:master Jan 9, 2019
// Opus only supports 48 and 96 kHz samplerates
constexpr int EncoderOpus::MASTER_SAMPLERATE = 48000;

const char* EncoderOpus::INVALID_SAMPLERATE_MESSAGE =
Copy link
Contributor

Choose a reason for hiding this comment

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

The OPUS error message is hidden behind other error message (moved downwards in the screens for readability).

  • When broadcasting:
    The warning is displayed beneath the connection error message.
    capto_capture 2019-01-11_10-39-12_am

  • When recording:
    The warning is displayed beneath the recording error message, given no path is set in the recordings folder (default)
    capto_capture 2019-01-11_10-38-32_am

@Palakis Palakis deleted the opus-encoder branch February 6, 2019 09:39
@mixxxbot mixxxbot mentioned this pull request Aug 22, 2022
@daschuer daschuer added broadcast Bugs pertaining to streaming radio broadcaster use-case and removed broadcasting labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Bugs pertaining to streaming radio broadcaster use-case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants