-
Notifications
You must be signed in to change notification settings - Fork 154
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
FFmpeg configuration updates fixing Windows build #385
Conversation
Signed-off-by: Robert Zhou <[email protected]>
Signed-off-by: Robert Zhou <[email protected]>
Signed-off-by: Robert Zhou <[email protected]>
Signed-off-by: Robert Zhou <[email protected]>
Chiming in to say that this patch fixed an openssl issue I was having with RV_DEPS_FFMPEG. Thanks for finding it! |
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 Once our internal CI runs on all platform, I'll comment about the refactor of the loop that I'd like to propose. |
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. |
I was hoping to use I am currently testing your changes on our internal CI. @meepzh |
This is fantastic @meepzh ! |
There was a problem hiding this 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.
653706c
into
AcademySoftwareFoundation:main
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
toRV_FFMPEG_CONFIG_OPTIONS
, sinceRV_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!