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

[David Plowman]

Hi Hans

Thanks for the reply!

On Tue, 27 Aug 2024 at 16:25, Hans de Goede <hdegoede at redhat.com> wrote:
>
> Hi David,
>
> 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.

>
> 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.

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!!

Thanks
David

>
> 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.
>
> 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.
>
> Regards,
>
> Hans
>
>
>
>
> > On Tue, 27 Aug 2024 at 10:41, Hans de Goede <hdegoede at redhat.com> 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
> >> - 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 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.
> >>
> >> 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;
> >>
> >> 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.
> >>
> >> 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
> >>
> >>
> >
>