<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 28 Jun 2021 at 18:15, 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 Tue, Jun 22, 2021 at 02:30:51PM +0100, Naushir Patuck wrote:<br>
> On Tue, 22 Jun 2021 at 14:20, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
> > Hi,<br>
> ><br>
> > Here is version 2 of this series.  The following changes have been<br>
> > introduced<br>
> > over v1:<br>
> ><br>
> > - Rework patch 1/2 to use a unique_ptr to store the parser object in the<br>
> > CamHelper class.<br>
> > - Switch to using std::initialiser_list in the constructor of MdParserSmia.<br>
> > - All suggestions from Laurent's feedback have been addressed in patch 2/2.<br>
> ><br>
> > The constructor for the unique_ptr<MdParserSmia> in patch 2/2 looks a bit<br>
> > awkward now since I have to explicitly write std::initialiser_list within<br>
> > std::make_unique, but I cannot see nice way around this.<br>
> <br>
> Something like the following could arguably be neater, but functionally<br>
> equivalent in patch 2/2:<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> index 18f5c3e7e520..bd42536085d9 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> @@ -30,6 +30,7 @@ using namespace RPiController;<br>
>  constexpr uint32_t gainReg = 0x157;<br>
>  constexpr uint32_t expHiReg = 0x15a;<br>
>  constexpr uint32_t expLoReg = 0x15b;<br>
> +constexpr std::initializer_list<uint32_t> registerList = { expHiReg,<br>
> expLoReg, gainReg };<br>
> <br>
>  class CamHelperImx219 : public CamHelper<br>
>  {<br>
> @@ -53,9 +54,7 @@ private:<br>
> <br>
>  CamHelperImx219::CamHelperImx219()<br>
>  #if ENABLE_EMBEDDED_DATA<br>
> -       : CamHelper(std::make_unique<MdParserSmia><br>
> -                       (std::initializer_list<uint32_t>({ expHiReg,<br>
> expLoReg, gainReg })),<br>
> -                   frameIntegrationDiff)<br>
> +       : CamHelper(std::make_unique<MdParserSmia>(registerList),<br>
> frameIntegrationDiff)<br>
>  #else<br>
>         : CamHelper({}, frameIntegrationDiff)<br>
>  #endif<br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 8869af6620cf..4c7f7fd9561b 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -23,6 +23,7 @@ constexpr uint32_t expHiReg = 0x0202;<br>
>  constexpr uint32_t expLoReg = 0x0203;<br>
>  constexpr uint32_t gainHiReg = 0x0204;<br>
>  constexpr uint32_t gainLoReg = 0x0205;<br>
> +constexpr std::initializer_list<uint32_t> registerList = { expHiReg,<br>
> expLoReg, gainHiReg, gainLoReg };<br>
> <br>
>  class CamHelperImx477 : public CamHelper<br>
>  {<br>
> @@ -46,9 +47,7 @@ private:<br>
>  };<br>
> <br>
>  CamHelperImx477::CamHelperImx477()<br>
> -       : CamHelper(std::make_unique<MdParserSmia><br>
> -                       (std::initializer_list<uint32_t>({ expHiReg,<br>
> expLoReg, gainHiReg, gainLoReg })),<br>
> -                   frameIntegrationDiff)<br>
> +       : CamHelper(std::make_unique<MdParserSmia>(registerList),<br>
> frameIntegrationDiff)<br>
>  {<br>
>  }<br>
> <br>
> If folks think this is better, I am happy to change it.<br>
<br>
I like that better as it's a bit more readable, but it's up to you.<br>
<br>
If you post a new version, you'll need to address the ov9281 that has<br>
just been merged, otherwise I can do so when applying. Please let me<br>
know how you'd like to proceed.<br></blockquote><div><br></div><div>I'll submit an updated patch series with your suggestions and rebased</div><div>onto master shortly.</div><div><br></div><div>Thanks!</div><div>Naush<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>
> > I have removed all previous tags from 1/2, as this is a completely different<br>
> > approach to the previous revision.<br>
> ><br>
> > Thanks,<br>
> > Naush<br>
> ><br>
> > Naushir Patuck (2):<br>
> >   ipa: raspberrypi: Use a unique_ptr for the metadata parser<br>
> >   ipa: raspberrypi: Generalise the SMIA metadata parser<br>
> ><br>
> >  src/ipa/raspberrypi/cam_helper.cpp        |  38 ++++---<br>
> >  src/ipa/raspberrypi/cam_helper.hpp        |   7 +-<br>
> >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 117 ++++----------------<br>
> >  src/ipa/raspberrypi/cam_helper_imx290.cpp |   2 +-<br>
> >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 123 ++++------------------<br>
> >  src/ipa/raspberrypi/cam_helper_ov5647.cpp |   2 +-<br>
> >  src/ipa/raspberrypi/md_parser.hpp         |  42 +++++---<br>
> >  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--<br>
> >  8 files changed, 155 insertions(+), 242 deletions(-)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>