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

[Naushir Patuck]

Hi Umang,

Thank you for your work.

On Fri, 30 Jun 2023 at 13:03, Umang Jain <umang.jain at 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 at 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
>