Check for failed flac transcodings so that issues become visible

app/services/audio_processor/ffmpeg.rb

@@ -13,26 +13,25 @@ class Ffmpeg < Base
                       album: :album,
                       year: :date }.freeze
 
-    COMMON_FLAC_FRAME_SIZE = 1152
+    COMMON_FLAC_FRAME_SIZE = 1024
 
     def transcode(new_path, audio_format, tags = {})
       assert_directory(new_path)
+
+      flac = (audio_format.codec == 'flac')
       # always transcode flacs to assert a common frame size
-      if same_format?(audio_format) && audio_format.codec != 'flac'
-        preserving_transcode(new_path, custom: metadata_args(tags))
+      if same_format?(audio_format) && !flac
+        transcode_preserving(new_path, custom: metadata_args(tags))
       else
-        options = codec_options(audio_format).merge(validate: true)
-        options[:custom] ||= []
-        options[:custom].push(*metadata_args(tags))
-        audio.transcode(new_path, options)
+        transcode_format(new_path, audio_format, tags).tap do
+          assert_transcoded_with_same_duration(new_path) if flac
+        end
       end
     end
 
     def trim(new_path, start, duration)
       assert_directory(new_path)
-      preserving_transcode(new_path,
-                           seek_time: start,
-                           duration: duration)
+      transcode_preserving(new_path, seek_time: start, duration: duration)
     end
 
     def concat(new_path, other_paths)
@@ -50,7 +49,7 @@ def concat(new_path, other_paths)
     def tag(tags)
       work_file = Tempfile.new(['tagged', File.extname(file)])
       begin
-        preserving_transcode(work_file.path, custom: metadata_args(tags))
+        transcode_preserving(work_file.path, custom: metadata_args(tags))
         FileUtils.mv(work_file.path, file, force: true)
       ensure
         work_file.close!
@@ -84,14 +83,29 @@ def audio
       @audio ||= FFMPEG::Movie.new(file)
     end
 
-    def preserving_transcode(new_path, options = {})
+    def transcode_preserving(new_path, options = {})
       audio.transcode(new_path,
                       options.reverse_merge(
                         audio_codec: 'copy',
                         validate: true
                       ))
     end
 
+    def transcode_format(new_path, audio_format, tags)
+      options = codec_options(audio_format).merge(validate: true, custom: metadata_args(tags))
+      options[:custom].push('-frame_size', COMMON_FLAC_FRAME_SIZE) if audio_format.codec == 'flac'
+      audio.transcode(new_path, options)
+    end
+
+    def codec_options(audio_format)
+      options = {
+        audio_codec: audio_format.codec,
+        audio_channels: audio_format.channels
+      }
+      options[:audio_bitrate] = audio_format.bitrate unless audio_format.encoding.lossless?
+      options
+    end
+
     def create_list_file(file, paths)
       entries = paths.map { |p| "file '#{p}'" }
       File.write(file, entries.join("\n"))
@@ -107,7 +121,7 @@ def concat_audio(new_path, list_file)
     def accurate_duration
       out = run_command(FFMPEG.ffmpeg_binary, '-i', file, '-acodec', 'copy', '-f', 'null', '-')
       segments = out.scan(/\btime=(\d+):(\d\d):(\d\d(\.\d+)?)\b/)
-      raise("Could not determine duration for #{file}: #{out}") if segments.blank?
+      raise(FFMPEG::Error, "Could not determine duration for #{file}: #{out}") if segments.blank?
 
       number_of_seconds(segments.last)
     end
@@ -121,7 +135,10 @@ def number_of_seconds(segments)
     def run_command(*command)
       FFMPEG.logger.info("Running command...\n#{command.join(' ')}\n")
       out, status = Open3.capture2e(*command)
-      raise("#{command} failed with status #{status}:\n#{out}") unless status.success?
+      unless status.success?
+        raise(FFMPEG::Error,
+              "#{command} failed with status #{status}:\n#{out}")
+      end
 
       out
     end
@@ -132,17 +149,6 @@ def metadata_args(tags)
       end
     end
 
-    def codec_options(audio_format)
-      options = {
-        audio_codec: audio_format.codec,
-        audio_bitrate: audio_format.bitrate,
-        audio_channels: audio_format.channels
-      }
-      options.delete(:audio_bitrate) if audio_format.encoding.lossless?
-      options[:custom] = %W[-frame_size #{COMMON_FLAC_FRAME_SIZE}] if audio_format.codec == 'flac'
-      options
-    end
-
     def same_format?(audio_format)
       audio_format.codec == codec &&
         audio_format.channels == channels &&
@@ -161,6 +167,16 @@ def assert_same_codecs(files)
       end
     end
 
+    # Transcoding flacs crashes sometimes. Check durations to actually note those crashes.
+    def assert_transcoded_with_same_duration(transcoded_file)
+      transcoded_duration = self.class.new(transcoded_file).duration
+      return unless (duration - transcoded_duration).abs > 1
+
+      raise FFMPEG::Error,
+            "Transcoded file has duration #{transcoded_duration}, " \
+            "while original has #{duration} (#{file}})"
+    end
+
   end
 
 end

test/services/audio_processor/ffmpeg_test.rb

@@ -4,7 +4,9 @@
 
 class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
 
-  setup { AudioGenerator.new.silent_files_for_audio_files }
+  delegate :silent_file, :silent_source_file, to: :audio_generator
+
+  setup { audio_generator.silent_files_for_audio_files }
 
   test 'reads mp3 codec' do
     assert_equal 'mp3', processor(:klangbecken_mai1_best).codec
@@ -81,7 +83,7 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     file = Tempfile.new(['same', '.flac'])
     begin
       format = AudioFormat.new('flac', nil, 2)
-      flac = AudioGenerator.new.silent_source_file(format)
+      flac = silent_source_file(format)
       same = AudioProcessor::Ffmpeg.new(flac)
                                    .transcode(file.path, AudioFormat.new('flac', nil, 2))
       assert_equal 'flac', same.audio_codec
@@ -91,11 +93,28 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     end
   end
 
+  test 'transcodes same format flac file to common frame size but fails if ffmpeg crashes' do
+    file = Tempfile.new(['same', '.flac'])
+    FileUtils.rm(file.path)
+    begin
+      format = AudioFormat.new('flac', nil, 2)
+      flac = silent_source_file(format)
+      result = silent_file(format, file.path, 1)
+      processor = AudioProcessor::Ffmpeg.new(flac)
+      processor.send(:audio).stubs(:transcode).returns(result)
+      assert_raises(FFMPEG::Error) do
+        processor.transcode(file.path, format)
+      end
+    ensure
+      file.close!
+    end
+  end
+
   test 'converts flac to mp3' do
     mp3 = Tempfile.new(['output', '.mp3'])
 
     begin
-      flac = AudioGenerator.new.silent_source_file(AudioFormat.new('flac', nil, 2))
+      flac = silent_source_file(AudioFormat.new('flac', nil, 2))
       output = AudioProcessor::Ffmpeg.new(flac).transcode(mp3.path, AudioFormat.new('mp3', 56, 2))
       assert_equal 56_000, output.audio_bitrate
       assert_equal 2, output.audio_channels
@@ -109,7 +128,7 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     flac = Tempfile.new(['output', '.flac'])
 
     begin
-      mp3 = AudioGenerator.new.silent_source_file(AudioFormat.new('mp3', 320, 2))
+      mp3 = silent_source_file(AudioFormat.new('mp3', 320, 2))
       output = AudioProcessor::Ffmpeg.new(mp3).transcode(flac.path, AudioFormat.new('flac', 1, 2))
       assert_equal 2, output.audio_channels
       assert_equal 'flac', output.audio_codec
@@ -149,9 +168,9 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     file = Tempfile.new(['merge', '.flac'])
     begin
       format = AudioFormat.new('flac', nil, 2)
-      flac1 = AudioGenerator.new.silent_source_file(format)
-      flac2 = AudioGenerator.new.silent_source_file(format)
-      flac3 = AudioGenerator.new.silent_source_file(format)
+      flac1 = silent_source_file(format)
+      flac2 = silent_source_file(format)
+      flac3 = silent_source_file(format)
       AudioProcessor::Ffmpeg.new(flac1).concat(file.path, [flac2, flac3])
 
       merge = FFMPEG::Movie.new(file.path)
@@ -166,9 +185,9 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     file = Tempfile.new(['merge', '.flac'])
     begin
       format = AudioFormat.new('flac', nil, 2)
-      flac1 = AudioGenerator.new.silent_source_file(format)
+      flac1 = silent_source_file(format)
       mp31 = audio_files(:g9s_mai_high).absolute_path
-      flac3 = AudioGenerator.new.silent_source_file(format)
+      flac3 = silent_source_file(format)
       assert_raises(ArgumentError) do
         AudioProcessor::Ffmpeg.new(flac1).concat(file.path, [mp31, flac3])
       end
@@ -181,7 +200,7 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     mp3 = Tempfile.new(['source', '.mp3'])
     FileUtils.rm_f(mp3.path)
     begin
-      AudioGenerator.new.silent_file(AudioFormat.new('mp3', 192, 2), mp3.path)
+      silent_file(AudioFormat.new('mp3', 192, 2), mp3.path)
       p = AudioProcessor::Ffmpeg.new(mp3.path)
       p.tag(title: 'title "yeah!"', artist: 'artist', album: 'Albüm', year: '2016')
       tags = read_tags(mp3.path)
@@ -197,6 +216,10 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
     end
   end
 
+  def audio_generator
+    @audio_generator ||= AudioGenerator.new
+  end
+
   def processor(audio_file)
     AudioProcessor::Ffmpeg.new(audio_files(audio_file).absolute_path)
   end