2017-02-25 23:41 CET

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0004627MPDInput Plugins - Streamingpublic2017-01-14 22:49
Reportersteveno 
Assigned Tocirrus 
PrioritynormalSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformlinuxOSOS Version
Product Version0.20 
Target VersionFixed in Version 
Summary0004627: Alsa input plugin holds mutex lock too often, resulting in buffer underruns in the output
DescriptionAudio output "stutters" every second or two when using an alsa:// URI. The verbose log contains many "buffer underrun" error messages.
The alsa capture PCM parameters are not optimal for use with poll(), resulting in spurious triggering of the event handler.
This behaviour was introduced in commit 95e53ac0a07ef0f7b1b1ce442dfb4f0d2bc3f84b
Steps To Reproduceplay any alsa:// stream
Additional InformationThe attached patch addresses these issues: -

1) It modifies the PCM hardware parameters to make the config more flexible for different sound cards by choosing buffer and period sizes dynamically.
2) It guarantees that an IO event is triggered only when there is at least one period of audio available from the device
3) It guarantees that data is read from the device in chunks of 1 period in size
4) It only locks the mutex when actually transferring data between threads
5) It fixes a race condition in the destructor to ensure the IO thread has finished with alsa resources before they are deleted
6) It re-factors the code to separate out alsa specific resources from MPD ones.
Tagsalsa
Attached Files
  • patch file icon mpd-alsa-input.patch (19,420 bytes) 2017-01-13 19:52 -
    diff --git a/src/event/MultiSocketMonitor.cxx b/src/event/MultiSocketMonitor.cxx
    index aa8450d..b709cb8 100644
    --- a/src/event/MultiSocketMonitor.cxx
    +++ b/src/event/MultiSocketMonitor.cxx
    @@ -87,10 +87,6 @@ MultiSocketMonitor::OnIdle()
     	if (ready) {
     		ready = false;
     		DispatchSockets();
    -
    -		/* TODO: don't refresh always; require users to call
    -		   InvalidateSockets() */
    -		refresh = true;
     	}
     
     	if (refresh) {
    diff --git a/src/event/MultiSocketMonitor.hxx b/src/event/MultiSocketMonitor.hxx
    index 40ce1b4..6f0471a 100644
    --- a/src/event/MultiSocketMonitor.hxx
    +++ b/src/event/MultiSocketMonitor.hxx
    @@ -178,7 +178,6 @@ private:
     
     	virtual void OnTimeout() final {
     		SetReady();
    -		IdleMonitor::Schedule();
     	}
     
     	virtual void OnIdle() final;
    diff --git a/src/input/plugins/AlsaInputPlugin.cxx b/src/input/plugins/AlsaInputPlugin.cxx
    index b3eceb5..66f5e80 100644
    --- a/src/input/plugins/AlsaInputPlugin.cxx
    +++ b/src/input/plugins/AlsaInputPlugin.cxx
    @@ -38,68 +38,344 @@
     #include "event/MultiSocketMonitor.hxx"
     #include "event/DeferredMonitor.hxx"
     #include "IOThread.hxx"
    +#include "AudioFormat.hxx"
     
     #include <alsa/asoundlib.h>
     
     #include <assert.h>
     #include <string.h>
    +#include <atomic>
     
    -static constexpr Domain alsa_input_domain("alsa");
    +static constexpr Domain alsa_input_domain("alsa_input");
     
     static constexpr const char *default_device = "hw:0,0";
     
    -// the following defaults are because the PcmDecoderPlugin forces CD format
    -static constexpr snd_pcm_format_t default_format = SND_PCM_FORMAT_S16;
    -static constexpr int default_channels = 2; // stereo
    -static constexpr unsigned int default_rate = 44100; // cd quality
    +/*
    + * the following default is chosen because the PcmDecoderPlugin forces CD format
    + */
    +static constexpr AudioFormat default_format = {
    +	44100,
    +	SampleFormat::S16,
    +	2,
    +};
    +
    +/*
    + * Function copied from Alsa output plugin
    + * Really needs factoring out into a shared header
    + */
    +gcc_const
    +static snd_pcm_format_t
    +ToAlsaPcmFormat(SampleFormat sample_format)
    +{
    +	switch (sample_format) {
    +	case SampleFormat::UNDEFINED:
    +		return SND_PCM_FORMAT_UNKNOWN;
    +
    +	case SampleFormat::DSD:
    +#ifdef HAVE_ALSA_DSD
    +		return SND_PCM_FORMAT_DSD_U8;
    +#else
    +		return SND_PCM_FORMAT_UNKNOWN;
    +#endif
    +
    +	case SampleFormat::S8:
    +		return SND_PCM_FORMAT_S8;
    +
    +	case SampleFormat::S16:
    +		return SND_PCM_FORMAT_S16;
    +
    +	case SampleFormat::S24_P32:
    +		return SND_PCM_FORMAT_S24;
     
    -static constexpr size_t ALSA_MAX_BUFFERED = default_rate * default_channels * 2;
    -static constexpr size_t ALSA_RESUME_AT = ALSA_MAX_BUFFERED / 2;
    +	case SampleFormat::S32:
    +		return SND_PCM_FORMAT_S32;
    +
    +	case SampleFormat::FLOAT:
    +		return SND_PCM_FORMAT_FLOAT;
    +	}
    +
    +	assert(false);
    +	gcc_unreachable();
    +}
    +
    +struct PollFdSet {
    +	struct pollfd *fdset;
    +	int count;
    +};
     
     /**
    - * This value should be the same as the read buffer size defined in
    - * PcmDecoderPlugin.cxx:pcm_stream_decode().
    - * We use it to calculate how many audio frames to buffer in the alsa driver
    - * before reading from the device. snd_pcm_readi() blocks until that many
    - * frames are ready.
    + * Helper class to wrap the snd_pcm_* API
    + * This class is not thread-safe,
    + * The constructor and Open() may be called from the decoder thread
    + * All other methods must be called from the IO thread.
      */
    -static constexpr size_t read_buffer_size = 4096;
    +class PCMStream {
    +	const char *device;
    +	snd_pcm_t *capture_handle;
    +	snd_pcm_uframes_t period;
    +	int frame_size;
    +	ReusableArray<pollfd> pfd_buffer;
    +	PollFdSet pollfds;
    +	bool open;
    +
    +public:
    +	PCMStream(const char *_device)
    +		:device(_device),
    +		 open(false) {
    +	}
    +
    +	~PCMStream() {
    +		if (IsOpen())
    +			snd_pcm_close(capture_handle);
    +	}
    +
    +	bool IsOpen() const {
    +		return open;
    +	}
    +
    +	size_t MinReadSize() const {
    +		return period * frame_size;
    +	}
    +
    +	PollFdSet &GetPollFds() {
    +		return pollfds;
    +	}
    +
    +
    +	void Open(AudioFormat format) {
    +		assert(!open);
    +		int err;
    +		if ((err = snd_pcm_open(&capture_handle, device,
    +					SND_PCM_STREAM_CAPTURE, 0)) < 0)
    +			throw FormatRuntimeError("Failed to open device: %s (%s)",
    +						 device, snd_strerror(err));
    +
    +		try {
    +			Configure(format);
    +
    +			int pfd_count = snd_pcm_poll_descriptors_count(capture_handle);
    +			struct pollfd *pfds = pfd_buffer.Get(pfd_count);
    +
    +			pfd_count = snd_pcm_poll_descriptors(capture_handle, pfds, pfd_count);
    +			if (pfd_count <= 0)
    +				throw FormatRuntimeError("Failed to obtain poll descriptors: %s (%s)",
    +						 device, snd_strerror(err));
    +
    +			pollfds.fdset = pfds;
    +			pollfds.count = pfd_count;
    +
    +		} catch (...) {
    +			snd_pcm_close(capture_handle);
    +			throw;
    +		}
    +
    +		open = true;
    +		snd_pcm_prepare(capture_handle);
    +	}
    +
    +	void Start() {
    +		int err = -EINVAL;
    +		if (!open || (err = snd_pcm_start(capture_handle)) < 0)
    +			throw FormatRuntimeError("Failed to start capture device: %s (%s)",
    +						 device, snd_strerror(err));
    +	}
    +
    +	bool Available() {
    +		auto available = snd_pcm_avail_update(capture_handle);
    +		if (available < 0)
    +			throw std::runtime_error("PCM stream error");
    +		return ((snd_pcm_uframes_t) available >= period);
    +	}
    +
    +	size_t Read(unsigned char *buffer, gcc_unused size_t size) {
    +		assert (size / frame_size > period);
    +		auto frames = snd_pcm_readi(capture_handle, buffer, period);
    +		if (frames <= 0)
    +			throw std::runtime_error("PCM stream error");
    +		return frames * frame_size;
    +	}
    +
    +	bool Recover() {
    +		switch (snd_pcm_state(capture_handle)) {
    +			case SND_PCM_STATE_XRUN:
    +				FormatDebug(alsa_input_domain, "Capture buffer underrun on %s", device);
    +				if (snd_pcm_prepare(capture_handle) < 0 ||
    +					snd_pcm_start(capture_handle) < 0)
    +					return false;
    +				break;
    +			case SND_PCM_STATE_PAUSED:
    +			case SND_PCM_STATE_SUSPENDED: {
    +				int err = snd_pcm_resume(capture_handle);
    +				if (err == -EAGAIN) {
    +					// TODO
    +					// schedule a retry
    +				}
    +				return (err == 0);
    +			}
    +			case SND_PCM_STATE_PREPARED:
    +				if (snd_pcm_start(capture_handle) < 0)
    +					return false;
    +				break;
    +			case SND_PCM_STATE_DRAINING:
    +			case SND_PCM_STATE_RUNNING:
    +				// nothing to do
    +				break;
    +			default:
    +				// unrecoverable error
    +				return false;
    +		}
    +		return true;
    +	}
    +
    +private:
    +	void Configure(AudioFormat format);
    +
    +};
    +
    +void
    +PCMStream::Configure(AudioFormat format)
    +{
    +	int err;
    +
    +	snd_pcm_hw_params_t *hw_params;
    +	if ((err = snd_pcm_hw_params_malloc(&hw_params)) < 0)
    +		throw FormatRuntimeError("Cannot allocate hardware parameter structure (%s)",
    +					 snd_strerror(err));
    +
    +	AtScopeExit(hw_params) {
    +		snd_pcm_hw_params_free(hw_params);
    +	};
    +
    +	if ((err = snd_pcm_hw_params_any(capture_handle, hw_params)) < 0)
    +		throw FormatRuntimeError("Cannot initialize hardware parameter structure (%s)",
    +					 snd_strerror(err));
    +
    +	if ((err = snd_pcm_hw_params_set_access(capture_handle, hw_params, SND_PCM_ACCESS_RW_INTERLEAVED)) < 0)
    +		throw FormatRuntimeError("Cannot set access type (%s)",
    +					 snd_strerror(err));
    +
    +	snd_pcm_format_t sformat = ToAlsaPcmFormat(format.format);
    +	if (sformat == SND_PCM_FORMAT_UNKNOWN)
    +		throw FormatRuntimeError("Unknown sample format (%s)",
    +					 sample_format_to_string(format.format));
    +
    +	if ((err = snd_pcm_hw_params_set_format(capture_handle, hw_params, sformat)) < 0)
    +		throw FormatRuntimeError("Cannot set sample format (%s)",
    +					 snd_strerror(err));
    +
    +	if ((err = snd_pcm_hw_params_set_channels(capture_handle, hw_params, format.channels)) < 0)
    +		throw FormatRuntimeError("Cannot set channels (%s)",
    +					 snd_strerror(err));
    +
    +	if ((err = snd_pcm_hw_params_set_rate(capture_handle, hw_params, format.sample_rate, 0)) < 0)
    +		throw FormatRuntimeError("Cannot set sample rate (%s)",
    +					 snd_strerror(err));
    +
    +	/* we set the alsa buffer to be as large as possible up to 500ms,
    +	 * the period (how often poll() fires) to be one quarter of the buffer size
    +	 */
    +	unsigned int time;
    +	int direction = -1;
    +	if ((err = snd_pcm_hw_params_get_buffer_time_max(hw_params,
    +							  &time, &direction)) < 0)
    +		throw FormatRuntimeError("Cannot get max buffer time (%s)",
    +					 snd_strerror(err));
    +
    +	if (time > 500000)
    +		time = 500000;
    +	if ((err = snd_pcm_hw_params_set_buffer_time_near(capture_handle, hw_params,
    +							  &time, &direction)) < 0)
    +		throw FormatRuntimeError("Cannot set buffer time (%s)",
    +					 snd_strerror(err));
    +
    +	snd_pcm_uframes_t bufsize;
    +	if ((err = snd_pcm_hw_params_get_buffer_size(hw_params, &bufsize)) < 0)
    +		throw FormatRuntimeError("Cannot get buffer size (%s)",
    +					 snd_strerror(err));
    +
    +	period = bufsize / 4;
    +	if ((err = snd_pcm_hw_params_set_period_size_near(capture_handle, hw_params,
    +							  &period, &direction)) < 0)
    +		throw FormatRuntimeError("Cannot set period size (%s)",
    +					 snd_strerror(err));
    +
    +	if ((err = snd_pcm_hw_params(capture_handle, hw_params)) < 0)
    +		throw FormatRuntimeError("Cannot set hw parameters (%s)",
    +					 snd_strerror(err));
    +
    +	snd_pcm_sw_params_t *sw_params;
    +
    +	snd_pcm_sw_params_malloc(&sw_params);
    +	snd_pcm_sw_params_current(capture_handle, sw_params);
    +
    +	AtScopeExit(sw_params) {
    +		snd_pcm_sw_params_free(sw_params);
    +	};
    +
    +	if ((err = snd_pcm_sw_params_set_avail_min(capture_handle, sw_params,
    +							 period)) < 0)
    +		throw FormatRuntimeError("unable to set avail min (%s)",
    +					 snd_strerror(err));
    +
    +	if ((err = snd_pcm_sw_params(capture_handle, sw_params)) < 0)
    +		throw FormatRuntimeError("unable to install sw params (%s)",
    +					 snd_strerror(err));
    +
    +	frame_size = format.GetFrameSize();
    +
    +	FormatDebug(alsa_input_domain, "device buffer size = %lu frames", bufsize);
    +	FormatDebug(alsa_input_domain, "period = %lu frames", period);
    +	FormatDebug(alsa_input_domain, "sample format = %s", snd_pcm_format_name(sformat));
    +	FormatDebug(alsa_input_domain, "sample rate = %d Hz", format.sample_rate);
    +	FormatDebug(alsa_input_domain, "sample channels = %d", format.channels);
    +}
    +
     
     class AlsaInputStream final
     	: public AsyncInputStream,
     	  MultiSocketMonitor, DeferredMonitor {
    -	snd_pcm_t *capture_handle;
    -	size_t frame_size;
     
    -	ReusableArray<pollfd> pfd_buffer;
    +	PCMStream *pcm;
    +	bool sockets_ready;
    +	std::atomic_bool closing;
     
     public:
     	AlsaInputStream(EventLoop &loop,
     			const char *_uri, Mutex &_mutex, Cond &_cond,
    -			snd_pcm_t *_handle, int _frame_size)
    +			PCMStream *_pcm)
     		:AsyncInputStream(_uri, _mutex, _cond,
    -				  ALSA_MAX_BUFFERED, ALSA_RESUME_AT),
    +				  _pcm->MinReadSize() * 4, _pcm->MinReadSize()),
     		 MultiSocketMonitor(loop),
     		 DeferredMonitor(loop),
    -		 capture_handle(_handle),
    -		 frame_size(_frame_size)
    +		 pcm(_pcm),
    +		 sockets_ready(false),
    +		 closing(false)
     	{
     		assert(_uri != nullptr);
     		assert(_handle != nullptr);
    -
    -		/* this mime type forces use of the PcmDecoderPlugin.
    -		   Needs to be generalised when/if that decoder is
    -		   updated to support other audio formats */
    +		assert(_pcm != nullptr);
    +
    +		/*
    +		 * this mime type forces use of the PcmDecoderPlugin.
    +		 * TODO
    +		 * Needs to be generalised:
    +		 * e.g. audio/x-mpd-pcm; format=48000:16:2
    +		 */
     		SetMimeType("audio/x-mpd-cdda-pcm");
    -		InputStream::SetReady();
     
    -		snd_pcm_start(capture_handle);
    +		InputStream::SetReady();
     
     		DeferredMonitor::Schedule();
     	}
     
     	~AlsaInputStream() {
    -		snd_pcm_close(capture_handle);
    +		/* Schedule deletion of PCMStream */
    +		do {
    +			closing = true;
    +			SafeInvalidateSockets();
    +			std::lock_guard<Mutex> protect(mutex);
    +			cond.wait(mutex);
    +		} while (pcm != nullptr);
     	}
     
     	static InputStream *Create(const char *uri, Mutex &mutex, Cond &cond);
    @@ -107,8 +383,6 @@ public:
     protected:
     	/* virtual methods from AsyncInputStream */
     	virtual void DoResume() override {
    -		snd_pcm_resume(capture_handle);
    -
     		InvalidateSockets();
     	}
     
    @@ -118,16 +392,11 @@ protected:
     	}
     
     private:
    -	static snd_pcm_t *OpenDevice(const char *device, int rate,
    -				     snd_pcm_format_t format, int channels);
    -
     	void Pause() {
     		AsyncInputStream::Pause();
     		InvalidateSockets();
     	}
     
    -	int Recover(int err);
    -
     	void SafeInvalidateSockets() {
     		DeferredMonitor::Schedule();
     	}
    @@ -138,11 +407,21 @@ private:
     
     	virtual std::chrono::steady_clock::duration PrepareSockets() override;
     	virtual void DispatchSockets() override;
    +
    +	void ReadPCMStream();
    +	void RecoverPCMStream();
    +	void ClosePCMStream();
    +	void AbortPCMStream();
     };
     
     inline InputStream *
     AlsaInputStream::Create(const char *uri, Mutex &mutex, Cond &cond)
     {
    +	/*
    +	 * TODO
    +	 * URI should allow inclusion of desired audio format
    +	 * e.g. alsa://hw:0,0?format=48000:16:2
    +	 */
     	const char *device = StringAfterPrefix(uri, "alsa://");
     	if (device == nullptr)
     		return nullptr;
    @@ -150,186 +429,105 @@ AlsaInputStream::Create(const char *uri, Mutex &mutex, Cond &cond)
     	if (*device == 0)
     		device = default_device;
     
    -	/* placeholders - eventually user-requested audio format will
    -	   be passed via the URI. For now we just force the
    -	   defaults */
    -	int rate = default_rate;
    -	snd_pcm_format_t format = default_format;
    -	int channels = default_channels;
    +	auto pcmstream = new PCMStream(device);
     
    -	snd_pcm_t *handle = OpenDevice(device, rate, format, channels);
    +	try {
    +		pcmstream->Open(default_format);
    +	}
    +	catch (...) {
    +		delete pcmstream;
    +		throw;
    +	}
     
    -	int frame_size = snd_pcm_format_width(format) / 8 * channels;
     	return new AlsaInputStream(io_thread_get(),
    -				   uri, mutex, cond,
    -				   handle, frame_size);
    +				   uri, mutex, cond, pcmstream);
     }
     
     std::chrono::steady_clock::duration
     AlsaInputStream::PrepareSockets()
     {
    -	if (IsPaused()) {
    +	if (closing) {
     		ClearSocketList();
    +		ClosePCMStream();
     		return std::chrono::steady_clock::duration(-1);
     	}
     
    -	int count = snd_pcm_poll_descriptors_count(capture_handle);
    -	if (count < 0) {
    +	if (IsPaused()) {
     		ClearSocketList();
    +		sockets_ready = false;
     		return std::chrono::steady_clock::duration(-1);
     	}
     
    -	struct pollfd *pfds = pfd_buffer.Get(count);
    +	if (!sockets_ready) {
    +		assert(pcm != nullptr);
    +		ReplaceSocketList(pcm->GetPollFds().fdset, pcm->GetPollFds().count);
    +		sockets_ready = true;
    +	}
     
    -	count = snd_pcm_poll_descriptors(capture_handle, pfds, count);
    -	if (count < 0)
    -		count = 0;
    +	RecoverPCMStream();
     
    -	ReplaceSocketList(pfds, count);
     	return std::chrono::steady_clock::duration(-1);
     }
     
     void
     AlsaInputStream::DispatchSockets()
     {
    -	const std::lock_guard<Mutex> protect(mutex);
    +	assert(pcm != nullptr);
    +	try {
    +		if (pcm->Available())
    +			ReadPCMStream();
    +	}
    +	catch (...) {
    +		RecoverPCMStream();
    +	}
    +}
    +
    +inline void
    +AlsaInputStream::ReadPCMStream()
    +{
    +	assert(pcm != nullptr);
    +
    +	std::lock_guard<Mutex> protect(mutex);
     
     	auto w = PrepareWriteBuffer();
    -	const snd_pcm_uframes_t w_frames = w.size / frame_size;
    -	if (w_frames == 0) {
    -		/* buffer is full */
    +	if (w.size < pcm->MinReadSize()) {
    +		/* not enough room in buffer */
     		Pause();
     		return;
     	}
     
    -	snd_pcm_sframes_t n_frames;
    -	while ((n_frames = snd_pcm_readi(capture_handle,
    -					 w.data, w_frames)) < 0) {
    -		if (Recover(n_frames) < 0) {
    -			postponed_exception = std::make_exception_ptr(std::runtime_error("PCM error - stream aborted"));
    -			cond.broadcast();
    -			return;
    -		}
    -	}
    +	auto nbytes = pcm->Read(w.data, w.size);
     
    -	size_t nbytes = n_frames * frame_size;
     	CommitWriteBuffer(nbytes);
     }
     
    -inline int
    -AlsaInputStream::Recover(int err)
    +void
    +AlsaInputStream::RecoverPCMStream()
     {
    -	switch(err) {
    -	case -EPIPE:
    -		LogDebug(alsa_input_domain, "Buffer Overrun");
    -		// drop through
    -#if GCC_CHECK_VERSION(7,0)
    -		[[fallthrough]];
    -#endif
    -
    -	case -ESTRPIPE:
    -	case -EINTR:
    -		err = snd_pcm_recover(capture_handle, err, 1);
    -		break;
    -	default:
    -		// something broken somewhere, give up
    -		err = -1;
    -	}
    -	return err;
    +	assert(pcm != nullptr);
    +		if (!pcm->Recover())
    +			AbortPCMStream();
     }
     
    -static void
    -ConfigureCapture(snd_pcm_t *capture_handle,
    -		 int rate, snd_pcm_format_t format, int channels)
    +void
    +AlsaInputStream::ClosePCMStream()
     {
    -	int err;
    -
    -	snd_pcm_hw_params_t *hw_params;
    -	if ((err = snd_pcm_hw_params_malloc(&hw_params)) < 0)
    -		throw FormatRuntimeError("Cannot allocate hardware parameter structure (%s)",
    -					 snd_strerror(err));
    -
    -	AtScopeExit(hw_params) {
    -		snd_pcm_hw_params_free(hw_params);
    -	};
    -
    -	if ((err = snd_pcm_hw_params_any(capture_handle, hw_params)) < 0)
    -		throw FormatRuntimeError("Cannot initialize hardware parameter structure (%s)",
    -					 snd_strerror(err));
    -
    -	if ((err = snd_pcm_hw_params_set_access(capture_handle, hw_params, SND_PCM_ACCESS_RW_INTERLEAVED)) < 0)
    -		throw FormatRuntimeError("Cannot set access type (%s)",
    -					 snd_strerror(err));
    -
    -	if ((err = snd_pcm_hw_params_set_format(capture_handle, hw_params, format)) < 0)
    -		throw FormatRuntimeError("Cannot set sample format (%s)",
    -					 snd_strerror(err));
    -
    -	if ((err = snd_pcm_hw_params_set_channels(capture_handle, hw_params, channels)) < 0)
    -		throw FormatRuntimeError("Cannot set channels (%s)",
    -					 snd_strerror(err));
    -
    -	if ((err = snd_pcm_hw_params_set_rate(capture_handle, hw_params, rate, 0)) < 0)
    -		throw FormatRuntimeError("Cannot set sample rate (%s)",
    -					 snd_strerror(err));
    -
    -	/* period needs to be big enough so that poll() doesn't fire too often,
    -	 * but small enough that buffer overruns don't occur if Read() is not
    -	 * invoked often enough.
    -	 * the calculation here is empirical; however all measurements were
    -	 * done using 44100:16:2. When we extend this plugin to support
    -	 * other audio formats then this may need to be revisited */
    -	snd_pcm_uframes_t period = read_buffer_size * 2;
    -	int direction = -1;
    -	if ((err = snd_pcm_hw_params_set_period_size_near(capture_handle, hw_params,
    -							  &period, &direction)) < 0)
    -		throw FormatRuntimeError("Cannot set period size (%s)",
    -					 snd_strerror(err));
    -
    -	if ((err = snd_pcm_hw_params(capture_handle, hw_params)) < 0)
    -		throw FormatRuntimeError("Cannot set parameters (%s)",
    -					 snd_strerror(err));
    -
    -	snd_pcm_sw_params_t *sw_params;
    -
    -	snd_pcm_sw_params_malloc(&sw_params);
    -	snd_pcm_sw_params_current(capture_handle, sw_params);
    -
    -	AtScopeExit(sw_params) {
    -		snd_pcm_sw_params_free(sw_params);
    -	};
    -
    -	if ((err = snd_pcm_sw_params_set_start_threshold(capture_handle, sw_params,
    -							 period)) < 0)
    -		throw FormatRuntimeError("unable to set start threshold (%s)",
    -					 snd_strerror(err));
    -
    -	if ((err = snd_pcm_sw_params(capture_handle, sw_params)) < 0)
    -		throw FormatRuntimeError("unable to install sw params (%s)",
    -					 snd_strerror(err));
    +	const std::lock_guard<Mutex> protect(mutex);
    +	delete pcm;
    +	pcm = nullptr;
    +	cond.broadcast();
     }
     
    -inline snd_pcm_t *
    -AlsaInputStream::OpenDevice(const char *device,
    -			    int rate, snd_pcm_format_t format, int channels)
    +void
    +AlsaInputStream::AbortPCMStream()
     {
    -	snd_pcm_t *capture_handle;
    -	int err;
    -	if ((err = snd_pcm_open(&capture_handle, device,
    -				SND_PCM_STREAM_CAPTURE, 0)) < 0)
    -		throw FormatRuntimeError("Failed to open device: %s (%s)",
    -					 device, snd_strerror(err));
    -
    -	try {
    -		ConfigureCapture(capture_handle, rate, format, channels);
    -	} catch (...) {
    -		snd_pcm_close(capture_handle);
    -		throw;
    -	}
    +	closing = true;
    +	InvalidateSockets();
     
    -	snd_pcm_prepare(capture_handle);
    +	postponed_exception = std::make_exception_ptr(std::runtime_error("PCM error - stream aborted"));
     
    -	return capture_handle;
    +	const std::lock_guard<Mutex> protect(mutex);
    +	cond.broadcast();
     }
     
     /*#########################  Plugin Functions  ##############################*/
    
    patch file icon mpd-alsa-input.patch (19,420 bytes) 2017-01-13 19:52 +
  • patch file icon 0001-input-alsa-change-sw_params-to-support-poll.patch (1,093 bytes) 2017-01-13 22:13 -
    From f97864e9a32d5adcb82e9a463febc46f716b1d5a Mon Sep 17 00:00:00 2001
    From: Steven O'Brien <steven_obrien1@yahoo.co.uk>
    Date: Fri, 13 Jan 2017 21:04:05 +0000
    Subject: [PATCH 1/2] input/alsa: change sw_params to support poll()
    
    ---
     src/input/plugins/AlsaInputPlugin.cxx | 7 ++++++-
     1 file changed, 6 insertions(+), 1 deletion(-)
    
    diff --git a/src/input/plugins/AlsaInputPlugin.cxx b/src/input/plugins/AlsaInputPlugin.cxx
    index 23b4188..8873226 100644
    --- a/src/input/plugins/AlsaInputPlugin.cxx
    +++ b/src/input/plugins/AlsaInputPlugin.cxx
    @@ -309,8 +309,13 @@ ConfigureCapture(snd_pcm_t *capture_handle,
     	};
     
     	if ((err = snd_pcm_sw_params_set_start_threshold(capture_handle, sw_params,
    -							 period)) < 0)
    + 							 period)) < 0)
     		throw FormatRuntimeError("unable to set start threshold (%s)",
    + 					 snd_strerror(err));
    +
    +	if ((err = snd_pcm_sw_params_set_avail_min(capture_handle, sw_params,
    +							 period)) < 0)
    +		throw FormatRuntimeError("unable to set avail min (%s)",
     					 snd_strerror(err));
     
     	if ((err = snd_pcm_sw_params(capture_handle, sw_params)) < 0)
    -- 
    2.7.4
    
    
  • patch file icon 0002-input-alsa-read-chunks-of-1-period-in-size.patch (3,928 bytes) 2017-01-13 22:15 -
    From c042f48de92a67e5a2c59efc8e58c4fc2a373b84 Mon Sep 17 00:00:00 2001
    From: Steven O'Brien <steven_obrien1@yahoo.co.uk>
    Date: Fri, 13 Jan 2017 21:08:02 +0000
    Subject: [PATCH 2/2] input/alsa: read chunks of 1 period in size
    
    ---
     src/input/plugins/AlsaInputPlugin.cxx | 73 +++++++++++++++++++----------------
     1 file changed, 40 insertions(+), 33 deletions(-)
    
    diff --git a/src/input/plugins/AlsaInputPlugin.cxx b/src/input/plugins/AlsaInputPlugin.cxx
    index 8873226..a7caf34 100644
    --- a/src/input/plugins/AlsaInputPlugin.cxx
    +++ b/src/input/plugins/AlsaInputPlugin.cxx
    @@ -54,17 +54,18 @@ static constexpr snd_pcm_format_t default_format = SND_PCM_FORMAT_S16;
     static constexpr int default_channels = 2; // stereo
     static constexpr unsigned int default_rate = 44100; // cd quality
     
    -static constexpr size_t ALSA_MAX_BUFFERED = default_rate * default_channels * 2;
    -static constexpr size_t ALSA_RESUME_AT = ALSA_MAX_BUFFERED / 2;
    -
    -/**
    - * This value should be the same as the read buffer size defined in
    - * PcmDecoderPlugin.cxx:pcm_stream_decode().
    - * We use it to calculate how many audio frames to buffer in the alsa driver
    - * before reading from the device. snd_pcm_readi() blocks until that many
    - * frames are ready.
    +/*
    + * Reading from the alsa device should be in chunks of 1 period
    + * We hard code the period to be 8192 frames
    + */
    +static constexpr size_t PERIOD = 8192;
    +
    +/*
    + * Buffer up to 4 periods, resume afer pause when buffer down to 1 period
    + * buffer sizes are in bytes (4 bytes per frame)
      */
    -static constexpr size_t read_buffer_size = 4096;
    +static constexpr size_t ALSA_MAX_BUFFERED = 4 * PERIOD * 4;
    +static constexpr size_t ALSA_RESUME_AT = PERIOD * 4;
     
     class AlsaInputStream final
     	: public AsyncInputStream,
    @@ -201,28 +202,40 @@ AlsaInputStream::PrepareSockets()
     void
     AlsaInputStream::DispatchSockets()
     {
    -	const std::lock_guard<Mutex> protect(mutex);
    -
    -	auto w = PrepareWriteBuffer();
    -	const snd_pcm_uframes_t w_frames = w.size / frame_size;
    -	if (w_frames == 0) {
    -		/* buffer is full */
    -		Pause();
    -		return;
    -	}
    +	snd_pcm_sframes_t ret = 0;
    +	try {
    +		ret = snd_pcm_avail_update(capture_handle);
    +		if (ret < 0)
    +			throw std::runtime_error("PCM stream error");
    +
    +		if ((snd_pcm_uframes_t) ret < PERIOD)
    +			/* not enough data yet to read */
    +			return;
     
    -	snd_pcm_sframes_t n_frames;
    -	while ((n_frames = snd_pcm_readi(capture_handle,
    -					 w.data, w_frames)) < 0) {
    -		if (Recover(n_frames) < 0) {
    +		const std::lock_guard<Mutex> protect(mutex);
    +
    +		auto w = PrepareWriteBuffer();
    +		const snd_pcm_uframes_t w_frames = w.size / frame_size;
    +		if (w_frames < PERIOD) {
    +			/* not enough space in buffer */
    +			Pause();
    +			return;
    +		}
    +
    +		ret = snd_pcm_readi(capture_handle, w.data, PERIOD);
    +		if (ret < 0)
    +			throw std::runtime_error("PCM stream error");
    +
    +		size_t nbytes = ret * frame_size;
    +		CommitWriteBuffer(nbytes);
    +	}
    +	catch (...) {
    +		if (Recover(ret) < 0) {
     			postponed_exception = std::make_exception_ptr(std::runtime_error("PCM error - stream aborted"));
     			cond.broadcast();
     			return;
     		}
     	}
    -
    -	size_t nbytes = n_frames * frame_size;
    -	CommitWriteBuffer(nbytes);
     }
     
     inline int
    @@ -282,13 +295,7 @@ ConfigureCapture(snd_pcm_t *capture_handle,
     		throw FormatRuntimeError("Cannot set sample rate (%s)",
     					 snd_strerror(err));
     
    -	/* period needs to be big enough so that poll() doesn't fire too often,
    -	 * but small enough that buffer overruns don't occur if Read() is not
    -	 * invoked often enough.
    -	 * the calculation here is empirical; however all measurements were
    -	 * done using 44100:16:2. When we extend this plugin to support
    -	 * other audio formats then this may need to be revisited */
    -	snd_pcm_uframes_t period = read_buffer_size * 2;
    +	snd_pcm_uframes_t period = PERIOD;
     	int direction = -1;
     	if ((err = snd_pcm_hw_params_set_period_size_near(capture_handle, hw_params,
     							  &period, &direction)) < 0)
    -- 
    2.7.4
    
    

-Relationships
+Relationships

-Notes

~0010265

cirrus (administrator)

It is nearly impossible to review such a large and obscure patch that does 6 things at a time. Can you please split it?

~0010266

steveno (reporter)

Here's the first patch. It turns on period based event notifications

~0010267

steveno (reporter)

... and the second one gets mpd to read from the stream in chunks of 1 period.
Together these two patches fix the immediate issue of stuttering, but don't handle all alsa errors gracefully

~0010272

cirrus (administrator)

Your first patch is a complete breakage. It makes MPD freeze completely, deadlocking while waiting for more input from ALSA, but the file descriptor never gets woken up after the first EAGAIN.

~0010273

cirrus (administrator)

Example strace from the I/O thread:

1484420765.439996 set_robust_list(0x7f965718d9e0, 24) = 0 <0.000011>
1484420765.440037 prctl(PR_SET_NAME, "io") = 0 <0.000010>
1484420765.440068 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656813, tv_nsec=518279990}) = 0 <0.000017>
1484420765.440104 epoll_wait(4, [{EPOLLIN, {u32=3104613296, u64=93861024932784}}], 16, -1) = 1 <0.014014>
1484420765.454148 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656813, tv_nsec=532359064}) = 0 <0.000015>
1484420765.454181 read(3, "\1\0\0\0\0\0\0\0", 8) = 8 <0.000010>
1484420765.454220 mmap(NULL, 134217728, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x7f964e98d000 <0.000012>
1484420765.454252 munmap(0x7f964e98d000, 23539712) = 0 <0.000011>
1484420765.454280 munmap(0x7f9654000000, 43569152) = 0 <0.000011>
1484420765.454310 mprotect(0x7f9650000000, 135168, PROT_READ|PROT_WRITE) = 0 <0.000011>
1484420765.454343 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656813, tv_nsec=532555252}) = 0 <0.000011>
1484420765.454381 epoll_ctl(4, EPOLL_CTL_ADD, 6, {EPOLLIN|EPOLLERR|0x20, {u32=1342179560, u64=140283564001512}}) = 0 <0.000011>
1484420765.454413 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.185478>
1484420765.639960 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656813, tv_nsec=718207063}) = 0 <0.000047>
1484420765.640065 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656813, tv_nsec=718287451}) = 0 <0.000023>
1484420765.640123 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = 0 <0.000059>
1484420765.640198 futex(0x7ffd404aacc4, FUTEX_CMP_REQUEUE_PRIVATE, 1, 2147483647, 0x7ffd404aac90, 2) = 1 <0.000009>
1484420765.640222 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.371189>
1484420766.011500 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=89745844}) = 0 <0.000046>
1484420766.011605 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=89827770}) = 0 <0.000024>
1484420766.011662 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EPIPE (Broken pipe) <0.000074>
1484420766.011762 ioctl(6, SNDRV_PCM_IOCTL_PREPARE, 0x1) = 0 <0.000022>
1484420766.011798 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EAGAIN (Resource temporarily unavailable) <0.000013>
1484420766.011829 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.185807>
1484420766.197706 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=275929507}) = 0 <0.000023>
1484420766.197786 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=276017857}) = 0 <0.000033>
1484420766.197874 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = 0 <0.000031>
1484420766.197921 futex(0x7ffd404aacc4, FUTEX_CMP_REQUEUE_PRIVATE, 1, 2147483647, 0x7ffd404aac90, 4) = 1 <0.000009>
1484420766.197947 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.371236>
1484420766.569275 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=647519483}) = 0 <0.000043>
1484420766.569379 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=647626761}) = 0 <0.000048>
1484420766.569485 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EPIPE (Broken pipe) <0.000076>
1484420766.569580 ioctl(6, SNDRV_PCM_IOCTL_PREPARE, 0x1) = 0 <0.000022>
1484420766.569616 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EAGAIN (Resource temporarily unavailable) <0.000014>
1484420766.569647 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.185815>
1484420766.755531 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=833775432}) = 0 <0.000044>
1484420766.755630 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656814, tv_nsec=833853935}) = 0 <0.000023>
1484420766.755686 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = 0 <0.000029>
1484420766.755731 futex(0x7ffd404aacc4, FUTEX_CMP_REQUEUE_PRIVATE, 1, 2147483647, 0x7ffd404aac90, 6) = 1 <0.000008>
1484420766.755757 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.371241>
1484420767.127092 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=205335306}) = 0 <0.000044>
1484420767.127199 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=205444540}) = 0 <0.000042>
1484420767.127307 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EPIPE (Broken pipe) <0.000077>
1484420767.127405 ioctl(6, SNDRV_PCM_IOCTL_PREPARE, 0x1) = 0 <0.000023>
1484420767.127444 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EAGAIN (Resource temporarily unavailable) <0.000015>
1484420767.127477 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.185770>
1484420767.313277 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=391490123}) = 0 <0.000008>
1484420767.313307 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=391517013}) = 0 <0.000008>
1484420767.313333 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = 0 <0.000013>
1484420767.313362 futex(0x7ffd404aacc4, FUTEX_CMP_REQUEUE_PRIVATE, 1, 2147483647, 0x7ffd404aac90, 8) = 1 <0.000009>
1484420767.313387 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.185696>
1484420767.499170 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=577393577}) = 0 <0.000023>
1484420767.499254 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=577479553}) = 0 <0.000026>
1484420767.499313 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = 0 <0.000034>
1484420767.499364 futex(0x7ffd404aacc4, FUTEX_CMP_REQUEUE_PRIVATE, 1, 2147483647, 0x7ffd404aac90, 10) = 1 <0.000008>
1484420767.499388 epoll_wait(4, [{EPOLLIN, {u32=1342179560, u64=140283564001512}}], 16, -1) = 1 <0.371210>
1484420767.870716 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=948972728}) = 0 <0.000090>
1484420767.870909 clock_gettime(CLOCK_MONOTONIC, {tv_sec=1656815, tv_nsec=949168147}) = 0 <0.000089>
1484420767.871101 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EPIPE (Broken pipe) <0.000102>
1484420767.871234 ioctl(6, SNDRV_PCM_IOCTL_PREPARE, 0x1) = 0 <0.000023>
1484420767.871279 ioctl(6, SNDRV_PCM_IOCTL_READI_FRAMES, 0x7f965718c960) = -1 EAGAIN (Resource temporarily unavailable) <0.000007>
1484420767.871303 epoll_wait(4,

~0010274

cirrus (administrator)

Oh, and your first patch description is wrong. It says:

"change sw_params to support poll()"

.. when in fact it does three things:

- change avail_min (is this what your description tried to describe?)
- enable non-blocking mode
- handle EAGAIN

The latter two have to do with supporting poll(), but they have nothing to do with the sw_params. The sw_params change however has nothing to do with poll(). This is confusing.

~0010275

cirrus (administrator)

And your patch has several whitespace changes which break the coding style of existing (otherwise unmodified) lines.

~0010276

cirrus (administrator)

I merged two out of three parts of your first patch, after finding a fix for the deadlock. The plugin is now non-blocking, which was a very important change.

~0010277

cirrus (administrator)

The buffer overflow problem has nothing to do with avail_min; at least on my computer, this change does not affect/improve anything. This is what fixed everything for me:

7e8b448985b47c8095ccf50bdfb7364cafc2a232

If you still want to submit changes for MPD, please use GitHub or the developer mailing list to send your code.
+Notes

-Issue History
Date Modified Username Field Change
2017-01-13 19:52 steveno New Issue
2017-01-13 19:52 steveno Status new => assigned
2017-01-13 19:52 steveno Assigned To => cirrus
2017-01-13 19:52 steveno File Added: mpd-alsa-input.patch
2017-01-13 19:52 steveno Tag Attached: alsa
2017-01-13 19:54 cirrus Note Added: 0010265
2017-01-13 22:13 steveno File Added: 0001-input-alsa-change-sw_params-to-support-poll.patch
2017-01-13 22:13 steveno Note Added: 0010266
2017-01-13 22:15 steveno File Added: 0002-input-alsa-read-chunks-of-1-period-in-size.patch
2017-01-13 22:15 steveno Note Added: 0010267
2017-01-14 20:05 cirrus Note Added: 0010272
2017-01-14 20:07 cirrus Note Added: 0010273
2017-01-14 20:10 cirrus Note Added: 0010274
2017-01-14 20:11 cirrus Note Added: 0010275
2017-01-14 21:01 cirrus Note Added: 0010276
2017-01-14 21:56 cirrus Status assigned => resolved
2017-01-14 21:56 cirrus Resolution open => fixed
2017-01-14 21:56 cirrus Note Added: 0010277
+Issue History