[libcamera-devel,v2,6/7] libcamera: camera: integrate streams and configuration

385 diff mbox series

Add retrieval and configuration of streams information and
configuration. The implementation in the Camera are minimalistic as the
heavily lifting are done by the pipeline handler implementations.

The single most important thing for the helpers in the Camera object is
to perform access control and making sure no request is forwarded to a
pipeline handler if the camera have been disconnected.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/camera.h |  7 ++++++
 src/libcamera/camera.cpp   | 48 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Hi Niklas,

Thank you for the patch.

On Fri, Jan 25, 2019 at 04:33:39PM +0100, Niklas Söderlund wrote:
> Add retrieval and configuration of streams information and
> configuration. The implementation in the Camera are minimalistic as the
> heavily lifting are done by the pipeline handler implementations.

s/heavily lifting are done/heavy lifting is done/

> The single most important thing for the helpers in the Camera object is
> to perform access control and making sure no request is forwarded to a
> pipeline handler if the camera have been disconnected.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera.h |  7 ++++++
>  src/libcamera/camera.cpp   | 48 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 7e358f8c0aa093cf..c6342ed81598921c 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -9,12 +9,15 @@
>  
>  #include <memory>
>  #include <string>
> +#include <vector>
>  
>  #include <libcamera/signal.h>
>  
>  namespace libcamera {
>  
>  class PipelineHandler;
> +class Stream;
> +class StreamConfiguration;
>  
>  class Camera final
>  {
> @@ -32,6 +35,10 @@ public:
>  	int acquire();
>  	void release();
>  
> +	std::vector<Stream> streams() const;

Do you expect streams to be modified, or should this return a const
vector ?

> +
> +	int configure(std::vector<StreamConfiguration *> &config);
> +

We may want some const here too, depending on your opinion of my review
of the previous e-mails.

>  private:
>  	Camera(PipelineHandler *pipe, const std::string &name);
>  	~Camera();
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index fd19e8cf6694cc1b..f90abfecd4e6bb48 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -6,6 +6,9 @@
>   */
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
> +
> +#include "pipeline_handler.h"
>  
>  #include "log.h"
>  #include "pipeline_handler.h"
> @@ -160,4 +163,49 @@ void Camera::release()
>  	acquired_ = false;
>  }
>  
> +/**
> + * \brief Retrieve the supported streams of the camera
> + *
> + * \return An array of streams supported by the camera device
> + */
> +std::vector<Stream> Camera::streams() const
> +{
> +	std::vector<Stream> streams;
> +
> +	if (pipe_)
> +		streams = pipe_->streams(this);
> +
> +	return streams;
> +}
> +
> +/**
> + * \brief Configure the camera device prior to capture

Missing \param.

> + *
> + * Prior to starting capture, the camera device must be configured to select a
> + * set of streams.
> + *
> + * The requested configuration \a config shall contain at least one stream and
> + * may contain multiple streams. For each stream an associated StreamFormat
> + * shall be supplied. Streams supported by the camera device not part of the
> + * \a config will be disabled.

We're missing a global explanation of how streams should be used.
There's information scattered in the related classes, but one or two
paragraphs in the Camera class documentation is also needed in my
opinion.

> + *
> + * Exclusive access to the camera device shall be ensured by a call to
> + * Camera::acquire() before calling this function, otherwise an -EACCES error

You can just write acquire() when the function is part of the same
class.

> + * will be returned.
> + *
> + * \param[in] config Array of stream configurations to setup

Ah no, not missing :-) Please move it just after \brief.

> + *
> + * \return 0 on success or a negative error code on error.
> + */
> +int Camera::configure(std::vector<StreamConfiguration *> &config)
> +{
> +	if (!pipe_)
> +		return -ENODEV;

This can't happen. We however need to block this call if the camera has
been disconnected.

> +	if (!acquired_)
> +		return -EACCES;
> +
> +	return pipe_->configure(this, config);
> +}
> +
>  } /* namespace libcamera */