[libcamera-devel,1/3] ipa: raspberrypi: Embed the metadata parser in the sensor CamHelper classes
diff mbox series

Message ID 20210615144211.173047-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Metadata parsing improvements (II)
Related show

Commit Message

Naushir Patuck June 15, 2021, 2:42 p.m. UTC 
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        | 1 -
 src/ipa/raspberrypi/cam_helper_imx219.cpp | 4 +++-
 src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +++-
 3 files changed, 6 insertions(+), 3 deletions(-)

Comments

Kieran Bingham June 18, 2021, 9:53 p.m. UTC | #1 
Hi Naush,

On 15/06/2021 15:42, 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.
> 

Reducing allocations always sounds like a win.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        | 1 -
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 4 +++-
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 062e94c4fef3..1474464c9257 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -48,7 +48,6 @@ CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
>  
>  CamHelper::~CamHelper()
>  {
> -	delete parser_;
>  }
>  
>  void CamHelper::Prepare(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..d951cd552a21 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -54,11 +54,13 @@ private:
>  	 * in units of lines.
>  	 */
>  	static constexpr int frameIntegrationDiff = 4;
> +
> +	MdParserImx219 imx219_parser;
>  };
>  
>  CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
> -	: CamHelper(new MdParserImx219(), frameIntegrationDiff)
> +	: CamHelper(&imx219_parser, frameIntegrationDiff)
>  #else
>  	: CamHelper(nullptr, frameIntegrationDiff)
>  #endif
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 25b36bce0dac..44f030ed7da9 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -47,10 +47,12 @@ private:
>  	 * in units of lines.
>  	 */
>  	static constexpr int frameIntegrationDiff = 22;
> +
> +	MdParserImx477 imx477_parser;
>  };
>  
>  CamHelperImx477::CamHelperImx477()
> -	: CamHelper(new MdParserImx477(), frameIntegrationDiff)
> +	: CamHelper(&imx477_parser, frameIntegrationDiff)
>  {
>  }
>  
>
David Plowman June 21, 2021, 8:02 a.m. UTC | #2 
Hi Naush

Thanks for this patch.

On Tue, 15 Jun 2021 at 15:42, 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>

Agree. no need for extra news and deletes.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks
David

> ---
>  src/ipa/raspberrypi/cam_helper.cpp        | 1 -
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 4 +++-
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 062e94c4fef3..1474464c9257 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -48,7 +48,6 @@ CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
>
>  CamHelper::~CamHelper()
>  {
> -       delete parser_;
>  }
>
>  void CamHelper::Prepare(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..d951cd552a21 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -54,11 +54,13 @@ private:
>          * in units of lines.
>          */
>         static constexpr int frameIntegrationDiff = 4;
> +
> +       MdParserImx219 imx219_parser;
>  };
>
>  CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
> -       : CamHelper(new MdParserImx219(), frameIntegrationDiff)
> +       : CamHelper(&imx219_parser, frameIntegrationDiff)
>  #else
>         : CamHelper(nullptr, frameIntegrationDiff)
>  #endif
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 25b36bce0dac..44f030ed7da9 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -47,10 +47,12 @@ private:
>          * in units of lines.
>          */
>         static constexpr int frameIntegrationDiff = 22;
> +
> +       MdParserImx477 imx477_parser;
>  };
>
>  CamHelperImx477::CamHelperImx477()
> -       : CamHelper(new MdParserImx477(), frameIntegrationDiff)
> +       : CamHelper(&imx477_parser, frameIntegrationDiff)
>  {
>  }
>
> --
> 2.25.1
>
Laurent Pinchart June 22, 2021, 10:20 a.m. UTC | #3 
Hi Naush,

Thank you for the patch.

On Tue, Jun 15, 2021 at 03:42:09PM +0100, 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/ipa/raspberrypi/cam_helper.cpp        | 1 -
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 4 +++-
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 062e94c4fef3..1474464c9257 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -48,7 +48,6 @@ CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
>  
>  CamHelper::~CamHelper()
>  {
> -	delete parser_;
>  }
>  
>  void CamHelper::Prepare(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..d951cd552a21 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -54,11 +54,13 @@ private:
>  	 * in units of lines.
>  	 */
>  	static constexpr int frameIntegrationDiff = 4;
> +
> +	MdParserImx219 imx219_parser;
>  };
>  
>  CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
> -	: CamHelper(new MdParserImx219(), frameIntegrationDiff)
> +	: CamHelper(&imx219_parser, frameIntegrationDiff)
>  #else
>  	: CamHelper(nullptr, frameIntegrationDiff)
>  #endif
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 25b36bce0dac..44f030ed7da9 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -47,10 +47,12 @@ private:
>  	 * in units of lines.
>  	 */
>  	static constexpr int frameIntegrationDiff = 22;
> +
> +	MdParserImx477 imx477_parser;
>  };
>  
>  CamHelperImx477::CamHelperImx477()
> -	: CamHelper(new MdParserImx477(), frameIntegrationDiff)
> +	: CamHelper(&imx477_parser, frameIntegrationDiff)
>  {
>  }
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index 062e94c4fef3..1474464c9257 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -48,7 +48,6 @@  CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
 
 CamHelper::~CamHelper()
 {
-	delete parser_;
 }
 
 void CamHelper::Prepare(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..d951cd552a21 100644
--- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
@@ -54,11 +54,13 @@  private:
 	 * in units of lines.
 	 */
 	static constexpr int frameIntegrationDiff = 4;
+
+	MdParserImx219 imx219_parser;
 };
 
 CamHelperImx219::CamHelperImx219()
 #if ENABLE_EMBEDDED_DATA
-	: CamHelper(new MdParserImx219(), frameIntegrationDiff)
+	: CamHelper(&imx219_parser, frameIntegrationDiff)
 #else
 	: CamHelper(nullptr, frameIntegrationDiff)
 #endif
diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
index 25b36bce0dac..44f030ed7da9 100644
--- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
@@ -47,10 +47,12 @@  private:
 	 * in units of lines.
 	 */
 	static constexpr int frameIntegrationDiff = 22;
+
+	MdParserImx477 imx477_parser;
 };
 
 CamHelperImx477::CamHelperImx477()
-	: CamHelper(new MdParserImx477(), frameIntegrationDiff)
+	: CamHelper(&imx477_parser, frameIntegrationDiff)
 {
 }