[libcamera-devel] [PATCH v4 4/6] libcamera: camera: extend camera object to support streams

[Kieran Bingham]

Hi Niklas,

On 29/01/2019 02:00, Niklas Söderlund wrote:
> A camera consists of one or more video streams originating from the same
> video source. The different streams could for example have access to
> different hardware blocks in the video pipeline and therefore be able to
> process the video source in different ways.
> 
> All static information describing each stream need to be recorded at
> camera creation. After a camera is created an application can retrieve
> the static information about its stream at any time.
> 
> Update all pipeline handlers to register one stream per camera, this
> will be extended in the future for some of the pipelines.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Just my usual Capitalisation comments of the Day here.
The discussion point about reconnecting cameras is only that ... a
discussion point.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  include/libcamera/camera.h           |  8 ++++++-
>  src/libcamera/camera.cpp             | 34 ++++++++++++++++++++++++----
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  9 ++++++--
>  src/libcamera/pipeline/uvcvideo.cpp  |  5 +++-
>  src/libcamera/pipeline/vimc.cpp      |  5 +++-
>  5 files changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 7e358f8c0aa093cf..3ebb8e96cc63b98a 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -15,12 +15,14 @@
>  namespace libcamera {
>  
>  class PipelineHandler;
> +class Stream;
>  
>  class Camera final
>  {
>  public:
>  	static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> -					      const std::string &name);
> +					      const std::string &name,
> +					      const std::vector<Stream *> &streams);
>  
>  	Camera(const Camera &) = delete;
>  	void operator=(const Camera &) = delete;
> @@ -32,6 +34,8 @@ public:
>  	int acquire();
>  	void release();
>  
> +	std::vector<Stream *> streams() const;
> +
>  private:
>  	Camera(PipelineHandler *pipe, const std::string &name);
>  	~Camera();
> @@ -41,8 +45,10 @@ private:
>  
>  	std::shared_ptr<PipelineHandler> pipe_;
>  	std::string name_;
> +	std::vector<Stream *> streams_;
>  
>  	bool acquired_;
> +	bool disconnected_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 500976b237bcbd2d..c8604ec8a8e6eaf4 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/stream.h>
>  
>  #include "log.h"
>  #include "pipeline_handler.h"
> @@ -56,13 +57,15 @@ LOG_DECLARE_CATEGORY(Camera)
>   * \brief Create a camera instance
>   * \param[in] name The name of the camera device
>   * \param[in] pipe The pipeline handler responsible for the camera device
> + * \param[in] streams Array of streams the camera provides

"Streams" (except pluralisation is a pain in Doxygen)

Here are some possible options to deal with this:

A: Array of \link Stream streams \endlink provided by the Camera
B: Array of streams (Stream) provided by the Camera
C: Array of all Stream objects provided by the Camera



>   *
>   * The caller is responsible for guaranteeing unicity of the camera name.
>   *
>   * \return A shared pointer to the newly created camera object
>   */
>  std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> -				       const std::string &name)
> +				       const std::string &name,
> +				       const std::vector<Stream *> &streams)
>  {
>  	struct Allocator : std::allocator<Camera> {
>  		void construct(void *p, PipelineHandler *pipe,
> @@ -76,7 +79,12 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
>  		}
>  	};
>  
> -	return std::allocate_shared<Camera>(Allocator(), pipe, name);
> +	std::shared_ptr<Camera> camera =
> +		std::allocate_shared<Camera>(Allocator(), pipe, name);
> +
> +	camera->streams_ = streams;
> +
> +	return camera;
>  }
>  
>  /**
> @@ -102,7 +110,8 @@ const std::string &Camera::name() const
>   */
>  
>  Camera::Camera(PipelineHandler *pipe, const std::string &name)
> -	: pipe_(pipe->shared_from_this()), name_(name), acquired_(false)
> +	: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),
> +	  disconnected_(false)
>  {
>  }
>  
> @@ -125,7 +134,7 @@ void Camera::disconnect()
>  {
>  	LOG(Camera, Debug) << "Disconnecting camera " << name_;
>  
> -	/** \todo Block API calls when they will be implemented. */
> +	disconnected_ = true;
>  	disconnected.emit(this);

Will we ever reconnect?
Or will this object just be destroyed and a new one created?

re-connecting an existing camera is tricky and requires keeping data
around that perhaps we don't want (or maybe it would have a timeout?) -
but it would mean an app handling a camera could automatically resume
play in the event that a cable was pulled out and put back in again? (or
a dodgy electrical link or such ... )


>  }
>  
> @@ -164,4 +173,21 @@ void Camera::release()
>  	acquired_ = false;
>  }
>  
> +/**
> + * \brief Retrieve all the camera's stream information
> + *
> + * Retrieve all of the camera's static stream information. The static
> + * information describes among other things how many streams the camera
> + * support and each streams capabilities.
> + *
> + * \return An array of all the camera's streams valid streams.
> + */
> +std::vector<Stream *> Camera::streams() const
> +{
> +	if (disconnected_)
> +		return std::vector<Stream *>{};
> +
> +	return streams_;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d74655d037728feb..f2029d17b4ad0273 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"
> @@ -37,6 +38,7 @@ private:
>  			: dev_(nullptr) {}
>  		~IPU3CameraData() { delete dev_; }
>  		V4L2Device *dev_;
> +		Stream stream_;
>  	};
>  
>  	std::shared_ptr<MediaDevice> cio2_;
> @@ -196,8 +198,11 @@ void PipelineHandlerIPU3::registerCameras()
>  		if (link->setEnabled(true))
>  			continue;
>  
> +		IPU3CameraData *data = new IPU3CameraData();
> +
>  		std::string cameraName = sensor->name() + " " + std::to_string(id);
> -		std::shared_ptr<Camera> camera = Camera::create(this, cameraName);
> +		std::vector<Stream *> streams{ &data->stream_ };
> +		std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
>  
>  		/*
>  		 * If V4L2 device creation fails, the Camera instance won't be
> @@ -209,10 +214,10 @@ void PipelineHandlerIPU3::registerCameras()
>  			LOG(IPU3, Error)
>  				<< "Failed to register camera["
>  				<< numCameras << "] \"" << cameraName << "\"";
> +			delete data;
>  			continue;
>  		}
>  
> -		IPU3CameraData *data = new IPU3CameraData();
>  		data->dev_ = videoDev;
>  		setCameraData(camera.get(),
>  			      std::move(std::unique_ptr<IPU3CameraData>(data)));
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index c51e8bc1f2c2bf25..8ea7ac74d2a5395d 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"
> @@ -25,6 +26,7 @@ public:
>  private:
>  	std::shared_ptr<MediaDevice> media_;
>  	V4L2Device *video_;
> +	Stream stream_;
>  };
>  
>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> @@ -64,7 +66,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  		return false;
>  	}
>  
> -	std::shared_ptr<Camera> camera = Camera::create(this, media_->model());
> +	std::vector<Stream *> streams{ &stream_ };
> +	std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
>  	registerCamera(std::move(camera));
>  	hotplugMediaDevice(media_.get());
>  
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f58a97d51619515d..9e1cf11a20c7a7e3 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"
> @@ -23,6 +24,7 @@ public:
>  
>  private:
>  	std::shared_ptr<MediaDevice> media_;
> +	Stream stream_;
>  };
>  
>  PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)
> @@ -56,7 +58,8 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
>  
>  	media_->acquire();
>  
> -	std::shared_ptr<Camera> camera = Camera::create(this, "Dummy VIMC Camera");
> +	std::vector<Stream *> streams{ &stream_ };
> +	std::shared_ptr<Camera> camera = Camera::create(this, "Dummy VIMC Camera", streams);
>  	registerCamera(std::move(camera));
>  
>  	return true;
> 

-- 
Regards
--
Kieran