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

Fix stalled pipe on Windows when using target quality #596

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

woot000
Copy link
Contributor

@woot000 woot000 commented Mar 18, 2022

This is my first time making changes to Rust code, so let me know if the styling is wrong or if I made a glaring mistake with this change. It works perfectly on my machine

@redzic
Copy link
Collaborator

redzic commented Mar 18, 2022

The cause of this problem appears to be related to rust-lang/rust#45572, where the pipe buffer becomes full, blocking further output. The only problem I have with this approach is that we lose the error reporting from the stderr of the child processes, so debugging any issues with target quality crashes would become a big pain without any error messages to go off of.

Perhaps we could look into some other solution for handling this? I would ideally want something that fixes this while keeping the error reporting that we currently have.

@woot000
Copy link
Contributor Author

woot000 commented Mar 18, 2022

I suppose stderr could be redirected to a file, though that would necessitate a lot of changes to the vmaf_probe function, which is out of the scope of my abilities for Rust coding

I think for the time being, this is okay as a temporary fix

)

I haven't seen any reports of this being an issue on Linux, so this will only affect Windows builds for the time being
gh actions should've let me off the hook for this silly oversight
Copy link
Collaborator

@redzic redzic left a comment

Choose a reason for hiding this comment

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

Sorry for the extremely slow response, I'll merge this as a temporary fix while I look for a solution that has proper error handling/reporting.

@mergify mergify bot merged commit 2eff949 into master-of-zen:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants