[libcamera-devel] API design feedback

Mon Oct 11 10:10:13 CEST 2021

Hi Larent,

On Fri, 8 Oct 2021 13:35:28 +0300
Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:
> On Sat, Sep 18, 2021 at 06:25:00PM +0200, Dorota Czaplejewicz wrote:
> > 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.  
> 
> Those classes are defined in headers in a separate directory
> (include/libcamera/internal/) and they're not installed. They are
> however documented using Doxygen and end up mixed with all public
> classes. We're planning to split the documentation in two parts, so the
> public API documentation will not show those API elements anymore. Would
> that be enough in your opinion, or would a separate namespace be a good
> additional improvement ?
> 
I concede that if it's gone from the docs then docs are not confusing, and if if the headers are not installed, then the IDE will usually behave as expected. There's still the case of developing libcamera itself and confusing the private and public portions, but I'm not sure if that can become an issue.

> > 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,  
> 
> It depends on the pipeline handlers, I expect the more specific pipeline
> handlers (RPi, IPU3, ...) to have enough knowledge about the device to
> implement validate() without calling to the kernel, while the simple
> pipeline handler or the UVC pipeline handler will have to validate the
> configuration with the help of the drivers (and in the UVC case, even
> the help of the hardware).
> 
I think if the API by design affords the freedom for the pipeline to make this operation context-sensitive, then the context should be made explicit in the API. That means something like `Camera::validate(CameraConfguration)` would make it clear.

While this may sound a little theoretical, this is an actual question I asked myself when trying to understand libcamera by example: which operations actually touch the camera, and which are bookkeeping? I would have missed `validate` and come back with the wrong idea if I hadn't adjusted configs in the kernel driver before.

> > That has the additional benefit that the user can construct the
> > desired parameters without having to call out to the Camera class
> > beforehand.  
> 
> Note that we subclass the CameraConfiguration in pipeline handlers to
> associate private data with the configuration (computed at validate()
> time, and then used at configure() time), so applications would still
> not be able to allocate a CameraConfiguration on their own. That could
> be changed though, with a different mechanism to store the private data
> without subclassing. This is something I'll experiment with as part of
> the configuration API rework.

I hit a variation of this when using libcamera in Megapixels: I use libcamera to apply my chosen config, and return the adjusted one. However, libcamera does not return the new config as a type I can compare or pass by value, which is what I'm interested in. My intuition says that such a type will be needed in a ton of applications, so it would make sense to make libcamera provide it and operate on it natively, instead of having each user implement it from scratch.

>> 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.  
>
> That was mostly done to be able to return an error code to the
> application, but in hindsight it was probably not a good idea. We'll
> likely drop start() and stop(), in the uncommon use case where an
> application would need to restart a camera manager, it can delete and
> recreate it (that's cheap compared to start() and stop()).

For when initialization can fail, I like the Result pattern I know from Rust, e.g.:

https://doc.rust-lang.org/std/fs/struct.File.html#method.create

It does not use an exception, but will not return a failed object either, so methods don't need to do the bookkeeping. In C++17 there's variant that can be made to work almost as nicely. I tried it out in the context of libcamera here:

https://source.puri.sm/Librem5/megapixels/-/blob/49e6d5d1da35e863d957e08f4a6db2441250be4a/src/libcam.cpp#L14

> > 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  
> 
> One use case is for applications that use multiple cameras, they may
> need to ensure exclusive access to all cameras before they configure
> them.

Thanks, that's something I did not think of.
> 
> > 	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)  
> 
> Our current API is based on signals (inspired by Qt) to decouple the
> emitter objects from their users. Passing callback functions manually
> make it more cumbersome. Pre-C++11 it was difficult, if not impossible,
> to use class members as callback functions in a generic way that
> wouldn't require an implementation of the horrible Java listener
> pattern. With lambdas it got better, but I still don't really like
> having to store pointers to the functions manually. Let's also note that
> signals are thread-aware, if the emitter and receiver are bound to
> different thread, signal delivery will go through a message queue. This
> is a feature that would not be nice to replicate manually in every
> emitter. Of course, we could still pass callback functions manually, and
> connect them to signals internally. One upside would be that the Signal
> class (and the associated BoundMethod classes) wouldn't need to be
> exposed in the public API, reducing the API surface.
> 
> Based on this explanation, what do you think would be best ?

I don't mind signals, and what I had in mind with this alternative is only a semantic nudge to do the right thing. As you said: "we could still pass callback functions manually, and connect them to signals internally".
> 
> > } // 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.  
> 
> We don't use exceptions, but we could ASSERT().
> 
> Would reference-counting Camera help here ? Same for CameraExclusive,
> the CameraStream could take a reference to it. The downside is that
> CameraExclusive would need to be handled as a shared pointer.
> 
I'm not sure if I prefer any of the 2 options. Both an assertion and a shared pointer would prevent unexplained crashes. The assertion is easier to debug library-side, while shared pointers are more robust to user's errors.

> This being said, we trade a bit one problem for another, what happens if
> the application calls camera_exclusive.start() multiple times ? Should
> calls beyond the first one return an error (as a null unique pointer
> in this case) ? We will still need a state machine in the Camera class
> (or possibly CameraExclusive) to handle this.
> 
Good point, and I think you now pointed out a problem in an unrelated project I contributed to.

Thinking from the user's point of view, returning an error would be more consistent with using the assertion above (only 1 reference, bound to a variable, behaves almost like a value), whereas returning another reference would be more consistent with shared pointers.

If CameraManager is allowed to return multiple references to the same camera, that would suggest that libcamera API is already fine with shared pointers, and does not attempt to make references seem like values.

> > 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.  
> 
> Undefined behaviour pretty much means that the program will crash,
> usually due to an ASSERT(). That's not very different from unhandled
> exceptions. Unlike some undefined behaviours of the C language that are
> used for clever purposes (mostly too clever ones, that then bite back),
> here we're essentially telling that it will get very, very bad, so bad
> that a crash is the most likely outcome.
> 
> Would it be better to explicitly state that the program will abort()
> instead of mentioning undefined behaviour ?

Yes, I think it's useful to differentiate UB from a crash. That UB can manifest itself in a number of confusing ways is the main problem.

The Rust stdlib considers itself free of UB, but panics are still used there (sometimes in questionable places) and documented:

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.enumerate
> 
> > 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/20211011/9122e824/attachment.sig>