[libcamera-devel,v2,0/2] Raspberry Pi: Metadata parsing improvements (II)
mbox series

Message ID 20210622132014.949961-1-naush@raspberrypi.com
Headers show
Series
  • Raspberry Pi: Metadata parsing improvements (II)
Related show

Message

Naushir Patuck June 22, 2021, 1:20 p.m. UTC
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.

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

Comments

Naushir Patuck June 22, 2021, 1:30 p.m. UTC | #1
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
>
>
David Plowman June 22, 2021, 2:25 p.m. UTC | #2
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
>>
Laurent Pinchart June 28, 2021, 5:15 p.m. UTC | #3
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(-)
Naushir Patuck June 29, 2021, 9:03 a.m. UTC | #4
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
>