Cover art handling: significantly reduce memory usage. fixes #93

README.md

@@ -7,7 +7,7 @@ _LMS_ is a self-hosted music streaming software: access your music collection fr
 A [demo](http://lms.demo.poupon.io) instance is available. Note the administration panel is not available.
 
 ## Main features
-* Low memory requirement: the demo instance runs on a Raspberry Pi3B+
+* Low memory requirements: the demo instance runs on a Raspberry Pi3B+
 * Recommendation engine
 * Audio transcode for maximum interoperability and low bandwith requirements
 * Multi-value tags: artists, genres, ...

conf/lms.conf

@@ -40,6 +40,11 @@ api-subsonic = true;
 # Turn on this option to allow the demo account creation/use
 demo = false;
 
-# Max entries in the logging throttler (1 entry per client)
+# Max entries in the login throttler (1 entry per client)
 login-throttler-max-entries = 10000;
 
+# Max external cover file size in MBytes
+cover-max-file-size = 10;
+
+# Max cover cache size in MBytes
+cover-max-cache-size = 30;

docker/Dockerfile

@@ -92,7 +92,7 @@ RUN \
 
 RUN \
 	DIR=/tmp/wt && mkdir -p ${DIR} && cd ${DIR} && \
-	cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=${PREFIX} && \
+	cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=${PREFIX} -DBUILD_EXAMPLES=NO && \
 	make && \
 	make install
 
@@ -108,6 +108,7 @@ RUN \
 	--prefix=${PREFIX} \
 	--enable-shared=yes \
 	--enable-static=no \
+	--disable-openmp \
 	--with-quantum-depth=8 \
 	--with-modules=no \
 	--with-frozenpaths=no \
@@ -197,8 +198,7 @@ ARG	RUNTIME_PACKAGES=" \
 	boost-system \
 	boost-thread \
 	libconfig++ \
-	taglib \
-	libgomp"
+	taglib"
 
 ARG	LMS_USER=lms
 ARG	LMS_GROUP=lms

src/libs/av/impl/AvInfo.cpp

@@ -45,7 +45,7 @@ static std::string averror_to_string(int error)
 }
 
 MediaFileException::MediaFileException(int avError)
-: AvException("MediaFileException: " + averror_to_string(avError))
+: AvException {"MediaFileException: " + averror_to_string(avError)}
 {
 }
 
@@ -181,8 +181,8 @@ MediaFile::hasAttachedPictures(void) const
 	return false;
 }
 
-std::vector<Picture>
-MediaFile::getAttachedPictures(std::size_t nbMaxPictures) const
+void
+MediaFile::visitAttachedPictures(std::function<void(const Picture&)> func) const
 {
 	static const std::map<int, std::string> codecMimeMap =
 	{
@@ -194,8 +194,6 @@ MediaFile::getAttachedPictures(std::size_t nbMaxPictures) const
 		{ AV_CODEC_ID_PPM, "image/x-portable-pixmap" },
 	};
 
-	std::vector<Picture> pictures;
-
 	for (std::size_t i = 0; i < _context->nb_streams; ++i)
 	{
 		AVStream *avstream = _context->streams[i];
@@ -225,15 +223,11 @@ MediaFile::getAttachedPictures(std::size_t nbMaxPictures) const
 
 		const AVPacket& pkt {avstream->attached_pic};
 
-		std::copy(pkt.data, pkt.data + pkt.size, std::back_inserter(picture.data));
+		picture.data = reinterpret_cast<const std::byte*>(pkt.data);
+		picture.dataSize = pkt.size;
 
-		pictures.push_back( picture );
-
-		if (pictures.size() >= nbMaxPictures)
-			break;
+		func(picture);
 	}
-
-	return pictures;
 }
 
 std::optional<MediaFileFormat>

src/libs/av/include/av/AvInfo.hpp

@@ -23,6 +23,7 @@
 
 #include <chrono>
 #include <filesystem>
+#include <functional>
 #include <map>
 #include <optional>
 #include <string>
@@ -40,7 +41,8 @@ void AvInit();
 struct Picture
 {
 	std::string mimeType;
-	std::vector<uint8_t> data;
+	const std::byte* data {};
+	std::size_t dataSize;
 };
 
 struct StreamInfo
@@ -76,11 +78,11 @@ class MediaFile
 		std::vector<StreamInfo>	getStreamInfo() const;
 		std::optional<std::size_t>	getBestStream() const; // none if failure/unknown
 		bool			hasAttachedPictures(void) const;
-		std::vector<Picture>	getAttachedPictures(std::size_t nbMaxPictures) const;
+		void			visitAttachedPictures(std::function<void(const Picture&)> func) const;
 
 	private:
 
-		std::filesystem::path	_p;
+		const std::filesystem::path	_p;
 		AVFormatContext* _context {};
 };
 

src/libs/cover/CMakeLists.txt

@@ -1,5 +1,6 @@
 
 add_library(lmscover SHARED
+	impl/CoverArt.cpp
 	impl/CoverArtGrabber.cpp
 	impl/Image.cpp
 	)

src/libs/cover/impl/CoverArt.cpp

@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2020 Emeric Poupon
+ *
+ * This file is part of LMS.
+ *
+ * LMS is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * LMS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with LMS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "CoverArt.hpp"
+
+namespace CoverArt
+{
+
+	CoverArt::CoverArt(EncodedImage image)
+		: _image {image}
+	{}
+
+	const std::byte*
+	CoverArt::getData() const
+	{
+		return _image.getData();
+	}
+
+	std::size_t
+	CoverArt::getDataSize() const
+	{
+		return _image.getDataSize();
+	}
+
+	std::string_view
+	CoverArt::getMimeType() const
+	{
+		return "image/jpeg";
+	}
+
+} // namespace CoverArt

src/libs/cover/impl/CoverArt.hpp

@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2020 Emeric Poupon
+ *
+ * This file is part of LMS.
+ *
+ * LMS is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * LMS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with LMS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "cover/ICoverArt.hpp"
+#include "Image.hpp"
+
+namespace CoverArt
+{
+
+	class CoverArt : public ICoverArt
+	{
+		public:
+			CoverArt(EncodedImage image);
+
+			const std::byte* getData() const override;
+			std::size_t getDataSize() const override;
+			std::string_view getMimeType() const override;
+
+		private:
+			EncodedImage _image;
+	};
+
+} // namespace CoverArt

src/libs/cover/impl/CoverArtGrabber.cpp

@@ -28,6 +28,8 @@
 #include "utils/Logger.hpp"
 #include "utils/Random.hpp"
 
+#include "CoverArt.hpp"
+
 namespace {
 
 bool
@@ -40,89 +42,156 @@ isFileSupported(const std::filesystem::path& file, const std::vector<std::filesy
 
 namespace CoverArt {
 
-std::unique_ptr<IGrabber> createGrabber(const std::filesystem::path& execPath)
+std::unique_ptr<IGrabber> createGrabber(const std::filesystem::path& execPath, std::size_t maxCacheSize, std::size_t maxFileSize)
 {
-	return std::make_unique<Grabber>(execPath);
+	return std::make_unique<Grabber>(execPath, maxCacheSize, maxFileSize);
 }
 
-Grabber::Grabber(const std::filesystem::path& execPath)
+Grabber::Grabber(const std::filesystem::path& execPath,
+		std::size_t maxCacheSize,
+		std::size_t maxFileSize)
+	: _maxCacheSize {maxCacheSize}
+	, _maxFileSize {maxFileSize}
 {
+	LMS_LOG(COVER, INFO) << "Max cache size = " << _maxCacheSize;
+	LMS_LOG(COVER, INFO) << "Max file size = " << _maxFileSize;
 	init(execPath);
 }
 
 void
 Grabber::setDefaultCover(const std::filesystem::path& p)
 {
-	_defaultCover = std::make_unique<Image>();
-	if (!_defaultCover->load(p))
+	try
+	{
+		RawImage defaultCover {p};
+		_defaultCover = defaultCover.encode();
+		LMS_LOG(COVER, INFO) << "Successfully read default cover image!";
+	}
+	catch (const ImageException& e)
+	{
 		throw LmsException("Cannot read default cover file '" + p.string() + "'");
 	}
+}
 
-static std::optional<Image>
-getFromAvMediaFile(const Av::MediaFile& input)
+static std::optional<EncodedImage>
+getFromAvMediaFile(const Av::MediaFile& input, Width width)
 {
-	std::vector<Image> res;
+	std::optional<EncodedImage> image;
 
-	for (auto& picture : input.getAttachedPictures(2))
+	input.visitAttachedPictures([&](const Av::Picture& picture)
 	{
-		Image image;
+		if (image)
+			return;
+
+		try
+		{
+			EncodedImage encodedImage {picture.data, picture.dataSize};
+
+			RawImage rawImage {encodedImage};
+			rawImage.scale(width);
+
+			image = rawImage.encode();
+		}
+		catch (const ImageException& e)
+		{
+			LMS_LOG(COVER, ERROR) << "Cannot read embedded cover: " << e.what();
+		}
+	});
 
-		if (image.load(picture.data))
 	return image;
-		else
-			LMS_LOG(COVER, ERROR) << "Cannot load embedded cover file in '" << input.getPath().string() << "'";
 }
 
-	LMS_LOG(COVER, DEBUG) << "No cover found in media file '" << input.getPath().string() << "'";
-	return std::nullopt;
+static std::optional<EncodedImage>
+getFromFile(const std::filesystem::path& p, Width width)
+{
+	std::optional<EncodedImage> image;
+
+	try
+	{
+		RawImage rawImage {p};
+		rawImage.scale(width);
+
+		image = rawImage.encode();
+	}
+	catch (const ImageException& e)
+	{
+		LMS_LOG(COVER, ERROR) << "Cannot read cover in file '" << p.string() << "': " << e.what();
 	}
 
-std::optional<Image>
-Grabber::getFromDirectory(const std::filesystem::path& p, std::string_view preferredFileName) const
+	return image;
+}
+
+EncodedImage
+Grabber::getDefault(Width width)
+{
+	{
+		std::shared_lock lock {_cacheMutex};
+
+		if (auto it {_defaultCache.find(width)}; it != std::cend(_defaultCache))
+			return it->second;
+	}
+
+	{
+		std::unique_lock lock {_cacheMutex};
+
+		if (auto it {_defaultCache.find(width)}; it != std::cend(_defaultCache))
+			return it->second;
+
+		RawImage rawImage {*_defaultCover};
+		rawImage.scale(width);
+		EncodedImage res {rawImage.encode()};
+
+		_defaultCache[width] = res;
+		LMS_LOG(COVER, DEBUG) << "Default cache entries = " << _defaultCache.size();
+
+		return res;
+	}
+}
+
+std::optional<EncodedImage>
+Grabber::getFromDirectory(const std::filesystem::path& p, std::string_view preferredFileName, Width width) const
 {
 	const std::multimap<std::string, std::filesystem::path> coverPaths {getCoverPaths(p)};
 
-	auto tryLoadImage = [](const std::filesystem::path& p, Image& image)
+	auto tryLoadImageFromFilename = [&](std::string_view fileName)
 	{
-		if (!image.load(p))
-		{
-			LMS_LOG(COVER, ERROR) << "Cannot load image in file '" << p.string() << "'";
-			return false;
-		}
+		std::optional<EncodedImage> image;
 
-		return true;
-	};
-
-	auto tryLoadImageFromFilename = [&](std::string_view fileName, Image& image)
-	{
 		auto range {coverPaths.equal_range(std::string {fileName})};
 		for (auto it {range.first}; it != range.second; ++it)
 		{
-			if (tryLoadImage(it->second, image))
-				return true;
+			image = getFromFile(it->second, width);
+			if (!image)
+				continue;
 		}
-		return false;
+		return image;
 	};
 
-	Image image;
+	std::optional<EncodedImage> image;
 
-	if (!preferredFileName.empty() && tryLoadImageFromFilename(preferredFileName, image))
+	if (!preferredFileName.empty())
+	{
+		image = tryLoadImageFromFilename(preferredFileName);
+		if (image)
 			return image;
+	}
 
 	for (std::string_view filename : _preferredFileNames)
 	{
-		if (tryLoadImageFromFilename(filename, image))
+		image = tryLoadImageFromFilename(filename);
+		if (image)
 			return image;
 	}
 
 	// Just pick one
 	for (const auto& [filename, coverPath] : coverPaths)
 	{
-		if (tryLoadImage(coverPath, image))
+		image = getFromFile(coverPath, width);
+		if (image)
 			return image;
 	}
 
-	return std::nullopt;
+	return image;
 }
 
 std::multimap<std::string, std::filesystem::path>
@@ -156,29 +225,32 @@ Grabber::getCoverPaths(const std::filesystem::path& directoryPath) const
 	return res;
 }
 
-std::optional<Image>
-Grabber::getFromTrack(const std::filesystem::path& p) const
+std::optional<EncodedImage>
+Grabber::getFromTrack(const std::filesystem::path& p, Width width) const
 {
+	std::optional<EncodedImage> image;
+
 	try
 	{
-		Av::MediaFile input(p);
+		Av::MediaFile input {p};
 
-		return getFromAvMediaFile(input);
+		image = getFromAvMediaFile(input, width);
 	}
-	catch (Av::MediaFileException& e)
+	catch (Av::AvException& e)
 	{
 		LMS_LOG(COVER, ERROR) << "Cannot get covers from track " << p.string() << ": " << e.what();
-		return std::nullopt;
-	}
 	}
 
-Image
-Grabber::getFromTrack(Database::Session& dbSession, Database::IdType trackId, std::size_t size)
+	return image;
+}
+
+EncodedImage
+Grabber::getFromTrackInternal(Database::Session& dbSession, Database::IdType trackId, Width width)
 {
 	using namespace Database;
 
-	const CacheEntryDesc cacheEntryDesc {CacheEntryDesc::Type::Track, trackId, size};
-	std::optional<Image> cover {loadFromCache(cacheEntryDesc)};
+	const CacheEntryDesc cacheEntryDesc {CacheEntryDesc::Type::Track, trackId, width};
+	std::optional<EncodedImage> cover {loadFromCache(cacheEntryDesc)};
 
 	if (cover)
 		return *cover;
@@ -203,33 +275,30 @@ Grabber::getFromTrack(Database::Session& dbSession, Database::IdType trackId, st
 	}
 
 	if (hasCover)
-		cover = getFromTrack(trackPath);
+		cover = getFromTrack(trackPath, width);
 
 	if (!cover)
-		cover = getFromDirectory(trackPath.parent_path(), trackPath.filename().replace_extension("").string());
+		cover = getFromDirectory(trackPath.parent_path(), trackPath.filename().replace_extension("").string(), width);
 
 	if (!cover && isMultiDisc)
 	{
 		if (trackPath.parent_path().has_parent_path())
-			cover = getFromDirectory(trackPath.parent_path().parent_path(), {});
+			cover = getFromDirectory(trackPath.parent_path().parent_path(), {}, width);
 	}
 
 	if (!cover)
-		cover = *_defaultCover;
-
-	cover->scale(Geometry {size, size});
+		cover = getDefault(width);
 
 	saveToCache(cacheEntryDesc, *cover);
 
 	return *cover;
 }
 
-
-Image
-Grabber::getFromRelease(Database::Session& session, Database::IdType releaseId, std::size_t size)
+EncodedImage
+Grabber::getFromReleaseInternal(Database::Session& session, Database::IdType releaseId, Width width)
 {
-	const CacheEntryDesc cacheEntryDesc {CacheEntryDesc::Type::Release, releaseId, size};
-	std::optional<Image> cover {loadFromCache(cacheEntryDesc)};
+	const CacheEntryDesc cacheEntryDesc {CacheEntryDesc::Type::Release, releaseId, width};
+	std::optional<EncodedImage> cover {loadFromCache(cacheEntryDesc)};
 
 	if (cover)
 		return *cover;
@@ -249,15 +318,11 @@ Grabber::getFromRelease(Database::Session& session, Database::IdType releaseId,
 
 	if (trackId)
 	{
-		cover = getFromTrack(session, *trackId, size);
+		cover = getFromTrackInternal(session, *trackId, width);
 	}
-	else
-	{
-		if (!cover)
-			cover = *_defaultCover;
 
-		cover->scale(Geometry {size, size});
-	}
+	if (!cover)
+		cover = getDefault(width);
 
 	saveToCache(cacheEntryDesc, *cover);
 
@@ -269,42 +334,43 @@ Grabber::flushCache()
 {
 	std::unique_lock lock {_cacheMutex};
 
-	LMS_LOG(COVER, DEBUG) << "Cache stats: hits = " << _cacheHits << ", misses = " << _cacheMisses;
+	LMS_LOG(COVER, DEBUG) << "Cache stats: hits = " << _cacheHits << ", misses = " << _cacheMisses << ", nb entries = " << _cache.size() << ", size = " << _cacheSize;
 	_cacheHits = 0;
 	_cacheMisses = 0;
+	_cacheSize = 0;
 	_cache.clear();
 }
 
-std::vector<uint8_t>
-Grabber::getFromTrack(Database::Session& session, Database::IdType trackId, Format format, std::size_t width)
+std::unique_ptr<ICoverArt>
+Grabber::getFromTrack(Database::Session& session, Database::IdType trackId, std::size_t width)
 {
-	const Image cover {getFromTrack(session, trackId, width)};
-
-	assert(format == Format::JPEG);
-	return cover.save(format);
+	CoverArt toto {getFromTrackInternal(session, trackId, width)};
+	return std::make_unique<CoverArt>(getFromTrackInternal(session, trackId, width));
 }
 
-std::vector<uint8_t>
-Grabber::getFromRelease(Database::Session& session, Database::IdType releaseId, Format format, std::size_t width)
+std::unique_ptr<ICoverArt>
+Grabber::getFromRelease(Database::Session& session, Database::IdType releaseId, std::size_t width)
 {
-	const Image cover {getFromRelease(session, releaseId, width)};
-
-	assert(format == Format::JPEG);
-	return cover.save(format);
+	return std::make_unique<CoverArt>(getFromReleaseInternal(session, releaseId, width));
 }
 
 void
-Grabber::saveToCache(const CacheEntryDesc& entryDesc, const Image& image)
+Grabber::saveToCache(const CacheEntryDesc& entryDesc, const EncodedImage& image)
 {
 	std::unique_lock lock {_cacheMutex};
 
-	if (_cache.size() >= _maxCacheEntries)
-		_cache.erase(Random::pickRandom(_cache));
+	while (_cacheSize + image.getDataSize() > _maxCacheSize && !_cache.empty())
+	{
+		auto it {Random::pickRandom(_cache)};
+		_cacheSize -= it->second.getDataSize();
+		_cache.erase(it);
+	}
 
+	_cacheSize += image.getDataSize();
 	_cache[entryDesc] = image;
 }
 
-std::optional<Image>
+std::optional<EncodedImage>
 Grabber::loadFromCache(const CacheEntryDesc& entryDesc)
 {
 	std::shared_lock lock {_cacheMutex};

src/libs/cover/impl/CoverArtGrabber.hpp

@@ -19,8 +19,8 @@
 
 #pragma once
 
+#include <atomic>
 #include <filesystem>
-#include <map>
 #include <optional>
 #include <shared_mutex>
 #include <string_view>
@@ -83,7 +83,7 @@ namespace CoverArt
 	class Grabber : public IGrabber
 	{
 		public:
-			Grabber(const std::filesystem::path& execPath);
+			Grabber(const std::filesystem::path& execPath, std::size_t maxCacheEntries, std::size_t maxFileSize);
 
 			Grabber(const Grabber&) = delete;
 			Grabber& operator=(const Grabber&) = delete;
@@ -93,32 +93,35 @@ namespace CoverArt
 		private:
 
 			void							setDefaultCover(const std::filesystem::path& defaultCoverPath) override;
-
-			std::vector<uint8_t>	getFromTrack(Database::Session& dbSession, Database::IdType trackId, Format format, std::size_t width) override;
-			std::vector<uint8_t>	getFromRelease(Database::Session& dbSession, Database::IdType releaseId, Format format, std::size_t width) override;
+			std::unique_ptr<ICoverArt>		getFromTrack(Database::Session& dbSession, Database::IdType trackId, Width width) override;
+			std::unique_ptr<ICoverArt>		getFromRelease(Database::Session& dbSession, Database::IdType releaseId, Width width) override;
 			void							flushCache() override;
 
+			EncodedImage					getFromTrackInternal(Database::Session& dbSession, Database::IdType trackId, Width width);
+			EncodedImage					getFromReleaseInternal(Database::Session& dbSession, Database::IdType releaseId, Width width);
 
-			Image					getFromTrack(Database::Session& dbSession, Database::IdType trackId, std::size_t size);
-			Image					getFromRelease(Database::Session& dbSession, Database::IdType releaseId, std::size_t size);
-
-			std::optional<Image>			getFromTrack(const std::filesystem::path& path) const;
+			std::optional<EncodedImage>		getFromTrack(const std::filesystem::path& path, Width width) const;
 			std::multimap<std::string, std::filesystem::path>	getCoverPaths(const std::filesystem::path& directoryPath) const;
-			std::optional<Image>			getFromDirectory(const std::filesystem::path& path, std::string_view preferredFileName) const;
+			std::optional<EncodedImage>		getFromDirectory(const std::filesystem::path& path, std::string_view preferredFileName, Width width) const;
+			EncodedImage					getDefault(Width width);
 
-			std::unique_ptr<Image> _defaultCover;	// unique_ptr to defer initializing
+			EncodedImage					resizeCoverOrFallback(EncodedImage image, Width width) const;
+
+			std::optional<EncodedImage>		_defaultCover;	// optional to defer initializing
 
 			std::shared_mutex _cacheMutex;
-			std::unordered_map<CacheEntryDesc, Image> _cache;
-			std::size_t _cacheMisses {};
-			std::size_t _cacheHits {};
+			std::unordered_map<CacheEntryDesc, EncodedImage> _cache;
+			std::unordered_map<Width, EncodedImage>	_defaultCache;
+			std::atomic<std::size_t>	_cacheMisses {};
+			std::atomic<std::size_t>	_cacheHits {};
+			std::size_t					_cacheSize {};
 
-			void saveToCache(const CacheEntryDesc& entryDesc, const Image& image);
-			std::optional<Image> loadFromCache(const CacheEntryDesc& entryDesc);
+			void saveToCache(const CacheEntryDesc& entryDesc, const EncodedImage& image);
+			std::optional<EncodedImage> loadFromCache(const CacheEntryDesc& entryDesc);
 
-			static inline constexpr std::size_t _maxCacheEntries {1000};
+			const std::size_t _maxCacheSize;
 			static inline const std::vector<std::filesystem::path> _fileExtensions {".jpg", ".jpeg", ".png", ".bmp"}; // TODO parametrize
-			static inline constexpr std::size_t _maxFileSize {10000000};
+			const std::size_t _maxFileSize;
 			static inline const std::vector<std::string> _preferredFileNames {"cover", "front"}; // TODO parametrize
 	};
 

src/libs/cover/impl/Image.cpp

@@ -19,6 +19,11 @@
 
 #include "Image.hpp"
 
+#include <atomic>
+#include <fstream>
+
+#include <magick/resource.h>
+
 #include "utils/Logger.hpp"
 
 namespace CoverArt {
@@ -27,132 +32,118 @@ void
 init(const std::filesystem::path& path)
 {
 	Magick::InitializeMagick(path.string().c_str());
+
+	if (!MagickLib::SetMagickResourceLimit(MagickLib::ThreadsResource, 1))
+		LMS_LOG(COVER, ERROR) << "Cannot set Magick thread resource limit to 1!";
+
+	if (!MagickLib::SetMagickResourceLimit(MagickLib::DiskResource, 0))
+		LMS_LOG(COVER, ERROR) << "Cannot set Magick disk resource limit to 0!";
+
+	LMS_LOG(COVER, INFO) << "Magick threads resource limit = " << GetMagickResourceLimit(MagickLib::ThreadsResource);
+	LMS_LOG(COVER, INFO) << "Magick Disk resource limit = " << GetMagickResourceLimit(MagickLib::DiskResource);
 }
 
-static
-std::string
-formatToMagick(Format format)
+EncodedImage::EncodedImage(const std::byte* data, std::size_t dataSize)
+: _blob {data, dataSize}
 {
-	switch (format)
+}
+
+EncodedImage::EncodedImage(Magick::Blob blob)
+:  _blob {blob}
 {
-		case Format::JPEG: return "JPEG";
 }
 
-	return "JPEG";
-}
-
-std::string
-formatToMimeType(Format format)
+const std::byte*
+EncodedImage::getData() const
 {
-	switch (format)
+	return reinterpret_cast<const std::byte*>(_blob.data());
+}
+
+std::size_t
+EncodedImage::getDataSize() const
 {
-		case Format::JPEG: return "JPEG";
+	return _blob.length();
 }
 
-	return "application/octet-stream";
-}
-
-
-bool
-Image::load(const std::vector<unsigned char>& rawData)
+RawImage::RawImage(const std::filesystem::path& p)
 {
 	try
 	{
-		Magick::Blob blob {&rawData[0], rawData.size()};
-		_image.read(blob);
-
-		return true;
+		_image.read(p.string().c_str());
 	}
 	catch (Magick::WarningCoder& e)
 	{
-		return true;
+		LMS_LOG(COVER, WARNING) << "Caught Magick WarningCoder while loading image '" << p.string() << "': " << e.what();
+	}
+	catch (Magick::Warning& e)
+	{
+		LMS_LOG(COVER, WARNING) << "Caught Magick warning while loading raw image '" << p.string() << "': " << e.what();
+		throw ImageException {std::string {"Magick read warning: "} + e.what()};
+	}
+	catch (Magick::Exception& e)
+	{
+		LMS_LOG(COVER, ERROR) << "Caught Magick exception while loading raw image '" << p.string() << "': " << e.what();
+		throw ImageException {std::string {"Magick read error: "} + e.what()};
+	}
+}
+
+RawImage::RawImage(const EncodedImage& encodedImage)
+{
+	try
+	{
+		_image.read(encodedImage._blob);
+	}
+	catch (Magick::WarningCoder& e)
+	{
+		LMS_LOG(COVER, WARNING) << "Caught Magick WarningCoder while loading raw image: " << e.what();
 	}
 	catch (Magick::Warning& e)
 	{
 		LMS_LOG(COVER, WARNING) << "Caught Magick warning while loading raw image: " << e.what();
-		return false;
+		throw ImageException {std::string {"Magick read warning: "} + e.what()};
 	}
 	catch (Magick::Exception& e)
 	{
 		LMS_LOG(COVER, ERROR) << "Caught Magick exception while loading raw image: " << e.what();
-		return false;
+		throw ImageException {std::string {"Magick read error: "} + e.what()};
 	}
 }
 
-bool
-Image::load(const std::filesystem::path& p)
+void
+RawImage::scale(std::size_t width)
 {
-	try
-	{
-		_image.read(p.string());
-
-		return true;
-	}
-	catch (Magick::WarningCoder& e)
-	{
-		return true;
-	}
-	catch (Magick::Warning& e)
-	{
-		LMS_LOG(COVER, WARNING) << "Caught Magick warning while loading raw image: " << e.what();
-		return false;
-	}
-	catch (Magick::Exception& e)
-	{
-		LMS_LOG(COVER, ERROR) << "Caught Magick exception while loading image from file '" << p.string() << "': " << e.what();
-		return false;
-	}
-}
-
-Geometry
-Image::getSize() const
-{
-	Magick::Geometry geometry {_image.size()};
-	return {geometry.width(), geometry.height()};
-}
-
-bool
-Image::scale(Geometry geometry)
-{
-	if (geometry.width == 0 || geometry.height == 0)
-		return false;
+	if (width == 0)
+		throw ImageException {"Bad width = 0"};
 
 	try
 	{
-		_image.resize( Magick::Geometry(geometry.width, geometry.height ) );
-
-		return true;
+		_image.resize(Magick::Geometry {static_cast<unsigned int>(width), static_cast<unsigned int>(width)});
 	}
 	catch (Magick::Exception& e)
 	{
 		LMS_LOG(COVER, ERROR) << "Caught Magick exception during scale: " << e.what();
-		return false;
+		throw ImageException {std::string {"Magick resize error: "} + e.what()};
 	}
 }
 
-std::vector<uint8_t>
-Image::save(Format format) const
+EncodedImage
+RawImage::encode() const
 {
-	std::vector<uint8_t> res;
-
 	try
 	{
 		Magick::Image outputImage {_image};
 
-		outputImage.magick(formatToMagick(format));
+		outputImage.magick("JPEG");
 
 		Magick::Blob blob;
 		outputImage.write(&blob);
 
-		auto begin = static_cast<const uint8_t*>(blob.data());
-		std::copy(begin, begin + blob.length(), std::back_inserter(res));
-		return res;
+		return EncodedImage {blob};
 	}
 	catch (Magick::Exception& e)
 	{
-		LMS_LOG(COVER, ERROR) << "Caught Magick exception during save:" << e.what();
-		res.clear();
-		return res;
+		LMS_LOG(COVER, ERROR) << "Caught Magick exception while encoding raw image: " << e.what();
+		throw ImageException {std::string {"Magick encode error: "} + e.what()};
 	}
 }
 

src/libs/cover/impl/Image.hpp

@@ -24,33 +24,50 @@
 
 #include <Magick++.h>
 
-#include "cover/CoverArt.hpp"
+#include "utils/Exception.hpp"
 
 namespace CoverArt
 {
-
 	void init(const std::filesystem::path& path);
 
-	class Image
+	// internal use only
+	class ImageException : public LmsException
 	{
 		public:
+			using LmsException::LmsException;
+	};
 
-			// input
-			bool	load(const std::vector<unsigned char>& rawData);
-			bool	load(const std::filesystem::path& p);
+	class EncodedImage
+	{
+		public:
+			EncodedImage() = default;
+			EncodedImage(const std::byte* data, std::size_t dataSize);
 
-			Geometry	getSize() const;
+			const std::byte* getData() const;
+			std::size_t getDataSize() const;
+
+		private:
+			friend class RawImage;
+			EncodedImage(Magick::Blob blob);
+
+			Magick::Blob _blob;
+	};
+
+	class RawImage
+	{
+		public:
+			RawImage(const std::filesystem::path& p);
+			RawImage(const EncodedImage& encodedImage);
 
 			// Operations
-			bool	scale(Geometry geometry);
+			void scale(std::size_t width);
 
 			// output
-			std::vector<uint8_t> save(Format format) const;
+			EncodedImage encode() const;
 
 		private:
 			Magick::Image _image;
 	};
 
-
 } // namespace CoverArt
 

src/libs/cover/include/cover/ICoverArt.hpp

@@ -1,6 +1,5 @@
-
 /*
- * Copyright (C) 2015 Emeric Poupon
+ * Copyright (C) 2020 Emeric Poupon
  *
  * This file is part of LMS.
  *
@@ -20,21 +19,20 @@
 
 #pragma once
 
-#include <string>
+#include <cstddef>
+#include <string_view>
 
 namespace CoverArt
 {
 
-	enum class Format
+	class ICoverArt
 	{
-		JPEG,
-	};
-	std::string formatToMimeType(Format format);
+		public:
+			virtual ~ICoverArt() = default;
 
-	struct Geometry
-	{
-		std::size_t width;
-		std::size_t height;
+			virtual const std::byte* getData() const = 0;
+			virtual std::size_t getDataSize() const = 0;
+			virtual std::string_view getMimeType() const = 0;
 	};
 
-}
+} // namespace CoverArt

src/libs/cover/include/cover/ICoverArtGrabber.hpp

@@ -19,17 +19,22 @@
 
 #pragma once
 
+#include <cstddef>
 #include <filesystem>
-#include <vector>
+#include <memory>
+#include <string_view>
 
 #include "database/Types.hpp"
-#include "cover/CoverArt.hpp"
+#include "cover/ICoverArt.hpp"
 
-namespace Database {
+namespace Database
+{
 	class Session;
 }
 
-namespace CoverArt {
+namespace CoverArt
+{
+	using Width = std::size_t;
 
 	class IGrabber
 	{
@@ -38,13 +43,13 @@ class IGrabber
 
 			virtual void			setDefaultCover(const std::filesystem::path& defaultCoverPath) = 0;
 
-		virtual std::vector<uint8_t>	getFromTrack(Database::Session& dbSession, Database::IdType trackId, Format format, std::size_t width) = 0;
-		virtual std::vector<uint8_t>	getFromRelease(Database::Session& dbSession, Database::IdType releaseId, Format format, std::size_t width) = 0;
+			virtual std::unique_ptr<ICoverArt>	getFromTrack(Database::Session& dbSession, Database::IdType trackId, Width width) = 0;
+			virtual std::unique_ptr<ICoverArt>	getFromRelease(Database::Session& dbSession, Database::IdType releaseId, Width width) = 0;
 
 			virtual void flushCache() = 0;
 	};
 
-std::unique_ptr<IGrabber> createGrabber(const std::filesystem::path& execPath);
+	std::unique_ptr<IGrabber> createGrabber(const std::filesystem::path& execPath, std::size_t maxCacheEntries, std::size_t maxFileSize);
 
 } // namespace CoverArt
 

src/libs/subsonic/impl/ParameterParsing.hpp

@@ -55,7 +55,7 @@ namespace API::Subsonic
 	{
 		std::vector<T> res {getMultiParametersAs<T>(parameterMap, param)};
 		if (res.empty())
-			throw RequiredParameterMissingError {};
+			throw RequiredParameterMissingError {param};
 
 		return res;
 	}
@@ -78,7 +78,7 @@ namespace API::Subsonic
 	{
 		auto res {getParameterAs<T>(parameterMap, param)};
 		if (!res)
-			throw RequiredParameterMissingError {};
+			throw RequiredParameterMissingError {param};
 
 		return *res;
 	}

src/libs/subsonic/impl/SubsonicResource.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2019 Emeric Poupon
+ * copyright (c) 2019 emeric poupon
  *
  * This file is part of LMS.
  *
@@ -546,7 +546,7 @@ handleCreatePlaylistRequest(RequestContext& context)
 		throw BadParameterGenericError {"songId"};
 
 	if (!name && !id)
-		throw RequiredParameterMissingError {};
+		throw RequiredParameterMissingError {"name or id"};
 
 	auto transaction {context.dbSession.createUniqueTransaction()};
 
@@ -1746,21 +1746,21 @@ handleGetCoverArt(RequestContext& context, const Wt::Http::Request& /*request*/,
 	std::size_t size {getParameterAs<std::size_t>(context.parameters, "size").value_or(256)};
 	size = clamp(size, std::size_t {32}, std::size_t {1024});
 
-	std::vector<unsigned char> data;
+	std::unique_ptr<CoverArt::ICoverArt> cover;
 	switch (id.type)
 	{
 		case Id::Type::Track:
-			data = Service<CoverArt::IGrabber>::get()->getFromTrack(context.dbSession, id.value, CoverArt::Format::JPEG, size);
+			cover = Service<CoverArt::IGrabber>::get()->getFromTrack(context.dbSession, id.value, size);
 			break;
 		case Id::Type::Release:
-			data = Service<CoverArt::IGrabber>::get()->getFromRelease(context.dbSession, id.value, CoverArt::Format::JPEG, size);
+			cover = Service<CoverArt::IGrabber>::get()->getFromRelease(context.dbSession, id.value, size);
 			break;
 		default:
 			throw BadParameterGenericError {"id"};
 	}
 
-	response.out().write(reinterpret_cast<const char*>(&data[0]), data.size());
-	response.setMimeType(CoverArt::formatToMimeType(CoverArt::Format::JPEG));
+	response.out().write(reinterpret_cast<const char*>(cover->getData()), cover->getDataSize());
+	response.setMimeType(std::string {cover->getMimeType()});
 }
 
 using RequestHandlerFunc = std::function<Response(RequestContext& context)>;

src/libs/subsonic/impl/SubsonicResponse.hpp

@@ -73,9 +73,14 @@ class GenericError : public Error
 class RequiredParameterMissingError : public Error
 {
 	public:
-		RequiredParameterMissingError() : Error {Code::RequiredParameterMissing} {}
+		RequiredParameterMissingError(std::string_view param)
+			: Error {Code::RequiredParameterMissing}
+			, _param {param}
+			{}
+
 	private:
-		std::string getMessage() const override { return "Required parameter is missing."; }
+		std::string getMessage() const override { return "Required parameter '" + _param + "' is missing."; }
+		std::string _param;
 };
 
 class ClientMustUpgradeError : public Error

src/lms/main.cpp

@@ -168,7 +168,9 @@ int main(int argc, char* argv[])
 		// Service initialization order is important
 		Service<Auth::IAuthTokenService> authTokenService {Auth::createAuthTokenService(config->getULong("login-throttler-max-entriees", 10000))};
 		Service<Auth::IPasswordService> passwordService {Auth::createPasswordService(config->getULong("login-throttler-max-entriees", 10000))};
-		Service<CoverArt::IGrabber> coverArtService {CoverArt::createGrabber(argv[0])};
+		Service<CoverArt::IGrabber> coverArtService {CoverArt::createGrabber(argv[0],
+				config->getULong("cover-max-cache-size", 30) * 1000 * 1000,
+				config->getULong("cover-max-file-size", 10) * 1000 * 1000)};
 		coverArtService->setDefaultCover(server.appRoot() + "/images/unknown-cover.jpg");
 		Service<Recommendation::IEngine> recommendationEngineService {Recommendation::createEngine(database)};
 		recommendationEngineService->requestLoad();

src/lms/ui/MediaPlayer.cpp

@@ -238,8 +238,6 @@ MediaPlayer::loadTrack(Database::IdType trackId, bool play, float replayGain)
 		if (!track)
 			return;
 
-		const std::string imgResourceMimeType {LmsApp->getImageResource()->getMimeType()};
-
 		const std::string transcodeResource {LmsApp->getAudioTranscodeResource()->getUrl(trackId)};
 		const std::string nativeResource {LmsApp->getAudioFileResource()->getUrl(trackId)};
 
@@ -255,8 +253,8 @@ MediaPlayer::loadTrack(Database::IdType trackId, bool play, float replayGain)
 			<< " artist: \"" << (!artists.empty() ? StringUtils::jsEscape(artists.front()->getName()) : "") << "\","
 			<< " release: \"" << (track->getRelease() ? StringUtils::jsEscape(track->getRelease()->getName()) : "") << "\","
 			<< " artwork: ["
-			<< "   { src: \"" << LmsApp->getImageResource()->getTrackUrl(trackId, ImageResource::Size::Small) << "\", sizes: \"128x128\",	type: \"" << imgResourceMimeType << "\" },"
-			<< "   { src: \"" << LmsApp->getImageResource()->getTrackUrl(trackId, ImageResource::Size::Large) << "\", sizes: \"512x512\",	type: \"" << imgResourceMimeType << "\" },"
+			<< "   { src: \"" << LmsApp->getImageResource()->getTrackUrl(trackId, ImageResource::Size::Small) << "\", sizes: \"128x128\",	type: \"image/jpeg\" },"
+			<< "   { src: \"" << LmsApp->getImageResource()->getTrackUrl(trackId, ImageResource::Size::Large) << "\", sizes: \"512x512\",	type: \"image/jpeg\" },"
 			<< " ]"
 			<< "};";
 		oss << "LMS.mediaplayer.loadTrack(params, " << (play ? "true" : "false") << ")"; // true to autoplay

src/lms/ui/resource/ImageResource.cpp

@@ -74,7 +74,7 @@ ImageResource::handleRequest(const Wt::Http::Request& request, Wt::Http::Respons
 		return;
 	}
 
-	std::vector<uint8_t> cover;
+	std::unique_ptr<CoverArt::ICoverArt> cover;
 
 	if (trackIdStr)
 	{
@@ -90,7 +90,7 @@ ImageResource::handleRequest(const Wt::Http::Request& request, Wt::Http::Respons
 		// DbSession are not thread safe
 		{
 			Wt::WApplication::UpdateLock lock {LmsApp};
-			cover = Service<CoverArt::IGrabber>::get()->getFromTrack(LmsApp->getDbSession(), *trackId, CoverArt::Format::JPEG, *size);
+			cover = Service<CoverArt::IGrabber>::get()->getFromTrack(LmsApp->getDbSession(), *trackId, *size);
 		}
 	}
 	else if (releaseIdStr)
@@ -104,7 +104,7 @@ ImageResource::handleRequest(const Wt::Http::Request& request, Wt::Http::Respons
 		// DbSession are not thread safe
 		{
 			Wt::WApplication::UpdateLock lock {LmsApp};
-			cover = Service<CoverArt::IGrabber>::get()->getFromRelease(LmsApp->getDbSession(), *releaseId, CoverArt::Format::JPEG, *size);
+			cover = Service<CoverArt::IGrabber>::get()->getFromRelease(LmsApp->getDbSession(), *releaseId, *size);
 		}
 	}
 	else
@@ -113,15 +113,9 @@ ImageResource::handleRequest(const Wt::Http::Request& request, Wt::Http::Respons
 		return;
 	}
 
-	response.setMimeType(getMimeType());
+	response.setMimeType(std::string {cover->getMimeType()});
 
-	response.out().write(reinterpret_cast<const char *>(&cover[0]), cover.size());
-}
-
-std::string
-ImageResource::getMimeType()
-{
-	return CoverArt::formatToMimeType(CoverArt::Format::JPEG);
+	response.out().write(reinterpret_cast<const char *>(cover->getData()), cover->getDataSize());
 }
 
 } // namespace UserInterface

src/lms/ui/resource/ImageResource.hpp

@@ -45,8 +45,6 @@ class ImageResource : public Wt::WResource
 		std::string getReleaseUrl(Database::IdType releaseId, Size size) const;
 		std::string getTrackUrl(Database::IdType trackId, Size size) const;
 
-		static std::string getMimeType();
-
 	private:
 		void handleRequest(const Wt::Http::Request& request, Wt::Http::Response& response) override;