[libcamera-devel] [PATCH v3 08/13] libcamera: pipeline_handler: Return unique_ptr from generateConfiguration()
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Oct 24 10:01:04 CEST 2022
On Mon, Oct 24, 2022 at 03:03:51AM +0300, Laurent Pinchart wrote:
> The PipelineHandler::generateConfiguration() function allocates a
> CameraConfiguration instance and returns it. The ownership of the
> instance is transferred to the caller. This is a perfect match for a
> std::unique_ptr<>, which the Camera::generateConfiguration() function
> already returns. Update PipelineHandler::generateConfiguration() to
> match it. This fixes a memory leak in one of the error return paths in
> the IPU3 pipeline handler.
>
> While at it, update the Camera::generateConfiguration() function
> documentation to drop the sentence that describes the ownership
> transfer, as that is implied by usage of std::unique_ptr<>.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> include/libcamera/internal/pipeline_handler.h | 2 +-
> src/libcamera/camera.cpp | 8 ++++----
> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++-----
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++----
> src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 8 +++++---
> src/libcamera/pipeline/vimc/vimc.cpp | 8 +++++---
> src/libcamera/pipeline_handler.cpp | 3 +--
> 9 files changed, 38 insertions(+), 34 deletions(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index b6139a88d421..96aab9d6459e 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -48,7 +48,7 @@ public:
> bool acquire();
> void release();
>
> - virtual CameraConfiguration *generateConfiguration(Camera *camera,
> + virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> const StreamRoles &roles) = 0;
> virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 9fe29ca9dca5..daef77016971 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -934,8 +934,7 @@ const std::set<Stream *> &Camera::streams() const
> * \context This function is \threadsafe.
> *
> * \return A CameraConfiguration if the requested roles can be satisfied, or a
> - * null pointer otherwise. The ownership of the returned configuration is
> - * passed to the caller.
> + * null pointer otherwise.
> */
> std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> {
> @@ -949,7 +948,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> if (roles.size() > streams().size())
> return nullptr;
>
> - CameraConfiguration *config = d->pipe_->generateConfiguration(this, roles);
> + std::unique_ptr<CameraConfiguration> config =
> + d->pipe_->generateConfiguration(this, roles);
> if (!config) {
> LOG(Camera, Debug)
> << "Pipeline handler failed to generate configuration";
> @@ -966,7 +966,7 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
>
> LOG(Camera, Debug) << msg.str();
>
> - return std::unique_ptr<CameraConfiguration>(config);
> + return config;
> }
>
> /**
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 3b892d9671c5..e4d79ea44aed 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -136,7 +136,7 @@ public:
>
> PipelineHandlerIPU3(CameraManager *manager);
>
> - CameraConfiguration *generateConfiguration(Camera *camera,
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> @@ -424,11 +424,12 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> {
> }
>
> -CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> - const StreamRoles &roles)
> +std::unique_ptr<CameraConfiguration>
> +PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
> {
> IPU3CameraData *data = cameraData(camera);
> - IPU3CameraConfiguration *config = new IPU3CameraConfiguration(data);
> + std::unique_ptr<IPU3CameraConfiguration> config =
> + std::make_unique<IPU3CameraConfiguration>(data);
>
> if (roles.empty())
> return config;
> @@ -494,7 +495,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> default:
> LOG(IPU3, Error)
> << "Requested stream role not supported: " << role;
> - delete config;
> return nullptr;
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 343f8cb2c7ed..7c54550005fa 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -323,7 +323,8 @@ class PipelineHandlerRPi : public PipelineHandler
> public:
> PipelineHandlerRPi(CameraManager *manager);
>
> - CameraConfiguration *generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> + const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -561,11 +562,12 @@ PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> {
> }
>
> -CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> - const StreamRoles &roles)
> +std::unique_ptr<CameraConfiguration>
> +PipelineHandlerRPi::generateConfiguration(Camera *camera, const StreamRoles &roles)
> {
> RPiCameraData *data = cameraData(camera);
> - CameraConfiguration *config = new RPiCameraConfiguration(data);
> + std::unique_ptr<CameraConfiguration> config =
> + std::make_unique<RPiCameraConfiguration>(data);
> V4L2SubdeviceFormat sensorFormat;
> unsigned int bufferCount;
> PixelFormat pixelFormat;
> @@ -640,13 +642,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> default:
> LOG(RPI, Error) << "Requested stream role not supported: "
> << role;
> - delete config;
> return nullptr;
> }
>
> if (rawCount > 1 || outCount > 2) {
> LOG(RPI, Error) << "Invalid stream roles requested";
> - delete config;
> return nullptr;
> }
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 1418dc9a47fb..50da92d4d6f8 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -142,7 +142,7 @@ class PipelineHandlerRkISP1 : public PipelineHandler
> public:
> PipelineHandlerRkISP1(CameraManager *manager);
>
> - CameraConfiguration *generateConfiguration(Camera *camera,
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> @@ -539,7 +539,8 @@ PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> * Pipeline Operations
> */
>
> -CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> +std::unique_ptr<CameraConfiguration>
> +PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> const StreamRoles &roles)
> {
> RkISP1CameraData *data = cameraData(camera);
> @@ -550,7 +551,8 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> return nullptr;
> }
>
> - CameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);
> + std::unique_ptr<CameraConfiguration> config =
> + std::make_unique<RkISP1CameraConfiguration>(camera, data);
> if (roles.empty())
> return config;
>
> @@ -595,7 +597,6 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> default:
> LOG(RkISP1, Warning)
> << "Requested stream role not supported: " << role;
> - delete config;
> return nullptr;
> }
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 37b3e7acd0a1..a32de7f36e13 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -311,8 +311,8 @@ class SimplePipelineHandler : public PipelineHandler
> public:
> SimplePipelineHandler(CameraManager *manager);
>
> - CameraConfiguration *generateConfiguration(Camera *camera,
> - const StreamRoles &roles) override;
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> + const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -1037,12 +1037,12 @@ SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
> {
> }
>
> -CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera,
> - const StreamRoles &roles)
> +std::unique_ptr<CameraConfiguration>
> +SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
> {
> SimpleCameraData *data = cameraData(camera);
> - CameraConfiguration *config =
> - new SimpleCameraConfiguration(camera, data);
> + std::unique_ptr<CameraConfiguration> config =
> + std::make_unique<SimpleCameraConfiguration>(camera, data);
>
> if (roles.empty())
> return config;
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index a28195450634..277465b72164 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -74,7 +74,7 @@ class PipelineHandlerUVC : public PipelineHandler
> public:
> PipelineHandlerUVC(CameraManager *manager);
>
> - CameraConfiguration *generateConfiguration(Camera *camera,
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> @@ -178,11 +178,13 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> {
> }
>
> -CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> +std::unique_ptr<CameraConfiguration>
> +PipelineHandlerUVC::generateConfiguration(Camera *camera,
> const StreamRoles &roles)
> {
> UVCCameraData *data = cameraData(camera);
> - CameraConfiguration *config = new UVCCameraConfiguration(data);
> + std::unique_ptr<CameraConfiguration> config =
> + std::make_unique<UVCCameraConfiguration>(data);
>
> if (roles.empty())
> return config;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index f85d05f77a61..204f5ad73f6d 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -84,7 +84,7 @@ class PipelineHandlerVimc : public PipelineHandler
> public:
> PipelineHandlerVimc(CameraManager *manager);
>
> - CameraConfiguration *generateConfiguration(Camera *camera,
> + std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> @@ -189,11 +189,13 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> {
> }
>
> -CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> +std::unique_ptr<CameraConfiguration>
> +PipelineHandlerVimc::generateConfiguration(Camera *camera,
> const StreamRoles &roles)
> {
> VimcCameraData *data = cameraData(camera);
> - CameraConfiguration *config = new VimcCameraConfiguration(data);
> + std::unique_ptr<CameraConfiguration> config =
> + std::make_unique<VimcCameraConfiguration>(data);
>
> if (roles.empty())
> return config;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 588a3db30e82..825aff5ac20a 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -233,8 +233,7 @@ void PipelineHandler::unlockMediaDevices()
> * handler.
> *
> * \return A valid CameraConfiguration if the requested roles can be satisfied,
> - * or a null pointer otherwise. The ownership of the returned configuration is
> - * passed to the caller.
> + * or a null pointer otherwise.
> */
>
> /**
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list