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

FFmpeg configuration updates fixing Windows build #385

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

meepzh
Copy link
Contributor

@meepzh meepzh commented Feb 4, 2024

Linked issues

Summarize your change.

This builds upon the work that Mark did to improve the configuration options for FFmpeg in #360 with Mark's blessing!

Describe the reason for the change.

This fixes use of PKG_CONFIG_PATH, RV_FFMPEG_COMMON_CONFIG_OPTIONS, RV_FFMPEG_PATCH_COMMAND_STEP (#15 (comment)), and other Windows-specific build issues such as #67 (comment).

Describe what you have tested and on which operating system.

This has been tested on Arch Linux 6.6.8 and Windows 10.0.22631, running the same test from here.

Add a list of changes, and note any that might need special attention during the review.

Specific details can be found here. I did not include the change to apply SEPARATE_ARGUMENTS to RV_FFMPEG_CONFIG_OPTIONS, since RV_FFMPEG_CONFIG_OPTIONS did not seem applicable given the CMake documentation, but I can include that change as well.

If possible, provide screenshots.

#360 (comment)

Thanks!

@wbpa-garrett
Copy link

Chiming in to say that this patch fixed an openssl issue I was having with RV_DEPS_FFMPEG. Thanks for finding it!

@cedrik-fuoco-adsk cedrik-fuoco-adsk self-requested a review February 20, 2024 13:21
@cedrik-fuoco-adsk
Copy link
Contributor

cedrik-fuoco-adsk commented Feb 20, 2024

Thanks for the contribution @meepzh!

I encountered the problem myself, and I came up with a similar solution. The difference is that I don't have that loop that you didn't like.

I'd like to propose a change for that loop. Do you have the time to do an extra commit? @meepzh
If you don't have time, no problem, I can take over the changes.

Once our internal CI runs on all platform, I'll comment about the refactor of the loop that I'd like to propose.

@meepzh
Copy link
Contributor Author

meepzh commented Feb 20, 2024

Thanks Cedrik! Sure, I can add it. I saw the commit you've pinged me in and learned some new things about CMake! I'll await your comment then.

@cedrik-fuoco-adsk
Copy link
Contributor

cedrik-fuoco-adsk commented Feb 20, 2024

I was hoping to use cmake_path from CMake to convert the paths, but I'm not 100% confident that it works in some edge cases. So for now, your changes are great and fix the issues. Thank you!

I am currently testing your changes on our internal CI. @meepzh

@bernie-laberge
Copy link
Contributor

bernie-laberge commented Feb 20, 2024

This is fantastic @meepzh !
You fixed the issue I had on my end on Windows with Mark's original PR. Thanks!

Copy link
Contributor

@bernie-laberge bernie-laberge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the fix !
The fix builds fine on my Mac+Windows and also on our internal CIs on all platforms.
This is great.

@bernie-laberge bernie-laberge merged commit 653706c into AcademySoftwareFoundation:main Feb 20, 2024
2 checks passed
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.

4 participants