Added timeout option to detect hung ffmpeg process #24

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
5 participants
@stakach
Contributor

stakach commented Apr 4, 2012

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

@dbackeus

This comment has been minimized.

Show comment Hide comment
@dbackeus

dbackeus Apr 4, 2012

Collaborator

Thanks for this.

It's something that has been on my todo list but I've been too annoyed with the complexity to do anything about it.

I might pull this in but do some refactoring before releasing. Some things I'm thinking about;

  • put the IO hack in a separate file
  • skip the timeout option and just have a healthy default that should work for everyone (ffmpeg should usually give feedback constantly right?)
  • add some tests (not sure if we can mock this behavior for the unit tests, for an integration test I have a file that always crashes at 84% so I could test on that but it would be super slow)

Feel free to discuss and work on these ideas. It might be some time before I can get to it myself.

Collaborator

dbackeus commented Apr 4, 2012

Thanks for this.

It's something that has been on my todo list but I've been too annoyed with the complexity to do anything about it.

I might pull this in but do some refactoring before releasing. Some things I'm thinking about;

  • put the IO hack in a separate file
  • skip the timeout option and just have a healthy default that should work for everyone (ffmpeg should usually give feedback constantly right?)
  • add some tests (not sure if we can mock this behavior for the unit tests, for an integration test I have a file that always crashes at 84% so I could test on that but it would be super slow)

Feel free to discuss and work on these ideas. It might be some time before I can get to it myself.

@stakach

This comment has been minimized.

Show comment Hide comment
@stakach

stakach Apr 4, 2012

Contributor

Cool, I'll point my gemfile to my fork and make these changes.

I made the timeout optional as I wasn't sure if you wanted to use the feature, rvideo has a 200 second timeout so I'll use that.

As for tests I have a few example files that fail. Might not be consistent across versions or OS's though.

Contributor

stakach commented Apr 4, 2012

Cool, I'll point my gemfile to my fork and make these changes.

I made the timeout optional as I wasn't sure if you wanted to use the feature, rvideo has a 200 second timeout so I'll use that.

As for tests I have a few example files that fail. Might not be consistent across versions or OS's though.

Stephen von Takach added some commits Apr 5, 2012

Stephen von Takach
Broke out IO monkey patch
* Merged screen shot code from @flatzo
* Updated tests
* Added documentation to the readme
* Defaults to 200 second timeout that is configurable
@stakach

This comment has been minimized.

Show comment Hide comment
@stakach

stakach Apr 5, 2012

Contributor

Ok, I've made all the changes. I've left the timeout option just made it default to 200 seconds and for the timeout to be configurable (no API changes). This way it can be turned off, modified, if desired.

I've also merged in the screenshot feature from @flatzo as it is handy and doesn't break existing code
I've bumped the version too.

Contributor

stakach commented Apr 5, 2012

Ok, I've made all the changes. I've left the timeout option just made it default to 200 seconds and for the timeout to be configurable (no API changes). This way it can be turned off, modified, if desired.

I've also merged in the screenshot feature from @flatzo as it is handy and doesn't break existing code
I've bumped the version too.

@rheaton

This comment has been minimized.

Show comment Hide comment
@rheaton

rheaton Apr 5, 2012

@stakach I'm gonna track my gem off your branch, with the error-handling and thumbnails. Hopefully it will get merged in soon! :)

rheaton commented Apr 5, 2012

@stakach I'm gonna track my gem off your branch, with the error-handling and thumbnails. Hopefully it will get merged in soon! :)

@dbackeus

This comment has been minimized.

Show comment Hide comment
@dbackeus

dbackeus Apr 10, 2012

Collaborator

I'm sorry but I would like to treat different pull requests as different pull requests as they are related to different issues.

The best practice is that you have separate branches for the different features and not request against your master as you might want to have many new features in there, bump version numbers etc, which is actually not part of the issue.

Right now I cannot even cherry pick individual commits as the one called "Broke up IO monkey patch" is actually doing a lot of other things.

So my only option for a merge would be to copy paste from your completed code and make my own commits. If you feel like you have time it would be nice if you could reset back a few commits and re-create them keeping focused on only the hung process.

It's nice to see that many people are interested in the screenshot method but if I pull it in I will do so from its own request.

Collaborator

dbackeus commented Apr 10, 2012

I'm sorry but I would like to treat different pull requests as different pull requests as they are related to different issues.

The best practice is that you have separate branches for the different features and not request against your master as you might want to have many new features in there, bump version numbers etc, which is actually not part of the issue.

Right now I cannot even cherry pick individual commits as the one called "Broke up IO monkey patch" is actually doing a lot of other things.

So my only option for a merge would be to copy paste from your completed code and make my own commits. If you feel like you have time it would be nice if you could reset back a few commits and re-create them keeping focused on only the hung process.

It's nice to see that many people are interested in the screenshot method but if I pull it in I will do so from its own request.

@rheaton

This comment has been minimized.

Show comment Hide comment
@rheaton

rheaton Apr 10, 2012

Whatever gets this in! I'm working on a Carrierwave extension (as a gem) and it would be great to be able to add screen-shots to it! :)

rheaton commented Apr 10, 2012

Whatever gets this in! I'm working on a Carrierwave extension (as a gem) and it would be great to be able to add screen-shots to it! :)

@stakach

This comment has been minimized.

Show comment Hide comment
@stakach

stakach Apr 10, 2012

Contributor

@rheaton happy to gemify my fork and place it on ruby gems. I need screenshots too.

Contributor

stakach commented Apr 10, 2012

@rheaton happy to gemify my fork and place it on ruby gems. I need screenshots too.

@flatzo

This comment has been minimized.

Show comment Hide comment
@flatzo

flatzo Apr 10, 2012

I totaly agree that screenshot should be merged and I'm pretty sure that with all those people wanting the feature, they'll consider adding it. In the mean time, instead of making a gem for so little changes, using bundler and a gemfile might be a better idea.

gem "streamio-ffmpeg", :git => "git://github.com/stakach/streamio-ffmpeg.git"

In the worst case, we could start maintaining a new fork as their is no restriction in the licence. But this is extremly demanding and could be easily avoided by just following te "rules". Let's see if @dbackeus merges screenshot in the next days. @stakach you might then consider creating a new branch, without the merging of my commits and make a new pull request to help @dbackeus maintaining the repo.

flatzo commented Apr 10, 2012

I totaly agree that screenshot should be merged and I'm pretty sure that with all those people wanting the feature, they'll consider adding it. In the mean time, instead of making a gem for so little changes, using bundler and a gemfile might be a better idea.

gem "streamio-ffmpeg", :git => "git://github.com/stakach/streamio-ffmpeg.git"

In the worst case, we could start maintaining a new fork as their is no restriction in the licence. But this is extremly demanding and could be easily avoided by just following te "rules". Let's see if @dbackeus merges screenshot in the next days. @stakach you might then consider creating a new branch, without the merging of my commits and make a new pull request to help @dbackeus maintaining the repo.

@stakach

This comment has been minimized.

Show comment Hide comment
@stakach

stakach Apr 10, 2012

Contributor

@flatzo sounds good. I should have pulled your changes in properly or forked from your branch.
If the screenshot code is merged I'll make a fork for the hung process detection and a new pull request.

Contributor

stakach commented Apr 10, 2012

@flatzo sounds good. I should have pulled your changes in properly or forked from your branch.
If the screenshot code is merged I'll make a fork for the hung process detection and a new pull request.

@dbackeus

This comment has been minimized.

Show comment Hide comment
@dbackeus

dbackeus Apr 11, 2012

Collaborator

I'll have a look at merging screenshot when I get back from journey to Marocco in about a weeks time.

Collaborator

dbackeus commented Apr 11, 2012

I'll have a look at merging screenshot when I get back from journey to Marocco in about a weeks time.

@dbackeus

This comment has been minimized.

Show comment Hide comment
@dbackeus

dbackeus Apr 30, 2012

Collaborator

Closing this in favor of #27

Collaborator

dbackeus commented Apr 30, 2012

Closing this in favor of #27

@dbackeus dbackeus closed this Apr 30, 2012

@rheaton

This comment has been minimized.

Show comment Hide comment
@rheaton

rheaton May 22, 2012

Is there going to be a version bump for this change? Is master stable right now?

Thanks, Rachel

rheaton commented May 22, 2012

Is there going to be a version bump for this change? Is master stable right now?

Thanks, Rachel

@stakach

This comment has been minimized.

Show comment Hide comment
@stakach

stakach May 22, 2012

Contributor

@rheaton this pull request was superseded by #27
It's not merged yet as @guyboertje is working on jRuby compatibility

Contributor

stakach commented May 22, 2012

@rheaton this pull request was superseded by #27
It's not merged yet as @guyboertje is working on jRuby compatibility

@guyboertje

This comment has been minimized.

Show comment Hide comment
@guyboertje

guyboertje May 23, 2012

Sorry chaps, haven't had a chance - been in NZ on family matters, will soon tho.

Sorry chaps, haven't had a chance - been in NZ on family matters, will soon tho.

@rheaton

This comment has been minimized.

Show comment Hide comment
@rheaton

rheaton May 23, 2012

@guyboertje and @stakach thanks! looking forward to it! :)

rheaton commented May 23, 2012

@guyboertje and @stakach thanks! looking forward to it! :)

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