[libcamera-devel] [PATCH v2 1/2] ipa: raspberrypi: Use a unique_ptr for the metadata parser

[David Plowman]

Hi Naush

Thanks for your work!

On Tue, 22 Jun 2021 at 14:20, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> The derived CamHelper class now allocates a metadata parser object through a
> unique_ptr that is passed to the base class constructor. This automates the
> lifetime management of the parser object.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        | 5 ++---
>  src/ipa/raspberrypi/cam_helper.hpp        | 5 +++--
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 4 ++--
>  src/ipa/raspberrypi/cam_helper_imx290.cpp | 2 +-
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 2 +-
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 062e94c4fef3..90498c37af98 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -40,15 +40,14 @@ CamHelper *CamHelper::Create(std::string const &cam_name)
>         return nullptr;
>  }
>
> -CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
> -       : parser_(parser), initialized_(false),
> +CamHelper::CamHelper(std::unique_ptr<MdParser> parser, unsigned int frameIntegrationDiff)
> +       : parser_(std::move(parser)), initialized_(false),

I can't say it doesn't feel ever so slightly strange to require a
unique_ptr to be passed in, which may even be a "null" one, which then
gets "moved" so that we take it over. But I really can't get too
worked up about it, and for all I know it may even be better in the
wonderful world of C++ so don't change it on my account! Therefore:

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

Thanks
David

>           frameIntegrationDiff_(frameIntegrationDiff)
>  {
>  }
>
>  CamHelper::~CamHelper()
>  {
> -       delete parser_;
>  }
>
>  void CamHelper::Prepare(Span<const uint8_t> buffer,
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index f53f5c39b01c..7371b9d97f66 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -6,6 +6,7 @@
>   */
>  #pragma once
>
> +#include <memory>
>  #include <string>
>
>  #include <libcamera/span.h>
> @@ -67,7 +68,7 @@ class CamHelper
>  {
>  public:
>         static CamHelper *Create(std::string const &cam_name);
> -       CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> +       CamHelper(std::unique_ptr<MdParser> parser, unsigned int frameIntegrationDiff);
>         virtual ~CamHelper();
>         void SetCameraMode(const CameraMode &mode);
>         virtual void Prepare(libcamera::Span<const uint8_t> buffer,
> @@ -92,7 +93,7 @@ protected:
>         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
>                                Metadata &metadata);
>
> -       MdParser *parser_;
> +       std::unique_ptr<MdParser> parser_;
>         CameraMode mode_;
>
>  private:
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index ec218dce5456..c85044a5fa6d 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -58,9 +58,9 @@ private:
>
>  CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
> -       : CamHelper(new MdParserImx219(), frameIntegrationDiff)
> +       : CamHelper(std::make_unique<MdParserImx219>(), frameIntegrationDiff)
>  #else
> -       : CamHelper(nullptr, frameIntegrationDiff)
> +       : CamHelper({}, frameIntegrationDiff)
>  #endif
>  {
>  }
> diff --git a/src/ipa/raspberrypi/cam_helper_imx290.cpp b/src/ipa/raspberrypi/cam_helper_imx290.cpp
> index 6f412e403f16..871c1f8eaec4 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx290.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx290.cpp
> @@ -30,7 +30,7 @@ private:
>  };
>
>  CamHelperImx290::CamHelperImx290()
> -       : CamHelper(nullptr, frameIntegrationDiff)
> +       : CamHelper({}, frameIntegrationDiff)
>  {
>  }
>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 25b36bce0dac..d72a9be0438e 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -50,7 +50,7 @@ private:
>  };
>
>  CamHelperImx477::CamHelperImx477()
> -       : CamHelper(new MdParserImx477(), frameIntegrationDiff)
> +       : CamHelper(std::make_unique<MdParserImx477>(), frameIntegrationDiff)
>  {
>  }
>
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index 12be6bf931a8..702c2d07f73a 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -38,7 +38,7 @@ private:
>   */
>
>  CamHelperOv5647::CamHelperOv5647()
> -       : CamHelper(nullptr, frameIntegrationDiff)
> +       : CamHelper({}, frameIntegrationDiff)
>  {
>  }
>
> --
> 2.25.1
>