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

Support empty AvailableData structs #138

Merged

Conversation

aliddell
Copy link
Member

@aliddell aliddell commented Dec 14, 2023

Closes #137.

  • Change version string from 0.3.0 to 0.3.0-rc1.
  • Updates acquire-core-libs and acquire-video-runtime submodules.
  • Updates acquire-driver-zarr driver to v0.1.8 (bugfix release).
  • Create an AvailableData struct on every call to runtime.get_available_data(). It's no longer Optional, i.e., will never be None.

🚨🚨 Potentially controversial decisions below 🚨🚨

  1. Remove the unused _store field from VideoFrame struct. This was causing test_setup() to fail when I did not explicitly delete frames returned by the video frame iterator.
  2. As a consequence of (1), the store field of VideoFrameIteratorInner struct was unused, so I removed this as well.
  3. As a consequence of both (1) and (2), the only place RawAvailableData is used is as a member inner of AvailableData, so I made AvailableData.inner an Option<RawAvailableData>, where it was previously an Option<Arc<RawAvailableData>>.

@aliddell aliddell requested review from nclack and andy-sweet and removed request for nclack December 14, 2023 19:58
@aliddell
Copy link
Member Author

aliddell commented Dec 20, 2023

@nclack @andy-sweet This PR is ready to go, but Ghanima needs to update rustc first. We should also address the eGrabber situation.

Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

Python code looks much better now.

But I'm less confident on the Rust changes. Here's what I understand.

  • RawAvailableData unmaps frame data when it's dropped.
  • AvailableData just wraps RawAvailableData as an optional and implements its methods appropriately.
  • AvailableDataContext maps frame data it's entered and will trigger RawAvailableData to be dropped when exited.

Based on this, as a Python client, I assume that the only way to access frame data safely is to use with runtime.get_available_data(...) and once that has exited, it is no longer safe to refer to any frames or their data unless i made a copy of that data.

Therefore, I think removing _store and the subsequent changes seems safe. Previously I understood that existed to share ownership of RawAvailableData and thus keep it alive (and its data mapped) while any of those objects (e.g. VideoFrame) continued to exist. That ownership model seems more complicated and possibly buggy (based on your comments about the failing tests) - it also contrasts with the paragraph above.

So, I roughly approve this. But it definitely needs a look from @nclack before merging.

Copy link
Member

@nclack nclack left a comment

Choose a reason for hiding this comment

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

We want to hide the invalidate method.

There is a lifetime issue with the VideoFrame - it would be nice if it were invalidated outside of the AvailableDataContext. Adding back the Arc<AvailableData> and generating an informative error should be fine.

That doesn't help with invalidated numpy arrays. I think we can address that in a separate issue. That probably requires making a custom object that implements the numpy array interface protocol. In the meantime we should document that the numpy array has a lifetime limited to the context.

src/runtime.rs Show resolved Hide resolved
@aliddell aliddell self-assigned this Jan 3, 2024
Copy link
Member

@nclack nclack left a comment

Choose a reason for hiding this comment

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

lgtm. I left a comment/question but I don't think it's blocking

src/runtime.rs Outdated
@@ -273,7 +282,8 @@ impl Drop for RawAvailableData {

#[pyclass]
pub(crate) struct AvailableData {
inner: Option<Arc<RawAvailableData>>,
inner: Option<RawAvailableData>,
valid: Arc<Mutex<bool>>,
Copy link
Member

Choose a reason for hiding this comment

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

This works but the valid flag is redundant with the Option on inner - they serve the same purpose. So I might have done Arc<Mutex<Option<RawAvailableData> here.

I'm wondering if the Arc<Mutex<...>> is really necessary though. I believe AvailableData is always accessed with the GIL.

Copy link
Member Author

Choose a reason for hiding this comment

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

I started out with store: Arc<AvailableData> as a member in both VideoFrameIterator and VideoFrame, but I couldn't create one in AvailableData::frames() since that only has a reference to self. So I went with this Arc<Mutex<bool>> approach, which unfortunately needs to have this member here if only as a way to signal downstream VideoFrameIterator and VideoFrame objects, since they can't get an Arc of the AvailableData itself.

But actually typing this out has made me think to try with store: Arc<&AvailableData>, but now I have to figure out how to satisfy the borrow checker. My Rust is weak or I'd have thought of it sooner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying it out I pretty quickly ran into this problem. (Explanation from PyO3 here.) Because we have these pyclasses, I can't specify lifetimes on the AvailableData reference. So unfortunately I think the ugly Arc<Mutex<bool>> option is the only way to go. We can discuss at standup.

Copy link
Member

@nclack nclack left a comment

Choose a reason for hiding this comment

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

lgtm! thanks!

@aliddell aliddell merged commit 9ac674f into acquire-project:main Jan 5, 2024
17 of 20 checks passed
@aliddell aliddell deleted the 137-support-empty-availabledata branch January 5, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

The runtime should support empty AvailableData structs
3 participants