Skip to content

Commit

Permalink
roc-streaminggh-748: Cleanup IDevice::close()
Browse files Browse the repository at this point in the history
- Destructors should not panic if close was called, but failed.
  Device should be marked as closed in all cases.

- Remove redundant log messages.

- Formatting & methods order.
  • Loading branch information
gavv authored and jeshwanthreddy13 committed Aug 6, 2024
1 parent 26db40a commit 7972728
Show file tree
Hide file tree
Showing 11 changed files with 84 additions and 82 deletions.
1 change: 1 addition & 0 deletions src/internal_modules/roc_pipeline/receiver_loop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ bool ReceiverLoop::has_clock() const {

status::StatusCode ReceiverLoop::close() {
core::Mutex::Lock lock(source_mutex_);

return source_.close();
}

Expand Down
11 changes: 6 additions & 5 deletions src/internal_modules/roc_pipeline/sender_loop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,6 @@ bool SenderLoop::has_state() const {
return sink_.has_state();
}

status::StatusCode SenderLoop::close() {
core::Mutex::Lock lock(sink_mutex_);
return sink_.close();
}

sndio::DeviceState SenderLoop::state() const {
core::Mutex::Lock lock(sink_mutex_);

Expand Down Expand Up @@ -199,6 +194,12 @@ bool SenderLoop::has_clock() const {
return sink_.has_clock();
}

status::StatusCode SenderLoop::close() {
core::Mutex::Lock lock(sink_mutex_);

return sink_.close();
}

status::StatusCode SenderLoop::write(audio::Frame& frame) {
roc_panic_if(init_status_ != status::StatusOK);

Expand Down
2 changes: 1 addition & 1 deletion src/internal_modules/roc_pipeline/sender_loop.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,9 @@ class SenderLoop : public PipelineLoop, private sndio::ISink {
virtual bool has_latency() const;
virtual core::nanoseconds_t latency() const;
virtual bool has_clock() const;
virtual status::StatusCode close()
virtual status::StatusCode write(audio::Frame& frame);
virtual ROC_ATTR_NODISCARD status::StatusCode flush();
virtual status::StatusCode close();

// Methods of PipelineLoop
virtual core::nanoseconds_t timestamp_imp() const;
Expand Down
2 changes: 1 addition & 1 deletion src/internal_modules/roc_sndio/pump.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ status::StatusCode Pump::close_all_devices_() {
if (devices[i]) {
status::StatusCode device_code = devices[i]->close();
if (device_code != status::StatusOK) {
roc_log(LogError, "pump: failed to close device with error %s",
roc_log(LogError, "pump: failed to close device: status=%s",
status::code_to_str(device_code));
if (first_error == status::StatusOK) {
first_error = device_code;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,15 +182,6 @@ PulseaudioDevice::~PulseaudioDevice() {
}
}

status::StatusCode PulseaudioDevice::close() {
roc_log(LogDebug, "pulseaudio %s: closing device", device_type_to_str(device_type_));

close_();
stop_mainloop_();

return status::StatusOK;
}

status::StatusCode PulseaudioDevice::init_status() const {
return init_status_;
}
Expand Down Expand Up @@ -221,6 +212,15 @@ status::StatusCode PulseaudioDevice::open(const char* device) {
return status::StatusOK;
}

status::StatusCode PulseaudioDevice::close() {
roc_log(LogDebug, "pulseaudio %s: closing device", device_type_to_str(device_type_));

close_();
stop_mainloop_();

return status::StatusOK;
}

DeviceType PulseaudioDevice::type() const {
return device_type_;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,6 @@ SndfileSink::~SndfileSink() {
}
}

status::StatusCode SndfileSink::close() {
return close_();
}

status::StatusCode SndfileSink::init_status() const {
return init_status_;
}
Expand All @@ -197,6 +193,10 @@ status::StatusCode SndfileSink::open(const char* driver, const char* path) {
return open_(driver, path);
}

status::StatusCode SndfileSink::close() {
return close_();
}

DeviceType SndfileSink::type() const {
return DeviceType_Sink;
}
Expand Down Expand Up @@ -277,7 +277,7 @@ status::StatusCode SndfileSink::open_(const char* driver, const char* path) {

sample_spec_.set_sample_rate((size_t)file_info_.samplerate);

roc_log(LogInfo, "sndfile sink: opened: %s",
roc_log(LogInfo, "sndfile sink: opened output file: %s",
audio::sample_spec_to_str(sample_spec_).c_str());

return status::StatusOK;
Expand All @@ -288,18 +288,17 @@ status::StatusCode SndfileSink::close_() {
return status::StatusOK;
}

roc_log(LogDebug, "sndfile sink: closing output");
roc_log(LogInfo, "sndfile sink: closing output file");

const int err = sf_close(file_);
file_ = NULL;

if (err != 0) {
roc_log(LogError,
"sndfile sink: sf_close() failed, cannot properly close output: %s",
roc_log(LogError, "sndfile sink: can't properly close output file: %s",
sf_error_number(err));
return status::StatusErrFile;
}

file_ = NULL;

return status::StatusOK;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ SndfileSource::~SndfileSource() {
}
}

status::StatusCode SndfileSource::close() {
return close_();
}

status::StatusCode SndfileSource::init_status() const {
return init_status_;
}
Expand All @@ -74,6 +70,10 @@ status::StatusCode SndfileSource::open(const char* driver, const char* path) {
return open_();
}

status::StatusCode SndfileSource::close() {
return close_();
}

DeviceType SndfileSource::type() const {
return DeviceType_Source;
}
Expand Down Expand Up @@ -116,8 +116,6 @@ status::StatusCode SndfileSource::rewind() {
if (file_) {
const status::StatusCode close_code = close_();
if (close_code != status::StatusOK) {
roc_log(LogError, "sndfile source: failed to close file during rewind: %s",
status::code_to_str(close_code));
return close_code;
}
}
Expand Down Expand Up @@ -204,7 +202,7 @@ status::StatusCode SndfileSource::open_() {
sample_spec_.channel_set().set_order(audio::ChanOrder_Smpte);
sample_spec_.channel_set().set_count((size_t)file_info_.channels);

roc_log(LogInfo, "sndfile source: opened: %s",
roc_log(LogInfo, "sndfile source: opened input file: %s",
audio::sample_spec_to_str(sample_spec_).c_str());

return status::StatusOK;
Expand All @@ -215,18 +213,17 @@ status::StatusCode SndfileSource::close_() {
return status::StatusOK;
}

roc_log(LogInfo, "sndfile source: closing input");
roc_log(LogInfo, "sndfile source: closing input file");

const int err = sf_close(file_);
file_ = NULL;

if (err != 0) {
roc_log(LogError,
"sndfile source: sf_close() failed, cannot properly close input: %s",
roc_log(LogError, "sndfile source: can't properly close input file: %s",
sf_error_number(err));
return status::StatusErrFile;
}

file_ = NULL;

return status::StatusOK;
}

Expand Down
20 changes: 9 additions & 11 deletions src/internal_modules/roc_sndio/target_sox/roc_sndio/sox_sink.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,6 @@ SoxSink::~SoxSink() {
}
}

status::StatusCode SoxSink::close() {
return close_();
}

status::StatusCode SoxSink::init_status() const {
return init_status_;
}
Expand Down Expand Up @@ -108,6 +104,10 @@ status::StatusCode SoxSink::open(const char* driver, const char* path) {
return status::StatusOK;
}

status::StatusCode SoxSink::close() {
return close_();
}

DeviceType SoxSink::type() const {
return DeviceType_Sink;
}
Expand Down Expand Up @@ -155,8 +155,6 @@ status::StatusCode SoxSink::pause() {
if (driver_type_ == DriverType_Device) {
const status::StatusCode close_code = close_();
if (close_code != status::StatusOK) {
roc_log(LogError, "sox sink: failed to close output during pause: %s",
status::code_to_str(close_code));
return close_code;
}
}
Expand Down Expand Up @@ -316,8 +314,8 @@ status::StatusCode SoxSink::open_() {
sample_spec_.channel_set().set_count(actual_chans);

roc_log(LogInfo,
"sox sink:"
" opened: bits=%lu rate=%lu req_rate=%lu chans=%lu req_chans=%lu is_file=%d",
"sox sink: opened output:"
" bits=%lu rate=%lu req_rate=%lu chans=%lu req_chans=%lu is_file=%d",
(unsigned long)output_->encoding.bits_per_sample, actual_rate, requested_rate,
actual_chans, requested_chans, (int)(driver_type_ == DriverType_File));

Expand All @@ -341,17 +339,17 @@ status::StatusCode SoxSink::close_() {
return status::StatusOK;
}

roc_log(LogDebug, "sox sink: closing output");
roc_log(LogInfo, "sox sink: closing output");

const int err = sox_close(output_);
output_ = NULL;

if (err != SOX_SUCCESS) {
roc_log(LogError, "sox sink: can't close output: %s", sox_strerror(err));
return driver_type_ == DriverType_File ? status::StatusErrFile
: status::StatusErrDevice;
}

output_ = NULL;

return status::StatusOK;
}

Expand Down
23 changes: 10 additions & 13 deletions src/internal_modules/roc_sndio/target_sox/roc_sndio/sox_source.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ SoxSource::~SoxSource() {
}
}

status::StatusCode SoxSource::close() {
return close_();
}

status::StatusCode SoxSource::init_status() const {
return init_status_;
}
Expand Down Expand Up @@ -112,6 +108,10 @@ status::StatusCode SoxSource::open(const char* driver, const char* path) {
return status::StatusOK;
}

status::StatusCode SoxSource::close() {
return close_();
}

DeviceType SoxSource::type() const {
return DeviceType_Source;
}
Expand Down Expand Up @@ -159,8 +159,6 @@ status::StatusCode SoxSource::pause() {
if (driver_type_ == DriverType_Device) {
const status::StatusCode close_code = close_();
if (close_code != status::StatusOK) {
roc_log(LogError, "sox source: failed to close input during pause: %s",
status::code_to_str(close_code));
return close_code;
}
}
Expand Down Expand Up @@ -213,8 +211,6 @@ status::StatusCode SoxSource::rewind() {
if (input_) {
const status::StatusCode close_code = close_();
if (close_code != status::StatusOK) {
roc_log(LogError, "sox source: failed to close input during rewind: %s",
status::code_to_str(close_code));
return close_code;
}
}
Expand Down Expand Up @@ -382,8 +378,8 @@ status::StatusCode SoxSource::open_() {
sample_spec_.channel_set().set_count(actual_chans);

roc_log(LogInfo,
"sox source:"
" opened: bits=%lu rate=%lu req_rate=%lu chans=%lu req_chans=%lu is_file=%d",
"sox source: opened input:"
" bits=%lu rate=%lu req_rate=%lu chans=%lu req_chans=%lu is_file=%d",
(unsigned long)input_->encoding.bits_per_sample, actual_rate, requested_rate,
actual_chans, requested_chans, (int)(driver_type_ == DriverType_File));

Expand Down Expand Up @@ -411,13 +407,14 @@ status::StatusCode SoxSource::close_() {
roc_log(LogInfo, "sox source: closing input");

const int err = sox_close(input_);
input_ = NULL;

if (err != SOX_SUCCESS) {
roc_log(LogError, "sox source: can't close input: %s", sox_strerror(err));
return status::StatusErrFile;
return driver_type_ == DriverType_File ? status::StatusErrFile
: status::StatusErrDevice;
}

input_ = NULL;

return status::StatusOK;
}

Expand Down
Loading

0 comments on commit 7972728

Please sign in to comment.