[libcamera-devel] API design feedback

[Dorota Czaplejewicz]

Hi,

I was asked to review the libcamera API, so here's my perspective, colored by my Rust experience (prefer RAII, dumb data, explicit dependencies), and by being a relative newbie to libcamera (still remember what confused me at first).

One of the first things is that the `libcamera` namespace has both elements meant for the consumers of the API, and implementation details. I've been told that libcamera wants to abstract the underlying API like media controller, and meant to be private, so things like `class MediaEntity` feel out of place when they are placed in the root namespace.

Nitpick about `CameraConfiguration::validate()` - I take "validate" to be a C++ const function giving a "valid/invalid" answer. This call actually modifies the data, so it should be rather named "adjust".

About explicitness: it's not clear what validate() checks against. Checking SimpleCameraConfiguration, the config has free access to a Camera instance, and that means that validate() potentially calls out to the camera driver, but it's not clear from the code. Some problems show up in the API: the returned enum has no "the camera has died, impossible to answer". I see 2 ways to escape, depending on what the intention in the API is:

1. If calling to the HW is needed, remove the camera from the config and do `Camera::validate(CameraConfguration)` (or the inverse),
2. If validation doesn't need to talk to HW, purge Camera instances from CameraConfiguration subclasses,

That has the additional benefit that the user can construct the desired parameters without having to call out to the Camera class beforehand.

I think that's on the docs, but there's nothing preventing the user from attempting to configure the camera with unvalidated config, but there's a kind of a hole saying "config must be validated" without saying what happens when it's not. Making config pure data solves that too.

About RAII: I noticed that both CameraManager and Camera suffer from manual management. Both of them use something close to a stack (even though Camera has a state machine which IMO is too general):

CameraManager -> start (Started)

Here I'm not even sure why `start` is even useful. I don't see a reason to create a CameraManager and then not start it. Then the destructor can be responsible for stopping it.

Camera -> acquire (Exclusive) -> configure (Configured) -> start (Started)

Here's also a stack, with only `release` making it somewhat complicated. 

The problem is that there are 4 states, and each state only exposes a subset of functionality. A lof of functionality is gated behind state checks, and it's all lumped inside a single class, making the docs harder to read - which functionality is available when? Each method needs a manual declaration for that, and manual work is fallible.

If the states were split into separate classes (3? 4?), then those problems are solved at the cost of some extra destructor checks, and more granular classes. Then, the user doesn't get their hopes crushed at runtime when they call the wrong method, but the compiler is going to stop them already, and there's no risk of leaving some calls on the wrong state. Additionally, the user doesn't need to call `release()` and `stop()` (call them in the destructor), and knows exactly when to do what (IDE completions).

An example usage:

```
{
cm = CameraManager()
camera = cm.get("foo")
camera.disconnected.connect(disco_handler)
controls = camera.controls()
// is there anything interesting to do with an exclusive, unconfigured camera?
// it's part of the problem that I don't know at a glance
camera_exclusive = camera.configure(my_config)
req = camera_exclusive.create_request()
camera_exclusive.bufferCompleted.connect(buffer_handler)
camera_stream = camera_exclusive.start()
// there's some freedom about where the handlers belong to.
// I think the following form is more explicit: the user can't forget about them.
// camera_stream = camera_exclusive.start(buffer_handler, request_handler)
} // destructors stop and release the camera and the manager without manual fiddling
```

The destructor tradeoff is that the `~Camera` destructor shouldn't be allowed to proceed while `CameraStream` is present. That might be exception-worthy.

I haven't looked into the Allocator, but I suspect something similar is going on there, with a similar solution.

Stemming clearly from Rust and reinforced by untold hours lost debugging stupid mistakes, please don't leave the duty of correctness on the caller when it can be avoided, especially outside of hot loops. Debugging undefined behaviour generally costs me more time than it would maintaining the needed bounds checks. As an example:

> `CameraConfiguration::at`  Calling this function with an invalid index results in undefined behaviour.

Can this instead throw an exception? That's much more explainable. I even volunteer to eliminate all places where UB is part of the contract.

I may be back with more once I start using the library.

Cheers,
Dorota
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210918/4ef650db/attachment.sig>