[libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing
David Plowman
david.plowman at raspberrypi.com
Fri Mar 26 10:32:40 CET 2021
Hi Naush
Thanks for the feedback!
On Thu, 25 Mar 2021 at 16:08, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for your work.
>
> On Mon, 22 Mar 2021 at 16:01, David Plowman <david.plowman at raspberrypi.com> 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?
>
>>
>>
>> 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...
> With or without this change and the fixup for the namespace scoping above:
>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
Thanks!
David
>
>>
>> +
>> +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;
>> --
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list