Precache actual files instead of storing lengths (#644)

Co-authored-by: Charlotte Van Petegem <[email protected]>

README.md

@@ -62,8 +62,7 @@ you want.
 
     * FFMPEG_LOG_LOCATION
     * RAILS_STORAGE_PATH
-    * FFMPEG_VERSION_LOCATION
-    * RAILS_TRANSCODE_CACHE
+    * RAILS_TRANSCODES_PATH
     * BOOTSNAP_CACHE_DIR
     * PIDFILE
     * RAILS_LOG_TO_STDOUT

app/controllers/tracks_controller.rb

@@ -57,22 +57,15 @@ def audio
     raise ActiveRecord::RecordNotFound.new('codec_conversion does not exist', 'codec_conversion') if conversion.nil? && params[:codec_conversion_id].present?
 
     if conversion.present?
-      item = TranscodedItem.find_by(audio_file:, codec_conversion: conversion)
-      if item.present? && File.exist?(item.path)
-        item.update(last_used: Time.current)
-        audio_with_file(item.path, item.codec_conversion.resulting_codec.mimetype)
-      else
-        if item.present?
-          # Maybe the file was lost, maybe the transcode just hadn't finished
-          # yet. Anyway, doing the transcode again doesn't really hurt.
-          ConvertTranscodeJob.perform_later(item)
-        else
-          TranscodedItem.create(audio_file:, codec_conversion: conversion)
-        end
-        # AudioFile will only do the conversion if the `ContentLength` doesn't exist yet.
-        content_length = audio_file.calc_audio_length(conversion)
-        audio_with_stream(audio_file.convert(conversion), conversion.resulting_codec.mimetype, content_length.length)
+      transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion: conversion)
+      unless transcoded_item.present? && File.exist?(transcoded_item.path)
+        transcoded_item.destroy! if transcoded_item.present? # The file was lost. This shouldn't happen, so delete the item
+
+        # This does the conversion inline
+        CreateTranscodedItemJob.perform_now(audio_file, conversion)
+        transcoded_item = TranscodedItem.find_by(audio_file:, codec_conversion: conversion)
       end
+      audio_with_file(transcoded_item.path, transcoded_item.mimetype)
     else
       audio_with_file(audio_file.full_path, audio_file.codec.mimetype)
     end

app/jobs/create_transcoded_item_job.rb

@@ -0,0 +1,22 @@
+class CreateTranscodedItemJob < ApplicationJob
+  queue_as :within_30_seconds
+
+  def perform(audio_file, codec_conversion)
+    # Check that this transcoded_item was not created while this job was in the queue
+    return if TranscodedItem.find_by(audio_file:, codec_conversion:).present?
+
+    uuid = TranscodedItem.uuid_for(codec_conversion)
+    path = TranscodedItem.path_for(codec_conversion, uuid)
+    FileUtils.mkdir_p Pathname.new(path).parent
+    audio_file.convert(codec_conversion, path)
+
+    TranscodedItem.transaction do
+      # Check that the transcoded item was not created while we were executing
+      if TranscodedItem.find_by(audio_file:, codec_conversion:).present?
+        FileUtils.rm_f path
+      else
+        TranscodedItem.create!(audio_file:, codec_conversion:, uuid:)
+      end
+    end
+  end
+end

app/jobs/queue_transcoded_items_for_codec_conversion_job.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class QueueTranscodedItemsForCodecConversionJob < ApplicationJob
+  queue_as :within_30_seconds
+
+  def perform(codec_conversion)
+    AudioFile.find_in_batches do |batch|
+      jobs = batch.filter_map do |af|
+        CreateTranscodedItemJob.new(af, codec_conversion) if Rails.configuration.queue_create_transcoded_item_if.call(af)
+      end
+      ActiveJob.perform_all_later(jobs)
+    end
+  end
+end

app/models/audio_file.rb

@@ -18,7 +18,6 @@ class AudioFile < ApplicationRecord
   belongs_to :location
   belongs_to :codec
   has_one :track, dependent: :nullify
-  has_many :content_lengths, dependent: :destroy
   has_many :transcoded_items, dependent: :destroy
 
   validates :filename, presence: true, uniqueness: { scope: :location }
@@ -27,7 +26,7 @@ class AudioFile < ApplicationRecord
   validates :sample_rate, presence: true
   validates :bit_depth, presence: true
 
-  after_save :queue_content_length_calculations
+  after_create :queue_create_transcoded_items
 
   def check_self
     return true if File.exist?(full_path)
@@ -36,44 +35,27 @@ def check_self
     false
   end
 
-  def convert(codec_conversion)
+  def convert(codec_conversion, out_file_name)
     parameters = codec_conversion.ffmpeg_params.split
-    stdin, stdout, = Open3.popen2(
+    Open3.popen2(
       'ffmpeg',
       '-i', full_path,
       '-f', codec_conversion.resulting_codec.extension,
       *parameters,
       '-map_metadata', '-1',
-      '-map', 'a', '-',
+      '-map', 'a',
+      out_file_name,
       err: [Rails.configuration.ffmpeg_log_location, 'a']
     )
-    stdin.close
-    stdout
-  end
-
-  def calc_audio_length(codec_conversion)
-    existing = ContentLength.find_by(audio_file: self, codec_conversion:)
-    return existing if existing.present?
-
-    stdout = convert(codec_conversion)
-    length = 0
-    while (bytes = stdout.read(16.kilobytes))
-      length += bytes.length
-    end
-
-    ContentLength.find_or_create_by(audio_file: self, codec_conversion:) do |cl|
-      cl.length = length
-    end
   end
 
   def full_path
     File.join(location.path, filename)
   end
 
-  def queue_content_length_calculations
-    ContentLength.where(audio_file: self).destroy_all
+  def queue_create_transcoded_items
     CodecConversion.find_each do |cc|
-      CalculateContentLengthJob.perform_later(self, cc)
+      CreateTranscodedItemJob.perform_later(self, cc)
     end
   end
 end

app/models/codec_conversion.rb

@@ -10,20 +10,28 @@
 
 class CodecConversion < ApplicationRecord
   belongs_to :resulting_codec, class_name: 'Codec'
-  has_many :content_lengths, dependent: :destroy
-  has_many :transcoded_items, dependent: :destroy
+
+  has_many :transcoded_items, dependent: :delete_all
 
   validates :name, presence: true, uniqueness: true
   validates :ffmpeg_params, presence: true
 
   scope :by_codec, ->(codec) { where(resulting_codec: codec) }
 
-  after_save :queue_content_length_calculations
+  before_destroy :delete_transcoded_item_files
+  after_save :reset_transcoded_items
+
+  delegate :mimetype, to: :resulting_codec
+
+  def reset_transcoded_items
+    transcoded_items.delete_all
+    delete_transcoded_item_files
+    QueueTranscodedItemsForCodecConversionJob.perform_later(self)
+  end
+
+  private
 
-  def queue_content_length_calculations
-    ContentLength.where(codec_conversion: self).destroy_all
-    AudioFile.find_each do |af|
-      CalculateContentLengthJob.perform_later(af, self)
-    end
+  def delete_transcoded_item_files
+    FileUtils.rm_rf TranscodedItem.codec_conversion_base_path(self)
   end
 end

app/models/transcoded_item.rb

@@ -3,44 +3,49 @@
 # Table name: transcoded_items
 #
 #  id                  :bigint           not null, primary key
-#  last_used           :datetime         not null
-#  path                :string           not null
+#  uuid                :string           not null
 #  created_at          :datetime         not null
 #  updated_at          :datetime         not null
 #  audio_file_id       :bigint           not null
 #  codec_conversion_id :bigint           not null
 #
 class TranscodedItem < ApplicationRecord
-  BASE_PATH = Rails.configuration.transcode_cache_path
+  BASE_PATH = Rails.configuration.transcode_storage_path
 
   belongs_to :audio_file
   belongs_to :codec_conversion
 
-  validates :path, presence: true, uniqueness: true
+  validates :audio_file_id, uniqueness: { scope: :codec_conversion_id }
+  validates :uuid, presence: true, uniqueness: { scope: :codec_conversion_id }
 
-  after_create -> { ConvertTranscodeJob.perform_later(self) }
-  after_destroy :delete_file
+  # Be careful when adding destroy callbacks! For performance reasons
+  # CodecConversion#destroy/CodecConversion#update ignore these (and take care
+  # of deleting the relevant files themselves).
+  after_commit :delete_file, on: :destroy
 
-  def initialize(params)
-    super
-    self.path = random_path
+  delegate :mimetype, to: :codec_conversion
+
+  def path
+    self.class.path_for(codec_conversion, uuid)
   end
 
-  def do_conversion
-    tmppath = random_path
-    FileUtils.mkdir_p Pathname.new(tmppath).parent
-    File.open(tmppath, 'w') { |f| IO.copy_stream(audio_file.convert(codec_conversion), f) }
-    FileUtils.mkdir_p Pathname.new(path).parent
-    FileUtils.mv(tmppath, path)
+  def self.uuid_for(codec_conversion)
+    loop do
+      uuid = SecureRandom.uuid
+      return uuid unless TranscodedItem.exists?(codec_conversion:, uuid:)
+    end
   end
 
-  private
+  def self.codec_conversion_base_path(codec_conversion)
+    File.join(BASE_PATH, codec_conversion.id.to_s)
+  end
 
-  def random_path
-    uuid = SecureRandom.uuid
-    File.join(BASE_PATH, uuid[0..1], uuid[2..3], uuid)
+  def self.path_for(codec_conversion, uuid)
+    File.join(codec_conversion_base_path(codec_conversion), uuid[0..1], uuid[2..3], uuid)
   end
 
+  private
+
   def delete_file
     FileUtils.rm_f path
   end

config/application.rb

@@ -40,7 +40,6 @@ class Application < Rails::Application
     config.active_job.queue_adapter = :good_job
     config.active_job.default_queue_name = :within_5_minutes
 
-    config.transcode_cache_expiry = -> { 1.day.ago }
-    config.recalculate_content_length_if = ->(af) { af.length > 299 || af.track.created_at.after?(1.month.ago) }
+    config.queue_create_transcoded_item_if = ->(af) { af.length > 299 || af.track.created_at.after?(1.month.ago) }
   end
 end