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

audioPipeFd and videoPipeFd used in close() are never initialized. #21

Closed
smallfly opened this issue Jul 1, 2015 · 5 comments
Closed

Comments

@smallfly
Copy link
Contributor

smallfly commented Jul 1, 2015

In the close() methods there are calls to setNonblocking():

//set pipes to non_blocking so we dont get stuck at the final writes
setNonblocking(audioPipeFd);
setNonblocking(videoPipeFd);

But it seems that the variables audioPipeFd and videoPipeFd are never initialized.
Should the calls the setNonblocking() be removed?

@timscaffidi
Copy link
Owner

yes it appears those should be moved instead to be called in a function of the audio/video writer threads instead, since the file descriptors are no longer held by the main thread

@smallfly
Copy link
Contributor Author

I'm not sure I understand what you mean. Could you give more details?

@timscaffidi
Copy link
Owner

Since the file descriptors moved to the writer threads, the main thead can't see them, so these calls are not going to do what is intended anymore. However, if the audio/video thread classes could have a public method that calls setNonblocking on their internal file descriptors, these new functions could be invoked in the same place as the current calls.

@smallfly
Copy link
Contributor Author

ok I see. I can look in to that later. But what does that mean for the current version of the addon. Could these 'broken' calls be the source of issues at runtime?

smallfly added a commit to smallfly/ofxVideoRecorder that referenced this issue Jul 20, 2015
timscaffidi added a commit that referenced this issue Jul 21, 2015
Fixed the 'non blocking pipe' issue - #21.
@timscaffidi
Copy link
Owner

They may have caused problems, but I haven't had time to adequately test. Thanks for the fix!

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

No branches or pull requests

2 participants