-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
libobs/graphics: Update libnsgif to 1.0.0 #11178
base: master
Are you sure you want to change the base?
Conversation
This could use further review, particularly on the struct changes, but generally on everything because I've never had to touch these files before. This could also use further testing. |
@@ -56,7 +56,7 @@ target_sources( | |||
util/windows/window-helpers.h | |||
) | |||
|
|||
target_compile_options(libobs PRIVATE $<$<COMPILE_LANGUAGE:C,CXX>:/EHc->) | |||
target_compile_options(libobs PRIVATE $<$<COMPILE_LANGUAGE:C,CXX>:/EHc-> /wd4244 /wd4267) |
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.
I'm not really a fan of disabling these warnings for all of libobs just because of a vendored dependency. Can we perhaps move it into deps/
?
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.
We can probably try, but it will require a "bit" more work.
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.
To be clear, I'm not a fan of it either.
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.
An attempt was made:
https://github.com/RytoEX/obs-studio/tree/really-update-libnsgif-and-move
However, while libobs compiles, other projects that link to libobs that do #include <graphics/image-file.h>
(obs-filters, obs-transitions, etc.) complain that they cannot find nsgif.h. I'm not sure what to do about that.
3599ae1
to
f42b501
Compare
Fixing clang-format complaint, hopefully. |
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.
Would be nice to get this moved over to the deps dir but I don't think that it's a big deal for now, especially if we want to fix up issues for the next update.
I want to suggest to hide |
cc @Lain-B and @derrod on the breaking changes concern raised by @norihiro My initial and primary concern was working out an update to an outdated vendored dependency. If there are additional concerns, such as moving the dependency to |
Description
Update libnsgif to from some ancient version to 1.0.0.
Motivation and Context
Wanted to attempt to update this while tackling #10993.
How Has This Been Tested?
Tested on Windows 11 with an animated GIF.
Types of changes
Checklist: