зеркало из https://github.com/mozilla/gecko-dev.git
Bug 1530715 - P41: Remove OwnedCriticalSection in cubeb stream. r=padenot
The custom mutex, OwnedCriticalSection, in the current code comes with the C-to-Rust translation work of cubeb_audiouniut.cpp. Its design is in C style and not fitted well in the Rust. Rust has a better mutex design. In the previous patches, all the data that may be touched on the different threads are moved into a struct wrapped by a Rust mutex. Those data will be touched in the critical sections created by the Rust mutex. Therefore, this custom mutex becomes redundant. It's time to say goodbye to it. Differential Revision: https://phabricator.services.mozilla.com/D34074 --HG-- extra : moz-landing-system : lando
This commit is contained in:
Chun-Min Chang
Родитель
381a586aed
Коммит
a97f2bb079
|
|
@ -3,4 +3,4 @@ git repository using the update.sh script.
|
|||
|
||||
The cubeb-coreaudio-rs git repository is: https://github.com/ChunMinChang/cubeb-coreaudio-rs
|
||||
|
||||
The git commit ID used was 729b0418d7ba1ba62bff2391d6b0e8b7515207b6 (2019-06-25 11:32:23 -0700)
|
||||
The git commit ID used was 6d49464259fa527203d7494e323be54845403b47 (2019-06-25 11:32:23 -0700)
|
||||
|
|
|
|||
|
|
@ -12,7 +12,6 @@ mod aggregate_device;
|
|||
mod auto_array;
|
||||
mod auto_release;
|
||||
mod mixer;
|
||||
mod owned_critical_section;
|
||||
mod property_address;
|
||||
mod resampler;
|
||||
mod utils;
|
||||
|
|
@ -28,7 +27,6 @@ use self::coreaudio_sys_utils::dispatch::*;
|
|||
use self::coreaudio_sys_utils::string::*;
|
||||
use self::coreaudio_sys_utils::sys::*;
|
||||
use self::mixer::*;
|
||||
use self::owned_critical_section::*;
|
||||
use self::property_address::*;
|
||||
use self::resampler::*;
|
||||
use self::utils::*;
|
||||
|
|
@ -2465,7 +2463,6 @@ impl ContextOps for AudioUnitContext {
|
|||
state_callback,
|
||||
global_latency_frames,
|
||||
));
|
||||
boxed_stream.init_mutex();
|
||||
|
||||
boxed_stream.core_stream_data = Mutex::new(CoreStreamData::new(
|
||||
boxed_stream.as_ref(),
|
||||
|
|
@ -2474,10 +2471,6 @@ impl ContextOps for AudioUnitContext {
|
|||
));
|
||||
|
||||
if let Err(r) = {
|
||||
// It's not critical to lock here, because no other thread has been started
|
||||
// yet, but it allows to assert that the lock has been taken in `AudioUnitStream::setup`.
|
||||
let mutex_ptr = &mut boxed_stream.mutex as *mut OwnedCriticalSection;
|
||||
let _lock = AutoLock::new(unsafe { &mut (*mutex_ptr) });
|
||||
let mut core_stream_data = boxed_stream.core_stream_data.lock().unwrap();
|
||||
core_stream_data.setup()
|
||||
} {
|
||||
|
|
@ -3337,7 +3330,6 @@ struct AudioUnitStream<'ctx> {
|
|||
data_callback: ffi::cubeb_data_callback,
|
||||
state_callback: ffi::cubeb_state_callback,
|
||||
device_changed_callback: Mutex<ffi::cubeb_device_changed_callback>,
|
||||
mutex: OwnedCriticalSection,
|
||||
// Frame counters
|
||||
frames_played: AtomicU64,
|
||||
frames_queued: u64,
|
||||
|
|
@ -3373,7 +3365,6 @@ impl<'ctx> AudioUnitStream<'ctx> {
|
|||
data_callback,
|
||||
state_callback,
|
||||
device_changed_callback: Mutex::new(None),
|
||||
mutex: OwnedCriticalSection::new(),
|
||||
frames_played: AtomicU64::new(0),
|
||||
frames_queued: 0,
|
||||
frames_read: AtomicI64::new(0),
|
||||
|
|
@ -3390,10 +3381,6 @@ impl<'ctx> AudioUnitStream<'ctx> {
|
|||
}
|
||||
}
|
||||
|
||||
fn init_mutex(&mut self) {
|
||||
self.mutex.init();
|
||||
}
|
||||
|
||||
fn add_device_listener(&self, listener: &device_property_listener) -> OSStatus {
|
||||
audio_object_add_property_listener(
|
||||
listener.device,
|
||||
|
|
@ -3433,9 +3420,6 @@ impl<'ctx> AudioUnitStream<'ctx> {
|
|||
}
|
||||
|
||||
{
|
||||
let mutex_ptr = &mut self.mutex as *mut OwnedCriticalSection;
|
||||
let _lock = AutoLock::new(unsafe { &mut (*mutex_ptr) });
|
||||
|
||||
let mut core_stream_data = self.core_stream_data.lock().unwrap();
|
||||
|
||||
assert!(
|
||||
|
|
@ -3565,8 +3549,6 @@ impl<'ctx> AudioUnitStream<'ctx> {
|
|||
}
|
||||
|
||||
fn destroy_internal(&mut self) {
|
||||
let mutex_ptr = &mut self.mutex as *mut OwnedCriticalSection;
|
||||
let _lock = AutoLock::new(unsafe { &mut (*mutex_ptr) });
|
||||
let mut core_stream_data = self.core_stream_data.lock().unwrap();
|
||||
core_stream_data.close();
|
||||
assert!(self.context.active_streams() >= 1);
|
||||
|
|
|
|||
|
|
@ -1,252 +0,0 @@
|
|||
// Copyright © 2018 Mozilla Foundation
|
||||
//
|
||||
// This program is made available under an ISC-style license. See the
|
||||
// accompanying file LICENSE for details.
|
||||
|
||||
extern crate libc;
|
||||
|
||||
use self::libc::*;
|
||||
use std::{fmt, mem};
|
||||
|
||||
pub struct OwnedCriticalSection {
|
||||
mutex: pthread_mutex_t,
|
||||
}
|
||||
|
||||
// Notice that `OwnedCriticalSection` only works after `init` is called.
|
||||
//
|
||||
// Since `pthread_mutex_t` cannot be copied, we don't initialize the `mutex` in
|
||||
// `new()`. The `let x = OwnedCriticalSection::new()` will temporarily creat a
|
||||
// `OwnedCriticalSection` struct inside `new()` and copy the temporary struct
|
||||
// to `x`, and then destroy the temporary struct. We should initialize the
|
||||
// `x.mutex` instead of the `mutex` created temporarily inside `new()`.
|
||||
//
|
||||
// Example:
|
||||
// let mut mutex = OwnedCriticalSection::new();
|
||||
// mutex.init();
|
||||
impl OwnedCriticalSection {
|
||||
pub fn new() -> Self {
|
||||
OwnedCriticalSection {
|
||||
mutex: PTHREAD_MUTEX_INITIALIZER,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn init(&mut self) {
|
||||
unsafe {
|
||||
let mut attr: pthread_mutexattr_t = mem::zeroed();
|
||||
let r = pthread_mutexattr_init(&mut attr);
|
||||
assert_eq!(r, 0);
|
||||
let r = pthread_mutexattr_settype(&mut attr, PTHREAD_MUTEX_ERRORCHECK);
|
||||
assert_eq!(r, 0);
|
||||
let r = pthread_mutex_init(&mut self.mutex, &attr);
|
||||
assert_eq!(r, 0);
|
||||
let _ = pthread_mutexattr_destroy(&mut attr);
|
||||
}
|
||||
}
|
||||
|
||||
fn destroy(&mut self) {
|
||||
unsafe {
|
||||
let r = pthread_mutex_destroy(&mut self.mutex);
|
||||
assert_eq!(r, 0);
|
||||
}
|
||||
}
|
||||
|
||||
fn lock(&mut self) {
|
||||
unsafe {
|
||||
let r = pthread_mutex_lock(&mut self.mutex);
|
||||
assert_eq!(r, 0, "Deadlock");
|
||||
}
|
||||
}
|
||||
|
||||
fn unlock(&mut self) {
|
||||
unsafe {
|
||||
let r = pthread_mutex_unlock(&mut self.mutex);
|
||||
assert_eq!(r, 0, "Unlocking unlocked mutex");
|
||||
}
|
||||
}
|
||||
|
||||
pub fn assert_current_thread_owns(&mut self) {
|
||||
unsafe {
|
||||
let r = pthread_mutex_lock(&mut self.mutex);
|
||||
assert_eq!(r, EDEADLK);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for OwnedCriticalSection {
|
||||
fn drop(&mut self) {
|
||||
self.destroy();
|
||||
}
|
||||
}
|
||||
|
||||
impl fmt::Debug for OwnedCriticalSection {
|
||||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
|
||||
write!(f, "OwnedCriticalSection {{ mutex @ {:p} }}", &self.mutex)
|
||||
}
|
||||
}
|
||||
|
||||
pub struct AutoLock<'a> {
|
||||
mutex: &'a mut OwnedCriticalSection,
|
||||
}
|
||||
|
||||
impl<'a> AutoLock<'a> {
|
||||
pub fn new(mutex: &'a mut OwnedCriticalSection) -> Self {
|
||||
mutex.lock();
|
||||
AutoLock { mutex }
|
||||
}
|
||||
}
|
||||
|
||||
impl<'a> Drop for AutoLock<'a> {
|
||||
fn drop(&mut self) {
|
||||
self.mutex.unlock();
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_create_critical_section() {
|
||||
let mut section = OwnedCriticalSection::new();
|
||||
section.init();
|
||||
section.lock();
|
||||
section.assert_current_thread_owns();
|
||||
section.unlock();
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[should_panic]
|
||||
fn test_critical_section_destroy_without_unlocking_locked() {
|
||||
let mut section = OwnedCriticalSection::new();
|
||||
section.init();
|
||||
section.lock();
|
||||
section.assert_current_thread_owns();
|
||||
// Get EBUSY(16) since we destroy the object
|
||||
// referenced by mutex while it is locked.
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[should_panic]
|
||||
fn test_critical_section_unlock_without_locking() {
|
||||
let mut section = OwnedCriticalSection::new();
|
||||
section.init();
|
||||
section.unlock();
|
||||
// Get EPERM(1) since it has no privilege to
|
||||
// perform the operation.
|
||||
}
|
||||
|
||||
// #[test]
|
||||
// #[should_panic]
|
||||
// fn test_critical_section_assert_without_locking() {
|
||||
// let mut section = OwnedCriticalSection::new();
|
||||
// section.init();
|
||||
// section.assert_current_thread_owns();
|
||||
// // pthread_mutex_lock() in assert_current_thread_owns() returns 0
|
||||
// // since lock() wasn't called, so the `assert_eq` in
|
||||
// // assert_current_thread_owns() fails.
|
||||
|
||||
// // When we finish this test since the above assertion fails,
|
||||
// // `destroy()` will be called within `drop()`. However,
|
||||
// // `destroy()` also will fail on its assertion since
|
||||
// // we didn't unlock the section.
|
||||
// }
|
||||
|
||||
#[test]
|
||||
fn test_critical_section_multithread() {
|
||||
use std::thread;
|
||||
use std::time::Duration;
|
||||
|
||||
struct Resource {
|
||||
value: u32,
|
||||
mutex: OwnedCriticalSection,
|
||||
}
|
||||
|
||||
let mut resource = Resource {
|
||||
value: 0,
|
||||
mutex: OwnedCriticalSection::new(),
|
||||
};
|
||||
resource.mutex.init();
|
||||
|
||||
// Make a vector to hold the children which are spawned.
|
||||
let mut children = vec![];
|
||||
|
||||
// Rust compilter doesn't allow a pointer to be passed across threads.
|
||||
// A hacky way to do that is to cast the pointer into a value, then
|
||||
// the value, which is actually an address, can be copied into threads.
|
||||
let resource_ptr = &mut resource as *mut Resource as usize;
|
||||
|
||||
for i in 0..10 {
|
||||
// Spin up another thread
|
||||
children.push(thread::spawn(move || {
|
||||
let res = unsafe {
|
||||
let ptr = resource_ptr as *mut Resource;
|
||||
&mut (*ptr)
|
||||
};
|
||||
assert_eq!(res as *mut Resource as usize, resource_ptr);
|
||||
|
||||
// Test fails after commenting `AutoLock` and since the order
|
||||
// to run the threads is random.
|
||||
// The scope of `_guard` is a critical section.
|
||||
let _guard = AutoLock::new(&mut res.mutex); // -------------+
|
||||
// |
|
||||
res.value = i; // | critical
|
||||
thread::sleep(Duration::from_millis(1)); // | section
|
||||
// |
|
||||
(i, res.value) // |
|
||||
})); // <-------------------------------------------------------+
|
||||
}
|
||||
|
||||
for child in children {
|
||||
// Wait for the thread to finish.
|
||||
let (num, value) = child.join().unwrap();
|
||||
assert_eq!(num, value)
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_dummy_mutex_multithread() {
|
||||
use std::sync::Mutex;
|
||||
use std::thread;
|
||||
use std::time::Duration;
|
||||
|
||||
struct Resource {
|
||||
value: u32,
|
||||
mutex: Mutex<()>,
|
||||
}
|
||||
|
||||
let mut resource = Resource {
|
||||
value: 0,
|
||||
mutex: Mutex::new(()),
|
||||
};
|
||||
|
||||
// Make a vector to hold the children which are spawned.
|
||||
let mut children = vec![];
|
||||
|
||||
// Rust compilter doesn't allow a pointer to be passed across threads.
|
||||
// A hacky way to do that is to cast the pointer into a value, then
|
||||
// the value, which is actually an address, can be copied into threads.
|
||||
let resource_ptr = &mut resource as *mut Resource as usize;
|
||||
|
||||
for i in 0..10 {
|
||||
// Spin up another thread
|
||||
children.push(thread::spawn(move || {
|
||||
let res = unsafe {
|
||||
let ptr = resource_ptr as *mut Resource;
|
||||
&mut (*ptr)
|
||||
};
|
||||
assert_eq!(res as *mut Resource as usize, resource_ptr);
|
||||
|
||||
// Test fails after commenting res.mutex.lock() since the order
|
||||
// to run the threads is random.
|
||||
// The scope of `_guard` is a critical section.
|
||||
let _guard = res.mutex.lock().unwrap(); // -----------------+
|
||||
// |
|
||||
res.value = i; // | critical
|
||||
thread::sleep(Duration::from_millis(1)); // | section
|
||||
// |
|
||||
(i, res.value) // |
|
||||
})); // <-------------------------------------------------------+
|
||||
}
|
||||
|
||||
for child in children {
|
||||
// Wait for the thread to finish.
|
||||
let (num, value) = child.join().unwrap();
|
||||
assert_eq!(num, value)
|
||||
}
|
||||
}
|
||||
Загрузка…
Ссылка в новой задаче