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