[libcamera-devel,2/6] ipa: raspberrypi: Set default values for member variables of MdParser
diff mbox series

Message ID 20210614095340.3051816-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Metadata parsing improvements
Related show

Commit Message

Naushir Patuck June 14, 2021, 9:53 a.m. UTC 
Set some sensible default values for member variables of the MdParser class.

Remove buffer_size_bytes_ along with some related asserts as this class now uses
libcamera::Span for buffer handling, and buffer_size_bytes_ is unused.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper_imx219.cpp | 1 -
 src/ipa/raspberrypi/cam_helper_imx477.cpp | 1 -
 src/ipa/raspberrypi/md_parser.hpp         | 4 ++--
 3 files changed, 2 insertions(+), 4 deletions(-)

Comments

David Plowman June 14, 2021, 4:32 p.m. UTC | #1 
Hi Naush

Thanks for fixing this, I should probably have spotted it when I (I
think it was me) made the Span change.

On Mon, 14 Jun 2021 at 10:53, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Set some sensible default values for member variables of the MdParser class.
>
> Remove buffer_size_bytes_ along with some related asserts as this class now uses
> libcamera::Span for buffer handling, and buffer_size_bytes_ is unused.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 1 -
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 1 -
>  src/ipa/raspberrypi/md_parser.hpp         | 4 ++--
>  3 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index e550fba62cde..ec218dce5456 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -128,7 +128,6 @@ MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer)
>                  * registers.
>                  */
>                 assert(bits_per_pixel_);
> -               assert(num_lines_ || buffer_size_bytes_);

I wonder if we should delete line_length_bytes_ and num_lines_ too?
After all, the caller always knows the line length of the buffer so
can adjust the Span accordingly, and it would generally make this
stuff a bit simpler. But no urgency, maybe that's more trouble than
it's worth at this point... so anyway:

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

Thanks
David

>                 /* Need to be ordered */
>                 uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
>                 reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index a4a58c15467d..25b36bce0dac 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -117,7 +117,6 @@ MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer)
>                  * registers.
>                  */
>                 assert(bits_per_pixel_);
> -               assert(num_lines_ || buffer_size_bytes_);
>                 /* Need to be ordered */
>                 uint32_t regs[4] = {
>                         EXPHI_REG,
> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
> index 86e0577614e0..25ba0e7c9400 100644
> --- a/src/ipa/raspberrypi/md_parser.hpp
> +++ b/src/ipa/raspberrypi/md_parser.hpp
> @@ -68,7 +68,8 @@ public:
>                 ERROR = 2
>         };
>
> -       MdParser() : reset_(true)
> +       MdParser()
> +               : reset_(true), bits_per_pixel_(0), num_lines_(0), line_length_bytes_(0)
>         {
>         }
>
> @@ -103,7 +104,6 @@ protected:
>         int bits_per_pixel_;
>         unsigned int num_lines_;
>         unsigned int line_length_bytes_;
> -       unsigned int buffer_size_bytes_;
>  };
>
>  /*
> --
> 2.25.1
>
Laurent Pinchart June 15, 2021, 2:39 a.m. UTC | #2 
Hi Naush,

Thank you for the patch.

On Mon, Jun 14, 2021 at 10:53:36AM +0100, Naushir Patuck wrote:
> Set some sensible default values for member variables of the MdParser class.
> 
> Remove buffer_size_bytes_ along with some related asserts as this class now uses
> libcamera::Span for buffer handling, and buffer_size_bytes_ is unused.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

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

> ---
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 1 -
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 1 -
>  src/ipa/raspberrypi/md_parser.hpp         | 4 ++--
>  3 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index e550fba62cde..ec218dce5456 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -128,7 +128,6 @@ MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer)
>  		 * registers.
>  		 */
>  		assert(bits_per_pixel_);
> -		assert(num_lines_ || buffer_size_bytes_);
>  		/* Need to be ordered */
>  		uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
>  		reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index a4a58c15467d..25b36bce0dac 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -117,7 +117,6 @@ MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer)
>  		 * registers.
>  		 */
>  		assert(bits_per_pixel_);
> -		assert(num_lines_ || buffer_size_bytes_);
>  		/* Need to be ordered */
>  		uint32_t regs[4] = {
>  			EXPHI_REG,
> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
> index 86e0577614e0..25ba0e7c9400 100644
> --- a/src/ipa/raspberrypi/md_parser.hpp
> +++ b/src/ipa/raspberrypi/md_parser.hpp
> @@ -68,7 +68,8 @@ public:
>  		ERROR = 2
>  	};
>  
> -	MdParser() : reset_(true)
> +	MdParser()
> +		: reset_(true), bits_per_pixel_(0), num_lines_(0), line_length_bytes_(0)
>  	{
>  	}
>  
> @@ -103,7 +104,6 @@ protected:
>  	int bits_per_pixel_;
>  	unsigned int num_lines_;
>  	unsigned int line_length_bytes_;
> -	unsigned int buffer_size_bytes_;
>  };
>  
>  /*

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
index e550fba62cde..ec218dce5456 100644
--- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
@@ -128,7 +128,6 @@  MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer)
 		 * registers.
 		 */
 		assert(bits_per_pixel_);
-		assert(num_lines_ || buffer_size_bytes_);
 		/* Need to be ordered */
 		uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
 		reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
index a4a58c15467d..25b36bce0dac 100644
--- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
@@ -117,7 +117,6 @@  MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer)
 		 * registers.
 		 */
 		assert(bits_per_pixel_);
-		assert(num_lines_ || buffer_size_bytes_);
 		/* Need to be ordered */
 		uint32_t regs[4] = {
 			EXPHI_REG,
diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
index 86e0577614e0..25ba0e7c9400 100644
--- a/src/ipa/raspberrypi/md_parser.hpp
+++ b/src/ipa/raspberrypi/md_parser.hpp
@@ -68,7 +68,8 @@  public:
 		ERROR = 2
 	};
 
-	MdParser() : reset_(true)
+	MdParser()
+		: reset_(true), bits_per_pixel_(0), num_lines_(0), line_length_bytes_(0)
 	{
 	}
 
@@ -103,7 +104,6 @@  protected:
 	int bits_per_pixel_;
 	unsigned int num_lines_;
 	unsigned int line_length_bytes_;
-	unsigned int buffer_size_bytes_;
 };
 
 /*