[libcamera-devel,2/9] libcamera: Add options() and setOptions() operations to Camera

15346 diff mbox series

Add options() and setOptions() operations to Camera. Both operations
takes a camera instance and reads or writes the options to it. Because
an option might change the supported characteristics of a camera module,
it could only be set before configuring.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 include/libcamera/camera.h                    |  3 +
 include/libcamera/internal/camera.h           |  2 +
 include/libcamera/internal/pipeline_handler.h |  2 +
 src/libcamera/camera.cpp                      | 59 +++++++++++++++++++
 src/libcamera/pipeline_handler.cpp            | 26 ++++++++
 5 files changed, 92 insertions(+)

Hi Han-Lin,

Thank you for the patch.

On Wed, Feb 09, 2022 at 03:19:10PM +0800, Han-Lin Chen wrote:
> Add options() and setOptions() operations to Camera. Both operations
> takes a camera instance and reads or writes the options to it. Because
> an option might change the supported characteristics of a camera module,
> it could only be set before configuring.

The problem of configuring pipeline handlers is something I've
considered before, but I haven't gone far enough to propose a good
solution. You're right that configuration can't change after
configuration, but I think we need an even stricter constraint. The
configuration file may affect the features and capabilities exposed by
the pipeline handler through the camera (for instance it could affect
control limits, due to different IPA configurations, but also possibly
the list of controls themselves, and of course the capabilities of the
streams, in particular the list of supported resolutions), and I think
it thus needs to be set before even querying the camera for any of the
supported features. There's a bit of a chicken and egg issue, we can't
set options before the Camera instance is constructed if the API to set
options it exposed by the Camera class.

One option would be to instead expose the API through the CameraManager
class, and require options to be set before the CameraManager is
started. The API would need to support per-camera options, which could
be set based on the camera ID. We may also decide to only support a
single option, the name of a top-level configuration file, which would
contain per-camera options and possibly point to other files (in
particular I think IPA tuning files should be separate, but we may want
to split other configuration data to separate files too - I'm not sure
what data and why though).

We may also rework the libcamera initialization API further, including
how and when Camera objects are constructed, and hower applications get
hold of them. Feel free to think out of the box here, we're really in
brainstorming mode.

You're tackling a lot of hard problems in a single patch series, so I'm
raising lots of potential issues in return. I'm sorry about that, please
rest assured I'm grateful for your work on this topic. I'll try to help
as much as possible.

> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  include/libcamera/camera.h                    |  3 +
>  include/libcamera/internal/camera.h           |  2 +
>  include/libcamera/internal/pipeline_handler.h |  2 +
>  src/libcamera/camera.cpp                      | 59 +++++++++++++++++++
>  src/libcamera/pipeline_handler.cpp            | 26 ++++++++
>  5 files changed, 92 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 5bb06584..1bb80a6d 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -98,9 +98,12 @@ public:
>  	Signal<Request *> requestCompleted;
>  	Signal<> disconnected;
>  
> +	int setOptions(const ControlList *options = nullptr);
> +
>  	int acquire();
>  	int release();
>  
> +	const ControlInfoMap &options() const;
>  	const ControlInfoMap &controls() const;
>  	const ControlList &properties() const;
>  
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> index 597426a6..c3cb1865 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -34,6 +34,8 @@ public:
>  	PipelineHandler *pipe() { return pipe_.get(); }
>  
>  	std::list<Request *> queuedRequests_;
> +
> +	ControlInfoMap optionInfo_;
>  	ControlInfoMap controlInfo_;
>  	ControlList properties_;
>  
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index c3e4c258..184e4b9a 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -45,6 +45,8 @@ public:
>  	MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,
>  					const DeviceMatch &dm);
>  
> +	virtual int setOptions(Camera *camera, const ControlList *options);
> +
>  	bool lock();
>  	void unlock();
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index bb856d60..9e95e6cc 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -480,6 +480,16 @@ Camera::Private::~Private()
>   * well, when implementing official support for control info updates.
>   */
>  
> +/**
> + * \var Camera::Private::optionInfo_
> + * \brief The set of options supported by the camera
> + *
> + * The optionInfo_ information shall be initialised by the pipeline handler when
> + * creating the camera.
> + *
> + * This member was meant to stay constant after the camera is created.
> + */
> +
>  /**
>   * \var Camera::Private::properties_
>   * \brief The list of properties supported by the camera
> @@ -802,6 +812,40 @@ int Camera::exportFrameBuffers(Stream *stream,
>  				      buffers);
>  }
>  
> +/**
> + * \brief Set Options for a camera.
> + * \param[in] options Options to be applied before configuring a camera
> + *
> + * Prior to configuring a camera, it's optional to set options to a camera to
> + * change its characteristics or internal behaviors. The supported options are
> + * depended on the platform. The caller should iterate supported Options by
> + * options() before setting them.
> + *
> + * Options might change the internal behavior of configure() and capturing
> + * images, and thus the function can only be called before configure().
> + * It's encouraged to re-check the results of controls(), properties() and
> + * generateConfiguration() after setting options to a camera.
> + *
> + * \context This function shall be synchronized by the caller with other
> + * functions that affect the camera state.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EACCES The camera is not in a state where it can set options
> + * \retval -EINVAL The options is not valid
> + */
> +int Camera::setOptions(const ControlList *options)
> +{
> +	Private *const d = _d();
> +
> +	int ret = d->isAccessAllowed(Private::CameraAvailable,
> +				     Private::CameraAcquired);
> +	if (ret < 0)
> +		return -EACCES;
> +
> +	return d->pipe_->invokeMethod(&PipelineHandler::setOptions,
> +				      ConnectionTypeBlocking, this, options);
> +}
> +
>  /**
>   * \brief Acquire the camera device for exclusive access
>   *
> @@ -894,6 +938,21 @@ const ControlInfoMap &Camera::controls() const
>  	return _d()->controlInfo_;
>  }
>  
> +/**
> + * \brief Retrieve the list of options supported by the camera
> + *
> + * The list of options supported by the camera and their associated
> + * constraints remain constant through the lifetime of the Camera object.
> + *
> + * \context This function is \threadsafe.
> + *
> + * \return A ControlInfoMap listing the options supported by the camera
> + */
> +const ControlInfoMap &Camera::options() const
> +{
> +	return _d()->optionInfo_;
> +}
> +
>  /**
>   * \brief Retrieve the list of properties of the camera
>   *
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 7ebd76ad..1f2a6d7e 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -142,6 +142,32 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>  	return media.get();
>  }
>  
> +/**
> + * \fn PipelineHandler::setOptions()
> + * \brief Set options for camera
> + * \param[in] camera The camera to set options
> + * \param[in] options Options to be applied to the Camera
> + *
> + * Set options to the camera. The supported options are different for each
> + * pipeline handler. The intended caller of the this function could iterate
> + * options supported by the camera with Camera::options().
> + *
> + * The intended caller of this function is the Camera class which will in turn
> + * be called from the application to indicate that a set of options should be
> + * applied. The user should check supported options with Camera::options()
> + * before setting them.
> + *
> + * \context This function is called from the CameraManager thread.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int PipelineHandler::setOptions([[maybe_unused]] Camera *camera,
> +				[[maybe_unused]] const ControlList *options)
> +{
> +	/* The default implementation which supported no options. */
> +	return -EINVAL;
> +}
> +
>  /**
>   * \brief Lock all media devices acquired by the pipeline
>   *