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

[Hans de Goede]

Hi David,

On 8/27/24 5:09 PM, David Plowman wrote:
> Hi Hans
> 
> Thanks for explaining this. It sounds good to me, I just had one
> question which may be relevant to this... or maybe it isn't - perhaps
> you can say what you think!
> 
> The thing Raspberry Pi has wanted for a long time is to be able to
> open/acquire/"get" a camera and to pass in specific tuning parameters
> for that camera. Back then, as an interim measure, we introduced an
> environment variable allowing a camera tuning file to be named
> globally, but we all knew this was a "bodge"!
> 
> Wind forward to the Pi 5, and we have many more users with more than
> one camera. Often, it's the exact same sensor and camera module and
> they want to run them with different tuning parameters. So the "bodge"
> really doesn't work for us any more. (And I'd expect that all vendors
> who routinely deal with multiple cameras would face similar issues.)
> 
> Is this something that it would make sense to address as part of the
> same question here? Or do you think it's a different problem that
> should be addressed separately?

Since the app will now be in control of creating the Camera objects
we could give CameraManager::get() an optional tuning file argument (1)
and then pass that along to the new pipeline handler method for
actually creating the camera, which I think should cover your
use-case nicely (2)

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 ?

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.

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