[libcamera-devel] [PATCH v2 3/7] libcamera: pipelines: add method to retrieve streams

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 25, 2019 at 04:33:36PM +0100, Niklas Söderlund wrote:
> A Camera object needs to be able to inquire its responsible
> PipelineHandler for which streams it supports. Add and implement such an
> interface to the PipelineHandler base class and all pipeline handler
> implementations.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  3 +++
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 12 ++++++++++++
>  src/libcamera/pipeline/uvcvideo.cpp      | 12 ++++++++++++
>  src/libcamera/pipeline/vimc.cpp          | 12 ++++++++++++
>  src/libcamera/pipeline_handler.cpp       | 12 ++++++++++++
>  5 files changed, 51 insertions(+)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 0db84217089e692a..f92cc5b5fa5e777b 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -17,6 +17,7 @@ namespace libcamera {
>  class CameraManager;
>  class DeviceEnumerator;
>  class MediaDevice;
> +class Stream;
>  
>  class Camera;
>  class CameraData
> @@ -38,6 +39,8 @@ public:
>  	PipelineHandler(CameraManager *manager);
>  	virtual ~PipelineHandler();
>  
> +	virtual std::vector<Stream> streams(const Camera *camera) const = 0;
> +

I thought pipeline handlers would create streams when they create the
camera, instead of on-demand at runtime. Wouldn't that be simpler for
pipeline handlers ? I envision the stream objects to be static for the
lifetime of the camera, so creating them on demand each time would be
more error prone as pipeline handlers could make mistakes and return
different streams each time.

>  	virtual bool match(DeviceEnumerator *enumerator) = 0;
>  
>  protected:
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d74655d037728feb..5c35c7a53b9347a3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -9,6 +9,7 @@
>  #include <vector>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
>  #include "log.h"
> @@ -27,6 +28,8 @@ public:
>  	PipelineHandlerIPU3(CameraManager *manager);
>  	~PipelineHandlerIPU3();
>  
> +	std::vector<Stream> streams(const Camera *camera) const;
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -60,6 +63,15 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()
>  		imgu_->release();
>  }
>  
> +std::vector<Stream> PipelineHandlerIPU3::streams(const Camera *camera) const
> +{
> +	std::vector<Stream> streams;
> +
> +	streams.push_back(Stream(0));
> +
> +	return streams;
> +}
> +
>  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 c51e8bc1f2c2bf25..e1f023245b8e63dd 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
>  #include "media_device.h"
> @@ -20,6 +21,8 @@ public:
>  	PipelineHandlerUVC(CameraManager *manager);
>  	~PipelineHandlerUVC();
>  
> +	std::vector<Stream> streams(const Camera *camera) const;
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -41,6 +44,15 @@ PipelineHandlerUVC::~PipelineHandlerUVC()
>  		media_->release();
>  }
>  
> +std::vector<Stream> PipelineHandlerUVC::streams(const Camera *camera) const
> +{
> +	std::vector<Stream> streams;
> +
> +	streams.push_back(Stream(0));
> +
> +	return streams;
> +}
> +
>  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch dm("uvcvideo");
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f58a97d51619515d..a96ec9f431361a32 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
>  #include "media_device.h"
> @@ -19,6 +20,8 @@ public:
>  	PipeHandlerVimc(CameraManager *manager);
>  	~PipeHandlerVimc();
>  
> +	std::vector<Stream> streams(const Camera *camera) const;
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -36,6 +39,15 @@ PipeHandlerVimc::~PipeHandlerVimc()
>  		media_->release();
>  }
>  
> +std::vector<Stream> PipeHandlerVimc::streams(const Camera *camera) const
> +{
> +	std::vector<Stream> streams;
> +
> +	streams.push_back(Stream(0));
> +
> +	return streams;
> +}
> +
>  bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch dm("vimc");
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 4d5344988cfe8aa2..a825e7bf882d23c2 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -84,6 +84,18 @@ PipelineHandler::~PipelineHandler()
>  	cameraData_.clear();
>  };
>  
> +/**
> + * \fn PipelineHandler::streams(const Camera *camera)
> + * \brief Retrive a array of all streams the camera provides

s/Retrive a/Retrieve the/

> + * \param[in] camera The camera to retrieve streams from
> + *
> + * This function is the interface to extract information about streams from a
> + * PipelineHandler.

That sentence doesn't seem to add extra information compared to the
\brief. I would drop it.

> + * \return A array of streams from the camera or a empty list if \a camera
> + *         is not part of the PipelineHandler.

No need for custom indentation.

And no need for this method if we create the streams when creating the
camera :-)

> + */
> +
>  /**
>   * \fn PipelineHandler::match(DeviceEnumerator *enumerator)
>   * \brief Match media devices and create camera instances

-- 
Regards,

Laurent Pinchart