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

[Hans de Goede]

Hi David,

On 8/27/24 6:02 PM, David Plowman wrote:
> 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.

Ok, that is good to know.



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

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.

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.

Regards,

Hans





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