RFC: API change: Make CameraManager provide a list of CameraDescription objects

[Laurent Pinchart]

On Tue, Aug 27, 2024 at 06:55:00PM +0200, Hans de Goede wrote:
> On 8/27/24 6:02 PM, David Plowman wrote:
> > On Tue, 27 Aug 2024 at 16:25, Hans de Goede wrote:
> >> On 8/27/24 5:09 PM, David Plowman wrote:
> >>> Hi Hans
> > 
> > <snip>
> > 
> >> The only problem I see here is that I guess that the tuning file
> >> may change certain parts of the CameraDescription like the list
> >> of available capture resolutions or some such. But we can simply
> >> document that overriding the default config may have this as a side
> >> effect. It seems reasonable to assume that apps which actually
> >> override the defaults know what they are doing and will not be hurt
> >> by the actual Camera not matching the CameraDescription for e.g.
> >> the resolution list, right ?
> > 
> > I'm not too worried there because our camera tuning files don't affect
> > anything about the sensor itself (such as modes, resolutions etc.),
> > they're purely about how to process pixels. So they're full of colour
> > matrices, AE mode definitions, AWB curves, noise calibrations, stuff
> > like that.
> 
> Ok, that is good to know.

We haven't clearly defined what should and shouldn't go in a tuning
file. I agree with David that tuning files shouldn't affect the
resolutions exposed by the camera. However, the controls exposed by the
camera, as well as their limits, will be influenced by the tuning file.
If we want to expose the controls as part of the camera description, we
will potentially have an issue. That's not a showstopper, we will "just"
have to decide on what characteristics of the camera will be exposed
before he camera is "created", and which of those will be allowed to
change. I don't have an opinion on this yet.

There is also a need to restrict or otherwise influence the "modes"
offered by the camera through a configuration file. Milan has posted a
patch series for that (that I still need to review, mea culpa). How we
use that configuration file is still to be decided. If I recall
correctly, it had per-camera configuration entries, identifying the
camera by ID. This could be used to specify a different tuning file per
camera. Selecting a tuning file programmatically when creating the
camera may still be implemented if it covers other use cases.

There's another use case that is hard to support today and requires
another mechanism in my opinion, which is selection of tuning files for
different camera modules based on the same sensor. For this, we need to
add the concept of a camera module identifier to the platform firmware
(starting with DT, so possibly taking the form of a compatible string),
expose it to userspace, and use it in libcamera. That should be the main
mechanism to handle differences between modules, I view selection of
tuning files through other mechanisms for this use case as a temporary
workaround at best. Let's keep this in mind to make sure we tackle this
problem sooner than later and don't need a temporary workaround.

> >> 1) Or perhaps better a configfile argument tying into the configfile
> >> work Milan is doing and that could then override the default tuning
> >> file path, I think allowing overriding the default configfile on
> >> CameraManager::get() will be a bit more generic then directly specifying
> >> a tuning file.

I should have read the whole e-mail thread before writing the above :-)

> > 
> > Ah OK, possibly need to find out more about what Milan is doing. We
> > actually have two types of files:
> > 
> > 1. Pipeline configuration files. This is about stuff the pipeline
> > handler can assume about how it will be asked for memory buffers, what
> > limits to apply for camera timeouts, kind of "system" things
> > 
> > 2. And the camera tuning file, as described above.
> > 
> > So obviously worth finding out if Milan is working on 1 or 2 or both/neither!!
> 
> Milan is trying to add a generic libcamera configuration file concept,
> the latest version of Milan's patches are here:
> 
> https://lists.libcamera.org/pipermail/libcamera-devel/2024-June/042916.html
> 
> Note this patch set could really use some help wrt someone reviewing it.

See above for the mea culpa. I'll try harder.

> Depending on how Rpi specific (and detailed) your pipeline-configs are
> I can see either the whole pipeling config stuff moving to Milan's generic
> configfile support, or just using the generic configfile support
> to select a rpi-pipeline-config using the existing rpi-pipeline-config
> format.
> 
> Actually that you may want to specify both a rpi-pipeline-config
> file + a tuning file is why I think passing a path to a config
> file in the generic config-file format Milan is working on
> to CameraManager::get() is a good idea, then that (optional)
> generic-config-file can override the path of both.
> 
> Other pipeline handlers may want to allow users to override different
> default settings. So I think (at a first glance) that adding a way
> for API consumers to override certain defaults might best be done
> on top of some generic configuration mechanism to keep the overal
> API generic.

I see the config file as a pandora's box. Not that we shouldn't open it,
but it will be painful as everybody will have crazy ideas about what to
put in there. I'm sure we'll figure out how to keep it sane.

At this point, I personally view the configuration file as primarily
targetting system integrators and OEMs. This doesn't mean that users
can't tinker with it (especially Raspberry Pi hackers, who hold a role
similar to OEMs), but I don't see end-users on desktop systems having
the ability to select a different configuration file through a
pipewire-based stack. I plan to review the configuration file support
architecture design from this viewpoint. This means that I would prefer
*not* allowing selection of a different configuration file
programmatically. I'm sure this will be seen as blocking some use cases,
probably rightfully so for a subset of them. Let's examine them on a
case-by-case basis and see how to best address them, possibly by
expanding the configuration file selection mechanism.

> >> 2) We don't even need to do this right away this could be done
> >> on top of an initial switch to the suggested new API.

Agreed, but I think we should still probably continue the discussion.

> >> Note one challenge which we still have which I did not mention in
> >> my original email is that once there is agreement on this we need
> >> to find someone who actually has time to implement the suggested
> >> changes.
> >>
> >>> On Tue, 27 Aug 2024 at 10:41, Hans de Goede wrote:
> >>>>
> >>>> Hi All,
> >>>>
> >>>> Currently libcamera opens /dev nodes at camera creation time and keeps them
> >>>> open as long as the CameraManager exists.
> >>>>
> >>>> This has a number of advantages:
> >>>>
> >>>> - This simplifies error handling, no need to worry about problems opening
> >>>>   /dev nodes after camera creation
> >>>>
> >>>> - This allows things like CameraConfiguration::validate() to work without
> >>>>   first needing to do something like call open() or acquire() on the camera
> >>>>
> >>>> - This is useful for pre-allocating kernelbuffers which may be tied to
> >>>>   a certain fd and which will be automatically free-ed when closing the fd
> >>>>
> >>>> But this also has a few disadvantages, which are especially problematic
> >>>> with long running daemons like pipewire.
> >>>>
> >>>> - This keeps kernel resources associated to the fds tied-up all the time

That isn't necessarily a downside. Allocating all resources ahead of
time helps making sure we won't fail allocating them later. That's
important in safety-critical systems for instance. This being said,
those application can simply acquire/get/create the camera at
initialization time.

> >>>> - In some cases (e.g. uvcvideo, atomisp, VCM subdevs) this will keep
> >>>>   the device associated with the /dev node powered up all the time leading
> >>>>   to a significant increased battery drain

For atomisp, should this be fixed on the kernel side ? For VCMs, we need
to revisit power management on the kernel side, it has been implemented
without a design :-S

> >>>>
> >>>> For uvcvideo the increased power-consumption is a really big problem and
> >>>> a blocker for rolling out pipewire libcamera support in Linux distros (1).
> >>>>
> >>>> I have discussed how to solve this with Laurent and we have come up with
> >>>> a proposal to change the libcamera API to move the creation of Camera
> >>>> objects from the CameraManager to the application, which keeps
> >>>> the advantages, while removing the resource / power consumption issue
> >>>> for long running daemons which don't need the camera open the whole time.
> >>>>
> >>>> The idea is that PipelineHandler::match() will open the /dev nodes
> >>>> temporarily to collect camera capabilities and then registers
> >>>> objects of a new CameraDescription class with the CameraManager instead
> >>>> of directly creating and registering Camera objects.

We'll bikeshed names of course :-) Inside libcamera, I would like to
keep the class named Camera. We could rename the public class, but that
would be quite annoying for applications. Another short term option
would be to move the internal class to a different namespace.

Longer term, we will replace the public C++ API with a C API, and offer
C++ bindings on the application side. I think that will allow using
different classes with identical names on the application and libcamera
side.

> >>>>
> >>>> At the API level this means replacing the cameras list in CameraManager:
> >>>>
> >>>>         std::vector<std::shared_ptr<Camera>> cameras() const;
> >>>>
> >>>> with:
> >>>>
> >>>>         std::vector<std::shared_ptr<CameraDescription>> cameras() const;

The vector of shared pointers isn't great, it causes lots of references
being acquired and released. I wonder if we could do better. I'm not
entirely sure show though.

> >>>> Applications can then use the CameraDescription objects to either pick
> >>>> a camera themselves or e.g. show a choice of available cameras to the user.
> >>>>
> >>>> Then once a camera has been selected the application can call
> >>>> CameraManager::get() to get the camera (2), after which everything works
> >>>> as before.

We'll probably bikeshed this name too. I initially considered the
function should be called create(), or even that the Camera class should
be be constructed by the application. I think we need to consider this
API already with the C API in mind.

> >>>>
> >>>> Input on this proposal is much appreciated, comments anyone ?
> >>>>
> >>>> Regards,
> >>>>
> >>>> Hans
> >>>>
> >>>>
> >>>> 1) I've posted a patch with a short-term fix for uvcvideo where
> >>>> the unwanted power-consumption is a really big problem:
> >>>> https://lists.libcamera.org/pipermail/libcamera-devel/2024-August/044279.html
> >>>>
> >>>> 2) This requires Camera ids to be unique, which according to:
> >>>> https://gitlab.freedesktop.org/pipewire/wireplumber/-/issues/708
> >>>> they are not at the moment

The ids are unique, it's the underlying V4L2 devices that are not
necessarily. I've replied to the bug report.

-- 
Regards,

Laurent Pinchart