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

Deprecate the "default" sequence acquisition implementation in CCameraBase #421

Open
marktsuchida opened this issue Dec 19, 2023 · 1 comment

Comments

@marktsuchida
Copy link
Member

In MMDevice's camera interface, a sequence acquisition is when the camera hardware is set up to acquire multiple frames without software control for each frame; it should be different from performing a series of snap acquisitions. (In fact, a snap probably should have just been a special case of sequence acquisition, with length 1, but I digress.)

Unfortunately, MMDevice's camera device base class CCameraBase (in DeviceBase.h) contains "default" implementations of StartSequenceAcquisition(), StopSequenceAcquisition(), and IsCapturing() that use a thread (BaseSequenceThread* thd_) to call SnapImage() in a loop. This is never a good idea -- cameras that truly only support snaps are rare, and even in that case, it would make much more sense to work around it at a higher level, such as in MMCore.

Most of our well-supported and widely used cameras do not use this problematic default implementation, instead providing their own proper sequence acquisition. However, some cameras do use it, and there might be some that partially use the associated functions in a convoluted way. It is also highly confusing to new developers of camera device adapters.

It would be nice if we could remove the problematic implementation or somehow mark it deprecated (in a way that would warn at compile time), but this is not possible because what we need to enforce is that cameras do override these functions. In addition, some cameras use a few functions that are part of the problematic implementation even though they do not use the BaseSequenceThread.

So I propose that we do the following:

  • Split CCameraBase<> into a new CCameraBase<> (without the bad sequence acquisition impl) and CLegacyCameraBase<> (which derives from CCameraBase<> and adds the bad impl; mark this class template as deprecated)
  • For those camera device adapters that do not compile with the new CCameraBase, modify them to derive from CLegacyCameraBase (there are ~20 of these on Windows)
  • For those camera devices that require CLegacyCameraBase but do not actually use the BaseSequenceThread, fix up usage of base class functions and convert to derive from the new CCameraBase
    • Some of these are simple transforms like replacing a call to OnThreadExiting() with a call to GetCoreCallback()->AcqFinished()
    • More complicated fixes that would require testing the hardware to do safely should be avoided until such opportunity arises (which might be never)

Note that none of these changes require bumping the device interface version, since they do not affect the binary interface of the DLLs.

@henrypinkard
Copy link
Member

Sound great to me! Seems low risk, and it would steer developers of new camera devices in the right direction in the future

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