-
Notifications
You must be signed in to change notification settings - Fork 77
Fix load_torchcodec_shared_libraries on Windows #1109
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
Conversation
Add support for locating FFmpeg DLLs on Windows.
|
Hi @aboood40091! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
NicolasHug
left a comment
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.
Thanks @aboood40091
I'm happy to try that out if CI is green. It's very hard to debug Windows issues for us because we don't have easy access to Windows machines, so I do appreciate the help on this.
What I do not understand though: why are users experiencing these issues, when our Windows CI jobs are just fine? There must be a fundamental env difference that I'm currently missing?
src/torchcodec/_core/ops.py
Outdated
| versions 4, 5, 6, 7, and 8. On Windows, ensure you've installed | ||
| the "full-shared" version. |
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 did a quick search and I wasn't able to find an authoritative source on what a full-shared version is. I assume it's an version that ships dll instead of statically linking all libraries.
My concern is that this error message may not be directly actionable for users, unless we add more details on how they can install such a "full-shared" version. These details aren't really in scope here, so maybe we can leave this error message as it was?
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.
Correct. "full-shared" is the version that ships DLLs instead of statically linking them. "full-shared" is the term used in the download page.
Personally, I did not know the shared version is the one that has all DLLs until I started digging enough, so I figured this information would be helpful.
If you still would like me to reverse the change to the message, please let me know.
Personally, I don't know. It actually should not work for you under normal circumstances according to the Python documentation due to a breaking change in Python 3.8. Refer to: The use of |
NicolasHug
left a comment
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.
Thanks for the PR @aboood40091 . CI is failing with an unrelated issue which is also visible in #1111. I'll try to fix that first, then hopefully this PR will be green and I'll merge it.
Dan-Flores
left a comment
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.
The two error signals are known and being tracked in #1047, lets merge this PR. Thanks!
Co-authored-by: Nicolas Hug <[email protected]>
Co-authored-by: Nicolas Hug <[email protected]>
This reverts commit 79f985a.
|
@NicolasHug in your latest changes, |
|
😮 thanks for catching this @aboood40091 . Let me submit a follow-up. We're planning to publish a bugfix release later today with your windows fix. |


Add support for locating FFmpeg DLLs on Windows.
Fixes #1108, #1014, and several other issues.