[libcamera-devel,RFC,1/7] libcamera: pipelines: implement start and stop of a camera

504 diff mbox series

The camera need methods to start and stop capturing once it have been
configured. Extend the pipeline API to provide this and implement stubs
in all pipeline handlers.

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

Hi Niklas,

Discussion points at the documentation of start() and stop(),

On 05/02/2019 01:06, Niklas Söderlund wrote:
> The camera need methods to start and stop capturing once it have been
> configured. Extend the pipeline API to provide this and implement stubs
> in all pipeline handlers.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  3 +++
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 14 ++++++++++++
>  src/libcamera/pipeline/uvcvideo.cpp      | 14 ++++++++++++
>  src/libcamera/pipeline/vimc.cpp          | 14 ++++++++++++
>  src/libcamera/pipeline_handler.cpp       | 27 ++++++++++++++++++++++++
>  5 files changed, 72 insertions(+)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index b4321f0fa0f765be..4bfe45aaf78e34ab 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -45,6 +45,9 @@ public:
>  	virtual int configureStreams(Camera *camera,
>  				     std::map<Stream *, StreamConfiguration> &config) = 0;
>  
> +	virtual int start(const Camera *camera) = 0;
> +	virtual void stop(const Camera *camera) = 0;
> +
>  	virtual bool match(DeviceEnumerator *enumerator) = 0;
>  
>  protected:
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 7823bbb55d9bde16..3bf196051f2ebdbc 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -34,6 +34,9 @@ public:
>  	int configureStreams(Camera *camera,
>  			     std::map<Stream *, StreamConfiguration> &config) override;
>  
> +	int start(const Camera *camera) override;
> +	void stop(const Camera *camera) override;
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -104,6 +107,17 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  	return 0;
>  }
>  
> +int PipelineHandlerIPU3::start(const Camera *camera)
> +{
> +	LOG(IPU3, Error) << "TODO: start camera";
> +	return 0;
> +}
> +
> +void PipelineHandlerIPU3::stop(const Camera *camera)
> +{
> +	LOG(IPU3, Error) << "TODO: stop camera";
> +}
> +
>  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 ad4d45d0698519b6..e6a15c58a63cf76b 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -30,6 +30,9 @@ public:
>  	int configureStreams(Camera *camera,
>  			     std::map<Stream *, StreamConfiguration> &config) override;
>  
> +	int start(const Camera *camera) override;
> +	void stop(const Camera *camera) override;
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -82,6 +85,17 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
>  	return 0;
>  }
>  
> +int PipelineHandlerUVC::start(const Camera *camera)
> +{
> +	LOG(UVC, Error) << "TODO: start camera";
> +	return 0;
> +}
> +
> +void PipelineHandlerUVC::stop(const Camera *camera)
> +{
> +	LOG(UVC, Error) << "TODO: stop camera";
> +}
> +
>  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch dm("uvcvideo");
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 900477e257232b70..11eca0cd4d9a6e0c 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -30,6 +30,9 @@ public:
>  	int configureStreams(Camera *camera,
>  			     std::map<Stream *, StreamConfiguration> &config) override;
>  
> +	int start(const Camera *camera) override;
> +	void stop(const Camera *camera) override;
> +
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> @@ -77,6 +80,17 @@ int PipeHandlerVimc::configureStreams(Camera *camera,
>  	return 0;
>  }
>  
> +int PipeHandlerVimc::start(const Camera *camera)
> +{
> +	LOG(VIMC, Error) << "TODO: start camera";
> +	return 0;
> +}
> +
> +void PipeHandlerVimc::stop(const Camera *camera)
> +{
> +	LOG(VIMC, Error) << "TODO: stop camera";
> +}
> +
>  bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch dm("vimc");
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 3a560e10c442717f..fa5f780cea34fc8f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -109,6 +109,33 @@ PipelineHandler::~PipelineHandler()
>   * \return 0 on success or a negative error code on error.
>   */
>  
> +/**
> + * \fn PipelineHandler::start()
> + * \brief Start capturing from a group of streams
> + * \param[in] camera The camera to start
> + *
> + * Start the group of streams that have been configured for capture by
> + * \a configureStreams().

Minor:

Aha - so looking at the doxygen manual, \a is not what I thought. It
emphasises the next word. It's not required for the linking. But that's
fine I don't think it hurts to highlight the configureStreams(), but it
might be a mis-use of \a command.

	http://www.doxygen.nl/manual/commands.html#cmda

^ This is a discussion point not a rejection of the \a :)


>                             The intended caller of this interface is
> + * the Camera class which will in turn will receive the call from the
> + * application to indicate that it's done configuring the streams
> + * and are ready to capture.
> + *

The intended caller? Or the only caller?

I don't think anyone else would manage the pipeline (unless we have
pipelines in pipelines ... but lets not go there).



> + * \return 0 on success or a negative error code on error.
> + */
> +
> +/**
> + * \fn PipelineHandler::stop()
> + * \brief Stop capturing from all running streams
> + * \param[in] camera The camera to stop
> + *
> + * Stop processing requests, finish processing all already queued requests
> + * and once all buffers on all streams have been dequeued stop capturing
> + * from all running streams. Finish processing of queued requests do not
> + * necessarily involve processing them by the capture hardware but notifying
> + * the application that a queued request was not processed due to the fact
> + * that the camera was stopped before they could be processed.

Some questions on stop():

Will we expect stop() to be a blocking call (perhaps with a timeout?) if
we want it to make sure all buffers and requests are stopped?

Or should this just be somewhat asynchronous - making sure that any
active processing finishes as soon as possible and that nothing new is
queued?

Should the stop function return a potential error if there is an issue
stopping the stream?

I would expect most of the time a stop is part of a tear down, but
perhaps there will be times we want to assert a clean shutdown has happened.



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