Skip to content

Allow camera device adapters to call InsertImage on snap #592

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

henrypinkard
Copy link
Member

@henrypinkard henrypinkard commented Mar 8, 2025

(Part of changes needed for the new camera API but should also work with existing camera API. Broken into its own PR for easier review.)

This PR allows camera device adapters, rather than having to maintain their own internal buffer for when SnapImage() is called, to instead simply call InsertImage (or for multi-channel cameras, call it once for each channel). The CoreCallback then determines whether the inserted data should be routed to the circular buffer or to an internal buffer maintained in the camera device instance in the core. Higher level code calling GetImage should not see any difference in behavior.

This will simplify writing device adapters for both old and new camera APIs, and it has the potential to improve performance, because copying of the snap buffer can use to multi-threaded copying code like in the circular buffer (in a future PR).

It might be a good addition to deprecate GetImageBuffer in the current camera api and make calling InsertImage the preferred approach.

@marktsuchida
Copy link
Member

I'm afraid this doesn't consdier how timing works for Snap/GetImage. SnapImage() must block for the duration of the camera exposure, but need not (and should not, if possible) wait for the image to be read out and transferred to the computer. GetImage() then waits for the read out (if necessary) and returns the image. If there is a shutter, it is closed (by autoshutter or the app) after SnapImage() returns to minimize unnecessary illumination. With many cameras and use cases, this behavior is quite critical.

That is not the nicest API design for multiple reasons, and (as you know) I also believe that in the long run we shouldn't have a separate mechanism for snap (it should just be a single-frame acquisition), but we cannot just lose the ability to perform snaps using the existing API with good timing characteristics -- we need a way to detect the end of exposure and return from CMMCore::snapImage() accordingly, in order to keep that API working correctly.

It might be possible to offer the option to cameras of returning from SnapImage() after scheduling an asynchronous (i.e. camera-owned thread) operation that will later send the image to the Core. But that would require lots of potentially error-prone code to be replicated by each camera adapter.

Another idea would be for MMCore to call SnapImage() on a background thread, and mandate cameras to send an "exposure finished" notification (which causes CMMCore::snapImage() to return) if it is going to use the InsertImage method.

But my recommendation would be for the current device snap API to remain a legacy feature with minimal (if any) changes to behavior. The new camera API could simply leave out a separate snap function. MMCore can then implement CMMCore::snapImage() using a single-frame acquisition. It should be strongly recommended that cameras send an exposure-finished notification, and snapImage() should return asap according to it (which will probably require a Core thread).

@henrypinkard
Copy link
Member Author

I'm afraid this doesn't consdier how timing works for Snap/GetImage. SnapImage() must block for the duration of the camera exposure, but need not (and should not, if possible) wait for the image to be read out and transferred to the computer.

I'm aware of the intended design. In practice, many camera adapters perform image buffer copying within SnapImage(), returning after readout rather than after exposure. I just looked through a random sample of them and a little over half of them do this.

Instead using InsertImage in these cases would:

  • Simplify device adapters by eliminating need for buffer management
  • Allow core-controlled optimized copying across all adapters

This would actually allow the shutter to close faster than their existing implementations.

For camera adapters that actually do what the API intends, yes you're right, the current GetImage mechanism does close the shutter faster. So that argues for not deprecating it, at least until the new camera API is merged.

Regardless, we need a mechanism like this so that new camera API can be backwards compatible with core.snapImage (which is why I made it). I see three possible ways to proceed:

  1. Merge this implementation as proposed, providing a simpler and potentially more performant mechanism that aligns with many adapters' current behavior
  2. Modify this PR to dissallow this behavior for existing cameras (so it would have no effect), but it still breaks up the new camera API in smaller PRs as you requested.
  3. Close this PR and incorporate these features only within the larger new camera API implementation

Which do you prefer?

@henrypinkard henrypinkard mentioned this pull request Mar 12, 2025
6 tasks
@marktsuchida
Copy link
Member

marktsuchida commented Apr 10, 2025

we need a mechanism like this so that new camera API can be backwards compatible with core.snapImage

Do you mean for new-style cameras to work with old-style apps, or the other way around? The latter, right?

In either case, I don't think we would want to require existing cameras to be modified in order to achieve compatibility, if that is what you mean. Modifying the Snap behavior like this would be too risky to do without testing each camera, which is not feasible. But we don't want the old API to be permanently required for un-updated cameras.

I think it should be possible (if not easy) to bridge between the old and new APIs so that the compatibility works both ways without modification, and I'm happy to put some work into getting that working. It may require things like spawning a thread in MMCore, perhaps. But let's leave this PR open until I (or anybody) has a good alternative.

@marktsuchida
Copy link
Member

[Update after chatting with @henrypinkard]

The main goal for this PR is to enable new-style cameras (#593) to work with old-style CMMCore::snapImage. So it should primarily be looked at as part of #593, not a change to how old-style cameras deliver snapped images.

Keeping it open for now but may close if it seems sufficient to merge as part of #593 (which branches off this PR already).

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