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

[multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4/opencv/discordcoreapi/pangolin/rtabmap #28434

Closed
wants to merge 14 commits into from

Conversation

reitowo
Copy link
Contributor

@reitowo reitowo commented Dec 19, 2022

Upgrade ffmpeg to 5.1.2.

Following downstream ports are updated:
opencv4 to 4.7.0.
opencv to 4.7.0.
discordcoreapi to 2023-01-02.
pangolin to eab3d3449a33a042b1ee7225e1b8b593b1b21e3e.
rtabmap to 0.20.16.

TODO:

  • Update all patches, remove unneeded patches if upstream fixed or file doesn't exists any more.
  • Fix the problem failed compiling bin2c.exe under MSVC environment.
  • Fix the problem cause linker to hang on triplet x64-windows by fallback to yasm by using lld-link. The build of ffmpeg 5.1.2 will hang at linking avcodec #28390
  • Fix the aarch64 asm failure by updating gas-preprocessor.
  • Keep using ffmpeg port name.

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • scripts/test_ports/vcpkg-ci-ffmpeg/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
@Sibras
Copy link
Contributor

Sibras commented Dec 19, 2022

This is a duplicate of #23312 . See the thread as to outstanding issues preventing upgrading to 5.x.
Although this one jumps to a newer version it was requested that 5.0 series updates are applied in order they were released (i.e. the 5.0 series before 5.1)
One #23312 is merged ill promptly perform the required updates to the latest version.

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
@reitowo
Copy link
Contributor Author

reitowo commented Dec 19, 2022

@Sibras As ffmpeg is continuing 4.4.x 5.0.x 5.1.x, how about making this port names ffmpeg51 and you are ffmpeg50? What do you guys think?
As in #28432 , I think we can do nothing about the original port ffmpeg, just considering it is actually ffmpeg44.

@reitowo
Copy link
Contributor Author

reitowo commented Dec 19, 2022

So are you updating every downstream ports to support 5.0?
But I think 5.0 and 5.1 might also have dependency issues, and I think we should be like opencv to create opencv2 opencv3 opencv4 as it is so much simpler for each port maintainer, no need to update all downstream ports as the workload could be unreasonable. You can't count on some random guy to PR next version and update all downstream. So I suggest create ffmpeg51.

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
@reitowo
Copy link
Contributor Author

reitowo commented Dec 19, 2022

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 28434 in repo microsoft/vcpkg

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Jan 14, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 14, 2023
@reitowo reitowo changed the title [multiple-ports] Upgrade ffmpeg to 5.1.2 [multiple-ports] Upgrade ffmpeg[5.1.2] opencv4[4.7.0] opencv[4.7.0] discordcoreapi pangoli Jan 14, 2023
@reitowo reitowo changed the title [multiple-ports] Upgrade ffmpeg[5.1.2] opencv4[4.7.0] opencv[4.7.0] discordcoreapi pangoli [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4 opencv discordcoreapi pangoli Jan 14, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 14, 2023
@reitowo reitowo changed the title [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4 opencv discordcoreapi pangoli [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4/opencv/discordcoreapi/pangoli Jan 14, 2023
@reitowo reitowo changed the title [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4/opencv/discordcoreapi/pangoli [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4/opencv/discordcoreapi/pangolin Jan 14, 2023
@reitowo reitowo changed the title [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4/opencv/discordcoreapi/pangolin [multiple-ports] Upgrade ffmpeg[5.1.2] + opencv4/opencv/discordcoreapi/pangolin/rtabmap Jan 14, 2023
@reitowo
Copy link
Contributor Author

reitowo commented Jan 14, 2023

Downstream ports passed, reopen this PR.

Let's discuss about a better solution how to deal with the introduction of lld-link/clang-cl as a workaround, and what to do about features that needs change compiler.

I think put in feature is acceptable, and if you don't want to we can just remove features and make the choice for users.

According to the above discussion, the safest way should be:

  • When detects MSVC under x64, replace it with lld-link to cooperate with nasm, and we can prevent fallback to yasm.
  • Do not use clang-cl, because it may cause undefined symbol (Reported by @Neumann-A , I didn't encounter such thing.)
  • Introduce clang as nvcc by default to enable cuda LLVM compilation.
  • By doing these as a default behaviour, we can remove all additional features (use-clang-cl, use-lld-link, use-cuda-llvm, disable-inline-asm)

What do you think?

@reitowo reitowo marked this pull request as ready for review January 14, 2023 10:39
@Sibras
Copy link
Contributor

Sibras commented Jan 14, 2023

So I have created split PRs for some of the dependencies updated in this PR. Some are based off work done by @cnSchwarzer here, while others are not.

OpenCV 28949
discordcoreapi 28956

I also managed to fix the nasm linking issues on windows but I did this in my own local update (that I did when doing the 5.0 update that got merged recently and was based off work done before this PR) so its not based off this PR. As this PR does some things differently and changes more things than was needed for a straight version update it was easier to just create a new PR instead of trying to work out how to get it to work with this one.

FFmpeg 5.1.2 28955

@reitowo
Copy link
Contributor Author

reitowo commented Jan 15, 2023

@Sibras Thanks for sorting out linking problem! Closing this PR in favor of that.

@reitowo reitowo closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants