[libcamera-devel,v2,4/7] libcamera: pipelines: add method to configure streams

383 diff mbox series

All the streams which are to be used needs to be configured at the same
time, that is passed from the application to the pipeline handler in a
single call. This would allow the pipeline handler to take any
dependences between the different streams and there configuration into
account when setting up the hardware.

This implementation do not interact with any hardware, instead it
extends all pipeline handlers with the argument validation needed to
make sure the configuration request is sound. It then proceeds to print
the requested frame dimensions to the log. Future work based on more
components are needed to make the pipeline specific implementations
truly useful.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/pipeline_handler.h |  3 +++
 src/libcamera/pipeline/ipu3/ipu3.cpp     | 21 +++++++++++++++++++++
 src/libcamera/pipeline/uvcvideo.cpp      | 24 ++++++++++++++++++++++++
 src/libcamera/pipeline/vimc.cpp          | 24 ++++++++++++++++++++++++
 src/libcamera/pipeline_handler.cpp       | 18 ++++++++++++++++++
 5 files changed, 90 insertions(+)

Hi Niklas,

Thank you for the patch.

On Fri, Jan 25, 2019 at 04:33:37PM +0100, Niklas Söderlund wrote:
> All the streams which are to be used needs to be configured at the same
> time, that is passed from the application to the pipeline handler in a
> single call. This would allow the pipeline handler to take any
> dependences between the different streams and there configuration into
> account when setting up the hardware.
> 
> This implementation do not interact with any hardware, instead it
> extends all pipeline handlers with the argument validation needed to
> make sure the configuration request is sound. It then proceeds to print
> the requested frame dimensions to the log. Future work based on more
> components are needed to make the pipeline specific implementations
> truly useful.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  3 +++
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 21 +++++++++++++++++++++
>  src/libcamera/pipeline/uvcvideo.cpp      | 24 ++++++++++++++++++++++++
>  src/libcamera/pipeline/vimc.cpp          | 24 ++++++++++++++++++++++++
>  src/libcamera/pipeline_handler.cpp       | 18 ++++++++++++++++++
>  5 files changed, 90 insertions(+)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index f92cc5b5fa5e777b..5813e00aec068356 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -18,6 +18,7 @@ class CameraManager;
>  class DeviceEnumerator;
>  class MediaDevice;
>  class Stream;
> +class StreamConfiguration;
>  
>  class Camera;
>  class CameraData
> @@ -41,6 +42,8 @@ public:
>  
>  	virtual std::vector<Stream> streams(const Camera *camera) const = 0;
>  
> +	virtual int configure(const Camera *camera, std::vector<StreamConfiguration *> &config) = 0;
> +

I'm wondering whether we should pass an
std::map<unsigned int, StreamConfiguration *> instead. This would ensure
that there would be no two StreamConfiguration for the same stream.
Otherwise you'd have to check for that error manually. In general it's a
good practice to design APIs in a way that minimizes the possibility of
using them incorrectly, as that reduces both error handling code and
potential incorrect uses.

Thinking out loud, I'm also wondering whether it shouldn't be a
std::map<unsigned int, StreamConfiguration>, otherwise the caller would
need to store the configurations separately. On the other hand that may
result in more copies in the caller. Maybe not the best idea actually.

Should the config be const, or do we allow the pipeline handler to
modify it ?

>  	virtual bool match(DeviceEnumerator *enumerator) = 0;
>  
>  protected:
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5c35c7a53b9347a3..2d790f72dc63c31b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -30,6 +30,8 @@ public:
>  
>  	std::vector<Stream> streams(const Camera *camera) const;
>  
> +	int configure(const Camera *camera, std::vector<StreamConfiguration *> &config);
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -72,6 +74,25 @@ std::vector<Stream> PipelineHandlerIPU3::streams(const Camera *camera) const
>  	return streams;
>  }
>  
> +int PipelineHandlerIPU3::configure(const Camera *camera,
> +				   std::vector<StreamConfiguration *> &config)
> +{
> +	StreamConfiguration *cfg;
> +
> +	if (config.size() != 1)
> +		return -EINVAL;
> +
> +	cfg = config.front();
> +
> +	if (!cfg || cfg->id() != 0)
> +		return -EINVAL;

Those check could be moved to the Camera::configure() function:

- We should check that the vector contains at least one entry, and not
  more than the number of streams.
- If we use a map we can't have multiple entries for the same stream,
  otherwise that should be checked too.
- If the map indexes on stream ID, the stream IDs should be validated.
  If it indexes on Stream object pointer, those pointers should be validated.
- As StreamConfiguration is created from a Stream, the cfg->id() check
  isn't needed. If we change StreamConfiguration creation then we'll
  have to update this accordingly.

> +	LOG(IPU3, Info) << "TODO: Configure the camera for resolution " <<
> +		cfg->width() << "x" << cfg->height();
> +
> +	return 0;
> +}
> +
>  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch cio2_dm("ipu3-cio2");
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index e1f023245b8e63dd..477a9fe7f56d9a75 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -9,12 +9,15 @@
>  #include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
> +#include "log.h"
>  #include "media_device.h"
>  #include "pipeline_handler.h"
>  #include "v4l2_device.h"
>  
>  namespace libcamera {
>  
> +LOG_DEFINE_CATEGORY(UVC)
> +
>  class PipelineHandlerUVC : public PipelineHandler
>  {
>  public:
> @@ -23,6 +26,8 @@ public:
>  
>  	std::vector<Stream> streams(const Camera *camera) const;
>  
> +	int configure(const Camera *camera, std::vector<StreamConfiguration *> &config);
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -53,6 +58,25 @@ std::vector<Stream> PipelineHandlerUVC::streams(const Camera *camera) const
>  	return streams;
>  }
>  
> +int PipelineHandlerUVC::configure(const Camera *camera,
> +				  std::vector<StreamConfiguration *> &config)
> +{
> +	StreamConfiguration *cfg;
> +
> +	if (config.size() != 1)
> +		return -EINVAL;
> +
> +	cfg = config.front();
> +
> +	if (!cfg || cfg->id() != 0)
> +		return -EINVAL;
> +
> +	LOG(UVC, Info) << "TODO: Configure the camera for resolution " <<
> +		cfg->width() << "x" << cfg->height();
> +
> +	return 0;
> +}
> +
>  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch dm("uvcvideo");
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index a96ec9f431361a32..f9d3b3a604805baa 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -9,11 +9,14 @@
>  #include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
> +#include "log.h"
>  #include "media_device.h"
>  #include "pipeline_handler.h"
>  
>  namespace libcamera {
>  
> +LOG_DEFINE_CATEGORY(VIMC)
> +
>  class PipeHandlerVimc : public PipelineHandler
>  {
>  public:
> @@ -22,6 +25,8 @@ public:
>  
>  	std::vector<Stream> streams(const Camera *camera) const;
>  
> +	int configure(const Camera *camera, std::vector<StreamConfiguration *> &config);
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -48,6 +53,25 @@ std::vector<Stream> PipeHandlerVimc::streams(const Camera *camera) const
>  	return streams;
>  }
>  
> +int PipeHandlerVimc::configure(const Camera *camera,
> +			       std::vector<StreamConfiguration *> &config)
> +{
> +	StreamConfiguration *cfg;
> +
> +	if (config.size() != 1)
> +		return -EINVAL;
> +
> +	cfg = config.front();
> +
> +	if (!cfg || cfg->id() != 0)
> +		return -EINVAL;
> +
> +	LOG(VIMC, Info) << "TODO: Configure the camera for resolution " <<
> +		cfg->width() << "x" << cfg->height();
> +
> +	return 0;
> +}
> +
>  bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch dm("vimc");
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index a825e7bf882d23c2..dc3535f105e3a992 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -96,6 +96,24 @@ PipelineHandler::~PipelineHandler()
>   *         is not part of the PipelineHandler.
>   */
>  
> +/**
> + * \fn PipelineHandler::configure(const Camera *camera, std::vector<StreamConfiguration *> &config)
> + * \brief Configure a set of streams for a camera
> + * \param[in] camera The camera to configure streams for
> + * \param[in] config A array of stream configurations to try and setup

When the noun starts with a vowel, you should write "an" instead of "a".

Does this function "try" a configuration ? What's our model for try/set
configurations ?

> + *
> + * This function is the interface to configure one or more streams of a camera
> + * for capture. The intended user for this interface is the Camera class which
> + * will receive the array of configurations to apply from the application.
> + *
> + * The caller needs to verify the StreamConfiguration objects in the passed
> + * array after the call as it might remove one or more to satisfy hardware
> + * limitations. The call might also alter any or all of the configuration
> + * parameters of any stream to fit within valid operational conditions.

This answers my question :-) I wonder if we shouldn't start simple with
an error when the parameters are not acceptable, and then extend with a
try API.

> + * \return 0 on success or a negative error code on error.
> + */
> +
>  /**
>   * \fn PipelineHandler::match(DeviceEnumerator *enumerator)
>   * \brief Match media devices and create camera instances