[libcamera-devel] [RFC PATCH 2/5] ipa: rpi: Make parsePdafData() available in the CamHelper class
Dave Stevenson
dave.stevenson at raspberrypi.com
Fri Jun 30 15:32:59 CEST 2023
On Fri, 30 Jun 2023 at 13:54, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> 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.
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 ®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