[libcamera-devel,RFC,2/5] ipa: rpi: Make parsePdafData() available in the CamHelper class
diff mbox series

Message ID 20230630120303.33023-3-umang.jain@ideasonboard.com
State New
Headers show
Series
  • ipa: rpi: CamHelper improvements
Related show

Commit Message

Umang Jain June 30, 2023, 12:03 p.m. UTC 
parsePdafData() parses the PDAF section of the sensor's embedded data
and the parsing logic is generic to all PDAF-supported sensors.
Once the PDAF-specific buffer section is identified correctly, any
sensor should be able to use this helper in order parse its PDAF data.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/rpi/cam_helper/cam_helper.cpp        | 29 +++++++++++++++
 src/ipa/rpi/cam_helper/cam_helper.h          |  6 ++++
 src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 37 --------------------
 3 files changed, 35 insertions(+), 37 deletions(-)

Comments

Naushir Patuck June 30, 2023, 12:54 p.m. UTC | #1 
Hi Umang,

Thank you for your work.

On Fri, 30 Jun 2023 at 13:03, Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> parsePdafData() parses the PDAF section of the sensor's embedded data

pdaf data is not strictly part of the embedded data, so this should probably
say sensor metadata.

> and the parsing logic is generic to all PDAF-supported sensors.
> Once the PDAF-specific buffer section is identified correctly, any
> sensor should be able to use this helper in order parse its PDAF data.

s/in order parse/in order to parse/

>
> No functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/rpi/cam_helper/cam_helper.cpp        | 29 +++++++++++++++
>  src/ipa/rpi/cam_helper/cam_helper.h          |  6 ++++
>  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 37 --------------------
>  3 files changed, 35 insertions(+), 37 deletions(-)
>
> diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp
> index ddd5e9a4..e9e0c496 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp
> @@ -253,6 +253,35 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>         metadata.set("device.status", deviceStatus);
>  }
>
> +bool CamHelper::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
> +{

While I do agree that generalising the pdaf is a good thing to do, we must make
it clear that this parsing is not really generic and only specific to a subset
of Sony sensors.  No doubt other Sony/non-Sony sensors will have
vastly different
pdaf data formats.

So could this function be called parseSonyPdafData() or something similar to
highlight this?  But also see below...

> +       const uint8_t *ptr = data.data();
> +       unsigned int len = data.size();
> +       size_t step = bpp >> 1; /* bytes per PDAF grid entry */
> +
> +       if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
> +               LOG(IPARPI, Error) << "PDAF data in unsupported format";
> +               return false;
> +       }
> +
> +       pdaf.init({ pdafStatsCols, pdafStatsRows });
> +
> +       ptr += 2 * step;
> +       for (unsigned i = 0; i < pdafStatsRows; ++i) {
> +               for (unsigned j = 0; j < pdafStatsCols; ++j) {
> +                       unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
> +                       int p = (((ptr[1] & 0x0f) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
> +                       PdafData pdafData;
> +                       pdafData.conf = c;
> +                       pdafData.phase = c ? p : 0;
> +                       pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
> +                       ptr += step;
> +               }
> +       }
> +
> +       return true;
> +}
> +
>  void CamHelper::populateMetadata([[maybe_unused]] const MdParser::RegisterMap &registers,
>                                  [[maybe_unused]] Metadata &metadata) const
>  {
> diff --git a/src/ipa/rpi/cam_helper/cam_helper.h b/src/ipa/rpi/cam_helper/cam_helper.h
> index 58a4b202..705796a2 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper.h
> +++ b/src/ipa/rpi/cam_helper/cam_helper.h
> @@ -16,6 +16,7 @@
>  #include "controller/camera_mode.h"
>  #include "controller/controller.h"
>  #include "controller/metadata.h"
> +#include "controller/pdaf_data.h"
>  #include "md_parser.h"
>
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -101,11 +102,16 @@ public:
>         virtual unsigned int mistrustFramesModeSwitch() const;
>
>  protected:
> +       static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
> +                                 PdafRegions &pdaf);
>         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
>                                Metadata &metadata);
>         virtual void populateMetadata(const MdParser::RegisterMap &registers,
>                                       Metadata &metadata) const;
>
> +       static constexpr int pdafStatsRows = 12;
> +       static constexpr int pdafStatsCols = 16;

Again, these are very sensor specific const values.  I'm a bit worried that the
generic/base camera helper is now becoming a bit sensor specific.  Maybe these
values need to be passed into parsePdafData()?

Regards,
Naush

> +
>         std::unique_ptr<MdParser> parser_;
>         CameraMode mode_;
>
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> index b24ee643..d0382d63 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> @@ -12,8 +12,6 @@
>
>  #include <libcamera/base/log.h>
>
> -#include "controller/pdaf_data.h"
> -
>  #include "cam_helper.h"
>  #include "md_parser.h"
>
> @@ -72,15 +70,9 @@ private:
>         /* Largest long exposure scale factor given as a left shift on the frame length. */
>         static constexpr int longExposureShiftMax = 7;
>
> -       static constexpr int pdafStatsRows = 12;
> -       static constexpr int pdafStatsCols = 16;
> -
>         void populateMetadata(const MdParser::RegisterMap &registers,
>                               Metadata &metadata) const override;
>
> -       static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
> -                                 PdafRegions &pdaf);
> -
>         bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);
>         void putAGCStatistics(StatisticsPtr stats);
>
> @@ -243,35 +235,6 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap &registers,
>         metadata.set("device.status", deviceStatus);
>  }
>
> -bool CamHelperImx708::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
> -{
> -       const uint8_t *ptr = data.data();
> -       unsigned int len = data.size();
> -       size_t step = bpp >> 1; /* bytes per PDAF grid entry */
> -
> -       if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
> -               LOG(IPARPI, Error) << "PDAF data in unsupported format";
> -               return false;
> -       }
> -
> -       pdaf.init({ pdafStatsCols, pdafStatsRows });
> -
> -       ptr += 2 * step;
> -       for (unsigned i = 0; i < pdafStatsRows; ++i) {
> -               for (unsigned j = 0; j < pdafStatsCols; ++j) {
> -                       unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
> -                       int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
> -                       PdafData pdafData;
> -                       pdafData.conf = c;
> -                       pdafData.phase = c ? p : 0;
> -                       pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
> -                       ptr += step;
> -               }
> -       }
> -
> -       return true;
> -}
> -
>  bool CamHelperImx708::parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp)
>  {
>         static constexpr unsigned int PipelineBits = Statistics::NormalisationFactorPow2;
> --
> 2.39.1
>
Dave Stevenson June 30, 2023, 1:32 p.m. UTC | #2 
On Fri, 30 Jun 2023 at 13:54, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi Umang,
>
> Thank you for your work.
>
> On Fri, 30 Jun 2023 at 13:03, Umang Jain <umang.jain@ideasonboard.com> wrote:
> >
> > parsePdafData() parses the PDAF section of the sensor's embedded data
>
> pdaf data is not strictly part of the embedded data, so this should probably
> say sensor metadata.
>
> > and the parsing logic is generic to all PDAF-supported sensors.
> > Once the PDAF-specific buffer section is identified correctly, any
> > sensor should be able to use this helper in order parse its PDAF data.
>
> s/in order parse/in order to parse/
>
> >
> > No functional changes intended in this patch.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/ipa/rpi/cam_helper/cam_helper.cpp        | 29 +++++++++++++++
> >  src/ipa/rpi/cam_helper/cam_helper.h          |  6 ++++
> >  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 37 --------------------
> >  3 files changed, 35 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp
> > index ddd5e9a4..e9e0c496 100644
> > --- a/src/ipa/rpi/cam_helper/cam_helper.cpp
> > +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp
> > @@ -253,6 +253,35 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> >         metadata.set("device.status", deviceStatus);
> >  }
> >
> > +bool CamHelper::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
> > +{
>
> While I do agree that generalising the pdaf is a good thing to do, we must make
> it clear that this parsing is not really generic and only specific to a subset
> of Sony sensors.  No doubt other Sony/non-Sony sensors will have
> vastly different
> pdaf data formats.

Definite need for caution over making this over-generically named.
imx708 supports two different PDAF implementations, and this is only
supporting one of them. The other is sending the raw shielded pixel
values, with the processing having to be done on the SoC.
imx519 and imx682 general datasheets also reference two types of PDAF data.

We may also have a slight issue over maintenance here.
Raspberry Pi obviously support and maintain IMX708. We have no
information to maintain PDAF parsing on IMX519 (or possibly IMX682),
so who is taking on responsibility for testing any updates to PDAF
parsing on those platforms? Or is it accepted that regressions may
happen there?

  Dave

> So could this function be called parseSonyPdafData() or something similar to
> highlight this?  But also see below...
>
> > +       const uint8_t *ptr = data.data();
> > +       unsigned int len = data.size();
> > +       size_t step = bpp >> 1; /* bytes per PDAF grid entry */
> > +
> > +       if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
> > +               LOG(IPARPI, Error) << "PDAF data in unsupported format";
> > +               return false;
> > +       }
> > +
> > +       pdaf.init({ pdafStatsCols, pdafStatsRows });
> > +
> > +       ptr += 2 * step;
> > +       for (unsigned i = 0; i < pdafStatsRows; ++i) {
> > +               for (unsigned j = 0; j < pdafStatsCols; ++j) {
> > +                       unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
> > +                       int p = (((ptr[1] & 0x0f) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
> > +                       PdafData pdafData;
> > +                       pdafData.conf = c;
> > +                       pdafData.phase = c ? p : 0;
> > +                       pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
> > +                       ptr += step;
> > +               }
> > +       }
> > +
> > +       return true;
> > +}
> > +
> >  void CamHelper::populateMetadata([[maybe_unused]] const MdParser::RegisterMap &registers,
> >                                  [[maybe_unused]] Metadata &metadata) const
> >  {
> > diff --git a/src/ipa/rpi/cam_helper/cam_helper.h b/src/ipa/rpi/cam_helper/cam_helper.h
> > index 58a4b202..705796a2 100644
> > --- a/src/ipa/rpi/cam_helper/cam_helper.h
> > +++ b/src/ipa/rpi/cam_helper/cam_helper.h
> > @@ -16,6 +16,7 @@
> >  #include "controller/camera_mode.h"
> >  #include "controller/controller.h"
> >  #include "controller/metadata.h"
> > +#include "controller/pdaf_data.h"
> >  #include "md_parser.h"
> >
> >  #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -101,11 +102,16 @@ public:
> >         virtual unsigned int mistrustFramesModeSwitch() const;
> >
> >  protected:
> > +       static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
> > +                                 PdafRegions &pdaf);
> >         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> >                                Metadata &metadata);
> >         virtual void populateMetadata(const MdParser::RegisterMap &registers,
> >                                       Metadata &metadata) const;
> >
> > +       static constexpr int pdafStatsRows = 12;
> > +       static constexpr int pdafStatsCols = 16;
>
> Again, these are very sensor specific const values.  I'm a bit worried that the
> generic/base camera helper is now becoming a bit sensor specific.  Maybe these
> values need to be passed into parsePdafData()?
>
> Regards,
> Naush
>
> > +
> >         std::unique_ptr<MdParser> parser_;
> >         CameraMode mode_;
> >
> > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > index b24ee643..d0382d63 100644
> > --- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > @@ -12,8 +12,6 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > -#include "controller/pdaf_data.h"
> > -
> >  #include "cam_helper.h"
> >  #include "md_parser.h"
> >
> > @@ -72,15 +70,9 @@ private:
> >         /* Largest long exposure scale factor given as a left shift on the frame length. */
> >         static constexpr int longExposureShiftMax = 7;
> >
> > -       static constexpr int pdafStatsRows = 12;
> > -       static constexpr int pdafStatsCols = 16;
> > -
> >         void populateMetadata(const MdParser::RegisterMap &registers,
> >                               Metadata &metadata) const override;
> >
> > -       static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
> > -                                 PdafRegions &pdaf);
> > -
> >         bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);
> >         void putAGCStatistics(StatisticsPtr stats);
> >
> > @@ -243,35 +235,6 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap &registers,
> >         metadata.set("device.status", deviceStatus);
> >  }
> >
> > -bool CamHelperImx708::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
> > -{
> > -       const uint8_t *ptr = data.data();
> > -       unsigned int len = data.size();
> > -       size_t step = bpp >> 1; /* bytes per PDAF grid entry */
> > -
> > -       if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
> > -               LOG(IPARPI, Error) << "PDAF data in unsupported format";
> > -               return false;
> > -       }
> > -
> > -       pdaf.init({ pdafStatsCols, pdafStatsRows });
> > -
> > -       ptr += 2 * step;
> > -       for (unsigned i = 0; i < pdafStatsRows; ++i) {
> > -               for (unsigned j = 0; j < pdafStatsCols; ++j) {
> > -                       unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
> > -                       int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
> > -                       PdafData pdafData;
> > -                       pdafData.conf = c;
> > -                       pdafData.phase = c ? p : 0;
> > -                       pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
> > -                       ptr += step;
> > -               }
> > -       }
> > -
> > -       return true;
> > -}
> > -
> >  bool CamHelperImx708::parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp)
> >  {
> >         static constexpr unsigned int PipelineBits = Statistics::NormalisationFactorPow2;
> > --
> > 2.39.1
> >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp
index ddd5e9a4..e9e0c496 100644
--- a/src/ipa/rpi/cam_helper/cam_helper.cpp
+++ b/src/ipa/rpi/cam_helper/cam_helper.cpp
@@ -253,6 +253,35 @@  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
 	metadata.set("device.status", deviceStatus);
 }
 
+bool CamHelper::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
+{
+	const uint8_t *ptr = data.data();
+	unsigned int len = data.size();
+	size_t step = bpp >> 1; /* bytes per PDAF grid entry */
+
+	if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
+		LOG(IPARPI, Error) << "PDAF data in unsupported format";
+		return false;
+	}
+
+	pdaf.init({ pdafStatsCols, pdafStatsRows });
+
+	ptr += 2 * step;
+	for (unsigned i = 0; i < pdafStatsRows; ++i) {
+		for (unsigned j = 0; j < pdafStatsCols; ++j) {
+			unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
+			int p = (((ptr[1] & 0x0f) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
+			PdafData pdafData;
+			pdafData.conf = c;
+			pdafData.phase = c ? p : 0;
+			pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
+			ptr += step;
+		}
+	}
+
+	return true;
+}
+
 void CamHelper::populateMetadata([[maybe_unused]] const MdParser::RegisterMap &registers,
 				 [[maybe_unused]] Metadata &metadata) const
 {
diff --git a/src/ipa/rpi/cam_helper/cam_helper.h b/src/ipa/rpi/cam_helper/cam_helper.h
index 58a4b202..705796a2 100644
--- a/src/ipa/rpi/cam_helper/cam_helper.h
+++ b/src/ipa/rpi/cam_helper/cam_helper.h
@@ -16,6 +16,7 @@ 
 #include "controller/camera_mode.h"
 #include "controller/controller.h"
 #include "controller/metadata.h"
+#include "controller/pdaf_data.h"
 #include "md_parser.h"
 
 #include "libcamera/internal/v4l2_videodevice.h"
@@ -101,11 +102,16 @@  public:
 	virtual unsigned int mistrustFramesModeSwitch() const;
 
 protected:
+	static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
+				  PdafRegions &pdaf);
 	void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
 			       Metadata &metadata);
 	virtual void populateMetadata(const MdParser::RegisterMap &registers,
 				      Metadata &metadata) const;
 
+	static constexpr int pdafStatsRows = 12;
+	static constexpr int pdafStatsCols = 16;
+
 	std::unique_ptr<MdParser> parser_;
 	CameraMode mode_;
 
diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
index b24ee643..d0382d63 100644
--- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
+++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
@@ -12,8 +12,6 @@ 
 
 #include <libcamera/base/log.h>
 
-#include "controller/pdaf_data.h"
-
 #include "cam_helper.h"
 #include "md_parser.h"
 
@@ -72,15 +70,9 @@  private:
 	/* Largest long exposure scale factor given as a left shift on the frame length. */
 	static constexpr int longExposureShiftMax = 7;
 
-	static constexpr int pdafStatsRows = 12;
-	static constexpr int pdafStatsCols = 16;
-
 	void populateMetadata(const MdParser::RegisterMap &registers,
 			      Metadata &metadata) const override;
 
-	static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
-				  PdafRegions &pdaf);
-
 	bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);
 	void putAGCStatistics(StatisticsPtr stats);
 
@@ -243,35 +235,6 @@  void CamHelperImx708::populateMetadata(const MdParser::RegisterMap &registers,
 	metadata.set("device.status", deviceStatus);
 }
 
-bool CamHelperImx708::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
-{
-	const uint8_t *ptr = data.data();
-	unsigned int len = data.size();
-	size_t step = bpp >> 1; /* bytes per PDAF grid entry */
-
-	if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
-		LOG(IPARPI, Error) << "PDAF data in unsupported format";
-		return false;
-	}
-
-	pdaf.init({ pdafStatsCols, pdafStatsRows });
-
-	ptr += 2 * step;
-	for (unsigned i = 0; i < pdafStatsRows; ++i) {
-		for (unsigned j = 0; j < pdafStatsCols; ++j) {
-			unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
-			int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
-			PdafData pdafData;
-			pdafData.conf = c;
-			pdafData.phase = c ? p : 0;
-			pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
-			ptr += step;
-		}
-	}
-
-	return true;
-}
-
 bool CamHelperImx708::parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp)
 {
 	static constexpr unsigned int PipelineBits = Statistics::NormalisationFactorPow2;