[libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 26 14:28:38 CET 2021
Hi David and Naush,
On Fri, Mar 26, 2021 at 11:14:56AM +0000, Naushir Patuck wrote:
> On Fri, 26 Mar 2021 at 11:07, David Plowman wrote:
> > On Fri, 26 Mar 2021 at 09:57, Naushir Patuck wrote:
> >> On Fri, 26 Mar 2021 at 09:32, David Plowman wrote:
> >>> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck wrote:
> >>>> On Mon, 22 Mar 2021 at 16:01, David Plowman wrote:
> >>>>>
> >>>>> CamHelpers get virtual Prepare() and Process() methods, running just
> >>>>> before and just after the ISP, just like Raspberry Pi Algorithms.
> >>>>>
> >>>>> The Prepare() method is able to parse the register dumps in embedded
> >>>>> data buffers, and can be specialised to perform custom processing when
> >>>>> necessary.
> >>>>>
> >>>>> The IPA itself only needs to call the new Prepare() and Process()
> >>>>> methods, and to supply exposure/gain values from the controls when the
> >>>>> CamHelper does not.
> >>>>>
> >>>>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> >>>>> ---
> >>>>> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++
> >>>>> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-
> >>>>> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------
> >>>>> 3 files changed, 83 insertions(+), 55 deletions(-)
> >>>>>
> >>>>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> >>>>> index 0ae0baa0..ce6482ba 100644
> >>>>> --- a/src/ipa/raspberrypi/cam_helper.cpp
> >>>>> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> >>>>> @@ -17,6 +17,11 @@
> >>>>> #include "md_parser.hpp"
> >>>>>
> >>>>> using namespace RPiController;
> >>>>> +using namespace libcamera;
> >>>>> +
> >>>>> +namespace libcamera {
> >>>>> +LOG_DECLARE_CATEGORY(IPARPI)
> >>>>> +}
> >>>>
> >>>> I don't think this namespace scope is needed. Seems to compile fine without.
> >>>
> >>> Funny one, this. It builds like that for me too but when I run it (for
> >>> example, qcam), it bombs out with
> >>>
> >>> build/src/qcam/qcam: symbol lookup error:
> >>> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined
> >>> symbol: _Z17logCategoryIPARPIv
> >>>
> >>> It seems to be looking for logCategory::IPARPI when I think it should
> >>> be libcamera::logCategory::IPARPI. I guess I'm not clear why the
> >>> linker doesn't flag that up... anyone know?
That's an interesting one, haven't seen it before. I would also have
expected the linker to complain.
This causes the linker to complain:
diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
index d1397a3211f0..687b1f5d6412 100644
--- a/src/ipa/raspberrypi/meson.build
+++ b/src/ipa/raspberrypi/meson.build
@@ -46,6 +46,7 @@ mod = shared_module(ipa_name,
name_prefix : '',
include_directories : rpi_ipa_includes,
dependencies : rpi_ipa_deps,
+ link_args : ['-Wl,--no-undefined'],
link_with : libipa,
install : true,
install_dir : ipa_install_dir)
I haven't investigated whether this is something we should enable
globally though, and why the default is to allow undefined symbols.
> >>>>> static std::map<std::string, CamHelperCreateFunc> cam_helpers;
> >>>>>
> >>>>> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
> >>>>> delete parser_;
> >>>>> }
> >>>>>
> >>>>> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
> >>>>> + Metadata &metadata)
> >>>>>
> >>>>> +{
> >>>>> + parseEmbeddedData(buffer, metadata);
> >>>>> +}
> >>>>
> >>>> Would it be useful to pass in the ControlList that contains the sensor setup
> >>>> (from DelayedControls) into this function? Would the CamHelper ever want
> >>>> to know these things?
> >>>>
> >>>> If the answer is yes, you could also pull in the case that deals with non-embedded
> >>>> data where we pull values from the ControlList. So something like:
> >>>>
> >>>> void CamHelper::Prepare(const Span<uint8_t> &buffer,
> >>>> Metadata &metadata, ControlList &SensorControls)
> >>>> {
> >>>> bool success = false;
> >>>>
> >>>> if (buffer.size())
> >>>> success = parseEmbeddedData(buffer, metadata);
> >>>>
> >>>> if (!success) {
> >>>> struct DeviceStatus deviceStatus = {};
> >>>> int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >>>> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >>>>
> >>>> deviceStatus.shutter_speed = Exposure(exposureLines);
> >>>> deviceStatus.analogue_gain = Gain(gainCode);
> >>>> metadata.Set("device.status", deviceStatus);
> >>>> }
> >>>> }
> >>>>
> >>>> What do you think? Might be a bit neater to have deviecStatus set in one place only.
> >>>
> >>> Interesting point, and in fact, my previous version of this patch
> >>> (labelled as "RFC") did exactly this. Obviously I back-tracked on
> >>> that, for the following reasons.
> >>>
> >>> The version you've outlined above is nice because it makes the IPA
> >>> file (raspberrypi.cpp) a bit tidier, functions like
> >>> CamHelper::Exposure and CamHelper::Gain only need to be called once.
> >>> The DeviceStatus only gets set in one place.
> >>>
> >>> The version I went for in the end has the advantage that it makes
> >>> CamHelper::Prepare responsible only for parsing sensor metadata.
> >>> There's nothing it has to remember to do if, for example, some
> >>> particular metadata isn't there.
> >>>
> >>> This means that the CamHelper::Prepare for my mysterious new sensor
> >>> doesn't have to do anything extra. It just parses the sensor's
> >>> statistics buffer and nothing else. As this kind of code - CamHelpers
> >>> - is likely to get duplicated more in future, potentially by 3rd
> >>> parties, I was keener to avoid little "gotchas" in there, and slight
> >>> uglification of raspberrypi.cpp seemed reasonable in return.
> >>>
> >>> But it's a close call and I'm definitely swayable...
> >>
> >> Perhaps it's a bit premature to think about this, but what if some
> >> stuff running in prepare() (e.g. fancy phase detect AF) needs the
> >> exposure and gain values used for the sensor, and we don't have
> >> embedded data to give us these values. You could possibly run this
> >> fancy AF in process() where you do get the values, but then you add
> >> a frame's worth of latency.
> >>
> >> It also possibly makes the code a bit cleaner for an up-coming
> >> change for imx477 ultra long exposures where I have to disable
> >> parsing when the exposure factor is set. But again, I have not
> >> written code for this yet to tell for sure.
> >
> > That's an interesting thought. Generally I'm quite strongly opposed
> > to "algorithm" code creeping into the helpers, the helpers should be
> > restricted to the minimum "parsing" duties. To take the AF example,
> > CamHelper::Prepare would put the PDAF information into some kind of
> > "pdaf" entry in our RPi metadata. Hopefully many sensors would be
> > able to re-use this "pdaf" metadata. Then there'd be a bona fide
> > "pdaf" algorithm, loaded as usual via the json file, that would
> > interpret it.
>
> Yes I agree that algorithm stuff should end up in the Controller, so maybe
> this is not something we deal with.
I agree too, especially given that sensor helpers should be moved to
libipa.
> > On the subject of long exposures, how can you tell whether you're in
> > the long exposure case - are the values enough?
>
> Unfortunately not. We have to use the fact that the helper (specifically
> GetVBlanking) will setup the long exposure factor based on the requested
> exposure time. Once this is done, we store state in the CamHelper to
> abort parsing until long exposure is turned off.
>
> I'm thinking about always reading the exposure/gain from the controls
> and filling in the DeviceStatus *before* we call CamHelper::Prepare.
> That way you have the values, and you can overwrite them if you know
> better. Would that be helpful to the imx477?
>
> Probably won't make much difference to imx477, but this might be a bit
> neater than we have it now. I think we should make this change!
>
> >> I'll leave it to you to choose :-)
> >>
> >>>> With or without this change and the fixup for the namespace scoping above:
> >>>>
> >>>> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> >>>>
> >>>>> +
> >>>>> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> >>>>> + [[maybe_unused]] Metadata &metadata)
> >>>>> +{
> >>>>> +}
> >>>>> +
> >>>>> uint32_t CamHelper::ExposureLines(double exposure_us) const
> >>>>> {
> >>>>> assert(initialized_);
> >>>>> @@ -139,6 +155,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,
> >>>>> + Metadata &metadata)
> >>>>>
> >>>>> +{
> >>>>> + if (buffer.size()) {
> >>>>> + struct DeviceStatus deviceStatus = {};
> >>>>> + bool success = false;
> >>>>> + uint32_t exposureLines, gainCode;
> >>>>> +
> >>>>> + parser_->SetBufferSize(buffer.size());
> >>>>> + success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&
> >>>>> + parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&
> >>>>> + parser_->GetGainCode(gainCode) == MdParser::Status::OK;
> >>>>> +
> >>>>> + if (success) {
> >>>>> + deviceStatus.shutter_speed = Exposure(exposureLines);
> >>>>> + deviceStatus.analogue_gain = Gain(gainCode);
> >>>>> +
> >>>>> + LOG(IPARPI, Debug) << "Metadata - Exposure : "
> >>>>> + << deviceStatus.shutter_speed
> >>>>> + << " Gain : "
> >>>>> + << deviceStatus.analogue_gain;
> >>>>> +
> >>>>> + metadata.Set("device.status", deviceStatus);
> >>>>> + } else
> >>>>> + LOG(IPARPI, Error) << "Embedded buffer parsing failed";
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> >>>>> CamHelperCreateFunc create_func)
> >>>>> {
> >>>>> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> >>>>> index 1b2d6eec..930ea39a 100644
> >>>>> --- a/src/ipa/raspberrypi/cam_helper.hpp
> >>>>> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> >>>>> @@ -8,7 +8,11 @@
> >>>>>
> >>>>> #include <string>
> >>>>>
> >>>>> +#include <libcamera/span.h>
> >>>>> +
> >>>>> #include "camera_mode.h"
> >>>>> +#include "controller/controller.hpp"
> >>>>> +#include "controller/metadata.hpp"
> >>>>> #include "md_parser.hpp"
> >>>>>
> >>>>> #include "libcamera/internal/v4l2_videodevice.h"
> >>>>> @@ -65,7 +69,9 @@ public:
> >>>>> CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> >>>>> virtual ~CamHelper();
> >>>>> void SetCameraMode(const CameraMode &mode);
> >>>>> - MdParser &Parser() const { return *parser_; }
> >>>>> + virtual void Prepare(const libcamera::Span<uint8_t> &buffer,
> >>>>> + Metadata &metadata);
> >>>>> + virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> >>>>> uint32_t ExposureLines(double exposure_us) const;
> >>>>> double Exposure(uint32_t exposure_lines) const; // in us
> >>>>> virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,
> >>>>> @@ -81,6 +87,9 @@ public:
> >>>>> virtual unsigned int MistrustFramesModeSwitch() const;
> >>>>>
> >>>>> protected:
> >>>>> + void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,
> >>>>> + Metadata &metadata);
> >>>>> +
> >>>>> MdParser *parser_;
> >>>>> CameraMode mode_;
> >>>>>
> >>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>> index 7904225a..e08b47c0 100644
> >>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>> @@ -103,8 +103,7 @@ private:
> >>>>> void returnEmbeddedBuffer(unsigned int bufferId);
> >>>>> void prepareISP(const ipa::RPi::ISPConfig &data);
> >>>>> void reportMetadata();
> >>>>> - bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
> >>>>> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> >>>>> + void fillDeviceStatus(const ControlList &sensorControls,
> >>>>> struct DeviceStatus &deviceStatus);
> >>>>> void processStats(unsigned int bufferId);
> >>>>> void applyFrameDurations(double minFrameDuration, double maxFrameDuration);
> >>>>> @@ -913,35 +912,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >>>>>
> >>>>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >>>>> {
> >>>>> - struct DeviceStatus deviceStatus = {};
> >>>>> - bool success = false;
> >>>>> + Span<uint8_t> embeddedBuffer;
> >>>>> +
> >>>>> + rpiMetadata_.Clear();
> >>>>>
> >>>>> if (data.embeddedBufferPresent) {
> >>>>> /*
> >>>>> * Pipeline handler has supplied us with an embedded data buffer,
> >>>>> - * so parse it and extract the exposure and gain.
> >>>>> + * we must pass it to the CamHelper for parsing.
> >>>>> */
> >>>>> - success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);
> >>>>> -
> >>>>> - /* Done with embedded data now, return to pipeline handler asap. */
> >>>>> - returnEmbeddedBuffer(data.embeddedBufferId);
> >>>>> + auto it = buffers_.find(data.embeddedBufferId);
> >>>>> + ASSERT(it != buffers_.end());
> >>>>> + embeddedBuffer = it->second.maps()[0];
> >>>>> }
> >>>>>
> >>>>> - if (!success) {
> >>>>> - /*
> >>>>> - * Pipeline handler has not supplied an embedded data buffer,
> >>>>> - * or embedded data buffer parsing has failed for some reason,
> >>>>> - * so pull the exposure and gain values from the control list.
> >>>>> - */
> >>>>> - int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >>>>> - int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >>>>> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> >>>>> - }
> >>>>> + /*
> >>>>> + * This may add the DeviceStatus to the metadata, and depending on the
> >>>>> + * sensor, may do additional custom processing.
> >>>>> + */
> >>>>> + helper_->Prepare(embeddedBuffer, rpiMetadata_);
> >>>>> +
> >>>>> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */
> >>>>> + DeviceStatus deviceStatus = {};
> >>>>> + if (rpiMetadata_.Get("device.status", deviceStatus))
> >>>>> + fillDeviceStatus(data.controls, deviceStatus);
> >>>>> +
> >>>>> + /* Done with embedded data now, return to pipeline handler asap. */
> >>>>> + if (data.embeddedBufferPresent)
> >>>>> + returnEmbeddedBuffer(data.embeddedBufferId);
> >>>>>
> >>>>> ControlList ctrls(ispCtrls_);
> >>>>>
> >>>>> - rpiMetadata_.Clear();
> >>>>> - rpiMetadata_.Set("device.status", deviceStatus);
> >>>>> controller_.Prepare(&rpiMetadata_);
> >>>>>
> >>>>> /* Lock the metadata buffer to avoid constant locks/unlocks. */
> >>>>> @@ -991,41 +992,12 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >>>>> setIspControls.emit(ctrls);
> >>>>> }
> >>>>>
> >>>>> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)
> >>>>> -{
> >>>>> - auto it = buffers_.find(bufferId);
> >>>>> - if (it == buffers_.end()) {
> >>>>> - LOG(IPARPI, Error) << "Could not find embedded buffer!";
> >>>>> - return false;
> >>>>> - }
> >>>>> -
> >>>>> - Span<uint8_t> mem = it->second.maps()[0];
> >>>>> - helper_->Parser().SetBufferSize(mem.size());
> >>>>> - RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());
> >>>>> - if (status != RPiController::MdParser::Status::OK) {
> >>>>> - LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
> >>>>> - return false;
> >>>>> - } else {
> >>>>> - uint32_t exposureLines, gainCode;
> >>>>> - if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
> >>>>> - LOG(IPARPI, Error) << "Exposure time failed";
> >>>>> - return false;
> >>>>> - }
> >>>>> -
> >>>>> - if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
> >>>>> - LOG(IPARPI, Error) << "Gain failed";
> >>>>> - return false;
> >>>>> - }
> >>>>> -
> >>>>> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> >>>>> - }
> >>>>> -
> >>>>> - return true;
> >>>>> -}
> >>>>> -
> >>>>> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> >>>>> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,
> >>>>> struct DeviceStatus &deviceStatus)
> >>>>> {
> >>>>> + int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >>>>> + int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >>>>> +
> >>>>> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> >>>>> deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >>>>>
> >>>>> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> >>>>> << deviceStatus.shutter_speed
> >>>>> << " Gain : "
> >>>>> << deviceStatus.analogue_gain;
> >>>>> +
> >>>>> + rpiMetadata_.Set("device.status", deviceStatus);
> >>>>> }
> >>>>>
> >>>>> void IPARPi::processStats(unsigned int bufferId)
> >>>>> @@ -1046,6 +1020,7 @@ void IPARPi::processStats(unsigned int bufferId)
> >>>>> Span<uint8_t> mem = it->second.maps()[0];
> >>>>> bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());
> >>>>> RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);
> >>>>> + helper_->Process(statistics, rpiMetadata_);
> >>>>> controller_.Process(statistics, &rpiMetadata_);
> >>>>>
> >>>>> struct AgcStatus agcStatus;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list