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

Aravis (camera) adapter #445

Merged
merged 30 commits into from
Jun 14, 2024
Merged

Conversation

HazenBabcock
Copy link
Contributor

This pull request adds a device adapter for the Aravis Project GenICam camera library.

I'm not sure this pull request is actually ready to go in. I'm opening this pull request now to get some feedback about what else should go into the device adapter and the ways that it does not correctly conform to the MM expected camera behavior.

It has been tested with two different cameras on 64bit linux (a mono FLIR camera and a color Basler camera). It supports mono and RGB images, exposure time and binning.

Hazen Babcock added 22 commits February 10, 2024 14:21
…rocessing. Fix large memory leak in sequence acquisition. Some code cleanup.
@HazenBabcock HazenBabcock changed the title Aravis (camera) adapter [WIP] Aravis (camera) adapter Feb 26, 2024
@marktsuchida
Copy link
Member

Awesome! I've been aware of Aravis and it's great to have a device adapter for it.

I note that there is no call to PrepareForAcq() and AcqFinished() when a sequence acquisition starts and ends. These are needed for Autoshutter to work corrently (the shutter is opened and closed in the respective calls). See, e.g., DemoCamera for usage (note that AcqFinished() should be called once the acquisition actually finished (e.g. due to finite frames requested), regardless of whether StopSequenceAcquisition() was called).

Another question is whether it would be a good idea to map the GenICam settings (other than the ones you specifically support) to MM properties. I think some other camera adapters that use vendor SDKs (that are based on GenICam) do something like that. Certainly optional, especially in an initial version.

Do you have any particular things you are wondering about? It's a bit hard to see through every possible issue just by looking at the code, so let me know if there are parts that you want me to pay special attention to.

@HazenBabcock
Copy link
Contributor Author

Thank you for the feedback, I will add PrepareForAcq() and AcqFinished(). Initially I want to make sure I'm doing the basics, i.e. acquiring images correctly. SnapImage() seems pretty straightforward, but the sequence acquisitions less so. For example, it looks to me like some device adapters subclass MMDeviceThreadBase and others don't. I don't think I understand the advantages of one approach over the other.

Do you mean map all of the available GenICam settings to MM properties? There are a lot and I don't think the Spinnaker or Basler device adapters do this. Or there is a specific subset that should be available through the device adapter?

One other question I have is whether MM provides functions for unpacking pixel formats like Mono10p or Mono12p. Formats like this are pretty common for these kinds of cameras, though I'm not sure how useful they would be in practice. It looks like the Spinnaker device adapter has some functions for this, but it might make more sense for these to be in separate file that any adapter can use? If they are not already.

Thank you for the interest.

@marktsuchida
Copy link
Member

some device adapters subclass MMDeviceThreadBase and others don't

No need to use it; please see #421. Also better to use std::thread for threading in new code (if you need threads).

For sequence acquisition, the way you have it (calling InsertImage() from within a frame callback) is the best way. (If the camera API required polling, then you'd need to start your own thread to fetch the frames. Many SDKs require this.)

Do you mean map all of the available GenICam settings to MM properties? There are a lot and I don't think the Spinnaker or Basler device adapters do this. Or there is a specific subset that should be available through the device adapter?

Yes, that is what I meant. I believe GigECamera (which is actually an adapter for the obsolete JAI SDK) does this by wrapping every GenICam node as an MM property -- but I could be wrong. Also it's hard to tell if it is a good idea or not. It was just an idea.

One other question I have is whether MM provides functions for unpacking pixel formats like Mono10p or Mono12p.

Not currently. I also don't know how important they are to support for microscopy use if the camera can also generate unpacked pixels, if all we do is unpack in software anyway. (It might be a different story if we could gain throughput by streaming Mono10p to an SSD, or even just user memory, for later processing.)

If supporting, it would probably make sense to have a file similar to MMDevice/Debayer.{h,cpp} to provide a common implementation. (If we want to make it more sophisticated (e.g. SIMD) we should probably build it into MMCore, but that'd be a whole new project requiring new API design.)

@HazenBabcock
Copy link
Contributor Author

I added PrepareForAcq() and AcqFinished() so I think sequence acquisition should now work as expected. At this point if there was someone else who is interested in testing this adapter and providing feedback that would be great. Otherwise I think "version 0" is ready to be merged?

Do you mean map all of the available GenICam settings to MM properties? There are a lot and I don't think the Spinnaker or Basler device adapters do this. Or there is a specific subset that should be available through the device adapter?

Yes, that is what I meant. I believe GigECamera (which is actually an adapter for the obsolete JAI SDK) does this by wrapping every GenICam node as an MM property -- but I could be wrong. Also it's hard to tell if it is a good idea or not. It was just an idea.

GenICam nodes have a "Visibility" property. It may make sense to create properties for all of the "Beginner" visibility nodes? I'm not sure how to do this programmatically right now, it seems like a pretty challenging thing to do..

One other question I have is whether MM provides functions for unpacking pixel formats like Mono10p or Mono12p.

Not currently. I also don't know how important they are to support for microscopy use if the camera can also generate unpacked pixels, if all we do is unpack in software anyway. (It might be a different story if we could gain throughput by streaming Mono10p to an SSD, or even just user memory, for later processing.)

This discussion reminds me that this can make a difference in maximum camera FPS, at least for some cameras, due to the bandwidth constraints of interfaces like USB3. So I think it is probably worth doing at some point.

@marktsuchida
Copy link
Member

@HazenBabcock Apologies for the long delay. The thing about generalized property support was just a thought, so let's leave it at that.

Can you add a license (BSD or LGPL) to a license.txt (and/or file headers)? Other than that, this should be ready to merge.

  • Does not interfere with build in the absence of the Aravis headers or libs

@HazenBabcock
Copy link
Contributor Author

I added a BSD license in the file headers.

@HazenBabcock
Copy link
Contributor Author

Everything is set now? Or?

@marktsuchida marktsuchida changed the title [WIP] Aravis (camera) adapter Aravis (camera) adapter Jun 14, 2024
@marktsuchida
Copy link
Member

Thanks for the reminder and sorry for the wait.

It will be good to later improve the build for this so that it can find a system-installed Aravis and so that it can run on Windows and possibly macOS. I'll create a separate issue for this and look into what I can do.

@marktsuchida marktsuchida merged commit ceb311c into micro-manager:main Jun 14, 2024
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