Merge branch 'master' into md/testing

Author: MassimoDeLuca

.github/CODEOWNERS

@@ -1,2 +1,2 @@
 # Repo Owners
-*  @Kyrluckechuck @Vidyard/create-platform
+*  @Kyrluckechuck @Vidyard/tech-stream

lib/ffmpeg/movie.rb

@@ -11,8 +11,13 @@ class Movie
     attr_reader :container
     attr_reader :error
 
+    attr_accessor :has_dynamic_resolution, :requires_pre_encode
+
     UNSUPPORTED_CODEC_PATTERN = /^Unsupported codec with id (\d+) for input stream (\d+)$/
 
+    @has_dynamic_resolution = false
+    @requires_pre_encode = nil
+
     def initialize(paths, analyzeduration = 15000000, probesize=15000000 )
       paths = [paths] unless paths.is_a? Array
 
@@ -98,12 +103,10 @@ def initialize(paths, analyzeduration = 15000000, probesize=15000000 )
 
           @video_stream = "#{video_stream[:codec_name]} (#{video_stream[:profile]}) (#{video_stream[:codec_tag_string]} / #{video_stream[:codec_tag]}), #{colorspace}, #{resolution} [SAR #{sar} DAR #{dar}]"
 
+          @rotation = nil
+
           video_stream[:side_data_list].each do |side_data_entry|
-            @rotation = if side_data_entry.key?(:rotation)
-                          side_data_entry[:rotation].to_i
-                        else
-                          nil
-                        end
+            @rotation = side_data_entry[:rotation].to_i if side_data_entry.key?(:rotation)
           end if video_stream.key?(:side_data_list)
         end
 
@@ -267,6 +270,51 @@ def any_streams_contain_audio?
       @any_streams_contain_audio ||= calc_any_streams_contain_audio
     end
 
+    # Get the max width and max height of all frames in the input movies and check if the resolutions of frames are consistent
+    def check_frame_resolutions
+      max_width = width || 0
+      max_height = height || 0
+      differing_frame_resolutions = false
+      last_dimensions = nil
+
+      unescaped_paths.each do |path|
+        local_movie = Movie.new(path)
+
+        # set max width and height from the movie metadata first
+        max_width = [max_width, local_movie.width].max
+        max_height = [max_height, local_movie.height].max
+
+        # Get each keyframe's resolution - this command should be fairly fast, ~30 seconds on a 3 hr video
+        command = "#{ffprobe_command} -v error -select_streams v:0 -show_entries frame=width,height -of csv=p=0 -skip_frame nokey #{Shellwords.escape(local_movie.path)}" # -skip_frame nokey speeds up processing significantly, only evaluating key frames which seems to be sufficient for frame resolution checks
+        FFMPEG.logger.info("Running check for varying resolution...\n#{command}\n")
+
+        Open3.popen3(command) do |_stdin, stdout, _stderr, _wait_thr|
+          stdout.each_line do |line|
+            frame_dimensions = line.split(',').first(2).map(&:to_i) # get the first two values as ffmpeg can sometimes provide an extra comma
+            frame_width, frame_height = frame_dimensions
+
+            next if frame_width.nil? || frame_height.nil?
+
+            # Update max width and max height
+            max_width = [max_width, frame_width].max
+            max_height = [max_height, frame_height].max
+
+            # Check if the current frame resolution differs from the last frame
+            if last_dimensions && frame_dimensions != last_dimensions
+              differing_frame_resolutions = true
+            end
+
+            last_dimensions = frame_dimensions
+          end
+        end
+      end
+
+      @has_dynamic_resolution = differing_frame_resolutions
+
+      # Return the max width, max height, and whether differing resolutions were found
+      [max_width, max_height, differing_frame_resolutions]
+    end
+
     protected
 
     def calc_any_streams_contain_audio

lib/ffmpeg/transcoder.rb

@@ -18,11 +18,22 @@ def self.timeout
       @@timeout
     end
 
+    def requires_pre_encode
+      # Stitches and trims with dynamic resolution require pre-encoding
+      @movie.requires_pre_encode ||= @movie.paths.size > 1 || (@movie.has_dynamic_resolution && @transcoder_options[:permit_dynamic_resolution_pre_encode])
+      return @movie.requires_pre_encode
+    end
+
     def initialize(movie, output_file, options = EncodingOptions.new, transcoder_options = {}, transcoder_prefix_options = {})
       @movie = movie
       @output_file = output_file
+      @transcoder_options = transcoder_options
 
-      if @movie.paths.size > 1
+      # If the movie has frames with varying resolutions, we need to pre-encode the movie for trims
+      # This is because ffmpeg can't reliably handle inputs that contain frames with differing resolutions particularly for trimming with `filter_complex`
+      @movie.check_frame_resolutions if @transcoder_options[:permit_dynamic_resolution_pre_encode]
+
+      if requires_pre_encode
         @movie.paths.each do |path|
           # Make the interim path folder if it doesn't exist
           dirname = "#{File.dirname(path)}/interim"
@@ -48,7 +59,6 @@ def initialize(movie, output_file, options = EncodingOptions.new, transcoder_opt
         raise ArgumentError, "Unknown options format '#{options.class}', should be either EncodingOptions, Hash or String."
       end
 
-      @transcoder_options = transcoder_options
       @transcoder_prefix_options = transcoder_prefix_options
       @errors = []
 
@@ -76,9 +86,9 @@ def encoded
     end
 
     private
+
     def pre_encode_if_necessary
-      # Don't pre-encode single inputs since it doesn't need any size conversion
-      return if @movie.interim_paths.size <= 1
+      return unless requires_pre_encode
 
       # Set a minimum frame rate
       output_frame_rate = [@raw_options[:frame_rate] || @movie.frame_rate, 30].max
@@ -95,7 +105,13 @@ def pre_encode_if_necessary
       @movie.unescaped_paths.each_with_index do |path, index|
         audio_map = determine_audio_for_pre_encode(path)
 
-        command = "#{@movie.ffmpeg_command} -y -i #{Shellwords.escape(path)} -movflags faststart #{pre_encode_options} -r #{output_frame_rate} -filter_complex \"[0:v]scale=#{max_width}:#{max_height}:force_original_aspect_ratio=decrease,pad=#{max_width}:#{max_height}:(ow-iw)/2:(oh-ih)/2:color=black,setsar=1[Scaled]\" -map \"[Scaled]\" #{audio_map} #{@movie.interim_paths[index]}"
+        # Only re-scale the video if there are multiple inputs. If a single input video contains dynamic resolution it can cause issues with the filter_complex filter
+        complex_filter = ""
+        if @movie.paths.size > 1
+          complex_filter = "-filter_complex \"[0:v]scale=#{max_width}:#{max_height}:force_original_aspect_ratio=decrease,pad=#{max_width}:#{max_height}:(ow-iw)/2:(oh-ih)/2:color=black,setsar=1[Scaled]\" -map \"[Scaled]\" #{audio_map}"
+        end
+
+        command = "#{@movie.ffmpeg_command} -y -i #{Shellwords.escape(path)} -movflags faststart #{pre_encode_options} -r #{output_frame_rate} #{complex_filter} #{@movie.interim_paths[index]}"
         FFMPEG.logger.info("Running pre-encoding...\n#{command}\n")
         output = ""
 
@@ -257,19 +273,12 @@ def convert_prefix_options_to_string(transcoder_prefix_options)
     end
 
     def calculate_interim_max_dimensions
-      max_width = @movie.width
-      max_height = @movie.height
-      # Find best highest resolution
-      @movie.unescaped_paths.each do |path|
-        local_movie = Movie.new(path)
-
-        # If the local resolution is larger than the current highest
-        max_width = [local_movie.width, max_width].max
-        max_height = [local_movie.height, max_height].max
-      end
+      # Get max width and max height of all inputs from each frame
+      max_width, max_height = @movie.check_frame_resolutions
 
       converted_width = (max_height * FIXED_LOWER_TO_UPPER_RATIO).ceil()
       converted_height = (max_width * FIXED_UPPER_TO_LOWER_RATIO).ceil()
+
       # Convert to always be a 16:9 ratio
       # If the converted width will not be a decrease in resolution, upscale the width
       if converted_width >= max_width

spec/ffmpeg/movie_spec.rb

@@ -545,5 +545,32 @@ module FFMPEG # rubocop:todo Metrics/ModuleLength
         movie.screenshot("#{tmp_path}/awesome.jpg", {seek_time: 2, dimensions: "640x480"}, {preserve_aspect_ratio: :width})
       end
     end
+
+    describe "#check_frame_resolutions" do
+      let(:movie) { Movie.new("#{fixture_path}/movies/awesome movie.mov") }
+      let(:movie_with_multiple_dimension_inputs) { Movie.new(["#{fixture_path}/movies/awesome_widescreen.mov", "#{fixture_path}/movies/sideways_movie.mov"]) }
+
+      it "handles empty frame resolution lines gracefully" do
+        allow(Open3).to receive(:popen3).and_yield(nil, "640,480\n\n", nil, nil)
+        expect(movie.check_frame_resolutions).to eq([640, 480, false])
+      end
+
+      describe 'returns the correct max width and height' do
+        it "when resolutions are consistent" do
+          allow(Open3).to receive(:popen3).and_yield(nil, "640,480\n640,480\n", nil, nil)
+          expect(movie.check_frame_resolutions).to eq([640, 480, false])
+        end
+
+        it "when resolutions are inconsistent" do
+          allow(Open3).to receive(:popen3).and_yield(nil, "640,480\n1920,300\n", nil, nil)
+          expect(movie.check_frame_resolutions).to eq([1920, 480, true])
+        end
+
+        it "when multiple inputs are provided" do
+          allow(Open3).to receive(:popen3).and_yield(nil, "640,480\n", nil, nil).and_yield(nil, "1920,300\n", nil, nil)
+          expect(movie_with_multiple_dimension_inputs.check_frame_resolutions).to eq([1920, 480, true])
+        end
+      end
+    end
   end
 end

spec/ffmpeg/transcoder_spec.rb

@@ -323,34 +323,108 @@ module FFMPEG
     describe "pre_encode_if_necessary" do
       let(:output_path) { "#{tmp_path}/pre_encode_out.mp4" }
 
-      it 'returns early if only a single video file' do
+      it 'returns early if only a single video file and no dynamic resolution' do
         transcoder = Transcoder.new(movie, output_path, EncodingOptions.new)
+        allow(movie).to receive(:has_dynamic_resolution).and_return(false)
         expect(movie).not_to receive(:height)
         transcoder.send(:pre_encode_if_necessary)
       end
 
+      it 'requires pre-encoding when movie has a single path and dynamic resolution is permitted' do
+        transcoder = Transcoder.new(movie, output_path, EncodingOptions.new, { permit_dynamic_resolution_pre_encode: true })
+        allow(movie).to receive(:has_dynamic_resolution).and_return(true)
+        expect(transcoder.requires_pre_encode).to be true
+      end
+
+      it 'does not require pre-encoding when movie has a single path and dynamic resolution is not permitted' do
+        transcoder = Transcoder.new(movie, output_path, EncodingOptions.new, { permit_dynamic_resolution_pre_encode: false })
+        allow(movie).to receive(:has_dynamic_resolution).and_return(true)
+        expect(transcoder.requires_pre_encode).to be false
+      end
+
       describe 'creates interim inputs with scaling correctly applied based on input files' do
         it 'with audio' do
           transcoder = Transcoder.new(movie_with_multiple_dimension_inputs, output_path, EncodingOptions.new)
+
+          expect(Open3).to receive(:popen3).exactly(2).times # prevent ffprobe calls from being evaluated (check_frame_resolutions)
           expect(Open3).to receive(:popen3).twice.with match(/.*\[0\:v\]scale\=854\:480.*pad\=854\:480\:\(ow\-iw\)\/2\:\(oh\-ih\)\/2\:color\=black.*\-map \"0\:a\".*/)
-          transcoder.send(:pre_encode_if_necessary)
 
+          transcoder.send(:pre_encode_if_necessary)
         end
 
         it 'with silent audio' do
           transcoder = Transcoder.new(movie_with_multiple_dimension_inputs_with_partial_audio, output_path, EncodingOptions.new)
+
+          expect(Open3).to receive(:popen3).exactly(2).times # prevent ffprobe calls from being evaluated (check_frame_resolutions)
           # Match silent audio fill
           expect(Open3).to receive(:popen3).once.with match(/.*\[0\:v\]scale\=960\:540.*pad\=960\:540\:\(ow\-iw\)\/2\:\(oh\-ih\)\/2\:color\=black.*\-map \"\[a\]\".*/)
           # Retain "real" audio
           expect(Open3).to receive(:popen3).once.with match(/.*\[0\:v\]scale\=960\:540.*pad\=960\:540\:\(ow\-iw\)\/2\:\(oh\-ih\)\/2\:color\=black.*\-map \"0\:a\".*/)
+
           transcoder.send(:pre_encode_if_necessary)
         end
 
         it 'without any audio' do
           transcoder = Transcoder.new(movie_with_multiple_dimension_inputs_with_no_audio, output_path, EncodingOptions.new)
+
+          expect(Open3).to receive(:popen3).exactly(2).times # prevent ffprobe calls from being evaluated (check_frame_resolutions)
           expect(Open3).to receive(:popen3).twice.with match(/.*\[0\:v\]scale\=960\:540.*pad\=960\:540\:\(ow\-iw\)\/2\:\(oh\-ih\)\/2\:color\=black.*((?!\-map \"0\:a\")(?!\-map \"\[a\]\")).*/)
+
+          transcoder.send(:pre_encode_if_necessary)
+        end
+      end
+
+      describe "correctly applies the complex filter" do
+        it "when more than one input is provided will apply the complex filter" do
+          transcoder = Transcoder.new(movie_with_two_inputs, output_path, EncodingOptions.new)
+
+          expect(Open3).to receive(:popen3).exactly(2).times # prevent ffprobe calls from being evaluated (check_frame_resolutions)
+          expect(Open3).to receive(:popen3).twice.with match(/-filter_complex/)
+
           transcoder.send(:pre_encode_if_necessary)
         end
+
+        it "when only one input is provided does not apply the complex filter" do
+          transcoder_options = { permit_dynamic_resolution_pre_encode: true }
+          transcoder = Transcoder.new(movie, output_path, EncodingOptions.new, transcoder_options)
+          allow(movie).to receive(:has_dynamic_resolution).and_return(true)
+
+          expect(Open3).not_to receive(:popen3).with('-filter_complex')
+
+          transcoder.send(:pre_encode_if_necessary)
+        end
+      end
+    end
+
+    describe "#requires_pre_encode" do
+      let(:output_path) { "#{tmp_path}/output.mp4" }
+
+      it 'requires pre-encoding when movie has multiple paths' do
+        transcoder = Transcoder.new(movie_with_two_inputs, output_path, EncodingOptions.new)
+        expect(transcoder.requires_pre_encode).to be true
+      end
+
+      it 'requires pre-encoding when movie has a single path and dynamic resolution is permitted' do
+        movie = Movie.new("#{fixture_path}/movies/awesome movie.mov")
+        transcoder_options = { permit_dynamic_resolution_pre_encode: true }
+        transcoder = Transcoder.new(movie, output_path, EncodingOptions.new, transcoder_options)
+        allow(movie).to receive(:has_dynamic_resolution).and_return(true)
+        expect(transcoder.requires_pre_encode).to be true
+      end
+
+      it 'does not require pre-encoding when movie has a single path and dynamic resolution is not permitted' do
+        movie = Movie.new("#{fixture_path}/movies/awesome movie.mov")
+        transcoder_options = { permit_dynamic_resolution_pre_encode: false }
+        transcoder = Transcoder.new(movie, output_path, EncodingOptions.new, transcoder_options)
+        allow(movie).to receive(:has_dynamic_resolution).and_return(true)
+        expect(transcoder.requires_pre_encode).to be false
+      end
+
+      it 'does not require pre-encoding when movie has a single path without dynamic resolution' do
+        movie = Movie.new("#{fixture_path}/movies/awesome movie.mov")
+        transcoder = Transcoder.new(movie, output_path, EncodingOptions.new)
+        allow(movie).to receive(:has_dynamic_resolution).and_return(false)
+        expect(transcoder.requires_pre_encode).to be false
       end
     end
   end