-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Fixed missing audio with video_reader backend #3934
Fixed missing audio with video_reader backend #3934
Conversation
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 changes look good. Do you think we should put a unit-test to test for this case?
Yeah that makes sense, added the unit test. |
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.
This failing test seems related. Marking as requires changes to avoid accidental merges. Let's check it out next week. :)
Following the reported internal issue, I took this patch and applied it on latest master:
Then opened the same file as on the bug report:
Not sure if it's the same issue or a different one. Thoughts? Edit: The problem persists even after setting the backend. Might be unrelated to the fixes of the PR and instead be a separate problem:
|
Sorry, the current PR fixes missing audio with
By default, we use |
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.
@prabhat00155 thanks for the PR, LGTM.
Edit:
Unfortunately I see a segmentation fault that looks relevant. See unittest_linux_cpu_py3.9
@datumbox The seg fault was caused by incompatible ffmpeg version, which was fixed in #4041. It should be fine now. |
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.
LGTM, thanks!
Summary: * Fixed missing audio with video_reader backend * Added unit test Reviewed By: fmassa Differential Revision: D29264318 fbshipit-source-id: de95e0bd38d2f844c756652fe42de99b1ab32210
Resolves #3890.