Expanded support for Libcamera Controls (ControlInfoMap and ControlInfo) #46
Conversation
|
I realized I skipped over the values method inside of ControlInfo. I have added an implementation that appears to work with the same systems mentioned before. I also expanded on top of the libcamera-meta to allow getting the id and names of the ControlId/PropertyId from the enum directly. |
SoZ0
changed the title
SoZ0
changed the title
|
Hey, sorry for not reviewing this earlier. The changes look good, but could you rebase or cherry pick on main branch? Many things not related to ControlInfo have been merged |
|
If this has no movement on it in a few days I'll probably grab the changes, resolve the conflicts and rebase. I made similar changes on my own fork (though very crude). I'm re-writing the project I used libcamera-rs for currently, so I'm in no hurry and don't want to step on anyones toes |
|
the other changes should be removed, working on getting the branch ready to merge with main |
|
should be good now, let me know if anything needs changing |
|
We might need to update the version of clippy we use in our CI as rust v1.86 is giving me clippy lints for this PR. |
|
@SoZ0 I have had quite a few demands on my time this week but i promise i will review this more thoroughly shortly. I was able to set controls and get there values dynamically with mainline, and so as this PR does not contain any examples, what it is you are able to do with this, PR? Im not trying to be dismissive, i will try and test this on hardware and make sure it builds for the supported versions before approving so was wondering what i should be looking for when testing etc :) |
|
no worries, I'll write up a few examples of what this changes exactly but the main gist of this was to expose more of ControlInfoMap and ControlInfo. I will try to have them done by Wednesday |
|
I have created the examples if you would like to look them over they live with the other examples:
I have tested the examples on several different cameras without issue so far:
I have tested all of these cameras on a compute module 4/5 with a MIPI/CSI connection and the Logitech cameras were tested on a Fedora 40/41/42 machine as well as the cm4/5 as a USB webcam. I have not tested all permutations of this list but I have been able to read up to 4 different camera module's controls at the same time (I ran out of ports) but I believe there is no limit or at least the limit is high enough to be used in reasonable any use case. There may be an edge case I could be missing still, but I feel very confident that the changes work, will not disrupt any existing code bases and is beneficial for the library. If you need any more information or would like to see and specific use cases please let me know. I will be glad to help out with anything you might need. |
| pub fn stop(&self) { | ||
| unsafe { | ||
| libcamera_camera_manager_stop(self.ptr.as_ptr()); | ||
| // libcamera_camera_manager_destroy(self.ptr.as_ptr()); |
There was a problem hiding this comment.
This commented out code implies that your not sure if this should or should not be called?
@kbingham do you have any insite on this one?
There was a problem hiding this comment.
this allows for some one to create a camera
call stop()
have the camera be stopped and then when tha camera is dropped
Stop will be called again, followed by destroy.
I assume this is ok?
There was a problem hiding this comment.
This commented out code implies that your not sure if this should or should not be called?
@kbingham do you have any insite on this one?
I will have to double check this, but if I remember correctly the issue I ran into with this line specifically was that if a camera lost connection (being unplugged) the current implementation would not allow libcamera to require the sensor. You have to stop the camera manager because libcamera reports the sensor as still being active and stopping the manager would release the lock then starting it again would allow the manager to get a lock on the sensor. When having stop drop the manager it would not allow the already gotten reference to be used again (I believe causing a segfault) meaning you would have to search for the same sensor in the manager (by checking for the same metadata or id) and get a new reference
I'm not 100% sure that is why I made this change again, when I get home I can double check for sure but I'm fairly confident that is the reason.
TDLR;
Not dropping allows keeping the same reference to a sensor when a sensor disconnects and reconnects to the host
There was a problem hiding this comment.
I don't think this is safe. This is what libcamera docs say:
/**
* \brief Stop the camera manager
*
* Before stopping the camera manager the caller is responsible for making
* sure all cameras provided by the manager are returned to the manager.
*
* After the manager has been stopped no resource provided by the camera
* manager should be consider valid or functional even if they for one
* reason or another have yet to be deleted.
*/
void CameraManager::stop()
So I will remove it in #68. The unplugged camera case needs to be investigated separately.
|
@SoZ0 sorry its taken me so long to review this. I found the volume of changes quite hard to review, so i split the up into separate commits https://github.com/fishrockz/libcamera-rs/commits/willsalmon/soz/controls/ which i found a bit easier to review. I dont know if this sort of thing is helpful to you or not but thought i would share in case they are |
| const char *libcamera_control_name(enum libcamera_control_id id) { | ||
| enum libcamera_control_id_enum libcamera_control_id(libcamera_control_id_t *control){ | ||
| return (enum libcamera_control_id_enum)control->id(); | ||
| } | ||
|
|
||
| const char *libcamera_control_name(libcamera_control_id_t *control){ | ||
| return control->name().c_str(); | ||
| } | ||
|
|
||
| enum libcamera_control_type libcamera_control_type(libcamera_control_id_t *control) { | ||
| return (enum libcamera_control_type) control->type(); | ||
| } | ||
|
|
||
| const libcamera_control_id_t *libcamera_control_from_id(enum libcamera_control_id_enum id){ | ||
| auto it = libcamera::controls::controls.find(id); | ||
| if (it != libcamera::controls::controls.end()) | ||
| return it->second; | ||
| else | ||
| return nullptr; | ||
| } | ||
|
|
||
| const char *libcamera_control_name_from_id(enum libcamera_control_id_enum id) { |
There was a problem hiding this comment.
Is this changing the public api of libcamera-sys ? I worry this might need a version bump if it is.
There was a problem hiding this comment.
Im not 100% sure I understand what you are referring to.
There should be no changes that remove or change functionality for the libcamera-sys only add new methods from structs that were missing
many of the changes in here and this section highlighted in specific is adding the missing functions and or giving a more verbose function signature as some of the methods else would share a very similar name or even have the exact name.
I cant recall exactly which functions it was but a few function names were simply too broad not allowing exposing the other libcamera functions with a convenient naming scheme.
If adding in the new functions and or changing the underlying naming of functions is enough to constitute the public api being changed then yes it has changed and will need a version bump. To be honest with the scale and broad impact of changes being made to me it only makes sense.
There was a problem hiding this comment.
So the api for libcamera_control_name seems to change.
const char *libcamera_control_name(enum libcamera_control_id id) {
changes to
const char *libcamera_control_name(libcamera_control_id_t *control){
Which if this makes its way to our rust api/abi will be a braking change for senver rules.
There was a problem hiding this comment.
yer so i had a little play and
And this function makes its way into the public api via the bindings
./target/debug/build/libcamera-sys-*/out/bindings.rs:
pub fn libcamera_control_name(control: *mut libcamera_control_id_t) -> *const ::std::os::raw::c_char;
to
pub fn libcamera_control_name(id: libcamera_control_id::Type) -> *const ::std::os::raw::c_char;
and it can be accessed by
use libcamera_sys::libcamera_control_name;I would suggest that we re ajust the cpp file so the changes are purely additive
Or we need to bump the version in this MR.
index efef19c..1672f13 100644
--- a/libcamera-sys/Cargo.toml
+++ b/libcamera-sys/Cargo.toml
@@ -1,6 +1,6 @@
[package]
name = "libcamera-sys"
-version = "0.4.0"
+version = "0.5.0"
edition = "2021"
description = "Low-level unsafe bindings to libcamera"
documentation = "https://docs.rs/libcamera-sys"There was a problem hiding this comment.
I could refactor it to have them all use:
enum libcamera_control_id_enum
and have the c++ api internally call:
const libcamera_control_id_t *libcamera_control_from_id(enum libcamera_control_id_enum id)
to acces the needed methods. I'm just not sure if this is the right way to go about this.
The current way its laid out in the PR follows how the other structs are handled where it uses the direct reference instead of getting a new one each time.
OR
We could approach it like this to all the effected methods:
const char *libcamera_control_name(enum libcamera_control_id id)
const char *libcamera_control_id_name(libcamera_control_id_t *control)
it just seems weird and a bit redundant to have it like this to me, using the struct pointer seems like the way the api should move towards when working with the controls.
I personally feel like a version bump should be the way to go, but I am willing to refactor the code if a version bump would rather be avoided
|
Thank you @SoZ0 for the PR, sorry it took so long to merge, the new features are really appreaciated. And also thanks @fishrockz for reviewing. I merged this and I will just bump libcamera-sys version, there are no dependents on it except for libcamera-rs. |
|
I was thinking we should split this bit out into its own MR, the ability to
reconnect sounds great but I would like to see a example and think about it
more
The commits I made would make this easy but I don't mind how it's done
…On Tue, 8 Jul 2025, 21:40 Jurgis, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In libcamera/src/camera_manager.rs
<#46 (comment)>
:
> unsafe { CameraList::from_ptr(NonNull::new(libcamera_camera_manager_cameras(self.ptr.as_ptr())).unwrap()) }
}
+ pub fn stop(&self) {
+ unsafe {
+ libcamera_camera_manager_stop(self.ptr.as_ptr());
+ // libcamera_camera_manager_destroy(self.ptr.as_ptr());
I don't think this is safe. This is what libcamera docs say:
/**
* \brief Stop the camera manager
*
* Before stopping the camera manager the caller is responsible for making
* sure all cameras provided by the manager are returned to the manager.
*
* After the manager has been stopped no resource provided by the camera
* manager should be consider valid or functional even if they for one
* reason or another have yet to be deleted.
*/
void CameraManager::stop()
So I will remove it in #68
<#68>. The unplugged
camera case needs to be investigated separately.
—
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABIRCETLUPWXJUGK2A5FTRD3HQUFHAVCNFSM6AAAAABTG6TCB2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSOJZGA2DSNBVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
4 participants
I believe i have implemented the rest of ControlInfoMap and ControlInfo and created a few methods to turn it into an iterator.
I have tested on my host machine (Libcamera 0.3.1, Fedora 40 - x86) and with cross compiling to arm64 (Libcamera 0.3.2, Debian Bookworm). The code works on both x86 and arm64 (tested with a compute module 4).
I am working on a project that requires quite extensive information from libcamera so I will continue to make pull request as I expand libcamera-sys and libcamera to support the remaining missing implementation I need