From 58659c2e30f9b8378d3a7cd73cac48f1ae6b5466 Mon Sep 17 00:00:00 2001 From: Phan An Date: Tue, 5 Jul 2022 15:47:26 +0200 Subject: [PATCH] feat: better supports for compilation when scanning --- app/Console/Commands/SyncCommand.php | 16 +- app/Repositories/AlbumRepository.php | 1 - app/Services/FileSynchronizer.php | 231 +++++----------------- app/Services/MediaSyncService.php | 60 +----- app/Services/UploadService.php | 3 +- app/Values/SongScanInformation.php | 114 +++++++++++ app/Values/SyncResult.php | 14 +- public/.gitignore | 6 +- public/img/{albums => covers}/.gitkeep | 0 tests/Unit/Services/UploadServiceTest.php | 5 +- 10 files changed, 180 insertions(+), 270 deletions(-) create mode 100644 app/Values/SongScanInformation.php rename public/img/{albums => covers}/.gitkeep (100%) diff --git a/app/Console/Commands/SyncCommand.php b/app/Console/Commands/SyncCommand.php index d61007ec..4c28dbfa 100644 --- a/app/Console/Commands/SyncCommand.php +++ b/app/Console/Commands/SyncCommand.php @@ -13,7 +13,7 @@ class SyncCommand extends Command { protected $signature = 'koel:sync {record? : A single watch record. Consult Wiki for more info.} - {--tags= : The comma-separated tags to sync into the database} + {--excludes= : The comma-separated tags to excludes from syncing} {--force : Force re-syncing even unchanged files}'; protected $description = 'Sync songs found in configured directory against the database.'; @@ -42,7 +42,7 @@ class SyncCommand extends Command return; } - $this->syngle($record); + $this->syncSingleRecord($record); } /** @@ -52,12 +52,12 @@ class SyncCommand extends Command { $this->info('Syncing media from ' . Setting::get('media_path') . PHP_EOL); - // Get the tags to sync. + // The excluded tags. // Notice that this is only meaningful for existing records. - // New records will have every applicable field sync'ed in. - $tags = $this->option('tags') ? explode(',', $this->option('tags')) : []; + // New records will have every applicable field synced in. + $excludes = $this->option('excludes') ? explode(',', $this->option('excludes')) : []; - $this->mediaSyncService->sync(null, $tags, $this->option('force'), $this); + $this->mediaSyncService->sync(null, $excludes, $this->option('force'), $this); $this->output->writeln( PHP_EOL . PHP_EOL @@ -68,8 +68,6 @@ class SyncCommand extends Command } /** - * SYNc a sinGLE file or directory. See my awesome pun? - * * @param string $record The watch record. * As of current we only support inotifywait. * Some examples: @@ -79,7 +77,7 @@ class SyncCommand extends Command * * @see http://man7.org/linux/man-pages/man1/inotifywait.1.html */ - public function syngle(string $record): void + public function syncSingleRecord(string $record): void { $this->mediaSyncService->syncByWatchRecord(new InotifyWatchRecord($record)); } diff --git a/app/Repositories/AlbumRepository.php b/app/Repositories/AlbumRepository.php index 3f7cb4a2..13368a03 100644 --- a/app/Repositories/AlbumRepository.php +++ b/app/Repositories/AlbumRepository.php @@ -44,7 +44,6 @@ class AlbumRepository extends AbstractRepository public function getByIds(array $ids, ?User $scopedUser = null): Collection { return Album::withMeta($scopedUser ?? $this->auth->user()) - ->isStandard() ->whereIn('albums.id', $ids) ->get(); } diff --git a/app/Services/FileSynchronizer.php b/app/Services/FileSynchronizer.php index 6d623e89..9c01d99a 100644 --- a/app/Services/FileSynchronizer.php +++ b/app/Services/FileSynchronizer.php @@ -6,10 +6,10 @@ use App\Models\Album; use App\Models\Artist; use App\Models\Song; use App\Repositories\SongRepository; +use App\Values\SongScanInformation; use getID3; -use getid3_lib; use Illuminate\Contracts\Cache\Repository as Cache; -use InvalidArgumentException; +use Illuminate\Support\Arr; use SplFileInfo; use Symfony\Component\Finder\Finder; use Throwable; @@ -20,12 +20,6 @@ class FileSynchronizer public const SYNC_RESULT_BAD_FILE = 2; public const SYNC_RESULT_UNMODIFIED = 3; - private getID3 $getID3; - private MediaMetadataService $mediaMetadataService; - private Helper $helper; - private SongRepository $songRepository; - private Cache $cache; - private Finder $finder; private ?int $fileModifiedTime = null; private ?string $filePath = null; @@ -43,35 +37,19 @@ class FileSynchronizer private ?string $syncError; public function __construct( - getID3 $getID3, - MediaMetadataService $mediaMetadataService, - Helper $helper, - SongRepository $songRepository, - Cache $cache, - Finder $finder + private getID3 $getID3, + private MediaMetadataService $mediaMetadataService, + private Helper $helper, + private SongRepository $songRepository, + private Cache $cache, + private Finder $finder ) { - $this->getID3 = $getID3; - $this->mediaMetadataService = $mediaMetadataService; - $this->helper = $helper; - $this->songRepository = $songRepository; - $this->cache = $cache; - $this->finder = $finder; } - /** @param string|SplFileInfo $path */ - public function setFile($path): self + public function setFile(string|SplFileInfo $path): self { - $splFileInfo = null; $splFileInfo = $path instanceof SplFileInfo ? $path : new SplFileInfo($path); - // Workaround for #344, where getMTime() fails for certain files with Unicode names on Windows. - try { - $this->fileModifiedTime = $splFileInfo->getMTime(); - } catch (Throwable $e) { - // Not worth logging the error. Just use current stamp for mtime. - $this->fileModifiedTime = time(); - } - $this->filePath = $splFileInfo->getPathname(); $this->fileHash = $this->helper->getFileHash($this->filePath); $this->song = $this->songRepository->getOneById($this->fileHash); // @phpstan-ignore-line @@ -80,121 +58,55 @@ class FileSynchronizer return $this; } - /** - * Get all applicable info from the file. - * - * @return array - */ - public function getFileInfo(): array + public function getFileInfo(): ?SongScanInformation { $info = $this->getID3->analyze($this->filePath); if (isset($info['error']) || !isset($info['playtime_seconds'])) { $this->syncError = isset($info['error']) ? $info['error'][0] : 'No playtime found'; - return []; + return null; } - // Copy the available tags over to comment. - // This is a helper from getID3, though it doesn't really work well. - // We'll still prefer getting ID3v2 tags directly later. - getid3_lib::CopyTagsToComments($info); - - $props = [ - 'artist' => '', - 'album' => '', - 'albumartist' => '', - 'compilation' => false, - 'title' => basename($this->filePath, '.' . pathinfo($this->filePath, PATHINFO_EXTENSION)), - 'length' => $info['playtime_seconds'], - 'track' => $this->getTrackNumberFromInfo($info), - 'disc' => (int) array_get($info, 'comments.part_of_a_set.0', 1), - 'lyrics' => '', - 'cover' => array_get($info, 'comments.picture', [null])[0], - 'path' => $this->filePath, - 'mtime' => $this->fileModifiedTime, - ]; - - $comments = array_get($info, 'comments_html'); - - if (!$comments) { - return $props; - } - - $this->gatherPropsFromTags($info, $comments, $props); - $props['compilation'] = (bool) $props['compilation'] || $this->isCompilation($props); - - return $props; + return SongScanInformation::fromGetId3Info($info); } /** - * Sync the song with all available media info against the database. + * Sync the song with all available media info into the database. * - * @param array $tags The (selective) tags to sync (if the song exists) + * @param array $excludes The tags to exclude (only taken into account if the song already exists) * @param bool $force Whether to force syncing, even if the file is unchanged */ - public function sync(array $tags, bool $force = false): int + public function sync(array $excludes = [], bool $force = false): int { if (!$this->isFileNewOrChanged() && !$force) { return self::SYNC_RESULT_UNMODIFIED; } - $info = $this->getFileInfo(); + $info = $this->getFileInfo()?->toArray(); if (!$info) { return self::SYNC_RESULT_BAD_FILE; } - // Fixes #366. If the file is new, we use all tags by simply setting $force to false. - if ($this->isFileNew()) { - $force = false; + if (!$this->isFileNew()) { + foreach ($excludes as $tag) { + unset($info[$tag]); + } } - if ($this->isFileChanged() || $force) { - // This is a changed file, or the user is forcing updates. - // In such a case, the user must have specified a list of tags to sync. - // A sample command could be: ./artisan koel:sync --force --tags=artist,album,lyrics - // We cater for these tags by removing those not specified. - - // There's a special case with 'album' though. - // If 'compilation' tag is specified, 'album' must be counted in as well. - // But if 'album' isn't specified, we don't want to update normal albums. - // This variable is to keep track of this state. - $changeCompilationAlbumOnly = false; - - if (in_array('compilation', $tags, true) && !in_array('album', $tags, true)) { - $tags[] = 'album'; - $changeCompilationAlbumOnly = true; - } - - $info = array_intersect_key($info, array_flip($tags)); - - // If the "artist" tag is specified, use it. - // Otherwise, re-use the existing model value. - $artist = isset($info['artist']) ? Artist::getOrCreate($info['artist']) : $this->song->album->artist; - - // If the "album" tag is specified, use it. - // Otherwise, re-use the existing model value. - if (isset($info['album'])) { - $album = $changeCompilationAlbumOnly - ? $this->song->album - : Album::getOrCreate($artist, $info['album'], array_get($info, 'compilation')); - } else { - $album = $this->song->album; - } - } else { - // The file is newly added. - $artist = Artist::getOrCreate($info['artist']); - $album = Album::getOrCreate($artist, $info['album'], array_get($info, 'compilation')); - } + $artist = Arr::get($info, 'artist') ? Artist::getOrCreate($info['artist']) : $this->song->artist; + $albumArtist = Arr::get($info, 'albumartist') ? Artist::getOrCreate($info['albumartist']) : $artist; + $album = Arr::get($info, 'album') ? Album::getOrCreate($albumArtist, $info['album']) : $this->song->album; if (!$album->has_cover) { - $this->generateAlbumCover($album, array_get($info, 'cover')); + $this->tryGenerateAlbumCover($album, Arr::get($info, 'cover', [])); } - $data = array_except($info, ['artist', 'albumartist', 'album', 'cover', 'compilation']); + $data = Arr::except($info, ['album', 'artist', 'albumartist', 'cover']); $data['album_id'] = $album->id; $data['artist_id'] = $artist->id; + $this->song = Song::updateOrCreate(['id' => $this->fileHash], $data); return self::SYNC_RESULT_SUCCESS; @@ -205,24 +117,27 @@ class FileSynchronizer * * @param array|null $coverData */ - private function generateAlbumCover(Album $album, ?array $coverData): void + private function tryGenerateAlbumCover(Album $album, ?array $coverData): void { - // If the album has no cover, we try to get the cover image from existing tag data - if ($coverData) { - $extension = explode('/', $coverData['image_mime']); - $extension = $extension[1] ?? 'png'; + try { + // If the album has no cover, we try to get the cover image from existing tag data + if ($coverData) { + $extension = explode('/', $coverData['image_mime']); + $extension = $extension[1] ?? 'png'; - $this->mediaMetadataService->writeAlbumCover($album, $coverData['data'], $extension); + $this->mediaMetadataService->writeAlbumCover($album, $coverData['data'], $extension); - return; - } + return; + } - // Or, if there's a cover image under the same directory, use it. - $cover = $this->getCoverFileUnderSameDirectory(); + // Or, if there's a cover image under the same directory, use it. + $cover = $this->getCoverFileUnderSameDirectory(); - if ($cover) { - $extension = pathinfo($cover, PATHINFO_EXTENSION); - $this->mediaMetadataService->writeAlbumCover($album, file_get_contents($cover), $extension); + if ($cover) { + $extension = pathinfo($cover, PATHINFO_EXTENSION); + $this->mediaMetadataService->writeAlbumCover($album, file_get_contents($cover), $extension); + } + } catch (Throwable) { } } @@ -230,8 +145,6 @@ class FileSynchronizer * Issue #380. * Some albums have its own cover image under the same directory as cover|folder.jpg/png. * We'll check if such a cover file is found, and use it if positive. - * - * @throws InvalidArgumentException */ private function getCoverFileUnderSameDirectory(): ?string { @@ -251,15 +164,15 @@ class FileSynchronizer $cover = $matches ? $matches[0] : null; - return $cover && $this->isImage($cover) ? $cover : null; + return $cover && static::isImage($cover) ? $cover : null; }); } - private function isImage(string $path): bool + private static function isImage(string $path): bool { try { return (bool) exif_imagetype($path); - } catch (Throwable $e) { + } catch (Throwable) { return false; } } @@ -290,60 +203,6 @@ class FileSynchronizer return $this->syncError; } - private function getTrackNumberFromInfo(array $info): int - { - $track = 0; - - // Apparently track numbers can be stored with different indices as the following. - $trackIndices = [ - 'comments.track', - 'comments.tracknumber', - 'comments.track_number', - ]; - - for ($i = 0; $i < count($trackIndices) && $track === 0; ++$i) { - $track = (int) array_get($info, $trackIndices[$i], [0])[0]; - } - - return $track; - } - - private function gatherPropsFromTags(array $info, array $comments, array &$props): void - { - $propertyMap = [ - 'artist' => 'artist', - 'albumartist' => 'band', - 'album' => 'album', - 'title' => 'title', - 'lyrics' => ['unsychronised_lyric', 'unsynchronised_lyric'], - 'compilation' => 'part_of_a_compilation', - ]; - - foreach ($propertyMap as $name => $tags) { - foreach ((array) $tags as $tag) { - $value = array_get($info, "tags.id3v2.$tag", [null])[0] ?: array_get($comments, $tag, [''])[0]; - - if ($value) { - $props[$name] = $value; - } - } - - // Fixes #323, where tag names can be htmlentities()'ed - if (is_string($props[$name]) && $props[$name]) { - $props[$name] = trim(html_entity_decode($props[$name])); - } - } - } - - private function isCompilation(array $props): bool - { - // A "compilation" property can be determined by: - // - "part_of_a_compilation" tag (used by iTunes), or - // - "albumartist" (used by non-retarded applications). - // Also, the latter is only valid if the value is NOT the same as "artist". - return $props['albumartist'] && $props['artist'] !== $props['albumartist']; - } - public function getSong(): ?Song { return $this->song; diff --git a/app/Services/MediaSyncService.php b/app/Services/MediaSyncService.php index 659d7928..7f3972c7 100644 --- a/app/Services/MediaSyncService.php +++ b/app/Services/MediaSyncService.php @@ -16,23 +16,6 @@ use Symfony\Component\Finder\Finder; class MediaSyncService { - /** - * All applicable tags in a media file that we cater for. - * Note that each isn't necessarily a valid ID3 tag name. - */ - public const APPLICABLE_TAGS = [ - 'artist', - 'album', - 'title', - 'length', - 'track', - 'disc', - 'lyrics', - 'cover', - 'mtime', - 'compilation', - ]; - public function __construct( private SongRepository $songRepository, private FileSynchronizer $fileSynchronizer, @@ -42,14 +25,7 @@ class MediaSyncService } /** - * Tags to be synced. - */ - protected array $tags = []; - - /** - * Sync the media. Oh sync the media. - * - * @param array $tags The tags to sync. + * @param array $excludes The tags to exclude. * Only taken into account for existing records. * New records will have all tags synced in regardless. * @param bool $force Whether to force syncing even unchanged files @@ -57,23 +33,18 @@ class MediaSyncService */ public function sync( ?string $mediaPath = null, - array $tags = [], + array $excludes = [], bool $force = false, ?SyncCommand $syncCommand = null ): void { $this->setSystemRequirements(); - $this->setTags($tags); $syncResult = SyncResult::init(); - $songPaths = $this->gatherFiles($mediaPath ?: Setting::get('media_path')); - - if ($syncCommand) { - $syncCommand->createProgressBar(count($songPaths)); - } + $syncCommand?->createProgressBar(count($songPaths)); foreach ($songPaths as $path) { - $result = $this->fileSynchronizer->setFile($path)->sync($this->tags, $force); + $result = $this->fileSynchronizer->setFile($path)->sync($excludes, $force); switch ($result) { case FileSynchronizer::SYNC_RESULT_SUCCESS: @@ -113,7 +84,7 @@ class MediaSyncService return iterator_to_array( $this->finder->create() ->ignoreUnreadableDirs() - ->ignoreDotFiles((bool) config('koel.ignore_dot_files')) // https://github.com/phanan/koel/issues/450 + ->ignoreDotFiles((bool) config('koel.ignore_dot_files')) // https://github.com/koel/koel/issues/450 ->files() ->followLinks() ->name('/\.(mp3|ogg|m4a|flac)$/i') @@ -151,23 +122,6 @@ class MediaSyncService } } - /** - * Construct an array of tags to be synced into the database from an input array of tags. - * If the input array is empty or contains only invalid items, we use all tags. - * Otherwise, we only use the valid items in it. - * - * @param array $tags - */ - public function setTags(array $tags = []): void - { - $this->tags = array_intersect($tags, self::APPLICABLE_TAGS) ?: self::APPLICABLE_TAGS; - - // We always keep track of mtime. - if (!in_array('mtime', $this->tags, true)) { - $this->tags[] = 'mtime'; - } - } - private function setSystemRequirements(): void { if (!app()->runningInConsole()) { @@ -194,7 +148,7 @@ class MediaSyncService private function handleNewOrModifiedFileRecord(string $path): void { - $result = $this->fileSynchronizer->setFile($path)->sync($this->tags); + $result = $this->fileSynchronizer->setFile($path)->sync(); if ($result === FileSynchronizer::SYNC_RESULT_SUCCESS) { $this->logger->info("Synchronized $path"); @@ -221,7 +175,7 @@ class MediaSyncService private function handleNewOrModifiedDirectoryRecord(string $path): void { foreach ($this->gatherFiles($path) as $file) { - $this->fileSynchronizer->setFile($file)->sync($this->tags); + $this->fileSynchronizer->setFile($file)->sync(); } $this->logger->info("Synced all song(s) under $path"); diff --git a/app/Services/UploadService.php b/app/Services/UploadService.php index 2af76c95..98d0e243 100644 --- a/app/Services/UploadService.php +++ b/app/Services/UploadService.php @@ -24,8 +24,7 @@ class UploadService $file->move($this->getUploadDirectory(), $targetFileName); $targetPathName = $this->getUploadDirectory() . $targetFileName; - $this->fileSynchronizer->setFile($targetPathName); - $result = $this->fileSynchronizer->sync(MediaSyncService::APPLICABLE_TAGS); + $result = $this->fileSynchronizer->setFile($targetPathName)->sync(); if ($result !== FileSynchronizer::SYNC_RESULT_SUCCESS) { @unlink($targetPathName); diff --git a/app/Values/SongScanInformation.php b/app/Values/SongScanInformation.php new file mode 100644 index 00000000..1b1e9a01 --- /dev/null +++ b/app/Values/SongScanInformation.php @@ -0,0 +1,114 @@ +getMTime(); + } catch (Throwable) { + // Just use current stamp for mtime. + return time(); + } + } + + /** @return array */ + public function toArray(): array + { + return [ + 'title' => $this->title, + 'album' => $this->albumName, + 'artist' => $this->artistName, + 'albumartist' => $this->albumArtistName, + 'track' => $this->track, + 'disc' => $this->disc, + 'lyrics' => $this->lyrics, + 'length' => $this->length, + 'cover' => $this->cover, + 'path' => $this->path, + 'mtime' => $this->mTime, + ]; + } +} diff --git a/app/Values/SyncResult.php b/app/Values/SyncResult.php index e3ded592..76811261 100644 --- a/app/Values/SyncResult.php +++ b/app/Values/SyncResult.php @@ -6,20 +6,8 @@ use Illuminate\Support\Collection; final class SyncResult { - /** @var Collection|array */ - public Collection $success; - - /** @var Collection|array */ - public Collection $bad; - - /** @var Collection|array */ - public Collection $unmodified; - - private function __construct(Collection $success, Collection $bad, Collection $unmodified) + private function __construct(public Collection $success, public Collection $bad, public Collection $unmodified) { - $this->success = $success; - $this->bad = $bad; - $this->unmodified = $unmodified; } public static function init(): self diff --git a/public/.gitignore b/public/.gitignore index 6cc61692..da39ce39 100644 --- a/public/.gitignore +++ b/public/.gitignore @@ -3,9 +3,9 @@ fonts # Ignore all (generated) images under img, but keep the folder structure img/* -!img/albums -img/albums/* -!img/albums/.gitkeep +!img/covers +img/covers/* +!img/covers/.gitkeep !img/artists img/artists/* !img/artists/.gitkeep diff --git a/public/img/albums/.gitkeep b/public/img/covers/.gitkeep similarity index 100% rename from public/img/albums/.gitkeep rename to public/img/covers/.gitkeep diff --git a/tests/Unit/Services/UploadServiceTest.php b/tests/Unit/Services/UploadServiceTest.php index 9b732ac0..79948862 100644 --- a/tests/Unit/Services/UploadServiceTest.php +++ b/tests/Unit/Services/UploadServiceTest.php @@ -7,7 +7,6 @@ use App\Exceptions\SongUploadFailedException; use App\Models\Setting; use App\Models\Song; use App\Services\FileSynchronizer; -use App\Services\MediaSyncService; use App\Services\UploadService; use Illuminate\Http\UploadedFile; use Mockery; @@ -56,7 +55,7 @@ class UploadServiceTest extends TestCase $this->fileSynchronizer ->shouldReceive('sync') ->once() - ->with(MediaSyncService::APPLICABLE_TAGS) + ->with() ->andReturn(FileSynchronizer::SYNC_RESULT_BAD_FILE); $this->fileSynchronizer @@ -91,7 +90,7 @@ class UploadServiceTest extends TestCase $this->fileSynchronizer ->shouldReceive('sync') ->once() - ->with(MediaSyncService::APPLICABLE_TAGS) + ->with() ->andReturn(FileSynchronizer::SYNC_RESULT_SUCCESS); $song = new Song();