-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added constructors to allow multiple running instances of FFmpeg #50
base: master
Are you sure you want to change the base?
Conversation
@evandixon Have you had success with this fork of the library? We are also using a fork of MediaToolkit and running into some concurrency issues currently in tests (since we have multiple tests that use this library running in parallel with xunit). The error we seem to be getting when there is concurrent access is I am thinking of pulling your changes into our fork as well, but wanted to check with you first to make sure it is working as expected for you, and to check and see if you have made any additional changes that are not in this pull request. Thanks, Dan Ludwig |
- There is no longer a singular FFmpeg process, because outside of the conversion function, it was only used during EngineBase.Dispose, where nothing happened because the conversion function disposes it - Added thread safety to EnsureFFmpegFileExists - Changed default path of the ffmpeg executable to be in the temporary directory, to avoid littering the root of the drive. Included in this path is a new Guid, which will allow it to be cleaned up without affecting other instances of Engine - Removed the event handler from FFmpegProcess.ErroDataReceived. In general (I don't know about this particular case), not removing event handlers can prevent the garbage collector from cleaning up the class
@danludwig Since I originally made this PR, I've started doing similar things for other tools. I've just pushed another commit that should fix additional concurrency issues. My original method of concurrent execution unfortunately overlooked exceptions caused by concurrent execution. I've since fixed it so that I'm notified when any exceptions are thrown. My latest commit has been tested in two ways:
Let me know if you see or experience any issues with this PR. Thanks for your interest, |
When doing batch conversions on multi-core computers, it takes less time to have multiple instances of FFmpeg running concurrently, which prior to this PR, was explicitly disabled. Now it's only disabled by default.