Capability flags in camera information

[ddatsko]

This PR adds a flags (CAMERA_CAP_FLAGS) setting in CAMERA_INFORMATION for camera_server plugin. This allows the user to set additional capabilities that cannot be determined automatically based on callback presence or so. For example, the implementation can not determine whether the CAMERA_CAP_FLAGS_CAN_CAPTURE_IMAGE_IN_VIDEO_MODE should be set, but this can be useful for some users. Another example of such a need is when the component also uses the tracking_server plugin for object tracking and the CAMERA_CAP_FLAGS_HAS_TRACKING_POINT or similar should be set in that case

[julianoes]

One thing I wonder is if those flags shouldn't be internal and set according to the callbacks set.

For instance, if you set the callback for zoom, then it would set the flag has_basic_zoom rather than having to explicitly set flags in a somewhat redundant step.

[JonasVautherin]

@julianoes: that's actually a good point!

@ddatsko: what do you think? Do you think it could work?

[ddatsko]

I was looking at C++ MAVSDK implementation, and it's done this way there, from what I could see. But then it's not clear, for example, whether to set CAMERA_CAP_FLAGS_CAN_CAPTURE_IMAGE_IN_VIDEO_MODE or not to set it. Also, if someone is using camera tracking using MAVSDK (for example, using QGroundControl with this PR or custom software, one will probably use the traking_server plugin for this and want to set the CAMERA_CAP_FLAGS_HAS_TRACKING_POINT capability, which cannot be deducted internally. Maybe it's a good idea to add these tracking capabilities to this plugin too, but it looks like a lot of code duplication for a simple feature, and I was unsure how to make it better.
@julianoes, what do you think of this?

[julianoes]

The tracking flag would have to go via component_impl as it's shared between tracking_server and camera_server. It's not pretty but would work.

I just fear that it's easy to forget setting the flags and then things won't work properly.

[ddatsko]

Could we maybe make some kind of "extra" capability flags that are set by the user in addition to ones deducted based on what callbacks are present? Then the enum would be smaller and would not contain flags like has_basic_zoom. It would be the same interface for users, but a possibility to set extra flags if needed

[julianoes]

Let's flip it around. Which flag do you need right now? If we know that, we can make sure you can set them.

[ddatsko]

I was mostly concerned with combining camera_server and tracking_server, and thought that doing so through the impl would require a dependency between two plugins. Maybe it's also a good idea to merge tracking_server into camera_server, as MavLink tracking is visual camera tracking, actually

[julianoes]

Merging the two plugins is probably not the worst idea. What do you think @JonasVautherin ?

[JonasVautherin]

It is right that the tracking is closely related to the camera. Why not!

[ddatsko]

I can also see that in MavLink itself the Camera tracking is just a part of Camera protocol alongside with other camera features (https://mavlink.io/en/services/camera.html#camera-tracking). The same logic follows in QGroundControl, where tracking is implemented as a part of camera control, so it seems reasonable to keep all the camera functionality in one place. @julianoes , what do you think?

[julianoes]

@ddatsko it makes the camera plugin "bigger" but avoids awkward sharing, so that makes sense.

[tbago]

CameraCapFlags is not independent message, so use enum type is more clear.

[ddatsko]

Tanks for the comment. However, this piece of code s already removed in the newest code version

[tbago]

I think add camera cap flags as following define is more clear.

// Type to represent a camera information.
message Information {
    enum CameraCapFlags
    {
        CAMERA_CAP_FLAGS_CAPTURE_VIDEO = 0; // Camera is able to record video
        CAMERA_CAP_FLAGS_CAPTURE_IMAGE = 1; // Camera is able to capture images
        CAMERA_CAP_FLAGS_HAS_MODES = 2; // Camera has separate Video and Image/Photo modes (MAV_CMD_SET_CAMERA_MODE)
        CAMERA_CAP_FLAGS_CAN_CAPTURE_IMAGE_IN_VIDEO_MODE = 3; // Camera can capture images while in video mode
        CAMERA_CAP_FLAGS_CAN_CAPTURE_VIDEO_IN_IMAGE_MODE = 4; // Camera can capture videos while in Photo/Image mode
        CAMERA_CAP_FLAGS_HAS_IMAGE_SURVEY_MODE = 5; // Camera has image survey mode (MAV_CMD_SET_CAMERA_MODE)
        CAMERA_CAP_FLAGS_HAS_BASIC_ZOOM = 6; // Camera has basic zoom control (MAV_CMD_SET_CAMERA_ZOOM)
        CAMERA_CAP_FLAGS_HAS_BASIC_FOCUS = 7; // Camera has basic focus control (MAV_CMD_SET_CAMERA_FOCUS)
        CAMERA_CAP_FLAGS_HAS_VIDEO_STREAM = 8; // Camera has video streaming capabilities (request VIDEO_STREAM_INFORMATION with MAV_CMD_REQUEST_MESSAGE for video streaming info)
        CAMERA_CAP_FLAGS_HAS_TRACKING_POINT = 9; // Camera supports tracking of a point on the camera view.
        CAMERA_CAP_FLAGS_HAS_TRACKING_RECTANGLE = 10; // Camera supports tracking of a selection rectangle on the camera view.
        CAMERA_CAP_FLAGS_HAS_TRACKING_GEO_STATUS = 11; // Camera supports tracking geo status (CAMERA_TRACKING_GEO_STATUS).
    };
    string vendor_name = 1; // Name of the camera vendor
    string model_name = 2; // Name of the camera model
    string firmware_version = 3; // Camera firmware version in major[.minor[.patch[.dev]]] format
    float focal_length_mm = 4; // Focal length
    float horizontal_sensor_size_mm = 5; // Horizontal sensor size
    float vertical_sensor_size_mm = 6; // Vertical sensor size
    uint32 horizontal_resolution_px = 7; // Horizontal image resolution in pixels
    uint32 vertical_resolution_px = 8; // Vertical image resolution in pixels
    uint32 lens_id = 9; // Lens ID
    uint32 definition_file_version = 10; // Camera definition file version (iteration)
    string definition_file_uri = 11; // Camera definition URI (http or mavlink ftp)
    repeated CameraCapFlags camera_cap_flags = 12;   // Camera capability flags (Array) 
}

[ddatsko]

Tanks for the comment. This piece of code s already removed in the newest code version, as we agreed that exposing capability flags to the user may be confusing. In the new version of code, tracking_server is merged into camera_server which makes it possible to set more flags (more specifically, HAS_TRACKING_...) internally, in impl files and avoid this enum

[ddatsko]

Hey, @JonasVautherin, @julianoes. Are there any steps required from my side to have the PR merged? I implemented the added functionality in C++ mavsdk, which hasn't received any review yet and now has conflicts due to new updates. I can resolve them and fix the problems spotted by a review if this PR is going to be merged so we can merge both of them at the same time

[julianoes]

@ddatsko sorry for letting this rot. I will take care of it today.