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.

Sign up for GitHub

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

Jump to bottom

Cubeb: rewrite locking #12805

Merged
merged 2 commits into from Oct 21, 2022
Merged

Cubeb: rewrite locking #12805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c91135c
Cubeb: rewrite locking
Vestrel Oct 13, 2022
e9ad751
Merge branch 'master' into audio_fixes
Megamouse Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions rpcs3/Emu/Audio/AudioBackend.h
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ enum class AudioChannelCnt : u32
enum class AudioStateEvent : u32
{
UNSPECIFIED_ERROR,
DEFAULT_DEVICE_CHANGED,
DEFAULT_DEVICE_MAYBE_CHANGED,
};

class AudioBackend
Expand Down Expand Up @@ -226,10 +226,10 @@ class AudioBackend
AudioFreq m_sampling_rate = AudioFreq::FREQ_48K;
AudioChannelCnt m_channels = AudioChannelCnt::STEREO;

shared_mutex m_cb_mutex{};
std::timed_mutex m_cb_mutex{};
std::function<u32(u32, void *)> m_write_callback{};

std::recursive_mutex m_state_cb_mutex{};
shared_mutex m_state_cb_mutex{};
std::function<void(AudioStateEvent)> m_state_callback{};

bool m_playing = false;
Expand Down
117 changes: 53 additions & 64 deletions rpcs3/Emu/Audio/Cubeb/CubebBackend.cpp
View file
Open in desktop
Expand Up @@ -22,16 +22,14 @@ CubebBackend::CubebBackend()
}
#endif

std::lock_guard lock(m_dev_sw_mutex);

if (int err = cubeb_init(&m_ctx, "RPCS3", nullptr))
cubeb *ctx{};
if (int err = cubeb_init(&ctx, "RPCS3", nullptr))
{
Cubeb.error("cubeb_init() failed: %i", err);
m_ctx = nullptr;
return;
}

if (int err = cubeb_register_device_collection_changed(m_ctx, CUBEB_DEVICE_TYPE_OUTPUT, device_collection_changed_cb, this))
if (int err = cubeb_register_device_collection_changed(ctx, CUBEB_DEVICE_TYPE_OUTPUT, device_collection_changed_cb, this))
{
Cubeb.error("cubeb_register_device_collection_changed() failed: %i", err);
}
Expand All @@ -40,7 +38,10 @@ CubebBackend::CubebBackend()
m_dev_collection_cb_enabled = true;
}

Cubeb.notice("Using backend %s", cubeb_get_backend_id(m_ctx));
Cubeb.notice("Using backend %s", cubeb_get_backend_id(ctx));

std::lock_guard cb_lock{m_state_cb_mutex};
m_ctx = ctx;
}

CubebBackend::~CubebBackend()
Expand Down Expand Up @@ -80,8 +81,19 @@ bool CubebBackend::Operational()

bool CubebBackend::DefaultDeviceChanged()
{
std::lock_guard lock{m_dev_sw_mutex};
return !m_reset_req.observe() && m_default_dev_changed;
if (m_default_device.empty() || m_reset_req.observe())
{
return false;
}

device_handle device = GetDevice();
if (!device.handle)
{
Cubeb.error("Selected device not found. Trying alternative approach...");
device = GetDefaultDeviceAlt(m_sampling_rate, m_sample_size, m_channels);
}

return !device.handle || device.id != m_default_device;
}

bool CubebBackend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSize sample_size, AudioChannelCnt ch_cnt)
Expand All @@ -92,10 +104,8 @@ bool CubebBackend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSize
return false;
}

std::lock_guard lock(m_cb_mutex);
std::lock_guard dev_sw_lock{m_dev_sw_mutex};
std::lock_guard state_cb_lock{m_state_cb_mutex};
CloseUnlocked();
Close();
std::lock_guard lock{m_cb_mutex};

const bool use_default_device = dev_id.empty() || dev_id == audio_device_enumerator::DEFAULT_DEV_ID;

Expand Down Expand Up @@ -128,6 +138,12 @@ bool CubebBackend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSize
device.ch_cnt = 2;
}

if (use_default_device)
{
std::lock_guard lock{m_state_cb_mutex};
m_default_device = device.id;
}

m_sampling_rate = freq;
m_sample_size = sample_size;
m_channels = static_cast<AudioChannelCnt>(std::min(static_cast<u32>(convert_channel_count(device.ch_cnt)), static_cast<u32>(ch_cnt)));
Expand Down Expand Up @@ -169,7 +185,7 @@ bool CubebBackend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSize
if (int err = cubeb_stream_start(m_stream))
{
Cubeb.error("cubeb_stream_start() failed: %i", err);
CloseUnlocked();
Close();
return false;
}

Expand All @@ -178,15 +194,10 @@ bool CubebBackend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSize
Cubeb.error("cubeb_stream_set_volume() failed: %i", err);
}

if (use_default_device)
{
m_default_device = device.id;
}

return true;
}

void CubebBackend::CloseUnlocked()
void CubebBackend::Close()
{
if (m_stream != nullptr)
{
Expand All @@ -196,24 +207,19 @@ void CubebBackend::CloseUnlocked()
}

cubeb_stream_destroy(m_stream);
m_stream = nullptr;
}

m_playing = false;
m_last_sample.fill(0);
{
std::lock_guard lock{m_cb_mutex};
m_stream = nullptr;
m_playing = false;
m_last_sample.fill(0);
}

m_default_dev_changed = false;
std::lock_guard lock{m_state_cb_mutex};
m_default_device.clear();
}

void CubebBackend::Close()
{
std::lock_guard lock(m_cb_mutex);
std::lock_guard dev_sw_lock{m_dev_sw_mutex};
std::lock_guard state_cb_lock(m_state_cb_mutex);
CloseUnlocked();
}

void CubebBackend::Play()
{
if (m_stream == nullptr)
Expand Down Expand Up @@ -427,14 +433,14 @@ long CubebBackend::data_cb(cubeb_stream* stream, void* user_ptr, void const* /*

std::unique_lock lock(cubeb->m_cb_mutex, std::defer_lock);

if (stream != cubeb->m_stream)
if (!cubeb->m_reset_req.observe() && lock.try_lock_for(std::chrono::microseconds{50}) && cubeb->m_write_callback && cubeb->m_playing)
Copy link
Contributor

@elad335 elad335 Oct 13, 2022 •
edited

Choose a reason for hiding this comment

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

Chancing a lock for a period of time needs to be well justified otherwise it looks like a workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's typically taken by nonblocking code for a short period of time, so it makes sense to spin a bit.

{
Cubeb.error("data_cb called with unknown stream");
return CUBEB_ERROR;
}
if (stream != cubeb->m_stream)
{
// Cubeb.error("data_cb called with unknown stream");
return CUBEB_ERROR;
}

if (!cubeb->m_reset_req.observe() && lock.try_lock() && cubeb->m_write_callback && cubeb->m_playing)
{
const u32 sample_size = cubeb->full_sample_size.observe();
const u32 bytes_req = nframes * sample_size;
u32 written = std::min(cubeb->m_write_callback(bytes_req, output_buffer), bytes_req);
Expand All @@ -454,7 +460,7 @@ long CubebBackend::data_cb(cubeb_stream* stream, void* user_ptr, void const* /*
{
// Stream parameters are modified only after stream_destroy. stream_destroy will return
// only after this callback returns, so it's safe to access full_sample_size here.
memset(output_buffer, 0, nframes * cubeb->full_sample_size.observe());
memset(output_buffer, 0, nframes * cubeb->full_sample_size);
}

return nframes;
Expand Down Expand Up @@ -528,7 +534,12 @@ void CubebBackend::device_collection_changed_cb(cubeb* context, void* user_ptr)
CubebBackend* const cubeb = static_cast<CubebBackend*>(user_ptr);
ensure(cubeb);

std::lock_guard lock{cubeb->m_dev_sw_mutex};
if (cubeb->m_reset_req.observe())
{
return;
}

std::lock_guard cb_lock{cubeb->m_state_cb_mutex};

if (context != cubeb->m_ctx)
{
Expand All @@ -539,34 +550,12 @@ void CubebBackend::device_collection_changed_cb(cubeb* context, void* user_ptr)
// Non default device is used (or default device cannot be detected)
if (cubeb->m_default_device.empty())
{
Cubeb.notice("Skipping default device enumeration.");
Cubeb.notice("Skipping default device notification");
return;
}

device_handle device = cubeb->GetDevice();
if (!device.handle)
{
Cubeb.notice("Selected device not found. Trying alternative approach...");
device = cubeb->GetDefaultDeviceAlt(cubeb->m_sampling_rate, cubeb->m_sample_size, cubeb->m_channels);
}

std::lock_guard cb_lock{cubeb->m_state_cb_mutex};

if (!device.handle)
if (cubeb->m_state_callback)
{
// No devices available
if (!cubeb->m_reset_req.test_and_set() && cubeb->m_state_callback)
{
cubeb->m_state_callback(AudioStateEvent::UNSPECIFIED_ERROR);
}
}
else if (!cubeb->m_reset_req.observe() && device.id != cubeb->m_default_device)
{
cubeb->m_default_dev_changed = true;

if (cubeb->m_state_callback)
{
cubeb->m_state_callback(AudioStateEvent::DEFAULT_DEVICE_CHANGED);
}
cubeb->m_state_callback(AudioStateEvent::DEFAULT_DEVICE_MAYBE_CHANGED);
}
}
5 changes: 1 addition & 4 deletions rpcs3/Emu/Audio/Cubeb/CubebBackend.h
View file
Open in desktop
Expand Up @@ -44,9 +44,8 @@ class CubebBackend final : public AudioBackend

atomic_t<bool> m_reset_req = false;

shared_mutex m_dev_sw_mutex{};
// Protected by callback mutex
std::string m_default_device{};
bool m_default_dev_changed = false;

bool m_dev_collection_cb_enabled = false;

Expand All @@ -55,8 +54,6 @@ class CubebBackend final : public AudioBackend
static void state_cb(cubeb_stream* stream, void* user_ptr, cubeb_state state);
static void device_collection_changed_cb(cubeb* context, void* user_ptr);

void CloseUnlocked();

struct device_handle
{
cubeb_devid handle{};
Expand Down
2 changes: 1 addition & 1 deletion rpcs3/Emu/Audio/FAudio/FAudioBackend.cpp
View file
Open in desktop
Expand Up @@ -234,7 +234,7 @@ void FAudioBackend::OnVoiceProcessingPassStart_func(FAudioVoiceCallback *cb_obj,
FAudioBackend *faudio = static_cast<FAudioBackend *>(cb_obj);

std::unique_lock lock(faudio->m_cb_mutex, std::defer_lock);
if (BytesRequired && !faudio->m_reset_req.observe() && lock.try_lock() && faudio->m_write_callback && faudio->m_playing)
if (BytesRequired && !faudio->m_reset_req.observe() && lock.try_lock_for(std::chrono::microseconds{50}) && faudio->m_write_callback && faudio->m_playing)
{
ensure(BytesRequired <= faudio->m_data_buf.size(), "FAudio internal buffer is too small. Report to developers!");

Expand Down
26 changes: 14 additions & 12 deletions rpcs3/Emu/Audio/XAudio2/XAudio2Backend.cpp
View file
Open in desktop
Expand Up @@ -80,12 +80,6 @@ XAudio2Backend::XAudio2Backend()
return;
}

if (HRESULT hr = enumerator->RegisterEndpointNotificationCallback(this); FAILED(hr))
{
XAudio.error("RegisterEndpointNotificationCallback() failed: %s (0x%08x)", std::system_category().message(hr), static_cast<u32>(hr));
return;
}

// All succeeded, "commit"
m_xaudio2_instance = std::move(instance);
m_device_enumerator = std::move(enumerator);
Expand Down Expand Up @@ -125,7 +119,7 @@ bool XAudio2Backend::Operational()

bool XAudio2Backend::DefaultDeviceChanged()
{
std::lock_guard lock{m_dev_sw_mutex};
std::lock_guard lock{m_state_cb_mutex};
return !m_reset_req.observe() && m_default_dev_changed;
}

Expand Down Expand Up @@ -162,17 +156,19 @@ void XAudio2Backend::CloseUnlocked()
m_master_voice = nullptr;
}

m_device_enumerator->UnregisterEndpointNotificationCallback(this);

m_playing = false;
m_last_sample.fill(0);

std::lock_guard lock(m_state_cb_mutex);
m_default_dev_changed = false;
m_current_device.clear();
}

void XAudio2Backend::Close()
{
std::lock_guard lock(m_cb_mutex);
std::lock_guard dev_sw_lock{m_dev_sw_mutex};
CloseUnlocked();
}

Expand Down Expand Up @@ -207,7 +203,6 @@ bool XAudio2Backend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSi
}

std::lock_guard lock(m_cb_mutex);
std::lock_guard dev_sw_lock{m_dev_sw_mutex};
CloseUnlocked();

const bool use_default_device = dev_id.empty() || dev_id == audio_device_enumerator::DEFAULT_DEV_ID;
Expand Down Expand Up @@ -282,6 +277,13 @@ bool XAudio2Backend::Open(std::string_view dev_id, AudioFreq freq, AudioSampleSi
return false;
}

if (HRESULT hr = m_device_enumerator->RegisterEndpointNotificationCallback(this); FAILED(hr))
{
XAudio.error("RegisterEndpointNotificationCallback() failed: %s (0x%08x)", std::system_category().message(hr), static_cast<u32>(hr));
CloseUnlocked();
return false;
}

if (HRESULT hr = m_source_voice->SetVolume(1.0f); FAILED(hr))
{
XAudio.error("SetVolume() failed: %s (0x%08x)", std::system_category().message(hr), static_cast<u32>(hr));
Expand Down Expand Up @@ -331,7 +333,7 @@ f64 XAudio2Backend::GetCallbackFrameLen()
void XAudio2Backend::OnVoiceProcessingPassStart(UINT32 BytesRequired)
{
std::unique_lock lock(m_cb_mutex, std::defer_lock);
if (BytesRequired && !m_reset_req.observe() && lock.try_lock() && m_write_callback && m_playing)
if (BytesRequired && !m_reset_req.observe() && lock.try_lock_for(std::chrono::microseconds{50}) && m_write_callback && m_playing)
{
ensure(BytesRequired <= m_data_buf.size(), "XAudio internal buffer is too small. Report to developers!");

Expand Down Expand Up @@ -386,7 +388,7 @@ HRESULT XAudio2Backend::OnDefaultDeviceChanged(EDataFlow flow, ERole role, LPCWS
return S_OK;
}

std::lock_guard lock{m_dev_sw_mutex};
std::lock_guard lock(m_state_cb_mutex);

// Non default device is used
if (m_current_device.empty())
Expand All @@ -404,7 +406,7 @@ HRESULT XAudio2Backend::OnDefaultDeviceChanged(EDataFlow flow, ERole role, LPCWS

if (m_state_callback)
{
m_state_callback(AudioStateEvent::DEFAULT_DEVICE_CHANGED);
m_state_callback(AudioStateEvent::DEFAULT_DEVICE_MAYBE_CHANGED);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion rpcs3/Emu/Audio/XAudio2/XAudio2Backend.h
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ class XAudio2Backend final : public AudioBackend, public IXAudio2VoiceCallback,

Microsoft::WRL::ComPtr<IMMDeviceEnumerator> m_device_enumerator{};

shared_mutex m_dev_sw_mutex{};
// Protected by state callback mutex
std::string m_current_device{};
bool m_default_dev_changed = false;

Expand Down