Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

Implement get_meta and reserve_image_shape for basic Storage devices #4

Merged

Conversation

aliddell
Copy link
Member

Also prepends internal storage API functions with their engine types so there aren't about a zillion functions with the same name.

Copy link

@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.

Again, I don't think I have enough context to approve/reject here. Prepending the storage types to the C function names seems fine and familiar enough to me. Can try to help out with the TIFF resolution limits if you need/want.

src/storage/tiff.cpp Outdated Show resolved Hide resolved
@aliddell aliddell marked this pull request as draft May 11, 2023 21:55
@aliddell aliddell changed the title Implement get_meta for basic Storage devices Implement get_meta and reserve_image_shape for basic Storage devices May 24, 2023
@aliddell aliddell marked this pull request as ready for review May 24, 2023 18:38
@@ -0,0 +1,5 @@
SET(CPACK_PACKAGE_VERSION_MAJOR 0)
Copy link
Member

Choose a reason for hiding this comment

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

recommend using git-versioning.cmake from e.g. acquire-video-runtime. That will set the package name based on the git tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I went this route was that I noticed that without setting CPACK_PACKAGE_VERSION_* in this file, all the package versions on the builds were automatically set to 0.1.1. But this must have been because there's no git-versioning.cmake in the driver repos (and no tags are set anyway). This way we can automate releases, too.

Comment on lines 63 to 65
*meta = (struct StoragePropertyMetadata){
.chunking = { 0 },
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*meta = (struct StoragePropertyMetadata){
.chunking = { 0 },
};
*meta = (struct StoragePropertyMetadata){0};

Comment on lines 120 to 121
{
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider commenting to indicate an empty body is expected

Suggested change
{
}
{ // no-op
}

An alternative to adding these empty functions is to allow the driver to set the function pointer to 0. This would require a change in the hal and a doc update in the kit.

#endif

acquire_export int
unit_test__raw_get_meta()
Copy link
Member

Choose a reason for hiding this comment

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

Consider making this a standalone test in the tests directory instead of a unit test.

See how list-devices is done in this repo for an example. It doesn't rely on the use of runtime. Instead it uses the hal to check things.

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.

👍

@nclack nclack merged commit c59ff9c into acquire-project:main May 26, 2023
aliddell added a commit to acquire-project/acquire-video-runtime that referenced this pull request May 26, 2023
Depends on
[acquire-driver-common/#4](acquire-project/acquire-driver-common#4).

Because `storage_properties_init` was changed to take 7 arguments, we
have to remove the last argument everywhere that function is called.

We also get storage property metadata from each storage engine in
`acquire_get_configuration_metadata`, and do a little cross-component
configuration in `acquire_start`.
nclack pushed a commit to acquire-project/acquire-driver-hdcam that referenced this pull request Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants