Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

evandixon
Copy link

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.

@danludwig
Copy link

@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 System.ComponentModel.Win32Exception : Access is denied.

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
@evandixon
Copy link
Author

evandixon commented Feb 8, 2017

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

  • Concurrently executing Engine.Convert with a single instance of Engine
  • Concurrently executing Engine.Convert with each execution using a unique instance of Engine

Let me know if you see or experience any issues with this PR.

Thanks for your interest,
Evan Dixon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants