Added timeout option to detect hung ffmpeg process #27

Merged
merged 4 commits into from Jun 27, 2012

Conversation

Projects
None yet
4 participants
Contributor

stakach commented Apr 24, 2012

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

Collaborator

dbackeus commented Apr 30, 2012

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.

Contributor

stakach commented Apr 30, 2012

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

Contributor

stakach commented Apr 30, 2012

Thoughts?

Collaborator

dbackeus commented Apr 30, 2012

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!

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.

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.

Contributor

stakach commented May 1, 2012

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

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

Collaborator

dbackeus commented May 2, 2012

That's awesome, hope it works out 👍

Contributor

stakach commented Jun 5, 2012

@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.

Collaborator

dbackeus commented Jun 5, 2012

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.

Contributor

stakach commented Jun 5, 2012

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

Contributor

stakach commented Jun 5, 2012

Also enjoy your holiday!

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.

Collaborator

dbackeus commented Jun 27, 2012

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

dbackeus added a commit that referenced this pull request Jun 27, 2012

Merge pull request #27 from stakach/master
Added timeout option to detect hung ffmpeg process

@dbackeus dbackeus merged commit 3b7053a into streamio:master Jun 27, 2012

Collaborator

dbackeus commented Jun 27, 2012

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+.

@@ -1,8 +1,22 @@
require 'open3'
require 'shellwords'
+if RUBY_PLATFORM =~ /(win|w)(32|64)$/
+ require 'win32/process'
@dbackeus

dbackeus Jun 27, 2012

Collaborator

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

+ require 'win32/process'
+ Process.kill(1, pid)
+ else
+ Process.kill('INT', pid)
@dbackeus

dbackeus Jun 27, 2012

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?

Contributor

stakach commented Jun 27, 2012

@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

Contributor

stakach commented Jun 27, 2012

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

Collaborator

dbackeus commented Jun 27, 2012

@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.

Contributor

stakach commented Jun 27, 2012

@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.

Collaborator

dbackeus commented Jun 28, 2012

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.

Contributor

stakach commented Jun 28, 2012

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

Epic work!

Contributor

stakach commented Jun 28, 2012

@dbackeus one more thing just to polish it off

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

Collaborator

dbackeus commented Jun 29, 2012

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