[libcamera-devel] [PATCH v4 4/5] android: camera_device: Get properties from configuration

[Jacopo Mondi]

Hi Laurent,

On Tue, Apr 13, 2021 at 07:22:56AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> >
> > +	/*
> > +	 * Get camera properties from the configuration file.
> > +	 *
> > +	 * The camera properties as recorded in the configuration file
> > +	 * supplement information that cannot be retrieved from the
> > +	 * libcamera::Camera at run time.
> > +	 */
> > +	const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());
> > +	if (!cameraProps.valid) {
> > +		LOG(HAL, Error) << "Failed to register camera: " << cam->id();
> > +		return;
> > +	}
>
> Could you please reply to the comment in v4 regarding UVC cameras ?
>

Sorry, I completely missed that part, I'll re-paste comment here

> What if we have a fully populated libcamera::Camera (with properties
> retrieved from the firmware) ? Should we still require a configuration
> file then ? The configuration file will likely be mandatory in the
> future to provide additional information that the firmware doesn't
> provide (such as lens-related information for instance), so I suppose
> it's fine to already make it mandatory.
>

I think so

> We should however not make the configuration mandatory for external
> cameras, this would break UVC support.

I see. It won't be possible to provide a configuration file for
pluggable cameras, although there might be vendors that embedds a UVC
camera and want a configuration file.

I'll see how this can be handled... we'll need to handle the
case where !cameraProps.valid and default properties to something
sensible for an external camera. I wonder if we ever get to the point
were we could specialize CameraDevice for the external use case, but
that's for later...


>
> Related to breakages, we need to also ensure that the configuration file
> will get deployed correctly in Chrome OS builds of libcamera, or the
> existing tests will start failing. I'm not sure how that's best handled,
> Hiro, maybe this is something that you could help us with ?


> > +
> >  	/* Create a CameraDevice instance to wrap the libcamera Camera. */
> >  	std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam);
> > -	int ret = camera->initialize();
> > +	int ret = camera->initialize(cameraProps);
> >  	if (ret) {
> >  		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> >  		return;
> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > index d9bf27989965..af1581da6579 100644
> > --- a/src/android/camera_hal_manager.h
> > +++ b/src/android/camera_hal_manager.h
> > @@ -19,6 +19,8 @@
> >
> >  #include <libcamera/camera_manager.h>
> >
> > +#include "camera_hal_config.h"
> > +
> >  class CameraDevice;
> >
> >  class CameraHalManager
> > @@ -50,6 +52,7 @@ private:
> >  	CameraDevice *cameraDeviceFromHalId(unsigned int id);
> >
> >  	std::unique_ptr<libcamera::CameraManager> cameraManager_;
> > +	CameraHalConfig halConfig_;
> >
> >  	const camera_module_callbacks_t *callbacks_;
> >  	std::vector<std::unique_ptr<CameraDevice>> cameras_;
>
> --
> Regards,
>
> Laurent Pinchart