-
Notifications
You must be signed in to change notification settings - Fork 9
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
Context manager for AvailableData #122
Context manager for AvailableData #122
Conversation
@@ -72,7 +72,7 @@ fn main() { | |||
let bindings = bindgen::Builder::default() | |||
.header("wrapper.h") | |||
.clang_arg(format!("-I{}/include", dst.display())) | |||
.parse_callbacks(Box::new(bindgen::CargoCallbacks)) | |||
.parse_callbacks(Box::new(bindgen::CargoCallbacks::new())) |
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.
What changed here? Something in bindgen?
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 beleive this means rerun_on_header_files is on by default
fn map_read(&self, stream_id: u32) -> Result<(*mut capi::VideoFrame, *mut capi::VideoFrame)> { | ||
let mut beg = null_mut(); | ||
let mut end = null_mut(); | ||
unsafe { | ||
capi::acquire_map_read(self.inner.as_ptr(), stream_id, &mut beg, &mut end).ok()?; | ||
} | ||
Ok((beg, end)) | ||
} | ||
|
||
fn unmap_read(&self, stream_id: u32, consumed_bytes: usize) -> Result<()> { | ||
unsafe { | ||
capi::acquire_unmap_read(self.inner.as_ptr(), stream_id, consumed_bytes).ok()?; | ||
} | ||
Ok(()) | ||
} |
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.
❤️
#132 strikes again. |
I wonder if there's a way to avoid the if. null context manager? @andy-sweet |
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.
Generally looks fine. We could consider adding support for an empty AvailableData
, which might help clean up some of the Python examples and avoid the need for some of the checks in the with
statements, but I think that can be done later too.
I think I found a few missed lines that could be deleted now. We could also consider removing the TODO at the end of __init__.py
, though this only adds context manager for available data and not for runtime/start/stop.
…ithub.com:nclack/acquire-python into 68-availabledata-relies-on-deletion-for-cleanup-1
Yeah, I agree that empty
I've removed those |
closes #68
This is an alternative to #70.
This is in draft until I feel more confident about the tests and the napari plugin. But I think it's close.