make sure to concat files of the same format

TODO.md

@@ -2,6 +2,7 @@
 
 ## Code
 
+* if mapping is already imported, mark all recordings as imported, too. this may happen if one recording is not ready at the import run and provides its file only later. too bad, then just discard this file.
 * create script to import partial broadcasts
 * reduce memory usage of import
 * create script to adjust playback formats to update old playback formats to the current ones

app/services/audio_processor/ffmpeg.rb

@@ -30,6 +30,7 @@ def trim(new_path, start, duration)
 
     def concat(new_path, other_paths)
       assert_directory(new_path)
+      assert_same_codecs(other_paths)
       list_file = Tempfile.new('list')
       begin
         create_list_file(list_file, [audio.path, *other_paths])
@@ -66,6 +67,14 @@ def duration
       @duration ||= accurate_duration
     end
 
+    def audio_format
+      AudioFormat.new(codec, bitrate || 1, channels)
+    end
+
+    def file_extension
+      ::File.extname(audio.path)[1..-1]
+    end
+
     private
 
     def audio
@@ -86,8 +95,10 @@ def create_list_file(file, paths)
     end
 
     def concat_audio(new_path, list_file)
+      # flacs are not concated correctly when using copy codec
+      concat_codec = codec == 'flac' ? 'flac' : 'copy'
       run_command(FFMPEG.ffmpeg_binary, '-y', '-f', 'concat', '-safe', '0', '-i',
-                  list_file.path, '-c', 'copy', new_path)
+                  list_file.path, '-c', concat_codec, new_path)
     end
 
     def accurate_duration
@@ -129,12 +140,21 @@ def codec_options(audio_format)
     def same_format?(audio_format)
       audio_format.codec == codec &&
         audio_format.channels == channels &&
-        (audio_format.encoding.lossless? || audio_format.bitrate == bitrate)
+        (audio_format.encoding.lossless? || audio_format.bitrate == bitrate) &&
+        audio_format.file_extension == file_extension
     end
 
     def assert_directory(file)
       FileUtils.mkdir_p(File.dirname(file))
     end
 
+    def assert_same_codecs(files)
+      extensions = ([audio.path] + files).map { |f| ::File.extname(f) }.uniq
+      if extensions.size > 1
+        raise ArgumentError,
+              "Cannot concat files with different extensions (#{extensions.join(', ')})"
+      end
+    end
+
   end
 end

app/services/import/recording/chooser.rb

@@ -3,6 +3,10 @@ module Recording
     # Compares an array of audio files and returns the best one.
     class Chooser
 
+      # Deal with inaccurate duration measurements by reducing
+      # the duration value granularity.
+      DURATION_TOLERANCE = 5.seconds
+
       attr_reader :variants
 
       def initialize(variants)
@@ -18,7 +22,7 @@ def by_audio_length
         # i.e. positions remain the same if the duration is equal.
         variants.sort_by.with_index do |v, i|
           duration = v.audio_duration > v.duration ? v.duration : v.audio_duration
-          [-duration, i]
+          [-(duration / DURATION_TOLERANCE.to_f).round, i]
         end
       end
 

app/services/import/recording/composer.rb

@@ -118,7 +118,7 @@ def trim_available(recording, start, duration)
       end
 
       def trim(file, start, duration)
-        new_tempfile.tap do |target_file|
+        new_tempfile(file).tap do |target_file|
           proc = AudioProcessor.new(file)
           proc.trim(target_file.path, start, duration)
         end
@@ -127,14 +127,41 @@ def trim(file, start, duration)
       def concat(list)
         return list.first if list.size <= 1
 
-        new_tempfile.tap do |target_file|
-          proc = AudioProcessor.new(list[0].path)
-          proc.concat(target_file.path, list[1..-1].collect(&:path))
+        with_same_format(list) do |unified|
+          new_tempfile(unified[0]).tap do |target_file|
+            proc = AudioProcessor.new(unified[0])
+            proc.concat(target_file.path, unified[1..-1])
+          end
         end
       end
 
-      def new_tempfile
-        Tempfile.new(['master', ::File.extname(first.path)])
+      def with_same_format(list)
+        unified = convert_all_to_same_format(list)
+        yield unified.collect(&:path)
+      ensure
+        unified && unified.each { |file| file.close! if file.respond_to?(:close!) }
+      end
+
+      def convert_all_to_same_format(list)
+        format = AudioProcessor.new(list.first.path).audio_format
+        list.map do |file|
+          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)
+        end
+      end
+
+      def new_tempfile(template = first.path)
+        Tempfile.new(['master', ::File.extname(template)])
       end
 
     end

test/services/audio_processor/ffmpeg_test.rb

@@ -105,6 +105,38 @@ class AudioProcessor::FfmpegTest < ActiveSupport::TestCase
       end
     end
 
+    test 'concats flac files' do
+      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)
+        AudioProcessor::Ffmpeg.new(flac1).concat(file.path, [flac2, flac3])
+
+        merge = FFMPEG::Movie.new(file.path)
+        assert_equal 9, merge.duration.round
+        assert_equal 'flac', merge.audio_codec
+      ensure
+        file.close!
+      end
+    end
+
+    test 'fails to concat different files files' do
+      file = Tempfile.new(['merge', '.flac'])
+      begin
+        format = AudioFormat.new('flac', nil, 2)
+        flac1 = AudioGenerator.new.silent_source_file(format)
+        mp31 = audio_files(:g9s_mai_high).absolute_path
+        flac3 = AudioGenerator.new.silent_source_file(format)
+        assert_raises(ArgumentError) do
+          AudioProcessor::Ffmpeg.new(flac1).concat(file.path, [mp31, flac3])
+        end
+      ensure
+        file.close!
+      end
+    end
+
     test 'converts flac to mp3' do
       mp3 = Tempfile.new(['output', '.mp3'])
 

test/services/import/recording/chooser_test.rb

@@ -11,15 +11,15 @@ class Import::Recording::ChooserTest < ActiveSupport::TestCase
        file1 = file('2016-01-01T235959+0200_001.mp3')
        AudioGenerator.new.silent_file(AudioFormat.new('mp3', 96, 1), file1, 2)
        file2 = file('2016-01-01T225959+0100_001.mp3')
-       AudioGenerator.new.silent_file(AudioFormat.new('mp3', 96, 1), file2, 4)
+       AudioGenerator.new.silent_file(AudioFormat.new('mp3', 96, 1), file2, 5)
        variants = [file1, file2].collect { |f| Import::Recording::File.new(f) }
        best = Import::Recording::Chooser.new(variants).best
        assert_equal file2, best.path
      end
 
-     test '#best returns the first file if audio lengths are equal' do
+     test '#best returns the first file if audio lengths are equal within tolerance' do
        file1 = file('2016-01-01T235959+0200_002.mp3')
-       AudioGenerator.new.silent_file(AudioFormat.new('mp3', 96, 1), file1, 2)
+       AudioGenerator.new.silent_file(AudioFormat.new('mp3', 96, 1), file1, 1)
        file2 = file('2016-01-01T225959+0100_002.mp3')
        AudioGenerator.new.silent_file(AudioFormat.new('mp3', 96, 1), file2, 2)
        variants = [file1, file2].collect { |f| Import::Recording::File.new(f) }

test/services/import/recording/composer_test.rb

@@ -90,6 +90,21 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T210000+0200_060.mp3')
 
     expect_concat(2)
+    expect_audio_format('mp3', 320)
+    expect_duration(mapping.recordings.first.path, 30)
+    expect_duration(mapping.recordings.second.path, 30)
+    expect_duration(mapping.recordings.last.path, 60)
+    composer.compose
+  end
+
+  test 'returns merged recording with unified format' do
+    composer = build_composer('2013-06-12T200000+0200_030.flac',
+                              '2013-06-12T203000+0200_030.ogg',
+                              '2013-06-12T210000+0200_060.flac')
+
+    expect_concat(2)
+    expect_transcode
+    expect_audio_format('flac', 1)
     expect_duration(mapping.recordings.first.path, 30)
     expect_duration(mapping.recordings.second.path, 30)
     expect_duration(mapping.recordings.last.path, 60)
@@ -102,6 +117,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T210000+0200_060.mp3')
 
     expect_concat(2)
+    expect_audio_format('mp3', 320)
     expect_duration(mapping.recordings.first.path, 20)
     expect_duration(mapping.recordings.second.path, 20)
     expect_duration(mapping.recordings.last.path, 20)
@@ -114,6 +130,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T210000+0200_060.mp3')
 
     expect_concat(2)
+    expect_audio_format('mp3', 320)
     expect_trim(:first, 0, 30)
     expect_trim(:last, 00, 60)
     expect_duration(mapping.recordings.first.path, 40)
@@ -127,6 +144,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T210000+0200_060.mp3')
 
     expect_concat(1)
+    expect_audio_format('mp3', 320)
     expect_duration(mapping.recordings.last.path, 60)
     expect_trim(:first, 10, 60)
     expect_duration(mapping.recordings.first.path, 70)
@@ -138,6 +156,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T210000+0200_060.mp3')
 
     expect_concat(1)
+    expect_audio_format('mp3', 320)
     expect_duration(mapping.recordings.last.path, 60)
     expect_trim(:first, 10, 40)
     expect_duration(mapping.recordings.first.path, 50)
@@ -158,6 +177,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T210000+0200_065.mp3')
 
     expect_concat(1)
+    expect_audio_format('mp3', 320)
     expect_duration(mapping.recordings.first.path, 60)
     expect_trim(:last, 0, 60)
     expect_duration(mapping.recordings.last.path, 65)
@@ -170,6 +190,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T213000+0200_060.mp3')
 
     expect_concat(2)
+    expect_audio_format('mp3', 320)
     expect_duration(mapping.recordings.second.path, 60)
     expect_trim(:first, 30, 30)
     expect_trim(:last, 0, 30)
@@ -184,6 +205,7 @@ class Import::Recording::ComposerTest < ActiveSupport::TestCase
                               '2013-06-12T213000+0200_060.mp3')
 
     expect_concat(2)
+    expect_audio_format('mp3', 320)
     expect_duration(mapping.recordings.second.path, 40)
     expect_trim(:first, 30, 20)
     expect_trim(:last, 0, 20)
@@ -221,6 +243,18 @@ def expect_concat(file_count)
     AudioProcessor.expects(:new).with(instance_of(String)).returns(proc)
   end
 
+  def expect_audio_format(codec, bitrate = 1)
+    proc = mock('processor')
+    proc.expects(:audio_format).returns(AudioFormat.new(codec, bitrate, 2))
+    AudioProcessor.expects(:new).with(instance_of(String)).returns(proc)
+  end
+
+  def expect_transcode
+    proc = mock('processor')
+    proc.expects(:transcode).with(instance_of(String), instance_of(AudioFormat))
+    AudioProcessor.expects(:new).with(instance_of(String)).returns(proc)
+  end
+
   def mapping
     @mapping ||= Import::BroadcastMapping.new
   end