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

Add rpath fixups to Mac non-GPL FFmpeg build #211

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Sep 13, 2024

We had previously thought that the rpath fixup stuff specific to Mac that torchaudio did was not necessary for us. The work I'm doing in #210 indicates that we actually do need this. What leads me to this conclusion is when I do otool -L (Mac's equivalent to ldd):

otool -L src/torchcodec/libtorchcodec4.dylib
src/torchcodec/libtorchcodec4.dylib:
	@rpath/libtorchcodec4.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libc10.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libpython3.9.dylib (compatibility version 3.9.0, current version 3.9.0)
	/Users/ec2-user/runner/_work/torchcodec/torchcodec/pytorch/torchcodec/ffmpeg/lib/libavutil.56.dylib (compatibility version 56.0.0, current version 56.70.100)
	/Users/ec2-user/runner/_work/torchcodec/torchcodec/pytorch/torchcodec/ffmpeg/lib/libavcodec.58.dylib (compatibility version 58.0.0, current version 58.134.100)
	/Users/ec2-user/runner/_work/torchcodec/torchcodec/pytorch/torchcodec/ffmpeg/lib/libavformat.58.dylib (compatibility version 58.0.0, current version 58.76.100)
	/Users/ec2-user/runner/_work/torchcodec/torchcodec/pytorch/torchcodec/ffmpeg/lib/libavdevice.58.dylib (compatibility version 58.0.0, current version 58.13.100)
	/Users/ec2-user/runner/_work/torchcodec/torchcodec/pytorch/torchcodec/ffmpeg/lib/libavfilter.7.dylib (compatibility version 7.0.0, current version 7.110.100)
	@rpath/libtorch.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libtorch_cpu.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1700.2[55](https://github.com/pytorch/torchcodec/actions/runs/10841142043/job/30084689904?pr=210#step:9:56).0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)

Note that things like libc++ and libtorch have @rpath/ in front of them, indicating (I believe) that they can be dynamically loaded. The FFmpeg libraries are hard-coded to where they were during build time. This causes failures at import time, because the libraries can't be found.

In order to test, I had versions of this PR where I hacked the build_ffmpeg.yaml workflow to run on the pull request. It succeeds: https://github.com/pytorch/torchcodec/actions/runs/10842665681

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 13, 2024
@scotts scotts force-pushed the ffmpeg_rpath branch 2 times, most recently from a77bca0 to ea0c1de Compare September 13, 2024 03:41
@scotts scotts marked this pull request as ready for review September 13, 2024 03:57
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

I'll stamp to unblock, but I don't really understand what this PR is doing. I have a decent understanding of rpath and library resolution on linux, but I don't know much about MacOS. So I don't know what all these -change and -id edits are doing.

Before merging, can you please show the output of otool -L src/torchcodec/libtorchcodec4.dylib on the newly built binaries?

I'll show you how to upload on S3.

@scotts
Copy link
Contributor Author

scotts commented Sep 13, 2024

@NicolasHug, agreed on not knowing exactly what everything is doing. I'm afraid the only real test is if it works in our build process once uploaded. In the event we need to fix something in this domain, I do think there's enough public docs for us to figure it out.

Based on what you asked, this PR does seem to be doing the right thing. This is what the library looks like from the old build:

[scottas@local macos_arm64] otool -L ffmpeg4/lib/libavutil.56.dylib         
ffmpeg4/lib/libavutil.56.dylib:
	/Users/ec2-user/runner/_work/torchcodec/torchcodec/pytorch/torchcodec/ffmpeg/lib/libavutil.56.dylib (compatibility version 56.0.0, current version 56.70.100)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2419.0.0)
	/System/Library/Frameworks/CoreVideo.framework/Versions/A/CoreVideo (compatibility version 1.2.0, current version 1.5.0)
	/System/Library/Frameworks/CoreMedia.framework/Versions/A/CoreMedia (compatibility version 1.0.0, current version 1.0.0)

And this is what it looks like after this PR:

[scottas@local macos_arm64] otool -L ffmpeg4/lib/libavutil.56.dylib 
ffmpeg4/lib/libavutil.56.dylib:
	@rpath/libavutil.56.dylib (compatibility version 56.0.0, current version 56.70.100)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2419.0.0)
	/System/Library/Frameworks/CoreVideo.framework/Versions/A/CoreVideo (compatibility version 1.2.0, current version 1.5.0)
	/System/Library/Frameworks/CoreMedia.framework/Versions/A/CoreMedia (compatibility version 1.0.0, current version 1.0.0)

That is, it looks like libavutil.56.dylib should now be able to be dynamically loaded, as it's no longer pointing to an exact file.

@scotts scotts merged commit 94c5114 into pytorch:main Sep 13, 2024
16 of 17 checks passed
@scotts scotts deleted the ffmpeg_rpath branch September 13, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants