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

Feature: Opus as additional audio codec #1259

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

eXpl0it3r
Copy link
Member

@eXpl0it3r eXpl0it3r commented Aug 2, 2017

SFML Tasks

  • Fix conflict and rebase onto master
  • Visual Studio builds play the audio at a slightly slower rate than the original
  • Conversion warnings between std::size_t, unsigned int, sf::Uint64, opus_uint64, ...
  • MinGW builds don't play a sound with Opus
  • Opus can't be found on some system (see CI builds)
  • MinGW builds have undefined references to some function
  • Test on OS X
  • Test on Linux

Original PR by @susnux #1077.

I've added the Windows binaries, however there are still some issues.

@eXpl0it3r eXpl0it3r added this to the 2.5 milestone Aug 2, 2017
@eXpl0it3r eXpl0it3r self-assigned this Aug 2, 2017
@eXpl0it3r eXpl0it3r added this to Requires Adjustments in SFML 2.5.0 Aug 2, 2017
@eXpl0it3r
Copy link
Member Author

@susnux Does the example opus file from the official opus website playback at the right speed for you on your system?

For me the Paper Lights by Ehren Starks example is about 20s slower than when played back on the website, in VLC or MPC-HC.

@susnux
Copy link
Contributor

susnux commented Aug 5, 2017

Will look into this as quick as possible, but the next few days I am quite busy with work.

@eXpl0it3r eXpl0it3r mentioned this pull request Sep 15, 2017
2 tasks
@SergioRAgostinho
Copy link
Contributor

SergioRAgostinho commented Jan 16, 2018

FYI: I just compiled this and the sound example is working but it's as you reported the number of samples per second seems to be incorrect. Vs the original, the song is playing at a lower tone. It feels like the number of samples per sec were incorrectly parsed from 48k to 44.1k. That justifies the 20 second difference.

OS: Ubuntu 16.04
Audio Interface (USB): Focusrite Scarlett 2i4

Side note: The graphics module is throwing a linker error which is unrelated to this PR.

// Retrieve the music attributes
const OpusHead* opusHead = op_head(m_opus, -1);
info.channelCount = opusHead->channel_count;
info.sampleRate = opusHead->input_sample_rate;
Copy link
Contributor

Choose a reason for hiding this comment

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

@eXpl0it3r
Copy link
Member Author

@susnux Any chance you get around to invest some more time into this?

@SergioRAgostinho
Copy link
Contributor

I'm interested in seeing in this feature pushed so if it's just some final cleanup stuff I can lend a hand.

This fixed the slowed play issue

diff --git a/src/SFML/Audio/SoundFileReaderOpus.cpp b/src/SFML/Audio/SoundFileReaderOpus.cpp
index 095113c..2dfa949 100644
--- a/src/SFML/Audio/SoundFileReaderOpus.cpp
+++ b/src/SFML/Audio/SoundFileReaderOpus.cpp
@@ -119,7 +119,7 @@ bool SoundFileReaderOpus::open(InputStream& stream, Info& info)
     // Retrieve the music attributes
     const OpusHead* opusHead = op_head(m_opus, -1);
     info.channelCount = opusHead->channel_count;
-    info.sampleRate = opusHead->input_sample_rate;
+    info.sampleRate = 48000;
     info.sampleCount = static_cast<std::size_t>(op_pcm_total(m_opus, -1) * opusHead->channel_count);
 
     // We must keep the channel count for the seek function

There's of course the downside that I'm not familiar with the inner details of the library and code style, so the review process might be more involved.

@eXpl0it3r
Copy link
Member Author

eXpl0it3r commented Jan 25, 2018

Would be great if you could create a pull request and base it onto the feature/opus branch.

Also you may want to add a comment with the link from above to the why it needs to be 480000.
Also what's the opusHead->input_sample_rate used for then?

I'll look into getting the merge conflict solved and get it up to speed with the latest master.

@SergioRAgostinho
Copy link
Contributor

Sure.

Also what's the opusHead->input_sample_rate used for then?

I assume it's relevant from a transcoding perspective. Although all opus sound uses a 48kHz sample rate raw audio, if you want to transcode it to another format and assuming the original sample rate was at a lower sample rate, you might want to resample the audio first to it's "original format" before encoding again, saving you some unnecessary samples from the encoding process.

{
assert(m_opus != NULL);

op_pcm_seek(m_opus, sampleOffset / m_channelCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the docstring for SoundFileReader::seek

////////////////////////////////////////////////////////////
/// \brief Change the current read position to the given sample offset
///
/// The sample offset takes the channels into account.
/// Offsets can be calculated like this:
/// `sampleNumber * sampleRate * channelCount`
/// If the given offset exceeds to total number of samples,
/// this function must jump to the end of the file.
///
/// \param sampleOffset Index of the sample to jump to, relative to the beginning
///
////////////////////////////////////////////////////////////
virtual void seek(Uint64 sampleOffset) = 0;

and the docstring for op_pcm_seek.

should it not also be divided by the sample rate, in this case 48000.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. It wouldn't be working at all if this was the case, plus Opus uses the same API as OGG (and FLAC) and there it isn't needed either, plus dividing by 48000 sound like a huge number, not surprising if this would introduce odd rounding issues.

Copy link
Member

Choose a reason for hiding this comment

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

The function wants a number of samples, that's what we give it. Dividing by the sample rate would give a number of seconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I don't get the message

/// Offsets can be calculated like this: 
/// `sampleNumber * sampleRate * channelCount` 

Disregarding the channelCount

sampleNumber still has dimension [samples]
sampleRate is [samples/sec]

according SoundFileReader::seek "offset" expression I would get [samples^2/sec]. What exactly is this? I'm I making a mistake somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

This part of the documentation was added later and is obviously wrong, indeed. Feel free to remove or correct it (sampleNumber should be "timeInSeconds").

@Foaly
Copy link
Contributor

Foaly commented Jan 25, 2018

Seeing the last commit made me wonder. From my knowledge SFML uses 44100 Hz for playback internally. If Opus is read out at 48000 Hz the sound should be pitched down when played back at 44100. Is there a resampling happening somewhere in the encoder or in OpenAL? I tried to answer it by looking at the code, but couldn't find anything.

@LaurentGomila
Copy link
Member

LaurentGomila commented Jan 25, 2018

Is there a resampling happening somewhere in the encoder or in OpenAL?

Sure there is! You can play samples with any source sample rate, it's a parameter (see SoundBuffer::loadFromSamples, or SoundStream::initialize for example).

@Foaly
Copy link
Contributor

Foaly commented Jan 26, 2018

Ah I did not know that! Very interesting! Thank you

@JonnyPtn
Copy link
Contributor

This works well on macOS 10.13, using opus/file installed via brew (And tested with SFML sound example). I'll hopefully add the precompiled libs soon, and give iOS a test

@eXpl0it3r eXpl0it3r moved this from Requires Adjustments to WIP in SFML 2.5.0 Feb 10, 2018
@eXpl0it3r eXpl0it3r removed this from WIP in SFML 2.5.0 Mar 12, 2018
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Mar 12, 2018
@eXpl0it3r eXpl0it3r modified the milestones: 2.5, 2.6 Mar 12, 2018
@eXpl0it3r
Copy link
Member Author

I'll move this to 2.6 for now, but if someone is willing to invest the required time to get it setup and tested across platforms before 2.5 it can be changed again.

@eXpl0it3r eXpl0it3r moved this from Discussion to WIP in SFML 2.6.0 Aug 13, 2018
@eXpl0it3r eXpl0it3r removed this from WIP in SFML 2.6.0 Feb 3, 2020
@eXpl0it3r eXpl0it3r force-pushed the feature/opus branch 2 times, most recently from c8e4678 to baa8a3f Compare November 18, 2020 20:40
@eXpl0it3r eXpl0it3r modified the milestones: 2.6, 3.0 Apr 5, 2021
@ChrisThrasher
Copy link
Member

https://github.com/ChrisThrasher/SFML/tree/feature/opus

I rebased this branch on the master branch and made a few fixes to get it to compile.

@ChrisThrasher
Copy link
Member

Looks like there are still plenty of lossy conversions to add static_cast to. Not a difficult problem to fix but it will be tedious to fix all those.

@eXpl0it3r eXpl0it3r added this to Backlog in SFML 3.0.0 via automation Mar 10, 2022
@eXpl0it3r eXpl0it3r modified the milestones: 3.0, 3.1 Jun 23, 2022
@eXpl0it3r eXpl0it3r removed this from Backlog in SFML 3.0.0 Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants