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

Fix for 4933: FFMPeg detection. #4963

Closed
wants to merge 12 commits into from

Conversation

@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented Jan 5, 2021

If a version of ffmpeg is discovered that is outside the recommended version - this continues to look for a better one.

Also fixes #4933, whereby a version being outside of the recommended results in no version being selected, as function returns false.

fix
@crobibero crobibero self-requested a review Jan 16, 2021
BaronGreenback and others added 4 commits Jan 16, 2021
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
{
_logger.LogWarning("FFmpeg: {Location}: Failed version check: {Path}", location, path);
if (EncoderValidator.BestVersion(version, _bestVersion))

This comment has been minimized.

@cvium

cvium Jan 18, 2021
Member

Couple of things:

  1. BestVersion is not a great function name and neither is the return value. It's very hard to decipher what it does.
  2. The xmldocs are incorrect or the usage is incorrect: <returns><c>false</c> if source is better, <c>true</c> if destination is better.</returns>.
  3. ValidateVersion returning a Version instead of a bool is also not great.

I would suggest that BestVersion is changed to something like TryUpdateVersion that conditionally updates the "current" version in the validator, then after the "loop" is completed, the path etc. are pulled from the validator into the the media encoder. Maybe some more changes are required in the "call stack" for this to make more sense though.

This comment has been minimized.

@BaronGreenback

BaronGreenback Jan 18, 2021
Author Contributor

changed names. Hopefully, it will be easier to follow now.

Copy link
Member

@Bond-009 Bond-009 left a comment

I don't think we should do this.
If a given ffmpeg is to old that's a configuration mistake, we shouldn't try to be smart and go around it.
If there is no ffmpeg given, we should use the (first) one in $PATH (let the OS handle that) not being smart about searching for one that meets our criteria.

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented Jan 19, 2021

@Bond-009 The code already checks in 3 places for a version of the ffmpeg. Custom Path, Argument path, then $PATH.

The problem is that it currently doesn't use the first one it finds if it's not within the min/max range.
It continues then says it hasn't found one.

It either needs to fail, or look for a better one. At present it does neither. #4933,

@Bond-009
Copy link
Member

@Bond-009 Bond-009 commented Jan 19, 2021

In the case of #4933 it should warn (like it did) and set the ffmpeg path to the path passed via env/cmd, if no path was passed we should just use"ffmpeg" and let the OS figure out the full path.
The current code is indeed incorrect, but making it more complicated isn't the right solution IMHO.

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented Jan 21, 2021

@Bond-009 : Alternative solution #5073

Release 10.7.0 automation moved this from Active PRs to Completed PRs Feb 27, 2021
@crobibero crobibero removed this from Completed PRs in Release 10.7.0 Feb 28, 2021
@BaronGreenback BaronGreenback deleted the BaronGreenback:ffmpegFix branch Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants