[libcamera-devel] [PATCH v4 2/9] libcamera: pipeline: ipa: raspberrypi: Rework drop frame signalling
Naushir Patuck
naush at raspberrypi.com
Wed Jul 22 09:53:06 CEST 2020
Hi Kieran,
Thank you for your review.
On Tue, 21 Jul 2020 at 11:59, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 20/07/2020 10:13, Naushir Patuck wrote:
> > The IPA now signals up front how many frames it wants the pipeline
> > handler to drop. This makes it easier to handle up-coming changes to the
> > buffer handling for import/export buffers.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> Looks good to me
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > ---
> > include/libcamera/ipa/raspberrypi.h | 2 +-
> > src/ipa/raspberrypi/raspberrypi.cpp | 20 +++++------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 36 +++++++++++--------
> > 3 files changed, 33 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a4937769..0f31b6ea 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -14,6 +14,7 @@ enum RPiConfigParameters {
> > RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> > RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > RPI_IPA_CONFIG_SENSOR = (1 << 2),
> > + RPI_IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > };
> >
> > enum RPiOperations {
> > @@ -21,7 +22,6 @@ enum RPiOperations {
> > RPI_IPA_ACTION_V4L2_SET_ISP,
> > RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
> > RPI_IPA_ACTION_RUN_ISP,
> > - RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
> > RPI_IPA_ACTION_EMBEDDED_COMPLETE,
> > RPI_IPA_EVENT_SIGNAL_STAT_READY,
> > RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 7bd04880..2d9fb9d8 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -65,8 +65,8 @@ class IPARPi : public IPAInterface
> > public:
> > IPARPi()
> > : lastMode_({}), controller_(), controllerInit_(false),
> > - frame_count_(0), check_count_(0), hide_count_(0),
> > - mistrust_count_(0), lsTable_(nullptr)
> > + frame_count_(0), check_count_(0), mistrust_count_(0),
> > + lsTableHandle_(0), lsTable_(nullptr)
> > {
> > }
> >
> > @@ -137,8 +137,6 @@ private:
> > uint64_t frame_count_;
> > /* For checking the sequencing of Prepare/Process calls. */
> > uint64_t check_count_;
> > - /* How many frames the pipeline handler should hide, or "drop". */
> > - unsigned int hide_count_;
> > /* How many frames we should avoid running control algos on. */
> > unsigned int mistrust_count_;
> > /* LS table allocation passed in from the pipeline handler. */
> > @@ -242,14 +240,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > */
> > frame_count_ = 0;
> > check_count_ = 0;
> > + int drop_frame = 0;
> > if (controllerInit_) {
> > - hide_count_ = helper_->HideFramesModeSwitch();
> > + drop_frame = helper_->HideFramesModeSwitch();
> > mistrust_count_ = helper_->MistrustFramesModeSwitch();
> > } else {
> > - hide_count_ = helper_->HideFramesStartup();
> > + drop_frame = helper_->HideFramesStartup();
> > mistrust_count_ = helper_->MistrustFramesStartup();
> > }
> >
> > + result->data.push_back(drop_frame);
> > + result->operation |= RPI_IPA_CONFIG_DROP_FRAMES;
> > +
> > struct AgcStatus agcStatus;
> > /* These zero values mean not program anything (unless overwritten). */
> > agcStatus.shutter_time = 0.0;
> > @@ -366,13 +368,11 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > * they are "unreliable".
> > */
> > prepareISP(embeddedbufferId);
> > + frame_count_++;
> >
> > /* Ready to push the input buffer into the ISP. */
> > IPAOperationData op;
> > - if (++frame_count_ > hide_count_)
> > - op.operation = RPI_IPA_ACTION_RUN_ISP;
> > - else
> > - op.operation = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;
> > + op.operation = RPI_IPA_ACTION_RUN_ISP;
>
> The bit field flag here looks better to me indeed!
>
> > op.data = { bayerbufferId & RPiIpaMask::ID };
> > queueFrameAction.emit(0, op);
> > break;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 6630ef57..d3639ce1 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -133,7 +133,7 @@ class RPiCameraData : public CameraData
> > public:
> > RPiCameraData(PipelineHandler *pipe)
> > : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> > - dropFrame_(false), ispOutputCount_(0)
> > + dropFrameCount_(0), ispOutputCount_(0)
> > {
> > }
> >
> > @@ -181,14 +181,15 @@ public:
> > std::queue<FrameBuffer *> embeddedQueue_;
> > std::deque<Request *> requestQueue_;
> >
> > + unsigned int dropFrameCount_;
> > +
> > private:
> > void checkRequestCompleted();
> > void tryRunPipeline();
> > void tryFlushQueues();
> > FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> >
> > - bool dropFrame_;
> > - int ispOutputCount_;
> > + unsigned int ispOutputCount_;
> > };
> >
> > class RPiCameraConfiguration : public CameraConfiguration
> > @@ -999,6 +1000,7 @@ int RPiCameraData::configureIPA()
> > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > &result);
> >
> > + unsigned int resultIdx = 0;
> > if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> > /*
> > * Setup our staggered control writer with the sensor default
> > @@ -1006,9 +1008,9 @@ int RPiCameraData::configureIPA()
> > */
> > if (!staggeredCtrl_) {
> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > - { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> > - { V4L2_CID_EXPOSURE, result.data[1] } });
> > - sensorMetadata_ = result.data[2];
> > + { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> > + { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> > + sensorMetadata_ = result.data[resultIdx++];
> > }
> >
> > /* Configure the H/V flip controls based on the sensor rotation. */
> > @@ -1025,6 +1027,11 @@ int RPiCameraData::configureIPA()
> > LOG(RPI, Error) << "V4L2 staggered set failed";
> > }
> >
> > + if (result.operation & RPI_IPA_CONFIG_DROP_FRAMES) {
> > + /* Configure the number of dropped frames required on startup. */
> > + dropFrameCount_ = result.data[resultIdx++];
>
> I assume given this is about dropping frames on startup, it only occurs
> once, and dropFrameCount_ is already zero at this point, so we're not
> losing a previous request to drop frames for instance.
That's correct. We now only signal how many frames to drop at
startup, and this signal will not make us lose a previous drop frame
request.
>
>
>
>
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1075,7 +1082,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > break;
> > }
> >
> > - case RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME:
> > case RPI_IPA_ACTION_RUN_ISP: {
> > unsigned int bufferId = action.data[0];
> > FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> > @@ -1084,7 +1090,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > << ", timestamp: " << buffer->metadata().timestamp;
> >
> > isp_[Isp::Input].dev()->queueBuffer(buffer);
> > - dropFrame_ = (action.operation == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;
> > ispOutputCount_ = 0;
> > break;
> > }
> > @@ -1250,7 +1255,7 @@ void RPiCameraData::clearIncompleteRequests()
> > void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
> > {
> > if (stream->isExternal()) {
> > - if (!dropFrame_) {
> > + if (!dropFrameCount_) {
> > Request *request = buffer->request();
> > pipe_->completeBuffer(camera_, request, buffer);
> > }
> > @@ -1262,7 +1267,7 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream
> > * simply memcpy to the Request buffer and requeue back to the
> > * device.
> > */
> > - if (stream == &unicam_[Unicam::Image] && !dropFrame_) {
> > + if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> > const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> > Request *request = requestQueue_.front();
> > FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> > @@ -1307,7 +1312,7 @@ void RPiCameraData::checkRequestCompleted()
> > * If we are dropping this frame, do not touch the request, simply
> > * change the state to IDLE when ready.
> > */
> > - if (!dropFrame_) {
> > + if (!dropFrameCount_) {
> > Request *request = requestQueue_.front();
> > if (request->hasPendingBuffers())
> > return;
> > @@ -1326,10 +1331,13 @@ void RPiCameraData::checkRequestCompleted()
> > * frame.
> > */
> > if (state_ == State::IpaComplete &&
> > - ((ispOutputCount_ == 3 && dropFrame_) || requestCompleted)) {
> > + ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {
> > state_ = State::Idle;
> > - if (dropFrame_)
> > - LOG(RPI, Info) << "Dropping frame at the request of the IPA";
> > + if (dropFrameCount_) {
> > + dropFrameCount_--;
> > + LOG(RPI, Info) << "Dropping frame at the request of the IPA ("
> > + << dropFrameCount_ << " left)";
> > + }
> > }
> > }
> >
> >
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list