[libcamera-devel] [RFC PATCH 2/5] ipa: rpi: Make parsePdafData() available in the CamHelper class
Naushir Patuck
naush at raspberrypi.com
Fri Jun 30 14:54:06 CEST 2023
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 ®isters,
> [[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 ®isters,
> 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 ®isters,
> 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 ®isters,
> 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
>
More information about the libcamera-devel
mailing list