Merge pull request #6023 from avalonmediasystem/video_check

Adjust non-media item filtering in ffprobe

app/models/master_file.rb

@@ -727,7 +727,7 @@ def saveDerivativesHash(derivative_hash)
 
   def reloadTechnicalMetadata!
     # Reset ffprobe
-    @ffprobe = Avalon::FFprobe.new(FileLocator.new(file_location).location)
+    @ffprobe = Avalon::FFprobe.new(FileLocator.new(file_location))
 
     # Formats like MP4 can be caught as both audio and video
     # so the case statement flows in the preferred order

lib/avalon/ffprobe.rb

@@ -14,47 +14,71 @@
 
 module Avalon
   class FFprobe
-    # media_path should be the output of `FileLocator.new(file_location).location`
-    def initialize(media_path)
+    # @param [FileLocator] media_file a file locator instance for the media file
+    def initialize(media_file)
+      @media_file = media_file
+    end
+
+    def json_output
+      return @json_output unless @json_output.nil?
+      return @json_output = {} unless valid_content_type?(@media_file)
       ffprobe = Settings&.ffprobe&.path || 'ffprobe'
-      raw_output = `#{ffprobe} -i "#{media_path}" -v quiet -show_format -show_streams -count_packets -of json`
+      raw_output = `#{ffprobe} -i "#{@media_file.location}" -v quiet -show_format -show_streams -of json`
       # $? is a variable for the exit status of the last executed process. 
       # Success == 0, any other value means the command failed in some way.
       unless $?.exitstatus == 0
-        @json_output = {}
         Rails.logger.error "File processing failed. Please ensure that FFprobe is installed and that the correct path is configured."
-        return
+        return @json_output = {}
       end
       @json_output = JSON.parse(raw_output).deep_symbolize_keys
-      @video_stream = @json_output[:streams].select { |stream| stream[:codec_type] == 'video' }.first
+      @json_output
     end
 
-    def video?
-      # ffprobe treats plain text files as ANSI or ASCII art. This sets the codec type to video
-      # but leaves display aspect ratio `nil`. If display_aspect_ratio is nil, return false.
-      # ffprobe treats image files as a single frame video. This sets the codec type to video
-      # but the packet/frame count will equal 1. If packet count equals 1, return false.
-      return true if @video_stream && @video_stream[:display_aspect_ratio] && @video_stream[:nb_read_packets].to_i > 1
+    def video_stream
+      return unless json_output.present?
+      @video_stream ||= json_output[:streams].select { |stream| stream[:codec_type] == 'video' }.first
+    end
+
+    def audio_stream
+      return unless json_output.present?
+      @audio_stream ||= json_output[:streams].select { |stream| stream[:codec_type] == 'audio' }.first
+    end
 
-      false
+    def video?
+      video_stream.present?
     end
 
     def audio?
-      @json_output[:streams]&.any? { |stream| stream[:codec_type] == 'audio' }
+      audio_stream.present?
     end
 
     def duration
       return unless video? || audio?
       # ffprobe return duration as seconds. Existing Avalon logic expects milliseconds.
-      (@json_output[:format][:duration].to_f * 1000).to_i
+      (json_output[:format][:duration].to_f * 1000).to_i
     end
 
     def display_aspect_ratio
-      @video_stream[:display_aspect_ratio] if video?
+      if video? && video_stream[:display_aspect_ratio].present?
+        video_stream[:display_aspect_ratio]
+      elsif video?
+        video_stream[:width].to_f / video_stream[:height].to_f
+      end
     end
 
     def original_frame_size
-      "#{@video_stream[:width]}x#{@video_stream[:height]}" if video?
+      "#{video_stream[:width]}x#{video_stream[:height]}" if video?
+    end
+
+    private
+
+    def valid_content_type? media_file
+      # Remove S3 credentials or other params from extension output
+      extension = File.extname(media_file.location)&.gsub(/[\?#].*/, '')
+      # Fall back on file extension if magic bytes fail to identify file
+      content_type = Marcel::MimeType.for media_file.reader, extension: extension
+
+      ['audio', 'video'].any? { |type| content_type.include?(type) }
     end
   end
 end

spec/lib/avalon/ffprobe_spec.rb

@@ -18,17 +18,19 @@
 describe Avalon::FFprobe do
   subject { described_class.new(test_file) }
 
-  let(:video_file) { Rails.root.join('spec', 'fixtures', 'videoshort.mp4').to_s }
-  let(:audioless_video_file) { Rails.root.join('spec', 'fixtures', 'videoshort_no_audio.mp4').to_s }
-  let(:audio_file) { Rails.root.join('spec', 'fixtures', 'jazz-performance.mp4').to_s }
+  let(:video_file) { FileLocator.new(Rails.root.join('spec', 'fixtures', 'videoshort.mp4').to_s) }
+  let(:audioless_video_file) { FileLocator.new(Rails.root.join('spec', 'fixtures', 'videoshort_no_audio.mp4').to_s) }
+  let(:audio_file) { FileLocator.new(Rails.root.join('spec', 'fixtures', 'jazz-performance.mp4').to_s) }
+  let(:image_file) { FileLocator.new(Rails.root.join('spec', 'fixtures', 'collection_poster.png').to_s) }
+  let(:text_file) { FileLocator.new(Rails.root.join('spec', 'fixtures', 'chunk_test.txt').to_s) }
 
   describe 'error handling' do
     let(:test_file) { video_file }
 
     it 'logs an error if ffprobe is misconfigured' do
       allow(Settings.ffprobe).to receive(:path).and_return('misconfigured/path')
       expect(Rails.logger).to receive(:error)
-      subject
+      subject.json_output
     end
   end
 
@@ -50,12 +52,12 @@
     end
 
     context 'with non-media files' do
-      let(:text_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'chunk_test.txt')) }
-      let(:image_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'collection_poster.png')) }
+      let(:text_test) { described_class.new(text_file) }
+      let(:image_test) { described_class.new(image_file) }
 
       it 'returns false' do
-        expect(text_file.video?).to be false
-        expect(image_file.video?).to be false
+        expect(text_test.video?).to be false
+        expect(image_test.video?).to be false
       end
     end
   end
@@ -86,12 +88,12 @@
     end
 
     context 'with non-media files' do
-      let(:text_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'chunk_test.txt')) }
-      let(:image_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'collection_poster.png')) }
+      let(:text_test) { described_class.new(text_file) }
+      let(:image_test) { described_class.new(image_file) }
 
       it 'returns false' do
-        expect(text_file.audio?).to be false
-        expect(image_file.audio?).to be false
+        expect(text_test.audio?).to be false
+        expect(image_test.audio?).to be false
       end
     end
   end
@@ -104,12 +106,12 @@
     end
 
     context 'with non-media files' do
-      let(:text_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'chunk_test.txt')) }
-      let(:image_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'collection_poster.png')) }
+      let(:text_test) { described_class.new(text_file) }
+      let(:image_test) { described_class.new(image_file) }
 
       it 'returns nil' do
-        expect(text_file.duration).to be nil
-        expect(image_file.duration).to be nil
+        expect(text_test.duration).to be nil
+        expect(image_test.duration).to be nil
       end
     end
   end
@@ -121,6 +123,13 @@
       it 'returns the display aspect ratio' do
         expect(subject.display_aspect_ratio).to eq '20:11'
       end
+
+      context 'file missing display aspect ratio' do
+        it 'calculates the display aspect ratio' do
+          subject.instance_variable_set(:@video_stream, subject.video_stream.except!(:display_aspect_ratio))
+          expect(subject.display_aspect_ratio).to eq(200.to_f / 110.to_f)
+        end
+      end
     end
 
     context 'with audio' do
@@ -131,12 +140,12 @@
     end
 
     context 'with non-media files' do
-      let(:text_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'chunk_test.txt')) }
-      let(:image_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'collection_poster.png')) }
+      let(:text_test) { described_class.new(text_file) }
+      let(:image_test) { described_class.new(image_file) }
 
       it 'returns nil' do
-        expect(text_file.display_aspect_ratio).to be nil
-        expect(image_file.display_aspect_ratio).to be nil
+        expect(text_test.display_aspect_ratio).to be nil
+        expect(image_test.display_aspect_ratio).to be nil
       end
     end
   end
@@ -159,12 +168,12 @@
     end
 
     context 'with non-media files' do
-      let(:text_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'chunk_test.txt')) }
-      let(:image_file) { described_class.new(Rails.root.join('spec', 'fixtures', 'collection_poster.png')) }
+      let(:text_test) { described_class.new(text_file) }
+      let(:image_test) { described_class.new(image_file) }
 
       it 'returns nil' do
-        expect(text_file.original_frame_size).to be nil
-        expect(image_file.original_frame_size).to be nil
+        expect(text_test.original_frame_size).to be nil
+        expect(image_test.original_frame_size).to be nil
       end
     end
   end