[libcamera-devel] [PATCH v3 1/4] pipeline: ipa: raspberrypi: Pass exposure/gain values to IPA though controls
Naushir Patuck
naush at raspberrypi.com
Mon Feb 22 14:09:45 CET 2021
Hi Laurent,
Thank you for your review feedback.
On Sun, 21 Feb 2021 at 22:18, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Feb 18, 2021 at 05:01:23PM +0000, Naushir Patuck wrote:
> > When running with sensors that had no embedded data, the pipeline handler
> > would fill a dummy embedded data buffer with gain/exposure values, and
> > pass this buffer to the IPA together with the bayer buffer. The IPA would
> > extract these values for use in the controller algorithms.
> >
> > Rework this logic entirely by having a new RPiCameraData::BayerFrame
> > queue to replace the existing bayer queue. In addition to storing the
> > FrameBuffer pointer, this also stores all the controls tracked by
> > DelayedControls for that frame in a ControlList. This includes include
> > exposure and gain values. On signalling RPi::IPA_EVENT_SIGNAL_ISP_PREPARE
> > IPA event, the pipeline handler now passes this ControlList from the
> > RPiCameraData::BayerFrame queue.
> >
> > The IPA now extracts the gain and exposure values from the ControlList
> > instead of using RPiController::MdParserRPi to parse the embedded data
> > buffer.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > include/libcamera/ipa/raspberrypi.mojom | 2 +
> > src/ipa/raspberrypi/raspberrypi.cpp | 132 +++++++++++-------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 58 ++++----
> > 3 files changed, 108 insertions(+), 84 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> > index 5a27b1e4fc2d..f733a2cd2a40 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -29,6 +29,8 @@ struct SensorConfig {
> > struct ISPConfig {
> > uint32 embeddedBufferId;
> > uint32 bayerBufferId;
> > + bool embeddedBufferPresent;
> > + ControlList controls;
> > };
> >
> > struct ConfigInput {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 1226ea514521..f9ab6417866e 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1,6 +1,6 @@
> > /* SPDX-License-Identifier: BSD-2-Clause */
> > /*
> > - * Copyright (C) 2019-2020, Raspberry Pi (Trading) Ltd.
> > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.
> > *
> > * rpi.cpp - Raspberry Pi Image Processing Algorithms
> > */
> > @@ -101,9 +101,11 @@ private:
> > bool validateIspControls();
> > void queueRequest(const ControlList &controls);
> > void returnEmbeddedBuffer(unsigned int bufferId);
> > - void prepareISP(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,
> > + struct DeviceStatus &deviceStatus);
> > void processStats(unsigned int bufferId);
> > void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> > void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> > @@ -447,7 +449,7 @@ void IPARPi::signalIspPrepare(const
> ipa::RPi::ISPConfig &data)
> > * avoid running the control algos for a few frames in case
> > * they are "unreliable".
> > */
> > - prepareISP(data.embeddedBufferId);
> > + prepareISP(data);
> > frameCount_++;
> >
> > /* Ready to push the input buffer into the ISP. */
> > @@ -909,67 +911,84 @@ void IPARPi::returnEmbeddedBuffer(unsigned int
> bufferId)
> > embeddedComplete.emit(bufferId & ipa::RPi::MaskID);
> > }
> >
> > -void IPARPi::prepareISP(unsigned int bufferId)
> > +void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> > {
> > struct DeviceStatus deviceStatus = {};
> > - bool success = parseEmbeddedData(bufferId, deviceStatus);
> > + bool success = false;
>
> The changes below are hard to read ('git show -b' helps after applying).
> For future patch series, if large changes in indentation can be isolated
> in a patch that performs no or very little functional changes, it can
> help review.
>
Apologies, I will keep that in mind for the future.
>
> >
> > - /* Done with embedded data now, return to pipeline handler asap. */
> > - returnEmbeddedBuffer(bufferId);
> > + if (data.embeddedBufferPresent) {
> > + /*
> > + * Pipeline handler has supplied us with an embedded data
> buffer,
> > + * so parse it and extract the exposure and gain.
> > + */
> > + success = parseEmbeddedData(data.embeddedBufferId,
> deviceStatus);
> >
> > - if (success) {
> > - ControlList ctrls(ispCtrls_);
> > + /* Done with embedded data now, return to pipeline handler
> asap. */
> > + returnEmbeddedBuffer(data.embeddedBufferId);
> > + }
> >
> > - rpiMetadata_.Clear();
> > - rpiMetadata_.Set("device.status", deviceStatus);
> > - controller_.Prepare(&rpiMetadata_);
> > + 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);
> > + }
> >
> > - /* Lock the metadata buffer to avoid constant
> locks/unlocks. */
> > - std::unique_lock<RPiController::Metadata>
> lock(rpiMetadata_);
> > + ControlList ctrls(ispCtrls_);
> >
> > - AwbStatus *awbStatus =
> rpiMetadata_.GetLocked<AwbStatus>("awb.status");
> > - if (awbStatus)
> > - applyAWB(awbStatus, ctrls);
> > + rpiMetadata_.Clear();
> > + rpiMetadata_.Set("device.status", deviceStatus);
> > + controller_.Prepare(&rpiMetadata_);
> >
> > - CcmStatus *ccmStatus =
> rpiMetadata_.GetLocked<CcmStatus>("ccm.status");
> > - if (ccmStatus)
> > - applyCCM(ccmStatus, ctrls);
> > + /* Lock the metadata buffer to avoid constant locks/unlocks. */
> > + std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);
> >
> > - AgcStatus *dgStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> > - if (dgStatus)
> > - applyDG(dgStatus, ctrls);
> > + AwbStatus *awbStatus =
> rpiMetadata_.GetLocked<AwbStatus>("awb.status");
> > + if (awbStatus)
> > + applyAWB(awbStatus, ctrls);
> >
> > - AlscStatus *lsStatus =
> rpiMetadata_.GetLocked<AlscStatus>("alsc.status");
> > - if (lsStatus)
> > - applyLS(lsStatus, ctrls);
> > + CcmStatus *ccmStatus =
> rpiMetadata_.GetLocked<CcmStatus>("ccm.status");
> > + if (ccmStatus)
> > + applyCCM(ccmStatus, ctrls);
> >
> > - ContrastStatus *contrastStatus =
> rpiMetadata_.GetLocked<ContrastStatus>("contrast.status");
> > - if (contrastStatus)
> > - applyGamma(contrastStatus, ctrls);
> > + AgcStatus *dgStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> > + if (dgStatus)
> > + applyDG(dgStatus, ctrls);
> >
> > - BlackLevelStatus *blackLevelStatus =
> rpiMetadata_.GetLocked<BlackLevelStatus>("black_level.status");
> > - if (blackLevelStatus)
> > - applyBlackLevel(blackLevelStatus, ctrls);
> > + AlscStatus *lsStatus =
> rpiMetadata_.GetLocked<AlscStatus>("alsc.status");
> > + if (lsStatus)
> > + applyLS(lsStatus, ctrls);
> >
> > - GeqStatus *geqStatus =
> rpiMetadata_.GetLocked<GeqStatus>("geq.status");
> > - if (geqStatus)
> > - applyGEQ(geqStatus, ctrls);
> > + ContrastStatus *contrastStatus =
> rpiMetadata_.GetLocked<ContrastStatus>("contrast.status");
> > + if (contrastStatus)
> > + applyGamma(contrastStatus, ctrls);
> >
> > - DenoiseStatus *denoiseStatus =
> rpiMetadata_.GetLocked<DenoiseStatus>("denoise.status");
> > - if (denoiseStatus)
> > - applyDenoise(denoiseStatus, ctrls);
> > + BlackLevelStatus *blackLevelStatus =
> rpiMetadata_.GetLocked<BlackLevelStatus>("black_level.status");
> > + if (blackLevelStatus)
> > + applyBlackLevel(blackLevelStatus, ctrls);
> >
> > - SharpenStatus *sharpenStatus =
> rpiMetadata_.GetLocked<SharpenStatus>("sharpen.status");
> > - if (sharpenStatus)
> > - applySharpen(sharpenStatus, ctrls);
> > + GeqStatus *geqStatus =
> rpiMetadata_.GetLocked<GeqStatus>("geq.status");
> > + if (geqStatus)
> > + applyGEQ(geqStatus, ctrls);
> >
> > - DpcStatus *dpcStatus =
> rpiMetadata_.GetLocked<DpcStatus>("dpc.status");
> > - if (dpcStatus)
> > - applyDPC(dpcStatus, ctrls);
> > + DenoiseStatus *denoiseStatus =
> rpiMetadata_.GetLocked<DenoiseStatus>("denoise.status");
> > + if (denoiseStatus)
> > + applyDenoise(denoiseStatus, ctrls);
> >
> > - if (!ctrls.empty())
> > - setIspControls.emit(ctrls);
> > - }
> > + SharpenStatus *sharpenStatus =
> rpiMetadata_.GetLocked<SharpenStatus>("sharpen.status");
> > + if (sharpenStatus)
> > + applySharpen(sharpenStatus, ctrls);
> > +
> > + DpcStatus *dpcStatus =
> rpiMetadata_.GetLocked<DpcStatus>("dpc.status");
> > + if (dpcStatus)
> > + applyDPC(dpcStatus, ctrls);
> > +
> > + if (!ctrls.empty())
> > + setIspControls.emit(ctrls);
> > }
> >
> > bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct
> DeviceStatus &deviceStatus)
> > @@ -985,6 +1004,7 @@ bool IPARPi::parseEmbeddedData(unsigned int
> bufferId, struct DeviceStatus &devic
> > 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) {
> > @@ -992,21 +1012,29 @@ bool IPARPi::parseEmbeddedData(unsigned int
> bufferId, struct DeviceStatus &devic
> > return false;
> > }
> >
> > - deviceStatus.shutter_speed =
> helper_->Exposure(exposureLines);
> > if (helper_->Parser().GetGainCode(gainCode) !=
> RPiController::MdParser::Status::OK) {
> > LOG(IPARPI, Error) << "Gain failed";
> > return false;
> > }
> >
> > - deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > - LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > - << deviceStatus.shutter_speed << " Gain
> : "
> > - << deviceStatus.analogue_gain;
> > + fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> > }
> >
> > return true;
> > }
> >
> > +void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> > + struct DeviceStatus &deviceStatus)
> > +{
> > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > + deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > +
> > + LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > + << deviceStatus.shutter_speed
> > + << " Gain : "
> > + << deviceStatus.analogue_gain;
> > +}
> > +
> > void IPARPi::processStats(unsigned int bufferId)
> > {
> > auto it = buffers_.find(bufferId);
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index ba74ace183db..b43d86166c63 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1,6 +1,6 @@
> > /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > /*
> > - * Copyright (C) 2019-2020, Raspberry Pi (Trading) Ltd.
> > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.
> > *
> > * raspberrypi.cpp - Pipeline handler for Raspberry Pi devices
> > */
> > @@ -197,7 +197,13 @@ public:
> > */
> > enum class State { Stopped, Idle, Busy, IpaComplete };
> > State state_;
> > - std::queue<FrameBuffer *> bayerQueue_;
> > +
> > + struct BayerFrame {
> > + FrameBuffer *buffer;
> > + ControlList controls;
> > + };
> > +
> > + std::queue<BayerFrame> bayerQueue_;
> > std::queue<FrameBuffer *> embeddedQueue_;
> > std::deque<Request *> requestQueue_;
> >
> > @@ -222,7 +228,7 @@ public:
> > private:
> > void checkRequestCompleted();
> > void tryRunPipeline();
> > - bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer
> *&embeddedBuffer);
> > + bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer
> *&embeddedBuffer);
> >
> > unsigned int ispOutputCount_;
> > };
> > @@ -1383,29 +1389,14 @@ void
> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > if (stream == &unicam_[Unicam::Image]) {
> > - bayerQueue_.push(buffer);
> > - } else {
> > - embeddedQueue_.push(buffer);
> > -
> > - ControlList ctrl =
> delayedCtrls_->get(buffer->metadata().sequence);
> > -
> > /*
> > - * Sensor metadata is unavailable, so put the expected ctrl
> > - * values (accounting for the staggered delays) into the
> empty
> > - * metadata buffer.
> > + * Lookup the sensor controls used for this frame sequence
> from
> > + * StaggeredCtrl and queue them along with the frame
> buffer.
>
> s/StaggeredCtrl/DelayedControls/ ?
>
> > */
> > - if (!sensorMetadata_) {
> > - unsigned int bufferId =
> unicam_[Unicam::Embedded].getBufferId(buffer);
> > - auto it = mappedEmbeddedBuffers_.find(bufferId);
> > - if (it != mappedEmbeddedBuffers_.end()) {
> > - uint32_t *mem = reinterpret_cast<uint32_t
> *>(it->second.maps()[0].data());
> > - mem[0] =
> ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > - mem[1] =
> ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> > - } else {
> > - LOG(RPI, Warning) << "Failed to find
> embedded buffer "
> > - << bufferId;
> > - }
> > - }
> > + ControlList ctrl =
> delayedCtrls_->get(buffer->metadata().sequence);
> > + bayerQueue_.push({ buffer, std::move(ctrl) });
> > + } else {
> > + embeddedQueue_.push(buffer);
> > }
> >
> > handleState();
> > @@ -1656,14 +1647,15 @@ void RPiCameraData::applyScalerCrop(const
> ControlList &controls)
> >
> > void RPiCameraData::tryRunPipeline()
> > {
> > - FrameBuffer *bayerBuffer, *embeddedBuffer;
> > + FrameBuffer *embeddedBuffer;
> > + BayerFrame bayerFrame;
> >
> > /* If any of our request or buffer queues are empty, we cannot
> proceed. */
> > if (state_ != State::Idle || requestQueue_.empty() ||
> > bayerQueue_.empty() || embeddedQueue_.empty())
> > return;
> >
> > - if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))
> > + if (!findMatchingBuffers(bayerFrame, embeddedBuffer))
> > return;
> >
> > /* Take the first request from the queue and action the IPA. */
> > @@ -1682,7 +1674,7 @@ void RPiCameraData::tryRunPipeline()
> > /* Set our state to say the pipeline is active. */
> > state_ = State::Busy;
> >
> > - unsigned int bayerId =
> unicam_[Unicam::Image].getBufferId(bayerBuffer);
> > + unsigned int bayerId =
> unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > unsigned int embeddedId =
> unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> >
> > LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > @@ -1692,24 +1684,26 @@ void RPiCameraData::tryRunPipeline()
> > ipa::RPi::ISPConfig ispPrepare;
> > ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData |
> embeddedId;
> > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;
> > + ispPrepare.embeddedBufferPresent = true;
>
> Shouldn't the be conditioned by embedded data being present ?
>
Yes, good catch! It is conditional on the next patches, but should be here
as well.
>
> > + ispPrepare.controls = std::move(bayerFrame.controls);
> > ipa_->signalIspPrepare(ispPrepare);
> > }
> >
> > -bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,
> FrameBuffer *&embeddedBuffer)
> > +bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame,
> FrameBuffer *&embeddedBuffer)
> > {
> > unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;
> >
> > /* Loop until we find a matching bayer and embedded data buffer. */
> > while (!bayerQueue_.empty()) {
> > /* Start with the front of the bayer queue. */
> > - bayerBuffer = bayerQueue_.front();
> > + bayerFrame = bayerQueue_.front();
>
> This will create a copy of the ControlList. Is there a way the logic in
> this function could be reworked to allow an std::move() here ?
>
Good point. I think it should be easy enough to allow a std::move() here.
>
> >
> > /*
> > * Find the embedded data buffer with a matching timestamp
> to pass to
> > * the IPA. Any embedded buffers with a timestamp lower
> than the
> > * current bayer buffer will be removed and re-queued to
> the driver.
> > */
> > - uint64_t ts = bayerBuffer->metadata().timestamp;
> > + uint64_t ts = bayerFrame.buffer->metadata().timestamp;
> > embeddedBuffer = nullptr;
> > while (!embeddedQueue_.empty()) {
> > FrameBuffer *b = embeddedQueue_.front();
> > @@ -1739,7 +1733,7 @@ bool
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
> > * the front of the queue. This buffer is
> now orphaned, so requeue
> > * it back to the device.
> > */
> > -
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > +
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);
> > bayerQueue_.pop();
> > bayerRequeueCount++;
> > LOG(RPI, Warning) << "Dropping unmatched
> input frame in stream "
> > @@ -1757,7 +1751,7 @@ bool
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
> >
> > LOG(RPI, Warning) << "Flushing bayer
> stream!";
> > while (!bayerQueue_.empty()) {
> > -
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > +
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);
> > bayerQueue_.pop();
> > }
> > flushedBuffers = true;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210222/32ca24b2/attachment-0001.htm>
More information about the libcamera-devel
mailing list