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

[Kieran Bingham]

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 at 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
> 

-- 
Regards
--
Kieran