[PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Feb 13 16:09:57 CET 2024
Hi Kieran
On Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-02-13 11:31:31)
> > Hi Dan
> >
> > On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:
> > > The rkisp1 driver uses a couple of different methods of tracking the
> > > number of the frame - standardise them on request->sequence() and
> > > remove the frame_ member of RkISP1CameraData which was being used to
> > > do the job.
> >
> > Request::Private::sequence_ gets assigned to
> > Camera::Private::requestSequence_ by the PipelineHandler base class.
> >
> > Camera::Private::requestSequence_ gets zeroed in
> > PipelineHandler::stop()
> >
> > Camera::Private::requestSequence_ gets incremented in
> > PipelineHandler::doQueueRequest()
> >
> > So I think this change is legit (and should be done on IPU3 as well if
> > it uses the same scheme that was implemented here)
>
> I agree there, if this series works well, then we should handle both
> IPU3 and RKISP1 together to keep things aligned, and do the same
> cleanups on both sides.
>
>
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > >
> > > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > > ---
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------
> > > 1 file changed, 10 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 5460dc11..22e553fe 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private
> > > public:
> > > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
> > > RkISP1SelfPath *selfPath)
> > > - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),
> > > + : Camera::Private(pipe), frameInfo_(pipe),
> > > mainPath_(mainPath), selfPath_(selfPath)
> > > {
> > > }
> > > @@ -99,7 +99,6 @@ public:
> > > Stream selfPathStream_;
> > > std::unique_ptr<CameraSensor> sensor_;
> > > std::unique_ptr<DelayedControls> delayedCtrls_;
> > > - unsigned int frame_;
> > > std::vector<IPABuffer> ipaBuffers_;
> > > RkISP1Frames frameInfo_;
> > >
> > > @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> > > RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,
> > > bool isRaw)
> > > {
> > > - unsigned int frame = data->frame_;
> > > + unsigned int frame = request->sequence();
>
> This worries me a little when we start thinking about what might happen
> if frames are dropped/application slows down.
>
> We probably need some tests in lc-compliance to validate what happens if
> the applciation *doesn't* keep up with the camera. I think before that
> would make the
>
make the ? :)
Anyway, I'm failing to see why this patch changes the model when it
comes to feeding the pipeline from the application.
Before this patch data->frame_ was increased at queueRequestDevice() time
and request->sequence_, which is used now, is similarly incremented by
the pipeline handler base class before calling queueRequestDevice().
The pipeline feeds one parameters buffer to the IPA in
queueRequestDevice() (aka when application queues a request), and once
the IPA has filled in the parameters it calls
RkISP1CameraData::paramFilled() which queues buffers to the capture
video devices.
If the application does not queue any request it is then my
understanding that the IPA doesn't get triggered and doesn't do any
processing.
The only event that happens asynchronously from the request queueing
is the frameStart event, on which we poke at delayed controls but do
not actually trigger any IPA processing. This has not changed as far
as I can tell..
> > >
> > > FrameBuffer *paramBuffer = nullptr;
> > > FrameBuffer *statBuffer = nullptr;
> > > @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> > >
> > > statBuffer = pipe_->availableStatBuffers_.front();
> > > pipe_->availableStatBuffers_.pop();
> > > + statBuffer->_d()->setRequest(request);
> > > }
> > >
> > > FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> > > @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> > > return ret;
> > > }
> > >
> > > - data->frame_ = 0;
> > > -
> > > if (!isRaw_) {
> > > ret = param_->streamOn();
> > > if (ret) {
> > > @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> > > if (!info)
> > > return -ENOENT;
> > >
> > > - data->ipa_->queueRequest(data->frame_, request->controls());
> > > + data->ipa_->queueRequest(request->sequence(), request->controls());
> > > if (isRaw_) {
> > > if (info->mainPathBuffer)
> > > data->mainPath_->queueBuffer(info->mainPathBuffer);
> > > @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> > > if (data->selfPath_ && info->selfPathBuffer)
> > > data->selfPath_->queueBuffer(info->selfPathBuffer);
> > > } else {
> > > - data->ipa_->fillParamsBuffer(data->frame_,
> > > + data->ipa_->fillParamsBuffer(request->sequence(),
> > > info->paramBuffer->cookie());
> > > }
> > >
> > > - data->frame_++;
> > > -
> > > return 0;
> > > }
> > >
> > > @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> > > if (isRaw_) {
> > > const ControlList &ctrls =
> > > data->delayedCtrls_->get(metadata.sequence);
> > > - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
> > > + data->ipa_->processStatsBuffer(request->sequence(),
> > > + 0, ctrls);
> > > }
> > > } else {
> > > if (isRaw_)
> > > @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > > {
> > > ASSERT(activeCamera_);
> > > RkISP1CameraData *data = cameraData(activeCamera_);
> > > + Request *request = buffer->request();
> > >
> > > RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> > > if (!info)
> > > @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > > return;
> > > }
> > >
> > > - if (data->frame_ <= buffer->metadata().sequence)
> > > - data->frame_ = buffer->metadata().sequence + 1;
>
> for instance, it looks like there is some handling here that is tracking
> data->frame_ to match the sequence number from the image pipelines.
>
It's rather easy to track this condition, just save frames to disk
when capturing with 'cam' this makes the application stall long enough
to have two (or more) stats buffers being dequeued in between two
capture requests being submitted.
> Now the sequencing is all based around the request sequences queued.
> That might be all fine, but I'm weary about how we might need to keep
> things aligned somewhere to the real sequence number from the camera.
> Especially when it comes to tracking what controls were trully applied
> through DelayedControls etc.
>
And that could actually be the real issue here.
We now do
data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),
data->delayedCtrls_->get(request->sequence()));
While before we had
data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
data->delayedCtrls_->get(buffer->metadata().sequence));
So yes, it seems to me in this new method are not counting 'frames'
anymore, which is something we should indeed be doing mostly to
synchronize the sensor and the ISP control values...
In fact, I see different behaviours between current master and this
series. I'm testing capturing 100 frames to disk (hence triggering
some stalls). With master I see the image being visibile for the first
frames, then getting very dark then stabilizing. With this series the
image starts all black, then stabilizes. I've not yet found out why or
even if it related, just leaving it here for Dan to see if he had
noticed anything similar in his testing..
>
>
> > > -
> > > - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
> > > - data->delayedCtrls_->get(buffer->metadata().sequence));
> > > + data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),
> > > + data->delayedCtrls_->get(request->sequence()));
> > > }
> > >
> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
> > > --
> > > 2.34.1
> > >
More information about the libcamera-devel
mailing list