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

[Naushir Patuck]

Hi Laurent,

Thank you for your review on this series.

On Tue, 15 Jun 2021 at 03:52, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> 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 :-)
>

Ack all your suggestions, and happy for you to merge patches 1-4 straight
away
while I rework 5/6 and 6/6 (will discuss that in the other email thread).

Regards,
Naush



>
> > >  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210615/b2aca436/attachment.htm>