[libcamera-devel] [PATCH 5/6] ipa: raspberrypi: Embed the metadata parser in the sensor CamHelper classes

[Laurent Pinchart]

Hi Naush,

On Mon, Jun 14, 2021 at 06:07:41PM +0100, Naushir Patuck wrote:
> On Mon, 14 Jun 2021, 10:53 am Naushir Patuck wrote:
> > This avoids the need for any dynamic allocations and lifetime management. The
> > base CamHelper class still accesses the parser through a pointer that is setup
> > by the derived class constructor.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp        | 5 ++---
> >  src/ipa/raspberrypi/cam_helper.hpp        | 2 +-
> >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 9 +++++----
> >  src/ipa/raspberrypi/cam_helper_imx290.cpp | 2 +-
> >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++-
> >  src/ipa/raspberrypi/cam_helper_ov5647.cpp | 2 +-
> >  6 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> > b/src/ipa/raspberrypi/cam_helper.cpp
> > index 062e94c4fef3..ab66e2ddef3e 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(unsigned int frameIntegrationDiff)
> > +       : parser_(nullptr), initialized_(false),
> >           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..460839079741 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -67,7 +67,7 @@ class CamHelper
> >  {
> >  public:
> >         static CamHelper *Create(std::string const &cam_name);
> > -       CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> > +       CamHelper(unsigned int frameIntegrationDiff);
> >         virtual ~CamHelper();
> >         void SetCameraMode(const CameraMode &mode);
> >         virtual void Prepare(libcamera::Span<const uint8_t> buffer,
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > index ec218dce5456..36dbe8cd941a 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > @@ -54,15 +54,16 @@ private:
> >          * in units of lines.
> >          */
> >         static constexpr int frameIntegrationDiff = 4;
> > +
> > +       MdParserImx219 imx219_parser;
> >  };
> >
> 
> >  CamHelperImx219::CamHelperImx219()
> > +       : CamHelper(frameIntegrationDiff)
> > +{
> >  #if ENABLE_EMBEDDED_DATA
> > -       : CamHelper(new MdParserImx219(), frameIntegrationDiff)
> > -#else
> > -       : CamHelper(nullptr, frameIntegrationDiff)
> > +       parser_ = &imx219_parser;
> >  #endif
> > -{
> >  }
> 
> Looking at this again, I think it might be better to pass the parser
> pointer thorough the CamHelper constructor as before. I'll change that back
> for v2.

The rest of the patch looks fine to me.

Should I push patch 1/6 to 4/6 already, or will you include them in a v2
? I have them ready in a branch :-)

> >  uint32_t CamHelperImx219::GainCode(double gain) const
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx290.cpp
> > b/src/ipa/raspberrypi/cam_helper_imx290.cpp
> > index 6f412e403f16..dea57b84bf0b 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..038a8583d311 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -47,11 +47,14 @@ private:
> >          * in units of lines.
> >          */
> >         static constexpr int frameIntegrationDiff = 22;
> > +
> > +       MdParserImx477 imx477_parser;
> >  };
> >
> >  CamHelperImx477::CamHelperImx477()
> > -       : CamHelper(new MdParserImx477(), frameIntegrationDiff)
> > +       : CamHelper(frameIntegrationDiff)
> >  {
> > +       parser_ = &imx477_parser;
> >  }
> >
> >  uint32_t CamHelperImx477::GainCode(double gain) const
> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > index 12be6bf931a8..fff648279d2a 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)
> >  {
> >  }
> >

-- 
Regards,

Laurent Pinchart