[libcamera-devel,2/4] ipa: raspberrypi: Remove MdParserRPi
diff mbox series

Message ID 20210216103140.1077307-3-naush@raspberrypi.com
State Changes Requested
Headers show
Series
  • Raspberry Pi: Embedded data usage
Related show

Commit Message

Naushir Patuck Feb. 16, 2021, 10:31 a.m. UTC 
With the recent change to pass a ControlList to the IPA with exposure
and gain values used for a frame, RPiController::MdParserRPi is not
needed any more. Remove all traces of it.

The derived CamHelper objects now pass nullptr values for the parser to
the base CamHelper class when sensors do not use metadata.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp        |  9 ++++--
 src/ipa/raspberrypi/cam_helper_imx219.cpp |  4 +--
 src/ipa/raspberrypi/cam_helper_ov5647.cpp |  3 +-
 src/ipa/raspberrypi/md_parser_rpi.cpp     | 37 -----------------------
 src/ipa/raspberrypi/md_parser_rpi.hpp     | 32 --------------------
 src/ipa/raspberrypi/meson.build           |  1 -
 6 files changed, 8 insertions(+), 78 deletions(-)
 delete mode 100644 src/ipa/raspberrypi/md_parser_rpi.cpp
 delete mode 100644 src/ipa/raspberrypi/md_parser_rpi.hpp

Comments

David Plowman Feb. 16, 2021, 1:54 p.m. UTC | #1 
Hi Naush

Thanks for this patch. Looks good to me!

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

Best regards
David

On Tue, 16 Feb 2021 at 10:32, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> With the recent change to pass a ControlList to the IPA with exposure
> and gain values used for a frame, RPiController::MdParserRPi is not
> needed any more. Remove all traces of it.
>
> The derived CamHelper objects now pass nullptr values for the parser to
> the base CamHelper class when sensors do not use metadata.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        |  9 ++++--
>  src/ipa/raspberrypi/cam_helper_imx219.cpp |  4 +--
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp |  3 +-
>  src/ipa/raspberrypi/md_parser_rpi.cpp     | 37 -----------------------
>  src/ipa/raspberrypi/md_parser_rpi.hpp     | 32 --------------------
>  src/ipa/raspberrypi/meson.build           |  1 -
>  6 files changed, 8 insertions(+), 78 deletions(-)
>  delete mode 100644 src/ipa/raspberrypi/md_parser_rpi.cpp
>  delete mode 100644 src/ipa/raspberrypi/md_parser_rpi.hpp
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 93d1b7b0296a..c9cdc39c5932 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -42,7 +42,8 @@ CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
>
>  CamHelper::~CamHelper()
>  {
> -       delete parser_;
> +       if (parser_)
> +               delete parser_;
>  }
>
>  uint32_t CamHelper::ExposureLines(double exposure_us) const
> @@ -88,8 +89,10 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
>  void CamHelper::SetCameraMode(const CameraMode &mode)
>  {
>         mode_ = mode;
> -       parser_->SetBitsPerPixel(mode.bitdepth);
> -       parser_->SetLineLengthBytes(0); /* We use SetBufferSize. */
> +       if (parser_) {
> +               parser_->SetBitsPerPixel(mode.bitdepth);
> +               parser_->SetLineLengthBytes(0); /* We use SetBufferSize. */
> +       }
>         initialized_ = true;
>  }
>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index 95b8e698fe3b..0e454d0de2dc 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -19,8 +19,6 @@
>  #include "cam_helper.hpp"
>  #if ENABLE_EMBEDDED_DATA
>  #include "md_parser.hpp"
> -#else
> -#include "md_parser_rpi.hpp"
>  #endif
>
>  using namespace RPiController;
> @@ -62,7 +60,7 @@ CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
>         : CamHelper(new MdParserImx219(), frameIntegrationDiff)
>  #else
> -       : CamHelper(new MdParserRPi(), frameIntegrationDiff)
> +       : CamHelper(nullptr, frameIntegrationDiff)
>  #endif
>  {
>  }
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index a7f417324048..75486e900d31 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -8,7 +8,6 @@
>  #include <assert.h>
>
>  #include "cam_helper.hpp"
> -#include "md_parser_rpi.hpp"
>
>  using namespace RPiController;
>
> @@ -38,7 +37,7 @@ private:
>   */
>
>  CamHelperOv5647::CamHelperOv5647()
> -       : CamHelper(new MdParserRPi(), frameIntegrationDiff)
> +       : CamHelper(nullptr, frameIntegrationDiff)
>  {
>  }
>
> diff --git a/src/ipa/raspberrypi/md_parser_rpi.cpp b/src/ipa/raspberrypi/md_parser_rpi.cpp
> deleted file mode 100644
> index 2b0bcfc5f034..000000000000
> --- a/src/ipa/raspberrypi/md_parser_rpi.cpp
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -/* SPDX-License-Identifier: BSD-2-Clause */
> -/*
> - * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> - *
> - * md_parser_rpi.cpp - Metadata parser for generic Raspberry Pi metadata
> - */
> -
> -#include <string.h>
> -
> -#include "md_parser_rpi.hpp"
> -
> -using namespace RPiController;
> -
> -MdParserRPi::MdParserRPi()
> -{
> -}
> -
> -MdParser::Status MdParserRPi::Parse(void *data)
> -{
> -       if (buffer_size_bytes_ < sizeof(rpiMetadata))
> -               return ERROR;
> -
> -       memcpy(&metadata, data, sizeof(rpiMetadata));
> -       return OK;
> -}
> -
> -MdParser::Status MdParserRPi::GetExposureLines(unsigned int &lines)
> -{
> -       lines = metadata.exposure;
> -       return OK;
> -}
> -
> -MdParser::Status MdParserRPi::GetGainCode(unsigned int &gain_code)
> -{
> -       gain_code = metadata.gain;
> -       return OK;
> -}
> diff --git a/src/ipa/raspberrypi/md_parser_rpi.hpp b/src/ipa/raspberrypi/md_parser_rpi.hpp
> deleted file mode 100644
> index 52f54f008056..000000000000
> --- a/src/ipa/raspberrypi/md_parser_rpi.hpp
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -/* SPDX-License-Identifier: BSD-2-Clause */
> -/*
> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> - *
> - * md_parser_rpi.hpp - Raspberry Pi metadata parser interface
> - */
> -#pragma once
> -
> -#include "md_parser.hpp"
> -
> -namespace RPiController {
> -
> -class MdParserRPi : public MdParser
> -{
> -public:
> -       MdParserRPi();
> -       Status Parse(void *data) override;
> -       Status GetExposureLines(unsigned int &lines) override;
> -       Status GetGainCode(unsigned int &gain_code) override;
> -
> -private:
> -       // This must be the same struct that is filled into the metadata buffer
> -       // in the pipeline handler.
> -       struct rpiMetadata
> -       {
> -               uint32_t exposure;
> -               uint32_t gain;
> -       };
> -       rpiMetadata metadata;
> -};
> -
> -}
> diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> index 9e9ea80b93ad..0baf7b7e8d4a 100644
> --- a/src/ipa/raspberrypi/meson.build
> +++ b/src/ipa/raspberrypi/meson.build
> @@ -17,7 +17,6 @@ rpi_ipa_includes = [
>  rpi_ipa_sources = files([
>      'raspberrypi.cpp',
>      'md_parser.cpp',
> -    'md_parser_rpi.cpp',
>      'cam_helper.cpp',
>      'cam_helper_ov5647.cpp',
>      'cam_helper_imx219.cpp',
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Feb. 16, 2021, 11:42 p.m. UTC | #2 
Hi Naush,

Thank you for the patch.

On Tue, Feb 16, 2021 at 10:31:38AM +0000, Naushir Patuck wrote:
> With the recent change to pass a ControlList to the IPA with exposure
> and gain values used for a frame, RPiController::MdParserRPi is not
> needed any more. Remove all traces of it.
> 
> The derived CamHelper objects now pass nullptr values for the parser to
> the base CamHelper class when sensors do not use metadata.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        |  9 ++++--
>  src/ipa/raspberrypi/cam_helper_imx219.cpp |  4 +--
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp |  3 +-
>  src/ipa/raspberrypi/md_parser_rpi.cpp     | 37 -----------------------
>  src/ipa/raspberrypi/md_parser_rpi.hpp     | 32 --------------------
>  src/ipa/raspberrypi/meson.build           |  1 -
>  6 files changed, 8 insertions(+), 78 deletions(-)
>  delete mode 100644 src/ipa/raspberrypi/md_parser_rpi.cpp
>  delete mode 100644 src/ipa/raspberrypi/md_parser_rpi.hpp
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 93d1b7b0296a..c9cdc39c5932 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -42,7 +42,8 @@ CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
>  
>  CamHelper::~CamHelper()
>  {
> -	delete parser_;
> +	if (parser_)
> +		delete parser_;

You can delete a nullptr, that's a no-op, so this change isn't needed.

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

>  }
>  
>  uint32_t CamHelper::ExposureLines(double exposure_us) const
> @@ -88,8 +89,10 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
>  void CamHelper::SetCameraMode(const CameraMode &mode)
>  {
>  	mode_ = mode;
> -	parser_->SetBitsPerPixel(mode.bitdepth);
> -	parser_->SetLineLengthBytes(0); /* We use SetBufferSize. */
> +	if (parser_) {
> +		parser_->SetBitsPerPixel(mode.bitdepth);
> +		parser_->SetLineLengthBytes(0); /* We use SetBufferSize. */
> +	}
>  	initialized_ = true;
>  }
>  
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index 95b8e698fe3b..0e454d0de2dc 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -19,8 +19,6 @@
>  #include "cam_helper.hpp"
>  #if ENABLE_EMBEDDED_DATA
>  #include "md_parser.hpp"
> -#else
> -#include "md_parser_rpi.hpp"
>  #endif
>  
>  using namespace RPiController;
> @@ -62,7 +60,7 @@ CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
>  	: CamHelper(new MdParserImx219(), frameIntegrationDiff)
>  #else
> -	: CamHelper(new MdParserRPi(), frameIntegrationDiff)
> +	: CamHelper(nullptr, frameIntegrationDiff)
>  #endif
>  {
>  }
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index a7f417324048..75486e900d31 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -8,7 +8,6 @@
>  #include <assert.h>
>  
>  #include "cam_helper.hpp"
> -#include "md_parser_rpi.hpp"
>  
>  using namespace RPiController;
>  
> @@ -38,7 +37,7 @@ private:
>   */
>  
>  CamHelperOv5647::CamHelperOv5647()
> -	: CamHelper(new MdParserRPi(), frameIntegrationDiff)
> +	: CamHelper(nullptr, frameIntegrationDiff)
>  {
>  }
>  
> diff --git a/src/ipa/raspberrypi/md_parser_rpi.cpp b/src/ipa/raspberrypi/md_parser_rpi.cpp
> deleted file mode 100644
> index 2b0bcfc5f034..000000000000
> --- a/src/ipa/raspberrypi/md_parser_rpi.cpp
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -/* SPDX-License-Identifier: BSD-2-Clause */
> -/*
> - * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> - *
> - * md_parser_rpi.cpp - Metadata parser for generic Raspberry Pi metadata
> - */
> -
> -#include <string.h>
> -
> -#include "md_parser_rpi.hpp"
> -
> -using namespace RPiController;
> -
> -MdParserRPi::MdParserRPi()
> -{
> -}
> -
> -MdParser::Status MdParserRPi::Parse(void *data)
> -{
> -	if (buffer_size_bytes_ < sizeof(rpiMetadata))
> -		return ERROR;
> -
> -	memcpy(&metadata, data, sizeof(rpiMetadata));
> -	return OK;
> -}
> -
> -MdParser::Status MdParserRPi::GetExposureLines(unsigned int &lines)
> -{
> -	lines = metadata.exposure;
> -	return OK;
> -}
> -
> -MdParser::Status MdParserRPi::GetGainCode(unsigned int &gain_code)
> -{
> -	gain_code = metadata.gain;
> -	return OK;
> -}
> diff --git a/src/ipa/raspberrypi/md_parser_rpi.hpp b/src/ipa/raspberrypi/md_parser_rpi.hpp
> deleted file mode 100644
> index 52f54f008056..000000000000
> --- a/src/ipa/raspberrypi/md_parser_rpi.hpp
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -/* SPDX-License-Identifier: BSD-2-Clause */
> -/*
> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> - *
> - * md_parser_rpi.hpp - Raspberry Pi metadata parser interface
> - */
> -#pragma once
> -
> -#include "md_parser.hpp"
> -
> -namespace RPiController {
> -
> -class MdParserRPi : public MdParser
> -{
> -public:
> -	MdParserRPi();
> -	Status Parse(void *data) override;
> -	Status GetExposureLines(unsigned int &lines) override;
> -	Status GetGainCode(unsigned int &gain_code) override;
> -
> -private:
> -	// This must be the same struct that is filled into the metadata buffer
> -	// in the pipeline handler.
> -	struct rpiMetadata
> -	{
> -		uint32_t exposure;
> -		uint32_t gain;
> -	};
> -	rpiMetadata metadata;
> -};
> -
> -}
> diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> index 9e9ea80b93ad..0baf7b7e8d4a 100644
> --- a/src/ipa/raspberrypi/meson.build
> +++ b/src/ipa/raspberrypi/meson.build
> @@ -17,7 +17,6 @@ rpi_ipa_includes = [
>  rpi_ipa_sources = files([
>      'raspberrypi.cpp',
>      'md_parser.cpp',
> -    'md_parser_rpi.cpp',
>      'cam_helper.cpp',
>      'cam_helper_ov5647.cpp',
>      'cam_helper_imx219.cpp',

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index 93d1b7b0296a..c9cdc39c5932 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -42,7 +42,8 @@  CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
 
 CamHelper::~CamHelper()
 {
-	delete parser_;
+	if (parser_)
+		delete parser_;
 }
 
 uint32_t CamHelper::ExposureLines(double exposure_us) const
@@ -88,8 +89,10 @@  uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
 void CamHelper::SetCameraMode(const CameraMode &mode)
 {
 	mode_ = mode;
-	parser_->SetBitsPerPixel(mode.bitdepth);
-	parser_->SetLineLengthBytes(0); /* We use SetBufferSize. */
+	if (parser_) {
+		parser_->SetBitsPerPixel(mode.bitdepth);
+		parser_->SetLineLengthBytes(0); /* We use SetBufferSize. */
+	}
 	initialized_ = true;
 }
 
diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
index 95b8e698fe3b..0e454d0de2dc 100644
--- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
@@ -19,8 +19,6 @@ 
 #include "cam_helper.hpp"
 #if ENABLE_EMBEDDED_DATA
 #include "md_parser.hpp"
-#else
-#include "md_parser_rpi.hpp"
 #endif
 
 using namespace RPiController;
@@ -62,7 +60,7 @@  CamHelperImx219::CamHelperImx219()
 #if ENABLE_EMBEDDED_DATA
 	: CamHelper(new MdParserImx219(), frameIntegrationDiff)
 #else
-	: CamHelper(new MdParserRPi(), frameIntegrationDiff)
+	: CamHelper(nullptr, frameIntegrationDiff)
 #endif
 {
 }
diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
index a7f417324048..75486e900d31 100644
--- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
+++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
@@ -8,7 +8,6 @@ 
 #include <assert.h>
 
 #include "cam_helper.hpp"
-#include "md_parser_rpi.hpp"
 
 using namespace RPiController;
 
@@ -38,7 +37,7 @@  private:
  */
 
 CamHelperOv5647::CamHelperOv5647()
-	: CamHelper(new MdParserRPi(), frameIntegrationDiff)
+	: CamHelper(nullptr, frameIntegrationDiff)
 {
 }
 
diff --git a/src/ipa/raspberrypi/md_parser_rpi.cpp b/src/ipa/raspberrypi/md_parser_rpi.cpp
deleted file mode 100644
index 2b0bcfc5f034..000000000000
--- a/src/ipa/raspberrypi/md_parser_rpi.cpp
+++ /dev/null
@@ -1,37 +0,0 @@ 
-/* SPDX-License-Identifier: BSD-2-Clause */
-/*
- * Copyright (C) 2020, Raspberry Pi (Trading) Limited
- *
- * md_parser_rpi.cpp - Metadata parser for generic Raspberry Pi metadata
- */
-
-#include <string.h>
-
-#include "md_parser_rpi.hpp"
-
-using namespace RPiController;
-
-MdParserRPi::MdParserRPi()
-{
-}
-
-MdParser::Status MdParserRPi::Parse(void *data)
-{
-	if (buffer_size_bytes_ < sizeof(rpiMetadata))
-		return ERROR;
-
-	memcpy(&metadata, data, sizeof(rpiMetadata));
-	return OK;
-}
-
-MdParser::Status MdParserRPi::GetExposureLines(unsigned int &lines)
-{
-	lines = metadata.exposure;
-	return OK;
-}
-
-MdParser::Status MdParserRPi::GetGainCode(unsigned int &gain_code)
-{
-	gain_code = metadata.gain;
-	return OK;
-}
diff --git a/src/ipa/raspberrypi/md_parser_rpi.hpp b/src/ipa/raspberrypi/md_parser_rpi.hpp
deleted file mode 100644
index 52f54f008056..000000000000
--- a/src/ipa/raspberrypi/md_parser_rpi.hpp
+++ /dev/null
@@ -1,32 +0,0 @@ 
-/* SPDX-License-Identifier: BSD-2-Clause */
-/*
- * Copyright (C) 2019, Raspberry Pi (Trading) Limited
- *
- * md_parser_rpi.hpp - Raspberry Pi metadata parser interface
- */
-#pragma once
-
-#include "md_parser.hpp"
-
-namespace RPiController {
-
-class MdParserRPi : public MdParser
-{
-public:
-	MdParserRPi();
-	Status Parse(void *data) override;
-	Status GetExposureLines(unsigned int &lines) override;
-	Status GetGainCode(unsigned int &gain_code) override;
-
-private:
-	// This must be the same struct that is filled into the metadata buffer
-	// in the pipeline handler.
-	struct rpiMetadata
-	{
-		uint32_t exposure;
-		uint32_t gain;
-	};
-	rpiMetadata metadata;
-};
-
-}
diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
index 9e9ea80b93ad..0baf7b7e8d4a 100644
--- a/src/ipa/raspberrypi/meson.build
+++ b/src/ipa/raspberrypi/meson.build
@@ -17,7 +17,6 @@  rpi_ipa_includes = [
 rpi_ipa_sources = files([
     'raspberrypi.cpp',
     'md_parser.cpp',
-    'md_parser_rpi.cpp',
     'cam_helper.cpp',
     'cam_helper_ov5647.cpp',
     'cam_helper_imx219.cpp',