From 53dd292b5570b150dbdc538d7618eb02e4d0b28e Mon Sep 17 00:00:00 2001 From: Ritiek Malhotra Date: Wed, 2 Jan 2019 18:54:27 +0000 Subject: [PATCH] Fix conversion conflicts when both input and output filenames are same (#459) * Workaround conversion conflicts by appending '.temp' to input filename * Fix tests * Add a test and some minor changes * Update CHANGES.md --- .gitignore | 4 +- CHANGES.md | 1 + spotdl/convert.py | 81 ++++++++++++++++++++--------- spotdl/downloader.py | 2 - spotdl/handle.py | 3 ++ test/test_download_with_metadata.py | 15 +++++- 6 files changed, 78 insertions(+), 28 deletions(-) diff --git a/.gitignore b/.gitignore index df54316..225ea5c 100755 --- a/.gitignore +++ b/.gitignore @@ -1,8 +1,10 @@ +# Spotdl generated files +*.m4a +*.mp3 config.yml Music/ *.txt *.m3u -upload.sh .pytest_cache/ diff --git a/CHANGES.md b/CHANGES.md index 47fc6be..2df60ba 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Refactored core downloading module ([@ritiek](https://github.com/ritiek)) (#410) ### Fixed +- Workaround conversion conflicts when input and output filename are same ([@ritiek](https://github.com/ritiek)) (#459) - Applied a check on result in case of search using Spotify-URL `--no-metadata` option ([@Amit-L](https://github.com/Amit-L)) (#452) - Included a missing `import spotipy` in downloader.py ([@ritiek](https://github.com/ritiek)) (#440) diff --git a/spotdl/convert.py b/spotdl/convert.py index e6e688e..5859c14 100644 --- a/spotdl/convert.py +++ b/spotdl/convert.py @@ -18,25 +18,46 @@ https://trac.ffmpeg.org/wiki/Encode/AAC def song(input_song, output_song, folder, avconv=False, trim_silence=False): """ Do the audio format conversion. """ - convert = Converter(input_song, output_song, folder, trim_silence) + if avconv and trim_silence: + raise ValueError("avconv does not support trim_silence") + if not input_song == output_song: log.info("Converting {0} to {1}".format(input_song, output_song.split(".")[-1])) elif input_song.endswith(".m4a"): log.info('Correcting container in "{}"'.format(input_song)) else: return 0 + + convert = Converter(input_song, output_song, folder, delete_original=True) if avconv: exit_code, command = convert.with_avconv() else: - exit_code, command = convert.with_ffmpeg() + exit_code, command = convert.with_ffmpeg(trim_silence=trim_silence) return exit_code, command class Converter: - def __init__(self, input_song, output_song, folder, trim_silence=False): - self.input_file = os.path.join(folder, input_song) + def __init__(self, input_song, output_song, folder, delete_original): + _, self.input_ext = os.path.splitext(input_song) + _, self.output_ext = os.path.splitext(output_song) + self.output_file = os.path.join(folder, output_song) - self.trim_silence = trim_silence + rename_to_temp = False + + same_file = os.path.abspath(input_song) == os.path.abspath(output_song) + if same_file: + # FFmpeg/avconv cannot have the same file for both input and output + # This would happen when the extensions are same, so rename + # the input track to append ".temp" + log.debug('Input file and output file are going will be same during encoding, will append ".temp" to input file just before starting encoding to avoid conflict') + input_song = output_song + ".temp" + rename_to_temp = True + delete_original = True + + self.input_file = os.path.join(folder, input_song) + + self.rename_to_temp = rename_to_temp + self.delete_original = delete_original def with_avconv(self): if log.level == 10: @@ -56,45 +77,48 @@ class Converter: "-y", ] - if self.trim_silence: - log.warning("--trim-silence not supported with avconv") + if self.rename_to_temp: + os.rename(self.output_file, self.input_file) log.debug(command) - return subprocess.call(command), command + code = subprocess.call(command) - def with_ffmpeg(self): + if self.delete_original: + log.debug('Removing original file: "{}"'.format(self.input_file)) + os.remove(self.input_file) + + return code, command + + def with_ffmpeg(self, trim_silence=False): ffmpeg_pre = "ffmpeg -y " if not log.level == 10: ffmpeg_pre += "-hide_banner -nostats -v panic " - _, input_ext = os.path.splitext(self.input_file) - _, output_ext = os.path.splitext(self.output_file) - ffmpeg_params = "" - if input_ext == ".m4a": - if output_ext == ".mp3": + if self.input_ext == ".m4a": + if self.output_ext == ".mp3": ffmpeg_params = "-codec:v copy -codec:a libmp3lame -ar 44100 " - elif output_ext == ".webm": + elif self.output_ext == ".webm": ffmpeg_params = "-codec:a libopus -vbr on " - elif output_ext == ".m4a": - ffmpeg_params = "-vn -acodec copy " + elif self.output_ext == ".m4a": + ffmpeg_params = "-acodec copy " - elif input_ext == ".webm": - if output_ext == ".mp3": + elif self.input_ext == ".webm": + if self.output_ext == ".mp3": ffmpeg_params = "-codec:a libmp3lame -ar 44100 " - elif output_ext == ".m4a": + elif self.output_ext == ".m4a": ffmpeg_params = "-cutoff 20000 -codec:a aac -ar 44100 " - if output_ext == ".flac": + if self.output_ext == ".flac": ffmpeg_params = "-codec:a flac -ar 44100 " # add common params for any of the above combination ffmpeg_params += "-b:a 192k -vn " - ffmpeg_pre += " -i" + ffmpeg_pre += "-i " - if self.trim_silence: + if trim_silence: ffmpeg_params += "-af silenceremove=start_periods=1 " command = ( @@ -104,5 +128,14 @@ class Converter: + [self.output_file] ) + if self.rename_to_temp: + os.rename(self.output_file, self.input_file) + log.debug(command) - return subprocess.call(command), command + code = subprocess.call(command) + + if self.delete_original: + log.debug('Removing original file: "{}"'.format(self.input_file)) + os.remove(self.input_file) + + return code, command diff --git a/spotdl/downloader.py b/spotdl/downloader.py index 608a5c3..00dc311 100644 --- a/spotdl/downloader.py +++ b/spotdl/downloader.py @@ -136,8 +136,6 @@ class Downloader: except FileNotFoundError: output_song = self.unconverted_filename(songname) - if not const.args.input_ext == const.args.output_ext: - os.remove(os.path.join(const.args.folder, input_song)) if not const.args.no_metadata and self.meta_tags is not None: metadata.embed( os.path.join(const.args.folder, output_song), self.meta_tags diff --git a/spotdl/handle.py b/spotdl/handle.py index e158703..676e695 100644 --- a/spotdl/handle.py +++ b/spotdl/handle.py @@ -273,6 +273,9 @@ def get_arguments(raw_args=None, to_group=True, to_merge=True): if parsed.write_m3u and not parsed.list: parser.error('--write-m3u can only be used with --list') + if parsed.avconv and parsed.trim_silence: + parser.error("--trim-silence can only be used with FFmpeg") + parsed.log_level = log_leveller(parsed.log_level) return parsed diff --git a/test/test_download_with_metadata.py b/test/test_download_with_metadata.py index a5ab54e..b68677a 100644 --- a/test/test_download_with_metadata.py +++ b/test/test_download_with_metadata.py @@ -112,6 +112,7 @@ class TestDownload: class TestFFmpeg: def test_convert_from_webm_to_mp3(self, filename_fixture, monkeypatch): + monkeypatch.setattr("os.remove", lambda x: None) expect_command = "ffmpeg -y -i {0}.webm -codec:a libmp3lame -ar 44100 -b:a 192k -vn {0}.mp3".format(os.path.join(const.args.folder, filename_fixture)) _, command = convert.song( filename_fixture + ".webm", filename_fixture + ".mp3", const.args.folder @@ -119,6 +120,7 @@ class TestFFmpeg: assert ' '.join(command) == expect_command def test_convert_from_webm_to_m4a(self, filename_fixture, monkeypatch): + monkeypatch.setattr("os.remove", lambda x: None) expect_command = "ffmpeg -y -i {0}.webm -cutoff 20000 -codec:a aac -ar 44100 -b:a 192k -vn {0}.m4a".format(os.path.join(const.args.folder, filename_fixture)) _, command = convert.song( filename_fixture + ".webm", filename_fixture + ".m4a", const.args.folder @@ -126,6 +128,7 @@ class TestFFmpeg: assert ' '.join(command) == expect_command def test_convert_from_m4a_to_mp3(self, filename_fixture, monkeypatch): + monkeypatch.setattr("os.remove", lambda x: None) expect_command = "ffmpeg -y -i {0}.m4a -codec:v copy -codec:a libmp3lame -ar 44100 -b:a 192k -vn {0}.mp3".format(os.path.join(const.args.folder, filename_fixture)) _, command = convert.song( filename_fixture + ".m4a", filename_fixture + ".mp3", const.args.folder @@ -133,6 +136,7 @@ class TestFFmpeg: assert ' '.join(command) == expect_command def test_convert_from_m4a_to_webm(self, filename_fixture, monkeypatch): + monkeypatch.setattr("os.remove", lambda x: None) expect_command = "ffmpeg -y -i {0}.m4a -codec:a libopus -vbr on -b:a 192k -vn {0}.webm".format(os.path.join(const.args.folder, filename_fixture)) _, command = convert.song( filename_fixture + ".m4a", filename_fixture + ".webm", const.args.folder @@ -140,15 +144,24 @@ class TestFFmpeg: assert ' '.join(command) == expect_command def test_convert_from_m4a_to_flac(self, filename_fixture, monkeypatch): + monkeypatch.setattr("os.remove", lambda x: None) expect_command = "ffmpeg -y -i {0}.m4a -codec:a flac -ar 44100 -b:a 192k -vn {0}.flac".format(os.path.join(const.args.folder, filename_fixture)) _, command = convert.song( filename_fixture + ".m4a", filename_fixture + ".flac", const.args.folder ) assert ' '.join(command) == expect_command + def test_correct_container_for_m4a(self, filename_fixture, monkeypatch): + expect_command = "ffmpeg -y -i {0}.m4a.temp -acodec copy -b:a 192k -vn {0}.m4a".format(os.path.join(const.args.folder, filename_fixture)) + _, command = convert.song( + filename_fixture + ".m4a", filename_fixture + ".m4a", const.args.folder + ) + assert ' '.join(command) == expect_command + class TestAvconv: - def test_convert_from_m4a_to_mp3(self, filename_fixture): + def test_convert_from_m4a_to_mp3(self, filename_fixture, monkeypatch): + monkeypatch.setattr("os.remove", lambda x: None) expect_command = "avconv -loglevel debug -i {0}.m4a -ab 192k {0}.mp3 -y".format(os.path.join(const.args.folder, filename_fixture)) _, command = convert.song( filename_fixture + ".m4a", filename_fixture + ".mp3", const.args.folder, avconv=True