[libcamera-devel,v1,2/6] pipeline: raspberrypi: Move freeBuffers() to the RPiCameraData class

15427 diff mbox series

This function used to clear the camera buffers does not belong in the
PipelineHandlerRPi class as it only access members of the RPiCameraData,
so move it.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Hi Naush

Thanks for this patch!

On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> This function used to clear the camera buffers does not belong in the
> PipelineHandlerRPi class as it only access members of the RPiCameraData,
> so move it.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index d604c473c26c..3781e4e0e3c4 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -189,6 +189,7 @@ public:
>         {
>         }
>
> +       void freeBuffers();
>         void frameStarted(uint32_t sequence);
>
>         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> @@ -329,7 +330,6 @@ private:
>         int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);
>         int queueAllBuffers(Camera *camera);
>         int prepareBuffers(Camera *camera);
> -       void freeBuffers(Camera *camera);
>         void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
>  };
>
> @@ -1049,7 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
>         /* Stop the IPA. */
>         data->ipa_->stop();
>
> -       freeBuffers(camera);
> +       data->freeBuffers();
>  }
>
>  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> @@ -1445,16 +1445,14 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
>         data->ipa_->mapBuffers(ipaBuffers);
>  }
>
> -void PipelineHandlerRPi::freeBuffers(Camera *camera)
> +void RPiCameraData::freeBuffers()
>  {
> -       RPiCameraData *data = cameraData(camera);
> -
>         /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */
> -       std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());
> -       data->ipa_->unmapBuffers(ipaBuffers);
> -       data->ipaBuffers_.clear();
> +       std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end());
> +       ipa_->unmapBuffers(ipaBuffers);
> +       ipaBuffers_.clear();
>
> -       for (auto const stream : data->streams_)
> +       for (auto const stream : streams_)
>                 stream->releaseBuffers();
>  }
>
> --
> 2.25.1
>

I agree this is a bit tidier, and of course is being tested as part of
picamera2.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

Hi Naush,

Thank you for the patch.

On Mon, Mar 07, 2022 at 12:46:29PM +0000, Naushir Patuck via libcamera-devel wrote:
> This function used to clear the camera buffers does not belong in the
> PipelineHandlerRPi class as it only access members of the RPiCameraData,
> so move it.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index d604c473c26c..3781e4e0e3c4 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -189,6 +189,7 @@ public:
>  	{
>  	}
>  
> +	void freeBuffers();
>  	void frameStarted(uint32_t sequence);
>  
>  	int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> @@ -329,7 +330,6 @@ private:
>  	int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);
>  	int queueAllBuffers(Camera *camera);
>  	int prepareBuffers(Camera *camera);
> -	void freeBuffers(Camera *camera);
>  	void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
>  };
>  
> @@ -1049,7 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
>  	/* Stop the IPA. */
>  	data->ipa_->stop();
>  
> -	freeBuffers(camera);
> +	data->freeBuffers();
>  }
>  
>  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> @@ -1445,16 +1445,14 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
>  	data->ipa_->mapBuffers(ipaBuffers);
>  }
>  
> -void PipelineHandlerRPi::freeBuffers(Camera *camera)
> +void RPiCameraData::freeBuffers()
>  {
> -	RPiCameraData *data = cameraData(camera);
> -
>  	/* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */
> -	std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());
> -	data->ipa_->unmapBuffers(ipaBuffers);
> -	data->ipaBuffers_.clear();
> +	std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end());
> +	ipa_->unmapBuffers(ipaBuffers);
> +	ipaBuffers_.clear();
>  
> -	for (auto const stream : data->streams_)
> +	for (auto const stream : streams_)
>  		stream->releaseBuffers();
>  }
>