From d0775e9c06ee404aba8a5b408820536c46cd636d Mon Sep 17 00:00:00 2001 From: bajankristof Date: Wed, 5 Jun 2024 14:32:23 +0200 Subject: [PATCH] feat!: improve progress reports, timeout- and error handling --- lib/ffmpeg.rb | 1 + lib/ffmpeg/io.rb | 56 ++++++++++++++++++------ lib/ffmpeg/media.rb | 2 +- lib/ffmpeg/timeout.rb | 53 ++++++++++++++++++++++ lib/ffmpeg/transcoder.rb | 80 +++++++++++++++++++++------------- lib/ffmpeg/version.rb | 2 +- spec/ffmpeg/media_spec.rb | 11 ++++- spec/ffmpeg/transcoder_spec.rb | 24 +++++----- 8 files changed, 169 insertions(+), 60 deletions(-) create mode 100644 lib/ffmpeg/timeout.rb diff --git a/lib/ffmpeg.rb b/lib/ffmpeg.rb index ff10f6b..e9c5aad 100644 --- a/lib/ffmpeg.rb +++ b/lib/ffmpeg.rb @@ -10,6 +10,7 @@ require_relative 'ffmpeg/version' require_relative 'ffmpeg/encoding_options' require_relative 'ffmpeg/errors' +require_relative 'ffmpeg/timeout' require_relative 'ffmpeg/io' require_relative 'ffmpeg/media' require_relative 'ffmpeg/stream' diff --git a/lib/ffmpeg/io.rb b/lib/ffmpeg/io.rb index 9fea0ff..0834de5 100644 --- a/lib/ffmpeg/io.rb +++ b/lib/ffmpeg/io.rb @@ -7,18 +7,47 @@ module FFMPEG # The IO class is a simple wrapper around IO objects that adds a timeout # to all read operations and fixes encoding issues. class IO - attr_accessor :timeout + attr_accessor :encoding, :timeout + + @encoding = 'UTF-8' + + class << self + attr_accessor :encoding + end def self.force_encoding(chunk) chunk[/test/] rescue ArgumentError - chunk.force_encoding('ISO-8859-1') + chunk.force_encoding(encoding) end def initialize(target) @target = target end + def each(&block) + timer = timeout.nil? ? nil : Timeout.start(timeout) + buffer = String.new + + until eof? + char = getc + case char + when "\n", "\r" + timer&.tick + timer&.pause + block.call(buffer) + timer&.resume + buffer = String.new + else + buffer << char + end + end + + block.call(buffer) unless buffer.empty? + ensure + timer&.cancel + end + %i[ getc gets @@ -26,31 +55,32 @@ def initialize(target) readline ].each do |symbol| define_method(symbol) do |*args| - Timeout.timeout(timeout) do - output = @target.send(symbol, *args) - self.class.force_encoding(output) - output - end + data = @target.send(symbol, *args) + self.class.force_encoding(data) unless data.nil? + data end end %i[ - each each_char each_line ].each do |symbol| - read = symbol == :each_char ? :getc : :gets define_method(symbol) do |*args, &block| - until eof? - output = send(read, *args) - block.call(output) + timer = timeout.nil? ? nil : Timeout.start(timeout) + @target.send(symbol, *args) do |data| + timer&.tick + timer&.pause + block.call(self.class.force_encoding(data)) + timer&.resume end + ensure + timer&.cancel end end def readlines(*args) lines = [] - lines << gets(*args) until eof? + each(*args) { |line| lines << line } lines end diff --git a/lib/ffmpeg/media.rb b/lib/ffmpeg/media.rb index 203ef2f..0ad5070 100644 --- a/lib/ffmpeg/media.rb +++ b/lib/ffmpeg/media.rb @@ -47,7 +47,7 @@ def initialize(path) @size = response.content_length else - raise Errno::ENOENT, "The file at '#{@path}' does not exist" unless File.exist?(path) + raise Errno::ENOENT, "The file at '#{@path}' does not exist" unless File.exist?(@path) @size = File.size(@path) end diff --git a/lib/ffmpeg/timeout.rb b/lib/ffmpeg/timeout.rb new file mode 100644 index 0000000..8c8205c --- /dev/null +++ b/lib/ffmpeg/timeout.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'timeout' + +module FFMPEG + # The Timeout class is a simple wrapper around the Timeout module that + # provides a more convenient API to handle timeouts in a loop. + class Timeout + def self.start(duration, message = nil) + new(duration, message) + end + + def pause + @paused = true + end + + def resume + @paused = false + end + + def tick + @last_tick = Time.now + end + + def cancel + return if @wait_thread.nil? + + @wait_thread.kill + @wait_thread.join + end + + private + + def initialize(duration, message = nil) + @duration = duration + @message = message + + @last_tick = Time.now + @current_thread = Thread.current + @wait_thread = Thread.new { loop } + @paused = false + end + + def loop + if !@paused && Time.now - @last_tick >= @duration + @current_thread.raise(::Timeout::Error, @message || self.class.name) + else + sleep 0.1 + loop + end + end + end +end diff --git a/lib/ffmpeg/transcoder.rb b/lib/ffmpeg/transcoder.rb index cf7c05b..7e3d153 100644 --- a/lib/ffmpeg/transcoder.rb +++ b/lib/ffmpeg/transcoder.rb @@ -6,7 +6,8 @@ module FFMPEG # The Transcoder class is responsible for transcoding multimedia files. # It accepts a Media object or a path to a multimedia file as input. class Transcoder - attr_reader :args, :output, :errors, :input_path, :output_path + attr_reader :args, :input_path, :output_path, + :output, :progress, :succeeded @timeout = 30 @@ -20,6 +21,7 @@ def initialize( options, validate: true, preserve_aspect_ratio: true, + progress_digits: 2, input_options: [], filters: [] ) @@ -34,9 +36,9 @@ def initialize( @options = options.is_a?(Hash) ? EncodingOptions.new(options) : options @validate = validate @preserve_aspect_ratio = preserve_aspect_ratio + @progress_digits = progress_digits @input_options = input_options @filters = filters - @errors = [] if @input_options.is_a?(Hash) @input_options = @input_options.reduce([]) do |acc, (key, value)| @@ -66,14 +68,18 @@ def command def run(&block) execute(&block) - return nil unless @validate + validate_result if @validate + end - validate_output_file(&block) - result + def finished? + !@succeeded.nil? end def succeeded? - @errors.empty? + return false unless @succeeded + return true unless @validate + + result&.valid? end def failed? @@ -81,6 +87,8 @@ def failed? end def result + return nil unless @succeeded + @result ||= Media.new(@output_path) if File.exist?(@output_path) end @@ -133,25 +141,14 @@ def prepare_seek_time end end - def validate_output_file - @errors << 'no output file created' unless File.exist?(@output_path) - @errors << 'encoded file is invalid' if result.nil? || !result.valid? + def validate_result + return result if result&.valid? - if succeeded? - yield(1.0) if block_given? - FFMPEG.logger.info(self.class) do - "Transcoding #{@input_path} to #{@output_path} succeeded\n" \ - "Command: #{command.join(' ')}\n" \ - "Output: #{@output}" - end - else - message = "Transcoding #{@input_path} to #{@output_path} failed\n" \ - "Command: #{command.join(' ')}\n" \ - "Errors: #{@errors.join(', ')}\n " \ - "Output: #{@output}\n" - FFMPEG.logger.error(self.class) { message } - raise Error, message - end + message = "Transcoding #{@input_path} to #{@output_path} produced invalid media\n" \ + "Command: #{command.join(' ')}\n" \ + "Output: #{@output}" + FFMPEG.logger.error(self.class) { message } + raise Error, message end def execute @@ -161,6 +158,8 @@ def execute end @output = String.new + @progress = 0.0 + @succeeded = nil FFMPEG.ffmpeg_popen3(*@args) do |_stdin, stdout, stderr, wait_thr| yield(0.0) if block_given? @@ -174,21 +173,42 @@ def execute @output << line next unless @media - next unless block_given? next unless line =~ /time=(\d+):(\d+):(\d+.\d+)/ # time=00:02:42.28 time = (::Regexp.last_match(1).to_i * 3600) + (::Regexp.last_match(2).to_i * 60) + ::Regexp.last_match(3).to_f - yield(time / @media.duration) + progress = (time / @media.duration).round(@progress_digits) + next unless progress < 1.0 || progress == @progress + + @progress = progress + yield(@progress) if block_given? end - @errors << 'ffmpeg returned non-zero exit code' unless wait_thr.value.success? - rescue Timeout::Error - message = "Transcoding #{@input_path} to #{@output_path} failed, process hung\n" \ + if wait_thr.value.success? + @succeeded = true + @progress = 1.0 + yield(@progress) if block_given? + + FFMPEG.logger.info(self.class) do + "Transcoding #{@input_path} to #{@output_path} succeeded\n" \ + "Command: #{command.join(' ')}\n" \ + "Output: #{@output}" + end + else + @succeeded = false + message = "Transcoding #{@input_path} to #{@output_path} failed\n" \ + "Command: #{command.join(' ')}\n" \ + "Output: #{@output}" + FFMPEG.logger.error(self.class) { message } + raise Error, message + end + rescue ::Timeout::Error + @succeeded = false + Process.kill(FFMPEG::SIGKILL, wait_thr.pid) + message = "Transcoding #{@input_path} to #{@output_path} timed out\n" \ "Command: #{command.join(' ')}\n" \ "Output: #{@output}" - Process.kill(FFMPEG::SIGKILL, wait_thr.pid) FFMPEG.logger.error(self.class) { message } raise Error, message end diff --git a/lib/ffmpeg/version.rb b/lib/ffmpeg/version.rb index 61103b6..908f3cc 100644 --- a/lib/ffmpeg/version.rb +++ b/lib/ffmpeg/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module FFMPEG - VERSION = '5.0.1' + VERSION = '6.0.0' end diff --git a/spec/ffmpeg/media_spec.rb b/spec/ffmpeg/media_spec.rb index fb85858..c4f16a0 100644 --- a/spec/ffmpeg/media_spec.rb +++ b/spec/ffmpeg/media_spec.rb @@ -58,7 +58,7 @@ module FFMPEG let(:stdout) { read_fixture_file("outputs/#{stdout_fixture_file}") } let(:stderr) { stderr_fixture_file ? read_fixture_file("outputs/#{stderr_fixture_file}") : '' } - before { allow(Open3).to receive(:capture3).and_return([stdout, stderr, double(success?: true)]) } + before { allow(Open3).to receive(:capture3).and_return([stdout, stderr, double(succeeded: true)]) } subject { described_class.new(__FILE__) } context 'cannot be parsed' do @@ -105,6 +105,15 @@ module FFMPEG it 'should not raise an error' do expect { subject }.not_to raise_error end + + context 'with IO encoding set to ISO-8859-1' do + before { FFMPEG::IO.encoding = 'ISO-8859-1' } + after { FFMPEG::IO.encoding = 'UTF-8' } + + it 'should not raise an error' do + expect { subject }.not_to raise_error + end + end end end end diff --git a/spec/ffmpeg/transcoder_spec.rb b/spec/ffmpeg/transcoder_spec.rb index 5df2469..e939289 100644 --- a/spec/ffmpeg/transcoder_spec.rb +++ b/spec/ffmpeg/transcoder_spec.rb @@ -65,8 +65,10 @@ module FFMPEG end it 'should fail when the timeout is exceeded' do - expect(FFMPEG.logger).to receive(:error) - expect { subject.run }.to raise_error(FFMPEG::Error, /Transcoding .+ failed, process hung/) + ::Timeout.timeout(5) do + expect(FFMPEG.logger).to receive(:error) + expect { subject.run }.to raise_error(FFMPEG::Error, /Transcoding .+ timed out/) + end end end @@ -75,7 +77,7 @@ module FFMPEG it 'should fail with non-zero exit code error' do expect(FFMPEG.logger).to receive(:error) - expect { subject.run }.to raise_error(FFMPEG::Error, /non-zero exit code/) + expect { subject.run }.to raise_error(FFMPEG::Error, /Transcoding .+ failed/) end end @@ -150,7 +152,9 @@ module FFMPEG end it 'should fail when the timeout is exceeded' do - expect { subject.run }.to raise_error(FFMPEG::Error, /Transcoding .+ failed, process hung/) + ::Timeout.timeout(5) do + expect { subject.run }.to raise_error(FFMPEG::Error, /Transcoding .+ timed out/) + end end end end @@ -227,14 +231,6 @@ module FFMPEG end end - context 'with invalid movie input' do - let(:media) { Media.new(__FILE__) } - - it 'should fail' do - expect { subject.run }.to raise_error(FFMPEG::Error, /no output file created/) - end - end - context 'with explicitly set duration' do let(:options) { { duration: 2 } } @@ -298,7 +294,7 @@ module FFMPEG it 'should fail' do expect do subject.run - end.to raise_error(FFMPEG::Error, /Transcoding .+ failed/) + end.to raise_error(FFMPEG::Error, /Transcoding .+ produced invalid media/) end end @@ -396,7 +392,7 @@ module FFMPEG let(:input) { "#{fixture_path}/images/wrong_type/img_%03d.tiff" } it 'should fail' do - expect { subject.run }.to raise_error(FFMPEG::Error, /encoded file is invalid/) + expect { subject.run }.to raise_error(FFMPEG::Error, /Transcoding .+ failed/) end end end