From f82e1453e476bd6c9114e2adf4144eb643950b0e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 13 Nov 2017 17:12:21 +0100 Subject: [PATCH 1/4] input/smbclient: use std::lock_guard --- src/input/plugins/SmbclientInputPlugin.cxx | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/input/plugins/SmbclientInputPlugin.cxx b/src/input/plugins/SmbclientInputPlugin.cxx index 8e802a8e1..35bb5c5b8 100644 --- a/src/input/plugins/SmbclientInputPlugin.cxx +++ b/src/input/plugins/SmbclientInputPlugin.cxx @@ -125,9 +125,13 @@ input_smbclient_open(const char *uri, size_t SmbclientInputStream::Read(void *ptr, size_t read_size) { - smbclient_mutex.lock(); - ssize_t nbytes = smbc_read(fd, ptr, read_size); - smbclient_mutex.unlock(); + ssize_t nbytes; + + { + const std::lock_guard lock(smbclient_mutex); + nbytes = smbc_read(fd, ptr, read_size); + } + if (nbytes < 0) throw MakeErrno("smbc_read() failed"); @@ -138,9 +142,13 @@ SmbclientInputStream::Read(void *ptr, size_t read_size) void SmbclientInputStream::Seek(offset_type new_offset) { - smbclient_mutex.lock(); - off_t result = smbc_lseek(fd, new_offset, SEEK_SET); - smbclient_mutex.unlock(); + off_t result; + + { + const std::lock_guard lock(smbclient_mutex); + result = smbc_lseek(fd, new_offset, SEEK_SET); + } + if (result < 0) throw MakeErrno("smbc_lseek() failed"); From 31ab78ae8e10af948ec95496df0d2abf1ea631a4 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 13 Nov 2017 17:08:00 +0100 Subject: [PATCH 2/4] input/{cdio,ffmpeg,file,smbclient}: unlock the mutex during blocking I/O InputStream::Read() and InputStream::Seek() are called with the mutex locked. That means the implementation must not block, or unlock the mutex before calling into blocking code. Previously, a slow CD drive could stall the whole MPD process, including the main thread, due to this problem. Closes #149 --- NEWS | 2 ++ src/input/plugins/CdioParanoiaInputPlugin.cxx | 7 ++++++- src/input/plugins/FfmpegInputPlugin.cxx | 15 +++++++++++++-- src/input/plugins/FileInputPlugin.cxx | 14 ++++++++++++-- src/input/plugins/SmbclientInputPlugin.cxx | 2 ++ 5 files changed, 35 insertions(+), 5 deletions(-) diff --git a/NEWS b/NEWS index b89fa5cf6..c961c25db 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,7 @@ ver 0.20.12 (not yet released) * input + - cdio_paranoia, ffmpeg, file, smbclient: reduce lock contention, + fixing lots of xrun problems - curl: fix seeking * decoder - ffmpeg: fix GCC 8 warning diff --git a/src/input/plugins/CdioParanoiaInputPlugin.cxx b/src/input/plugins/CdioParanoiaInputPlugin.cxx index 39f90dcc0..e5a63a8d6 100644 --- a/src/input/plugins/CdioParanoiaInputPlugin.cxx +++ b/src/input/plugins/CdioParanoiaInputPlugin.cxx @@ -270,7 +270,10 @@ CdioParanoiaInputStream::Seek(offset_type new_offset) lsn_relofs = new_offset / CDIO_CD_FRAMESIZE_RAW; offset = new_offset; - cdio_paranoia_seek(para, lsn_from + lsn_relofs, SEEK_SET); + { + const ScopeUnlock unlock(mutex); + cdio_paranoia_seek(para, lsn_from + lsn_relofs, SEEK_SET); + } } size_t @@ -292,6 +295,8 @@ CdioParanoiaInputStream::Read(void *ptr, size_t length) //current sector was changed ? if (lsn_relofs != buffer_lsn) { + const ScopeUnlock unlock(mutex); + rbuf = cdio_paranoia_read(para, nullptr); s_err = cdda_errors(drv); diff --git a/src/input/plugins/FfmpegInputPlugin.cxx b/src/input/plugins/FfmpegInputPlugin.cxx index 4ba9b470a..9ca16aa67 100644 --- a/src/input/plugins/FfmpegInputPlugin.cxx +++ b/src/input/plugins/FfmpegInputPlugin.cxx @@ -104,7 +104,13 @@ input_ffmpeg_open(const char *uri, size_t FfmpegInputStream::Read(void *ptr, size_t read_size) { - auto result = avio_read(h, (unsigned char *)ptr, read_size); + int result; + + { + const ScopeUnlock unlock(mutex); + result = avio_read(h, (unsigned char *)ptr, read_size); + } + if (result <= 0) { if (result < 0) throw MakeFfmpegError(result, "avio_read() failed"); @@ -126,7 +132,12 @@ FfmpegInputStream::IsEOF() noexcept void FfmpegInputStream::Seek(offset_type new_offset) { - auto result = avio_seek(h, new_offset, SEEK_SET); + int64_t result; + + { + const ScopeUnlock unlock(mutex); + result = avio_seek(h, new_offset, SEEK_SET); + } if (result < 0) throw MakeFfmpegError(result, "avio_seek() failed"); diff --git a/src/input/plugins/FileInputPlugin.cxx b/src/input/plugins/FileInputPlugin.cxx index fc51715ad..2d2850c74 100644 --- a/src/input/plugins/FileInputPlugin.cxx +++ b/src/input/plugins/FileInputPlugin.cxx @@ -87,14 +87,24 @@ input_file_open(gcc_unused const char *filename, void FileInputStream::Seek(offset_type new_offset) { - reader.Seek((off_t)new_offset); + { + const ScopeUnlock unlock(mutex); + reader.Seek((off_t)new_offset); + } + offset = new_offset; } size_t FileInputStream::Read(void *ptr, size_t read_size) { - size_t nbytes = reader.Read(ptr, read_size); + size_t nbytes; + + { + const ScopeUnlock unlock(mutex); + nbytes = reader.Read(ptr, read_size); + } + offset += nbytes; return nbytes; } diff --git a/src/input/plugins/SmbclientInputPlugin.cxx b/src/input/plugins/SmbclientInputPlugin.cxx index 35bb5c5b8..80d406692 100644 --- a/src/input/plugins/SmbclientInputPlugin.cxx +++ b/src/input/plugins/SmbclientInputPlugin.cxx @@ -128,6 +128,7 @@ SmbclientInputStream::Read(void *ptr, size_t read_size) ssize_t nbytes; { + const ScopeUnlock unlock(mutex); const std::lock_guard lock(smbclient_mutex); nbytes = smbc_read(fd, ptr, read_size); } @@ -145,6 +146,7 @@ SmbclientInputStream::Seek(offset_type new_offset) off_t result; { + const ScopeUnlock unlock(mutex); const std::lock_guard lock(smbclient_mutex); result = smbc_lseek(fd, new_offset, SEEK_SET); } From aea37e46e3e26183dd94479ec941bfb14acd89e6 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 14 Nov 2017 11:30:28 +0100 Subject: [PATCH 3/4] encoder/vorbis: default to quality 3 Don't require a quality or bitrate setting. If nothing is set, don't fail startup - just go with a good default. A quality setting of 3 is what "oggenc" defaults to as well. --- NEWS | 2 ++ doc/user.xml | 4 ++-- src/encoder/plugins/VorbisEncoderPlugin.cxx | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index c961c25db..04b7da8d4 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,8 @@ ver 0.20.12 (not yet released) - vorbis: fix Tremor support * player - log message when decoder is too slow +* encoder + - vorbis: default to quality 3 * output - fix hanging playback with soxr resampler diff --git a/doc/user.xml b/doc/user.xml index e010d5e3e..5cc741227 100644 --- a/doc/user.xml +++ b/doc/user.xml @@ -3068,8 +3068,8 @@ run Sets the quality for VBR. -1 is the lowest quality, - 10 is the highest quality. Cannot be used with - bitrate. + 10 is the highest quality. Defaults to 3. Cannot + be used with bitrate. diff --git a/src/encoder/plugins/VorbisEncoderPlugin.cxx b/src/encoder/plugins/VorbisEncoderPlugin.cxx index 68ea479ee..5a8e2826a 100644 --- a/src/encoder/plugins/VorbisEncoderPlugin.cxx +++ b/src/encoder/plugins/VorbisEncoderPlugin.cxx @@ -62,7 +62,7 @@ private: }; class PreparedVorbisEncoder final : public PreparedEncoder { - float quality; + float quality = 3; int bitrate; public: @@ -97,7 +97,7 @@ PreparedVorbisEncoder::PreparedVorbisEncoder(const ConfigBlock &block) value = block.GetBlockValue("bitrate"); if (value == nullptr) - throw std::runtime_error("neither bitrate nor quality defined"); + return; quality = -2.0; From 014f8cd693a1a5b5c3b0ebeeb6b799701a5132f1 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 14 Nov 2017 12:00:14 +0100 Subject: [PATCH 4/4] output/httpd: flush encoder after tag Without the flush, ReadPage() may not return any data, or not all data. This may result in incomplete ddata the new "header" page, corrupting streams with some encoders such as Vorbis. Fixes #145 --- NEWS | 1 + src/output/plugins/httpd/HttpdOutputPlugin.cxx | 1 + 2 files changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 04b7da8d4..3f70c64b8 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,7 @@ ver 0.20.12 (not yet released) - vorbis: default to quality 3 * output - fix hanging playback with soxr resampler + - httpd: flush encoder after tag; fixes corrupt Vorbis stream ver 0.20.11 (2017/10/18) * storage diff --git a/src/output/plugins/httpd/HttpdOutputPlugin.cxx b/src/output/plugins/httpd/HttpdOutputPlugin.cxx index 966669b1e..0a811b706 100644 --- a/src/output/plugins/httpd/HttpdOutputPlugin.cxx +++ b/src/output/plugins/httpd/HttpdOutputPlugin.cxx @@ -468,6 +468,7 @@ HttpdOutput::SendTag(const Tag &tag) try { encoder->SendTag(tag); + encoder->Flush(); } catch (const std::runtime_error &) { /* ignore */ }