New MM::StorageDevice and addition to the MMCore API #535
Conversation
|
Super excited about this coming to MMCore and sorry for the lack of reaction until now! Would it be okay (before getting into actual code review) if I rebased all your commits to remove the changes that are not direclty part of StorageDevice (mostly the switch of Visual Studio version) and force-push? I'm pretty sure I can do this without losing the branching and merging in your commit history. This is both (almost certainly) necessary for eventual merging, and should also make the diff a lot easier to view. (Am right that the code doesn't actually need VS2022?) (We do want to switch to Visual Studio 2022 very soon, but that really needs to be a separate pull request that is coordinated with build infrastructure updates. That can happen before or after this PR gets merged, depending on what's easiest/needed.) |
|
Hi Mark
The timing is excellent. I have been swamped lately and made little
progress in 2025, but I am on it again. Yes, please, if you know how to fix
the mess I made with my fork and PR, I'd be grateful. I don't care about
branches other than mmstorage-draft.
No, it does not need VS2022. That was a mistake on my part.
I am preparing another PR for the micro-manager repo (the java GUI) to
integrate the new API with the Micro-manager data storage classes. Nico was
helping me, but it turned out to be a more significant challenge for me
than it was to work on MMCore.
Best
Nenad
…On Wed, Jan 29, 2025 at 3:09 PM Mark Tsuchida ***@***.***> wrote:
Super excited about this coming to MMCore and sorry for the lack of
reaction until now!
Would it be okay (before getting into actual code review) if I rebased all
your commits to remove the changes that are not direclty part of
StorageDevice (mostly the switch of Visual Studio version) and force-push?
I'm pretty sure I can do this without losing the branching and merging in
your commit history.
This is both (almost certainly) necessary for eventual merging, and should
also make the diff a lot easier to view.
(Am right that the code doesn't actually need VS2022?)
(We do want to switch to Visual Studio 2022 very soon, but that really
needs to be a separate pull request that is coordinated with build
infrastructure updates. That can happen before or after this PR gets
merged, depending on what's easiest/needed.)
—
Reply to this email directly, view it on GitHub
<#535 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMVLVOY3YGQYDVN6BA7CCQL2NFNQ3AVCNFSM6AAAAABTMPAI2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRTGEZDMNZVHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
was waiting to not be the first person :) ... but also wanted to express my enthusiasm and appreciation for the work being done here. this would be very exciting to have in some form 🚀 |
|
Just pushed a commit that reverts the Visual Studio platform toolset to v142 (VS 2019). Nenad, can you do a git pull before adding other commits? Also, make sure in Visual Studio to select "do not upgrade" if VS offers to upgrade the toolset. You may need to run the VS installer to install the v142 (VS2019) platform toolsets on your system. Regretfully, there are still many project files flagged as changed, at first sight that seems to be due to differences in line ending, so nothing to be really worried about, but it still makes this PR unwieldy. |
|
Just added Go2Scope to the solution and can build this. I copied acquire-zar.dll (b.t.w., can the acquire-zar code eventually be moved to the public 3rdparty? I assume this is all released under an open license?) to my Micro-Manager installation, but Go2Scope stays greyed out in the Hardware configuration wizard. I guess this is because it is a device type not recognized by my HCW? Happy to help with the Java code if that is needed to fully evaluate this PR. As expected, the UI works well with this Core cope. |
|
Hi Nico
Let's talk tomorrow and get that working together. If you copied the zarr
library it should not be grayed out.
Talley recently told me that there is an update to acquire-zarr.dll that
would require changing some code. He did the change on his end, but I
didn't just yet. Maybe the versions don't match.
Acquire zarr is an open source project and can be moved to 3rdpartypublic.
Best
Nenad
…On Thu, Feb 13, 2025 at 5:22 PM Nico Stuurman ***@***.***> wrote:
Just added Go2Scope to the solution and can build this. I copied
acquire-zar.dll (b.t.w., can the acquire-zar code eventually be moved to
the public 3rdparty? I assume this is all released under an open license?)
to my Micro-Manager installation, but Go2Scope stays greyed out in the
Hardware configuration wizard. I guess this is because it is a device type
not recognized by my HCW? Happy to help with the Java code if that is
needed to fully evaluate this PR. As expected, the UI works well with this
Core cope.
—
Reply to this email directly, view it on GitHub
<#535 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMVLVO3LRKZM5YVQ6UP5SHL2PVAMLAVCNFSM6AAAAABTMPAI2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJYGA2DSMRXGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
[image: nicost]*nicost* left a comment
(micro-manager/mmCoreAndDevices#535)
<#535 (comment)>
Just added Go2Scope to the solution and can build this. I copied
acquire-zar.dll (b.t.w., can the acquire-zar code eventually be moved to
the public 3rdparty? I assume this is all released under an open license?)
to my Micro-Manager installation, but Go2Scope stays greyed out in the
Hardware configuration wizard. I guess this is because it is a device type
not recognized by my HCW? Happy to help with the Java code if that is
needed to fully evaluate this PR. As expected, the UI works well with this
Core cope.
—
Reply to this email directly, view it on GitHub
<#535 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMVLVO3LRKZM5YVQ6UP5SHL2PVAMLAVCNFSM6AAAAABTMPAI2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJYGA2DSMRXGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
… implemented for bigtiff, doc strings updated
Use VS2019 build tools for the new projects for consistency with everything else.
|
Before rebasing this branch to remove the VS2022 conversion and other issues with micromanager.sln, I have pushed the current state of the branch to |
marktsuchida
force-pushed
the
mmstorage-draft
branch
from
addd0af to
d14d2c4
Compare
|
Now I have force-pushed a rebased branch to the PR branch (mmstorage-draft in the go2scope fork). Briefly, the rebase
Full details of the rebaseAfter studying (with gitk) the branches and merges within the PR, I found that
I created a new branch git rebase -i --rebase-merges 4441b057 mmstorage-rebasewhere 4441b05 is the commit on The vast majority of the overwhelning amount of commits and branches are Then I deleted the unnecessary lines:
I then also deleted all the branches upstream of the deleted merge commits. The goal is to rewirte history such that the changes of VS versions and the I anticipated that there would still be conflicts during the rebase at 'zarr
There were also 2 conflicts involving changes to micromanagerSD.slnf, because After the rebase, I made sure that no commits were missing (except for the After the first round of rebasing, which simplified the whole branch, I did a In addition, I added a new commit that sets the Then I tested the build and found that MMCore does not build, due to Finally, it (somewhat miraclulously) turned out that there were no code git rebase --rebase-merges mainI reset |
|
We might need to do another round of conflict resolution before merging, depending on the order of events (there's another pending PR that adds device types), but I think this is now in a state where we can review and test -- and I look forward to diving into the details over the next days! BTW if you want to bring the rebased branch to your local clone, assuming that the remote git fetch origin
git checkout mmstorage-draft
git reset --hard origin/mmstorage-draft |
|
And don't worry about the failed checks -- I'll fix those before merging, but that's simplest to do just before merging. |
MMDevice/MMDevice.h
Outdated
| virtual int Create(const char* path, const char* name, int numberOfDimensions, const int shape[], | ||
| MM::StorageDataType pixType, const char* meta, char* handle) = 0; |
There was a problem hiding this comment.
Is there an important reason to use a string handle? Wondering if it might be simpler to use a uint64_t handle if the value itself has no meaning.
EDIT: And (as we discussed today) letting MMCore generate the handle could make it easier to use multiple datasets, potentially from different storage devices.
There was a problem hiding this comment.
Also, for the metadata parameter, I would suggest using a pointer and size (std::size_t) rather than a null-terminated string. This would not only eliminate a scan on the receiving side to determine the length, but would also allow metadata that contains internal NUL bytes (not that we'd recommend that).
This applies to all passing of metadata in and out, of course.
MMDevice/MMDevice.h
Outdated
| * \return Progress value (0-100), or -1 if idle or not implemented | ||
| * \note Assumes single operation execution at a time | ||
| */ | ||
| virtual int GetProgress(const char* handle) = 0; |
There was a problem hiding this comment.
Is this intended to be called on a second thread while the first thread is making the call to perform an operation?
MMDevice/MMDevice.h
Outdated
| * \param handle Handle of the dataset to delete | ||
| * \return Status code indicating success or failure | ||
| */ | ||
| virtual int Delete(char* handle) = 0; |
There was a problem hiding this comment.
Does this also effectively close the handle? Why does this take a handle rather than a path?
MMDevice/MMDevice.h
Outdated
| virtual int AddImage(const char* handle, int sizeInBytes, unsigned char* pixels, | ||
| int coordinates[], int numCoordinates, const char* imageMeta) = 0; |
There was a problem hiding this comment.
Sizes should be std::size_t or std::uint64_t (for memory sizes and on-disk sizes, resp.).
I think coordinates should also be 64-bit, just to be safe. numCoordinates can be std::size_t.
MMDevice/MMDevice.h
Outdated
| virtual const unsigned char* GetImage(const char* handle, int coordinates[], | ||
| int numCoordinates) = 0; |
There was a problem hiding this comment.
Can this have an int return value so that it can indicate an error?
Also, how long is the returned pointer valid? If it is not until the handle is closed (which would be problematic), we probably need a "release" function for images (analogous to the one for metadata).
…eted AddImage and replaced with append.
| std::string CMMCore::getDeviceNameToOpenDataset(const char* path) | ||
| { | ||
| // TODO: | ||
| throw CMMError("Feature not supported", MMERR_GENERIC); | ||
| } |
There was a problem hiding this comment.
Given that more than one storage device could open the same file format, I don't know if this function can be well defined. Maybe better to either return a collection of storage devices, or provide instead bool isDatasetSupportedByStorage(storageLabel, path). The latter gives apps more freedom (they can find the first compatible or all, check in a preferred order, or just check for a particular storage).
| FocusDirectionAwayFromSample, | ||
| }; | ||
|
|
||
| enum StorageDataType |
There was a problem hiding this comment.
It might be best to call this just PixelFormat, so we can (later) use it for other things, too.
There was a problem hiding this comment.
("Format", not "type", because "type" may imply integer data type.)
| StorageDataType_UNKNOWN = 0, | ||
| StorageDataType_GRAY8, // uint8 | ||
| StorageDataType_GRAY16, // uint16 | ||
| StorageDataType_RGB32 // RGBA, 8bits per color, A ignored defaults to 0 |
There was a problem hiding this comment.
The odd format that MMCore currently requires is BGRA32, not RGBA.
It's a good question whether storage devices should support that format, though, because usually we'd want to write RGB24 to disk. The enum could have constants for both.
(We should add support for cameras to simply send RGB24 images in the near future, after the current batch of PRs have been merged. That will help with direct streaming to disk from RGB cameras.)
| // Maximum length of serialized metadata strings | ||
| const int MaxMetadataLength = 200000; | ||
|
|
There was a problem hiding this comment.
I don't see this being used, and also don't think there should be an arbitrary limit to metadata length, so remove?
| int StorageInstance::ConfigureDimension(const char* handle, int dimension, const char* name, const char* meaning) | ||
| { | ||
| RequireInitialized(__func__); | ||
|
|
There was a problem hiding this comment.
The check to forbid configuring dimensions (1) on a read-only dataset and (2) on a dataset to which images have already been appended should probably be done here, so that that logic is not repeated in every storage adapter.
(Or are there cases when (2) is not disallowed?)
Same also for ConfigureCoordinates.
MMCore/Devices/StorageInstance.cpp
Outdated
| int StorageInstance::AppendImage(const char* handle, int sizeInBytes, unsigned char* pixels, const char* imageMeta) | ||
| { | ||
| RequireInitialized(__func__); | ||
| return GetImpl()->AppendImage(handle, sizeInBytes, pixels, imageMeta); |
There was a problem hiding this comment.
MMCore should throw an error if an attempt is made to add an image to a read-only dataset (again, so that each storage device doesn't have to repeat the logic and we get a consistent error message).
Same with AddImage (if keeping).
MMCore/Devices/StorageInstance.cpp
Outdated
| int StorageInstance::SetCustomMeta(const char* handle, const std::string& key, const std::string& meta) | ||
| { | ||
| RequireInitialized(__func__); | ||
| return GetImpl()->SetCustomMetadata(handle, key.c_str(), meta.c_str()); |
There was a problem hiding this comment.
Read-only datasets do not allow setting custom metadata, correct?
If so, MMCore should check.
MMCore/MMCore.cpp
Outdated
| * \param handle - currently open dataset handle | ||
| * \param imageMeta - image metadata | ||
| */ | ||
| void CMMCore::appendNextToDataset(const char* handle, const std::vector<long>& coordinates, const char* imageMeta) throw (CMMError) |
There was a problem hiding this comment.
Possibly name popNext[Image]AndAppendToDataset(), to be easily compared with the plain popNextImage() and appendImageToDataset()?
- This won't work for
Multi Camera. That can be addressed later (possibly with Henry's per-camera buffering). - Applications won't be able to store metadata values generated by cameras using this mechanism. This also can be addressed later if found necessary.
(Just noting the limitations so we are aware of them.)
MMCore/MMCore.cpp
Outdated
| * \param coordinates - array of coordinates, one for each dimension | ||
| * \return - image pixels | ||
| */ | ||
| STORAGEIMGOUT CMMCore::getImageFromDataset(const char* handle, const std::vector<long>& coordinates) throw (CMMError) |
There was a problem hiding this comment.
Just pointing out that this API forces MMCoreJ and pymmcore to immediately make a copy of the image buffer, because the buffer can go away at any moment. (Strictly speaking, even that is not fully safe because the copy happens outside of the device module lock.)
It would be nice to add (in the future) a way to share ownership of the image buffer, so that it can be kept alive as (say) a NumPy array without making a copy. But this need not be part of the first iteration, because the focus is presumably on getting camera-to-storage streaming working. We might want to think about buffer lifetime in the device API, though (I already commented there).
…ets from different storage devices; added explicit deviceLabel to alternative create and load methods
Overview
Introducing the new MM::StorageDevice. There are two main changes to the micro-manger back end: a new device type (Storage) and the extension of the MMCoreAPI. Both changes are lateral because they extend capabilities without affecting existing functionality. There are no new dependencies for the code base. Any existing client code, including the Micro-manager front end, can safely ignore this extension.
The new feature enables MMCore to save datasets on the back end using a storage device adapter. Developers can add support for various formats by creating device adapters like they would add support for a new camera. There are two significant advantages in utilizing this architecture:
The most important one is that the interface between the microscopy application and the file storage is defined by the MMCore API and allows for designing generic, storage/format-independent acquisition engines and acquisition scripts. To change the file format, we only change the device configuration, but the acquisition code remains the same. Or mainly the same, except for some specific properties - pretty much as what happens when we change the camera. Adding support for new file formats or using better implementations does not involve changing the existing code in any significant way.
The second advantage is that by embedding the storage implementation in the MMCore, we can achieve a higher efficiency (data rate), as images do not have to cross the MMCore API boundary.
Status
We plan on adding more documentation and some examples over the coming days and weeks. This PR is work in progress and only a draft to get some early feedback. Implementation is currently functional, but incomplete. Also, the micro-manager build for the new version may not be completely compatible with the main branch. This will be corrected in the next update.
To go over the new API, you can see MMCore.h and MMDevice.h. There are currently two device adapters: BigTiff (with chunking) and Zarr (based on https://github.com/acquire-project/acquire-driver-zarr), both located in the "go2scope" device library. The bigTiff adapter is fully functional, while the Zarr currently supports only writing, not reading.
We are still working on integrating the now MMCore back-end storage into the Micro-manager application (Java). It works but is still not ready for the PR.
Credits
Nathan Clack and the Chan Zuckerberg Initiative team provided material, advisory, and moral support for this effort. Nico Stuurman and Talley Lambert provided early feedback and help. Milos Jovanovic built the fully functional and highly efficient BigTiff adapter based on the early specification of the interface.