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

Replace OpenAL by PortAudio #1040

Closed

Conversation

roby2014
Copy link
Member

@roby2014 roby2014 commented Feb 27, 2024

Description

  • Replace OpenAL by PortAudio
  • PortAudio Implementation of AudioDevice
  • AudioDevice sample (output)
  • 3D simple stuff

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.
  • Add entry to the changelog's unreleased section.

@roby2014 roby2014 self-assigned this Feb 27, 2024
@roby2014 roby2014 linked an issue Feb 27, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Feb 28, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/docs-preview/pr-1040/
on branch gh-pages at 2024-03-02 22:16 UTC

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

@@ -0,0 +1,15 @@
#include <stdio.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-deprecated-headers ⚠️
inclusion of deprecated C++ header stdio.h; consider using cstdio instead

Suggested change
#include <stdio.h>
#include <cstdio>

auto err = Pa_Initialize();
if (err != paNoError)
{
CUBOS_FAIL("PortAudio failed to initialize: {}", Pa_GetErrorText(err));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for global function Pa_GetErrorText

Suggested change
CUBOS_FAIL("PortAudio failed to initialize: {}", Pa_GetErrorText(err));
CUBOS_FAIL("PortAudio failed to initialize: {}", paGetErrorText(err));

PortAudioDevice::PortAudioDevice(const std::string& specifier)
{
#ifdef WITH_PORTAUDIO
auto err = Pa_Initialize();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for global function Pa_Initialize

Suggested change
auto err = Pa_Initialize();
auto err = paInitialize();

PortAudioDevice::~PortAudioDevice()
{
#ifdef WITH_PORTAUDIO
Pa_Terminate();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for global function Pa_Terminate

Suggested change
Pa_Terminate();
paTerminate();

int PortAudioDevice::getDeviceCount()
{
#ifdef WITH_PORTAUDIO
return Pa_GetDeviceCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for global function Pa_GetDeviceCount

Suggested change
return Pa_GetDeviceCount();
return paGetDeviceCount();

// TODO: should we store PaDeviceInfos instead of strings?
for (int i = 0; i < getDeviceCount(); i++)
{
devices.push_back(std::string(Pa_GetDeviceInfo(i)->name));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for global function Pa_GetDeviceInfo

Suggested change
devices.push_back(std::string(Pa_GetDeviceInfo(i)->name));
devices.push_back(std::string(paGetDeviceInfo(i)->name));

// TODO: should we store PaDeviceInfos instead of strings?
for (int i = 0; i < getDeviceCount(); i++)
{
devices.push_back(std::string(Pa_GetDeviceInfo(i)->name));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-use-emplace ⚠️
use emplace_back instead of push_back

Suggested change
devices.push_back(std::string(Pa_GetDeviceInfo(i)->name));
devices.emplace_back(Pa_GetDeviceInfo(i)->name);

#endif // WITH_PORTAUDIO
}

void PortAudioDevice::setListenerPosition(const glm::vec3&)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change
void PortAudioDevice::setListenerPosition(const glm::vec3&)
void PortAudioDevice::setListenerPosition(const glm::vec3& /*position*/)

#endif // WITH_PORTAUDIO
}

void PortAudioDevice::setListenerOrientation(const glm::vec3&, const glm::vec3&)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change
void PortAudioDevice::setListenerOrientation(const glm::vec3&, const glm::vec3&)
void PortAudioDevice::setListenerOrientation(const glm::vec3& /*forward*/, const glm::vec3& /*up*/)

#endif // WITH_PORTAUDIO
}

void PortAudioDevice::setListenerVelocity(const glm::vec3&)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change
void PortAudioDevice::setListenerVelocity(const glm::vec3&)
void PortAudioDevice::setListenerVelocity(const glm::vec3& /*velocity*/)

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 93 lines in your changes are missing coverage. Please review.

Project coverage is 37.68%. Comparing base (bc3df3b) to head (02f11e5).
Report is 11 commits behind head on main.

❗ Current head 02f11e5 differs from pull request most recent head ac2742d. Consider uploading reports for the commit ac2742d to get more accurate results

Files Patch % Lines
core/src/al/port_audio_device.cpp 0.00% 83 Missing ⚠️
core/src/al/audio_device.cpp 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
- Coverage   37.70%   37.68%   -0.03%     
==========================================
  Files         302      303       +1     
  Lines       25036    25051      +15     
==========================================
  Hits         9441     9441              
- Misses      15595    15610      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/3)

PortAudioOutputCallbackFn callback;

private:
int outputDeviceID;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
int outputDeviceID;
int mOutputDeviceId;


private:
int outputDeviceID;
PaStream* stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member stream

Suggested change
PaStream* stream;
PaStream* mStream;

Comment on lines +25 to +26
static int paCallback(const void*, void* outputBuffer, unsigned long framesPerBuffer,
const PaStreamCallbackTimeInfo*, PaStreamCallbackFlags flags, void* userData)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change
static int paCallback(const void*, void* outputBuffer, unsigned long framesPerBuffer,
const PaStreamCallbackTimeInfo*, PaStreamCallbackFlags flags, void* userData)
static int paCallback(const void* /*unused*/, void* outputBuffer, unsigned long framesPerBuffer,
const PaStreamCallbackTimeInfo* /*unused*/, PaStreamCallbackFlags flags, void* userData)

static int paCallback(const void*, void* outputBuffer, unsigned long framesPerBuffer,
const PaStreamCallbackTimeInfo*, PaStreamCallbackFlags flags, void* userData)
{
PortAudioDevice* portAudioDevice = static_cast<PortAudioDevice*>(userData);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-use-auto ⚠️
use auto when initializing with a cast to avoid duplicating the type name

Suggested change
PortAudioDevice* portAudioDevice = static_cast<PortAudioDevice*>(userData);
auto* portAudioDevice = static_cast<PortAudioDevice*>(userData);


PortAudioDevice::~PortAudioDevice()
{
if (stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-implicit-bool-conversion ⚠️
implicit conversion PaStream * (aka void *) -> bool

Suggested change
if (stream)
if (stream != nullptr)

if (stream)
{
// FIXME: double stop?»
stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ clang-analyzer-optin.cplusplus.VirtualCall ⚠️
Call to virtual method PortAudioDevice::stop during destruction bypasses virtual dispatch

devices.clear();
for (int i = 0; i < deviceCount(); i++)
{
auto deviceInfo = Pa_GetDeviceInfo(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-qualified-auto ⚠️
auto deviceInfo can be declared as const auto *deviceInfo

Suggested change
auto deviceInfo = Pa_GetDeviceInfo(i);
const auto *deviceInfo = Pa_GetDeviceInfo(i);

{
if (i == deviceIndex)
{
auto deviceInfo = Pa_GetDeviceInfo(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-qualified-auto ⚠️
auto deviceInfo can be declared as const auto *deviceInfo

Suggested change
auto deviceInfo = Pa_GetDeviceInfo(i);
const auto *deviceInfo = Pa_GetDeviceInfo(i);

{
if (i == deviceIndex)
{
auto deviceInfo = Pa_GetDeviceInfo(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-qualified-auto ⚠️
auto deviceInfo can be declared as const auto *deviceInfo

Suggested change
auto deviceInfo = Pa_GetDeviceInfo(i);
const auto *deviceInfo = Pa_GetDeviceInfo(i);

Comment on lines 205 to 209
#ifdef WITH_PORTAUDIO
CUBOS_TODO();
#else
UNSUPPORTED();
#endif // WITH_PORTAUDIO
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-named-parameter ⚠️
all parameters should be named in a function

Suggested change
#ifdef WITH_PORTAUDIO
CUBOS_TODO();
#else
UNSUPPORTED();
#endif // WITH_PORTAUDIO
#ifdef WITH_PORTAUDIO
CUBOS_TODO();
#else
UNSUPPORTED();
#endif // WITH_PORTAUDIO

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (2/3)

PortAudioOutputCallbackFn callback;

private:
int outputDeviceID;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
int outputDeviceID;
int mOutputDeviceId;

}

PortAudioDevice::PortAudioDevice(int deviceIndex)
: outputDeviceID(deviceIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
: outputDeviceID(deviceIndex)
: mOutputDeviceId(deviceIndex)


CUBOS_INFO("PortAudio initialized");

if (outputDeviceID == -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
if (outputDeviceID == -1)
if (mOutputDeviceId == -1)

Comment on lines 47 to 49
outputDeviceID = Pa_GetDefaultOutputDevice();
CUBOS_INFO("PortAudio will use default output device ('{}') :", outputDeviceID);
printDeviceInformation(outputDeviceID);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
outputDeviceID = Pa_GetDefaultOutputDevice();
CUBOS_INFO("PortAudio will use default output device ('{}') :", outputDeviceID);
printDeviceInformation(outputDeviceID);
mOutputDeviceId = Pa_GetDefaultOutputDevice();
CUBOS_INFO("PortAudio will use default output device ('{}') :", mOutputDeviceId);
printDeviceInformation(mOutputDeviceId);

CUBOS_INFO("Custom output callback function set");

PaStreamParameters outputParameters;
outputParameters.device = outputDeviceID;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
outputParameters.device = outputDeviceID;
outputParameters.device = mOutputDeviceId;

PaStreamParameters outputParameters;
outputParameters.device = outputDeviceID;
// FIXME: what should be specified by the user?
outputParameters.channelCount = Pa_GetDeviceInfo(outputDeviceID)->maxOutputChannels;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
outputParameters.channelCount = Pa_GetDeviceInfo(outputDeviceID)->maxOutputChannels;
outputParameters.channelCount = Pa_GetDeviceInfo(mOutputDeviceId)->maxOutputChannels;

// FIXME: what should be specified by the user?
outputParameters.channelCount = Pa_GetDeviceInfo(outputDeviceID)->maxOutputChannels;
outputParameters.sampleFormat = paFloat32;
outputParameters.suggestedLatency = Pa_GetDeviceInfo(outputDeviceID)->defaultLowOutputLatency;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
outputParameters.suggestedLatency = Pa_GetDeviceInfo(outputDeviceID)->defaultLowOutputLatency;
outputParameters.suggestedLatency = Pa_GetDeviceInfo(mOutputDeviceId)->defaultLowOutputLatency;


private:
int outputDeviceID;
PaStream* stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-use-default-member-init ⚠️
use default member initializer for stream

Suggested change
PaStream* stream;
PaStream* stream{nullptr};


PortAudioDevice::PortAudioDevice(int deviceIndex)
: outputDeviceID(deviceIndex)
, stream(nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-use-default-member-init ⚠️
use default member initializer for stream

Suggested change
, stream(nullptr)
,


private:
int outputDeviceID;
PaStream* stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member stream

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (3/3)

PortAudioOutputCallbackFn callback;

private:
int outputDeviceID;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member outputDeviceID

Suggested change
int outputDeviceID;
int mOutputDeviceId;


private:
int outputDeviceID;
PaStream* stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-identifier-naming ⚠️
invalid case style for private member stream

Suggested change
PaStream* stream;
PaStream* mStream;

audio->enumerateDevices(devices, true);

audio->init([](void* outputBuffer, unsigned long framesPerBuffer, unsigned long, void*) -> int {
float* out = static_cast<float*>(outputBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-use-auto ⚠️
use auto when initializing with a cast to avoid duplicating the type name

Suggested change
float* out = static_cast<float*>(outputBuffer);
auto* out = static_cast<float*>(outputBuffer);

float* out = static_cast<float*>(outputBuffer);
for (unsigned int i = 0; i < framesPerBuffer; i++)
{
*out++ = 0.5f * static_cast<float>(std::sin(2.0 * 3.141592653589793 * 440.0 * i / 44100.0));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ readability-uppercase-literal-suffix ⚠️
floating point literal has suffix f, which is not uppercase

Suggested change
*out++ = 0.5f * static_cast<float>(std::sin(2.0 * 3.141592653589793 * 440.0 * i / 44100.0));
*out++ = 0.5F * static_cast<float>(std::sin(2.0 * 3.141592653589793 * 440.0 * i / 44100.0));

@roby2014
Copy link
Member Author

Closing due to: members already working in another solution

@roby2014 roby2014 closed this Sep 22, 2024
@roby2014 roby2014 deleted the 1005-replace-openal-audio-device-by-another-backend branch September 22, 2024 19:20
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.

Replace OpenAL audio device by another backend
1 participant