<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review on this series.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 15 Jun 2021 at 03:52, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Mon, Jun 14, 2021 at 06:07:41PM +0100, Naushir Patuck wrote:<br>
> On Mon, 14 Jun 2021, 10:53 am Naushir Patuck wrote:<br>
> > This avoids the need for any dynamic allocations and lifetime management. The<br>
> > base CamHelper class still accesses the parser through a pointer that is setup<br>
> > by the derived class constructor.<br>
> ><br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > ---<br>
> > src/ipa/raspberrypi/cam_helper.cpp | 5 ++---<br>
> > src/ipa/raspberrypi/cam_helper.hpp | 2 +-<br>
> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 9 +++++----<br>
> > src/ipa/raspberrypi/cam_helper_imx290.cpp | 2 +-<br>
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++-<br>
> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 2 +-<br>
> > 6 files changed, 14 insertions(+), 11 deletions(-)<br>
> ><br>
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > index 062e94c4fef3..ab66e2ddef3e 100644<br>
> > --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > @@ -40,15 +40,14 @@ CamHelper *CamHelper::Create(std::string const<br>
> > &cam_name)<br>
> > return nullptr;<br>
> > }<br>
> ><br>
> > -CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)<br>
> > - : parser_(parser), initialized_(false),<br>
> > +CamHelper::CamHelper(unsigned int frameIntegrationDiff)<br>
> > + : parser_(nullptr), initialized_(false),<br>
> > frameIntegrationDiff_(frameIntegrationDiff)<br>
> > {<br>
> > }<br>
> ><br>
> > CamHelper::~CamHelper()<br>
> > {<br>
> > - delete parser_;<br>
> > }<br>
> ><br>
> > void CamHelper::Prepare(Span<const uint8_t> buffer,<br>
> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > index f53f5c39b01c..460839079741 100644<br>
> > --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > @@ -67,7 +67,7 @@ class CamHelper<br>
> > {<br>
> > public:<br>
> > static CamHelper *Create(std::string const &cam_name);<br>
> > - CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);<br>
> > + CamHelper(unsigned int frameIntegrationDiff);<br>
> > virtual ~CamHelper();<br>
> > void SetCameraMode(const CameraMode &mode);<br>
> > virtual void Prepare(libcamera::Span<const uint8_t> buffer,<br>
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > index ec218dce5456..36dbe8cd941a 100644<br>
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > @@ -54,15 +54,16 @@ private:<br>
> > * in units of lines.<br>
> > */<br>
> > static constexpr int frameIntegrationDiff = 4;<br>
> > +<br>
> > + MdParserImx219 imx219_parser;<br>
> > };<br>
> ><br>
> <br>
> > CamHelperImx219::CamHelperImx219()<br>
> > + : CamHelper(frameIntegrationDiff)<br>
> > +{<br>
> > #if ENABLE_EMBEDDED_DATA<br>
> > - : CamHelper(new MdParserImx219(), frameIntegrationDiff)<br>
> > -#else<br>
> > - : CamHelper(nullptr, frameIntegrationDiff)<br>
> > + parser_ = &imx219_parser;<br>
> > #endif<br>
> > -{<br>
> > }<br>
> <br>
> Looking at this again, I think it might be better to pass the parser<br>
> pointer thorough the CamHelper constructor as before. I'll change that back<br>
> for v2.<br>
<br>
The rest of the patch looks fine to me.<br>
<br>
Should I push patch 1/6 to 4/6 already, or will you include them in a v2<br>
? I have them ready in a branch :-)<br></blockquote><div><br></div><div>Ack all your suggestions, and happy for you to merge patches 1-4 straight away</div><div>while I rework 5/6 and 6/6 (will discuss that in the other email thread).</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > uint32_t CamHelperImx219::GainCode(double gain) const<br>
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx290.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx290.cpp<br>
> > index 6f412e403f16..dea57b84bf0b 100644<br>
> > --- a/src/ipa/raspberrypi/cam_helper_imx290.cpp<br>
> > +++ b/src/ipa/raspberrypi/cam_helper_imx290.cpp<br>
> > @@ -30,7 +30,7 @@ private:<br>
> > };<br>
> ><br>
> > CamHelperImx290::CamHelperImx290()<br>
> > - : CamHelper(nullptr, frameIntegrationDiff)<br>
> > + : CamHelper(frameIntegrationDiff)<br>
> > {<br>
> > }<br>
> ><br>
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > index 25b36bce0dac..038a8583d311 100644<br>
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > @@ -47,11 +47,14 @@ private:<br>
> > * in units of lines.<br>
> > */<br>
> > static constexpr int frameIntegrationDiff = 22;<br>
> > +<br>
> > + MdParserImx477 imx477_parser;<br>
> > };<br>
> ><br>
> > CamHelperImx477::CamHelperImx477()<br>
> > - : CamHelper(new MdParserImx477(), frameIntegrationDiff)<br>
> > + : CamHelper(frameIntegrationDiff)<br>
> > {<br>
> > + parser_ = &imx477_parser;<br>
> > }<br>
> ><br>
> > uint32_t CamHelperImx477::GainCode(double gain) const<br>
> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > index 12be6bf931a8..fff648279d2a 100644<br>
> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > @@ -38,7 +38,7 @@ private:<br>
> > */<br>
> ><br>
> > CamHelperOv5647::CamHelperOv5647()<br>
> > - : CamHelper(nullptr, frameIntegrationDiff)<br>
> > + : CamHelper(frameIntegrationDiff)<br>
> > {<br>
> > }<br>
> ><br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>