-
Notifications
You must be signed in to change notification settings - Fork 267
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
Add libheif (plus some small fixes) #2796
base: master
Are you sure you want to change the base?
Conversation
build/media-suite_compile.sh
Outdated
sed -i 's/find_package(vvdec 2.3.0)/find_package(vvdec 3.0.0)/' CMakeLists.txt | ||
sed -i 's/find_package(Doxygen)/#/' CMakeLists.txt # no configurable option? | ||
sed -i 's/find_package(Brotli)/#/' CMakeLists.txt # linking difficulties | ||
sed -i 's/find_package(TIFF)/#/' heifio/CMakeLists.txt # configure & linking difficulties |
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 would prefer to fix these upstream if possible. If not, or if upstream rejects changes to fix these, then I am fine with adding them here.
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'll open issue in the upstream... later... it's midnight where I live...
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.
Man, I don't think they are going to be fixed anytime soon, maybe we can merge it for now, or transfer them into patches?
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 would prefer transferring to patches since it makes it easier for me to know if it breaks or is no longer needed.
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.
Can you push to the branch and patches repo when you decide to merge this PR? Or shall I do it and also open a PR in the patches repo?
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 would prefer that you do it so the authorship of the changes is preserved
Where's that comment suggesting a shared option? |
Transfer the CMakeLists edits to patch. Add shared option. And some cosmetics.
Idk, I think it was something about making it so that most of the binaries are shared or something since they link a lot of libraries? |
I moved the comment to the feature request - essentially, with shared compilation it's 4x small exe + 1x large heif.dll instead of 4x large exe. Compling av1an w/ mabs has both options, shared and static. I know the shared result from compiling libheif myself manually once, but gave up on it due to the hassle with broken doxygen (and everything else the pull reques solves by patching/sed) |
Implements #2516
It's not perfect but at least most important functions are working, and I don't think I can fix the remaining issues.