Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added timeout option to detect hung ffmpeg process #27

Merged
merged 4 commits into from

4 participants

@stakach

Based on the method used in https://github.com/applicaster/rvideo

@dbackeus
Collaborator

Thanks fort the new pull request!

Before including this feature I would like to see if it can be made compatible with the jRuby fix mentioned in issue #25.

Do you have time available to have a look at that?

I think ruby implementation compatibility might be higher priority than hung process.

@stakach

Well https://github.com/applicaster/rvideo uses open4 which is where I grabbed the IO hack from.

However it looks like it is a jRuby bug in 1.9 mode, it is probably better to wait for jRuby to fix the standard library then to add an external dependency.

Link to bug
http://jira.codehaus.org/browse/JRUBY-5705
http://jira.codehaus.org/browse/JRUBY-5710
http://jira.codehaus.org/browse/JRUBY-6409
http://jira.codehaus.org/browse/JRUBY-6195

@stakach

Thoughts?

@dbackeus
Collaborator

I agree that we should let jRuby fix it if the bug is in their end.

I mentioned this in #25 but will let the creator of that issue respond if he has something to add in the discussion.

If there are no further arguments for including open4 in the next few days I'll merge your code!

@guyboertje

Using thread.kill in jRuby is problematic as the underlying java thread is not killed. I have implemented jRuby timeout before in patches to http clients. To avoid the thread kill requirement I have created two threads both of which append to a SizedQueue of size 1. If the timeout thread wakes from sleeping before the ffmpeg process then it writes to the Q and the main thread continues, but if the ffmpeg thread completes first, all is good and continues, the sleeping thread can be woken and it will write to the queue which will have no effect.

@guyboertje

In jRuby popen4 comes from the jruby devs. But in MRI it comes from Ara Howard where it is an enhancement to the MRI code.

By using popen4, we would be bypassing the JIRA bugs listed.

@stakach

@guyboertje do you want to fork the main project, merge in my commits then update the code to support jRuby?

@guyboertje

Yeah, I will do that and see if I can get the best out of MRI and JRUBY for hung processes.

@dbackeus
Collaborator

That's awesome, hope it works out :thumbsup:

@stakach

@guyboertje do you mind if we merge this pull request and you can merge your changes in at a later date?

@dbackeus can you merge this into the trunk? It's in line with https://github.com/streamio/streamio-magick and won't make any practical difference to jRuby support in the mean time.

@dbackeus
Collaborator

I did not get the connection to streamio-magick?

I'm going to Italy over the weekend. Will be back wednesday so will look at the merge at that time. Maybe we have a comment from @guyboertje then as well.

@stakach

streamio-magick uses popen3 too so wouldn't work on jRuby either.

@stakach

Also enjoy your holiday!

@guyboertje

I am finding it hard to make time for this but I will do it.

Please merge and we can deal with my changes later.

@dbackeus
Collaborator

Ok I'm going to merge this and then do some refactoring on the code - revert the version bumps etc.

@dbackeus dbackeus merged commit 3b7053a into streamio:master
@dbackeus
Collaborator

So the timeout spec isn't actually passing. Could someone provide a video file which is known to hang ffmpeg? The smaller the better if we are to include it in the repo.

I had a failing one but it seems it works just fine in ffmpeg version 10.2+.

@dbackeus dbackeus commented on the diff
lib/ffmpeg/transcoder.rb
@@ -1,8 +1,22 @@
require 'open3'
require 'shellwords'
+if RUBY_PLATFORM =~ /(win|w)(32|64)$/
+ require 'win32/process'
@dbackeus Collaborator

Why are we requiring this here and again in io monkey?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbackeus dbackeus commented on the diff
lib/ffmpeg/io_monkey.rb
((48 lines not shown))
+
+ private
+
+
+ def new_thread(pid, *a, &b)
+ cur = Thread.current
+ Thread.new(*a) do |*a|
+ begin
+ b[*a]
+ rescue Exception => e
+ cur.raise e
+ if RUBY_PLATFORM =~ /(win|w)(32|64)$/
+ require 'win32/process'
+ Process.kill(1, pid)
+ else
+ Process.kill('INT', pid)
@dbackeus Collaborator

Why was the killing process changed from SIGKILL to INT?

Isn't it that if the process is hung we might not get out of it with such a soft method as INT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stakach

@dbackeus I was using a file from: http://ffmpeg.org/trac/ffmpeg/ticket/488 to test the hung process stuff.

In terms of sigkill you are probably right. In fact this is probably the best way to do it: Process.kill("SIGKILL", pid) unless(Process.kill "SIGTERM", pid)

The require 'win32/process' can be included where ever, transcoder.rb probably makes more sense as it should be loaded before the process is created

@stakach

Although, sigkill might as well be used by itself. Can't remember why I changed it - read something somewhere probably.

@dbackeus
Collaborator

@stakach Already tried that file but latest ffmpeg discovers that the file is corrupt and does not proceed with transcoding.

Just pushed a bit of refactoring on the io monkey: fbc47de. Should be more readable now.

@stakach

@dbackeus I had to use an older version of ffmpeg to test.
The refactor looks good!

An alternative could be to write a dummy ffmpeg application that does nothing but hang. I've still got staging environments setup with the old ffmpeg's. So can run some quick confirmation tests once the refactor is done.

@dbackeus
Collaborator

Thanks for the idea with a dummy ffmpeg. I just pushed a working spec!

Actually makes more sense this way - we're implementing funcionality to save us from any kind of freezing ffmpeg - not just related to faulty movies.

Also added a spec for using transcoder with the timeout disabled just to make sure that code path remains stable.

@stakach

@dbackeus looks awesome mate! The dummy ffmpeg is much more elegant then anything I envisioned too.

Epic work!

@stakach

@dbackeus one more thing just to polish it off

We could move pid = wait_thr.pid into the if @@timeout statement

@dbackeus
Collaborator

Good call. No need for a variable at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 24, 2012
  1. @stakach

    Added hung process detection

    stakach authored
  2. @stakach

    version bump

    stakach authored
  3. @stakach

    Class variable change

    stakach authored
Commits on Jun 7, 2012
  1. Tested hung process detection on Windows and Nix with a version of FF…

    Stephen von Takach authored
    …MPEG that hangs every time.
This page is out of date. Refresh to see the latest.
View
16 README.md
@@ -154,6 +154,22 @@ FFMPEG.ffmpeg_binary = '/usr/local/bin/ffmpeg'
This will cause the same command to run as "/usr/local/bin/ffmpeg -i /path/to/input.file ..." instead.
+
+Automatically kill hung processes
+---------------------------------
+
+By default, streamio will wait for 200 seconds between IO feedback from the FFMPEG process. After which an error is logged and the process killed.
+It is possible to modify this behaviour by setting a new default:
+
+``` ruby
+# Change the timeout
+Transcoder.timeout = 30
+
+# Disable the timeout altogether
+Transcoder.timeout = false
+```
+
+
Copyright
---------
View
72 lib/ffmpeg/io_monkey.rb
@@ -0,0 +1,72 @@
+
+
+if RUBY_VERSION =~ /1\.8/
+ #
+ # This is useful when `timeout.rb`, which, on M.R.I 1.8, relies on green threads, does not work consistently.
+ #
+ begin
+ require 'system_timer'
+ MyTimer = SystemTimer
+ rescue LoadError
+ require 'timeout'
+ MyTimer = Timeout
+ end
+else
+ require 'timeout'
+ MyTimer = Timeout
+end
+
+
+#
+# Monkey Patch timeout support into the IO class
+#
+class IO
+ def each_with_timeout(pid, timeout, sep_string=$/)
+ q = Queue.new
+ th = nil
+
+ timer_set = lambda do |timeout|
+ th = new_thread(pid){ to(timeout){ q.pop } }
+ end
+
+ timer_cancel = lambda do |timeout|
+ th.kill if th rescue nil
+ end
+
+ timer_set[timeout]
+ begin
+ self.each(sep_string) do |buf|
+ timer_cancel[timeout]
+ yield buf
+ timer_set[timeout]
+ end
+ ensure
+ timer_cancel[timeout]
+ end
+ end
+
+
+ private
+
+
+ def new_thread(pid, *a, &b)
+ cur = Thread.current
+ Thread.new(*a) do |*a|
+ begin
+ b[*a]
+ rescue Exception => e
+ cur.raise e
+ if RUBY_PLATFORM =~ /(win|w)(32|64)$/
+ require 'win32/process'
+ Process.kill(1, pid)
+ else
+ Process.kill('INT', pid)
@dbackeus Collaborator

Why was the killing process changed from SIGKILL to INT?

Isn't it that if the process is hung we might not get out of it with such a soft method as INT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+ end
+ end
+ end
+
+ def to timeout = nil
+ MyTimer.timeout(timeout){ yield }
+ end
+end
View
61 lib/ffmpeg/transcoder.rb
@@ -1,8 +1,22 @@
require 'open3'
require 'shellwords'
+if RUBY_PLATFORM =~ /(win|w)(32|64)$/
+ require 'win32/process'
@dbackeus Collaborator

Why are we requiring this here and again in io monkey?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+end
+
module FFMPEG
class Transcoder
+ @@timeout = 200
+
+ def self.timeout=(time)
+ @@timeout = time == false ? false : time.to_i
+ end
+
+ def self.timeout
+ @@timeout
+ end
+
def initialize(movie, output_file, options = EncodingOptions.new, transcoder_options = {})
@movie = movie
@output_file = output_file
@@ -28,26 +42,39 @@ def run
FFMPEG.logger.info("Running transcoding...\n#{command}\n")
output = ""
last_output = nil
- Open3.popen3(command) do |stdin, stdout, stderr|
- yield(0.0) if block_given?
- stderr.each("r") do |line|
- fix_encoding(line)
- output << line
- if line.include?("time=")
- if line =~ /time=(\d+):(\d+):(\d+.\d+)/ # ffmpeg 0.8 and above style
- time = ($1.to_i * 3600) + ($2.to_i * 60) + $3.to_f
- elsif line =~ /time=(\d+.\d+)/ # ffmpeg 0.7 and below style
- time = $1.to_f
- else # better make sure it wont blow up in case of unexpected output
- time = 0.0
+ Open3.popen3(command) do |stdin, stdout, stderr, wait_thr|
+ pid = wait_thr.pid
+ begin
+ yield(0.0) if block_given?
+ next_line = Proc.new do |line|
+ fix_encoding(line)
+ output << line
+ if line.include?("time=")
+ if line =~ /time=(\d+):(\d+):(\d+.\d+)/ # ffmpeg 0.8 and above style
+ time = ($1.to_i * 3600) + ($2.to_i * 60) + $3.to_f
+ elsif line =~ /time=(\d+.\d+)/ # ffmpeg 0.7 and below style
+ time = $1.to_f
+ else # better make sure it wont blow up in case of unexpected output
+ time = 0.0
+ end
+ progress = time / @movie.duration
+ yield(progress) if block_given?
+ end
+ if line =~ /Unsupported codec/
+ FFMPEG.logger.error "Failed encoding...\nCommand\n#{command}\nOutput\n#{output}\n"
+ raise "Failed encoding: #{line}"
end
- progress = time / @movie.duration
- yield(progress) if block_given?
end
- if line =~ /Unsupported codec/
- FFMPEG.logger.error "Failed encoding...\nCommand\n#{command}\nOutput\n#{output}\n"
- raise "Failed encoding: #{line}"
+
+ if @@timeout != false
+ stderr.each_with_timeout(pid, @@timeout, "r", &next_line)
+ else
+ stderr.each("r", &next_line)
end
+
+ rescue Timeout::Error => e
+ FFMPEG.logger.error "Process hung...\nCommand\n#{command}\nOutput\n#{output}\n"
+ raise "Process hung"
end
end
View
2  lib/ffmpeg/version.rb
@@ -1,3 +1,3 @@
module FFMPEG
- VERSION = "0.8.5"
+ VERSION = "0.8.6"
end
View
1  lib/streamio-ffmpeg.rb
@@ -6,6 +6,7 @@
require 'ffmpeg/version'
require 'ffmpeg/errors'
require 'ffmpeg/movie'
+require 'ffmpeg/io_monkey'
require 'ffmpeg/transcoder'
require 'ffmpeg/encoding_options'
View
10 spec/ffmpeg/transcoder_spec.rb
@@ -29,6 +29,16 @@ module FFMPEG
FFMPEG.logger.should_receive(:info).at_least(:once)
end
+ it "should fail when IO timeout is exceeded" do
+ FFMPEG.logger.should_receive(:error)
+ movie = Movie.new("#{fixture_path}/movies/awesome_widescreen.mov")
+ Transcoder.timeout = 1
+ transcoder = Transcoder.new(movie, "#{tmp_path}/timeout.mp4")
+ lambda { transcoder.run }.should raise_error(RuntimeError, /Process hung/)
+ end
+
+ Transcoder.timeout = 200
+
it "should transcode the movie with progress given an awesome movie" do
FileUtils.rm_f "#{tmp_path}/awesome.flv"
View
4 streamio-ffmpeg.gemspec
@@ -13,8 +13,8 @@ Gem::Specification.new do |s|
s.summary = "Reads metadata and transcodes movies."
s.description = "Simple yet powerful wrapper around ffmpeg to get metadata from movies and do transcoding."
- s.add_development_dependency("rspec", "~> 2.7")
- s.add_development_dependency("rake", "~> 0.9.2")
+ s.add_development_dependency("rspec", ">= 2.7")
+ s.add_development_dependency("rake", ">= 0.9.2")
s.files = Dir.glob("lib/**/*") + %w(README.md LICENSE CHANGELOG)
end
Something went wrong with that request. Please try again.