Retry transcoding with different frame size if it fails

app/services/audio_processor.rb

@@ -3,6 +3,11 @@
 # Interface for audio processors.
 module AudioProcessor
 
+  COMMON_FLAC_FRAME_SIZE = 1152
+
+  class FailingFrameSizeError < StandardError
+  end
+
   mattr_writer :klass
 
   def self.klass

app/services/audio_processor/ffmpeg.rb

@@ -13,19 +13,21 @@ class Ffmpeg < Base
                       album: :album,
                       year: :date }.freeze
 
-    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) && !flac
+      if same_format?(audio_format)
         transcode_preserving(new_path, custom: metadata_args(tags))
       else
-        transcode_format(new_path, audio_format, tags).tap do
-          assert_transcoded_with_same_duration(new_path) if flac
-        end
+        transcode_format(new_path, audio_format, tags)
+      end
+    end
+
+    def transcode_flac(new_path, audio_format, frame_size = AudioProcessor::COMMON_FLAC_FRAME_SIZE)
+      options = transcode_options(audio_format)
+      options[:custom] = ['-frame_size', frame_size]
+      audio.transcode(new_path, options).tap do
+        assert_transcoded_with_same_duration(new_path)
       end
     end
 
@@ -92,13 +94,13 @@ def transcode_preserving(new_path, options = {})
     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'
+      options = transcode_options(audio_format).merge(custom: metadata_args(tags))
       audio.transcode(new_path, options)
     end
 
-    def codec_options(audio_format)
+    def transcode_options(audio_format)
       options = {
+        validate: true,
         audio_codec: audio_format.codec,
         audio_channels: audio_format.channels
       }
@@ -170,11 +172,11 @@ def assert_same_codecs(files)
     # 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
+      return if (duration - transcoded_duration).abs < 1
 
-      raise FFMPEG::Error,
+      raise AudioProcessor::FailingFrameSizeError,
             "Transcoded file has duration #{transcoded_duration}, " \
-            "while original has #{duration} (#{file}})"
+            "while original has #{duration} (#{transcoded_file}})"
     end
 
   end

app/services/import/recording/composer.rb

@@ -13,6 +13,7 @@ module Recording
     class Composer
 
       include Loggable
+      MAX_TRANSCODE_RETRIES = 5
 
       attr_reader :mapping, :recordings
 
@@ -144,7 +145,7 @@ def trim_available(recording, start, duration)
 
       def trim(file, start, duration)
         inform("Trimming #{file} from #{start.round}s to #{(start + duration).round}s")
-        new_tempfile(file).tap do |target_file|
+        new_tempfile(::File.extname(file)).tap do |target_file|
           proc = AudioProcessor.new(file)
           proc.trim(target_file.path, start, duration)
         end
@@ -154,7 +155,7 @@ def concat(list)
         return list.first if list.size <= 1
 
         with_same_format(list) do |unified|
-          new_tempfile(unified[0]).tap do |target_file|
+          new_tempfile(::File.extname(unified[0])).tap do |target_file|
             proc = AudioProcessor.new(unified[0])
             proc.concat(target_file.path, unified[1..])
           end
@@ -165,30 +166,59 @@ def with_same_format(list)
         unified = convert_all_to_same_format(list)
         yield unified.map(&:path)
       ensure
-        unified&.each { |file| file.close! if file.respond_to?(:close!) }
+        close_files(unified) if unified
       end
 
       def convert_all_to_same_format(list)
         format = AudioProcessor.new(list.first.path).audio_format
+        if format.codec == 'flac'
+          # always convert flacs so they have the same frame size
+          convert_list_to_flac(list, format)
+        else
+          convert_list_to_format(list, format)
+        end
+      end
+
+      def convert_list_to_flac(list, format)
+        frame_size ||= AudioProcessor::COMMON_FLAC_FRAME_SIZE
+        converted = list.map { |file| convert_to_format(file, format) }
+      rescue AudioProcessor::FailingFrameSizeError
+        close_files(converted) if converted
+        frame_size += 1
+        max_frame_size = AudioProcessor::COMMON_FLAC_FRAME_SIZE + MAX_TRANSCODE_RETRIES
+        frame_size <= max_frame_size ? retry : raise
+      end
+
+      def convert_list_to_format(list, format)
         list.map do |file|
-          # always convert flacs to assert a common frame size
-          if ::File.extname(file.path) != ".#{format.file_extension}" || format.codec == 'flac'
-            convert_to_format(file, format)
-          else
+          if ::File.extname(file.path) == ".#{format.file_extension}"
             file
+          else
+            convert_to_format(file, format)
           end
         end
       end
 
       def convert_to_format(file, format)
-        Tempfile.new(['master', ".#{format.file_extension}"]).tap do |target_file|
-          proc = AudioProcessor.new(file.path)
-          proc.transcode(target_file.path, format)
+        processor = AudioProcessor.new(file.path)
+        new_tempfile(".#{format.file_extension}").tap do |target_file|
+          processor.transcode(target_file.path, format)
+        end
+      end
+
+      def convert_to_flac(file, format, frame_size)
+        processor = AudioProcessor.new(file.path)
+        new_tempfile(".#{format.file_extension}").tap do |target_file|
+          processor.transcode_flac(target_file.path, format, frame_size)
         end
       end
 
-      def new_tempfile(template = first.path)
-        Tempfile.new(['master', ::File.extname(template)])
+      def close_files(list)
+        list.each { |file| file.close! if file.respond_to?(:close!) }
+      end
+
+      def new_tempfile(extension)
+        Tempfile.new(['master', extension])
       end
 
     end

test/services/audio_processor/ffmpeg_test.rb

@@ -85,7 +85,7 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
       format = AudioFormat.new('flac', nil, 2)
       flac = silent_source_file(format)
       same = AudioProcessor::Ffmpeg.new(flac)
-                                   .transcode(file.path, AudioFormat.new('flac', nil, 2))
+                                   .transcode_flac(file.path, AudioFormat.new('flac', nil, 2))
       assert_equal 'flac', same.audio_codec
       assert_equal 2, same.audio_channels
     ensure
@@ -102,8 +102,8 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
       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)
+      assert_raises(AudioProcessor::FailingFrameSizeError) do
+        processor.transcode_flac(file.path, format)
       end
     ensure
       file.close!