| Message ID | 20210614095340.3051816-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Mon, 14 Jun 2021, 10:53 am Naushir Patuck, <naush@raspberrypi.com> 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@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. > > > 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) > { > } > > -- > 2.25.1 > >
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@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 :-) > > 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) > > { > > } > >
Hi Laurent, Thank you for your review on this series. On Tue, 15 Jun 2021 at 03:52, Laurent Pinchart < laurent.pinchart@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@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 >
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 -{ } 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) { }
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@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(-)