[libcamera-devel] [PATCH v3 9/9] ipu3: Fixes frame delay

[Cheng-Hao Yang]

Hi Umang,

On Wed, Jul 27, 2022 at 9:25 PM Umang Jain <umang.jain at ideasonboard.com>
wrote:

> Hello,
>
> On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:
> > Like the shipped ipu3 HAL, handle the frame delay with one extra input,
>
>
> Can you point to the shipped ipu3 HAL frame delay handling code here for
> reference?
>
>
Sure. Added.


> > needed for the first frame and StillCapture's non-sequential frame
> > requests.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >   src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++++++-----
> >   1 file changed, 62 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index e26c2736..8689cf8b 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -90,6 +90,9 @@ public:
> >
> >       ControlInfoMap ipaControls_;
> >
> > +     bool firstRequest_ = true;
> > +     int lastRequestSequence_ = -1;
>
>
> this is only used under hasCapture so maybe lastCaptureRequestSeq_ is
> more appropriate?
>
>
Right, thanks!


> > +
> >   private:
> >       void metadataReady(unsigned int id, const ControlList &metadata);
> >       void paramsBufferReady(unsigned int id);
> > @@ -565,6 +568,9 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> CameraConfiguration *c)
> >       CIO2Device *cio2 = &data->cio2_;
> >       V4L2DeviceFormat outputFormat;
> >
> > +     data->firstRequest_ = true;
> > +     data->lastRequestSequence_ = -1;
> > +
>
>
> Does this needs to be reset on every start()/stop() cycles?
>
>
Ah, right. Resetting them when start() should be enough.


> >       ImgUDevice::PipeConfig imguConfig1 = config->imguConfig1();
> >       useImgu1_ = !imguConfig1.isNull();
> >
> > @@ -1427,6 +1433,11 @@ void IPU3CameraData::paramsBufferReady(unsigned
> int id)
> >       if (!info)
> >               return;
> >
> > +     int yuvCount = firstRequest_ ? 2 : 1;
> > +     int stillCount = firstRequest_ || (lastRequestSequence_ + 1) !=
> static_cast<int>(info->request->sequence()) ? 2 : 1;
> > +
>
>
> This is getting a bit non-trivial to follow. Can you probably include a
> brief comment here to explain some rationale?
>
> As per my understanding, if the lastRequestSequence_ was a StillCapture,
> stillCount will be 1 for current request capture, and it will 2 for all
> other cases?
>
> We need stillCount to be 2 (usually) because .... ?
>
>
If it's the first request, or the StillCapture request doesn't come
sequentially, we'll need to queue the buffers twice (i.e. |stillCount|
being 2) to mitigate the frame delay.
Added a comment in the code. Please check :)


> > +     firstRequest_ = false;
> > +
> >       bool hasYuv = false;
> >       bool hasCapture = false;
> >       /* Queue all buffers from the request aimed for the ImgU. */
> > @@ -1436,33 +1447,53 @@ void IPU3CameraData::paramsBufferReady(unsigned
> int id)
> >
> >               if (stream == &outStream_) {
> >                       hasYuv = true;
> > -                     imgu0_->output_->queueBuffer(outbuffer);
> > +
> > +                     for (int i = 0; i < yuvCount; ++i) {
> > +                             bufferReturnCounters[outbuffer] += 1;
> > +                             imgu0_->output_->queueBuffer(outbuffer);
>
>
> Have you checked / discussed what happens if you re-queue the same
> buffer on videodevice twice (before even dequeuing it)? I am not sure,
> because the current documentation doesn't specify this case I think.
>
> I've asked around.
>
>
Yeah, I've tried it on soraka and discussed it with Han-lin & Laurent &
Tomasz. It's also the way that the shipped HAL is handling the frame delay.
Please check :)


> > +                     }
> >               } else if (stream == &vfStream_) {
> >                       hasYuv = true;
> > -                     imgu0_->viewfinder_->queueBuffer(outbuffer);
> > +
> > +                     for (int i = 0; i < yuvCount; ++i) {
> > +                             bufferReturnCounters[outbuffer] += 1;
> > +
>  imgu0_->viewfinder_->queueBuffer(outbuffer);
> > +                     }
> >               } else if (stream == &outCaptureStream_) {
> >                       hasCapture = true;
> >
> > -                     imgu1_->output_->queueBuffer(outbuffer);
> > +                     for (int i = 0; i < stillCount; ++i) {
> > +                             bufferReturnCounters[outbuffer] += 1;
> > +                             imgu1_->output_->queueBuffer(outbuffer);
> > +                     }
> >               }
> >       }
> >
> >       if (hasYuv) {
> > -             bufferReturnCounters[info->rawBuffer] += 1;
> > -
> > -             imgu0_->param_->queueBuffer(info->paramBuffer);
> > -             imgu0_->stat_->queueBuffer(info->statBuffer);
> > -             imgu0_->input_->queueBuffer(info->rawBuffer);
> > +             for (int i = 0; i < yuvCount; ++i) {
> > +                     bufferReturnCounters[info->paramBuffer] += 1;
> > +                     bufferReturnCounters[info->statBuffer] += 1;
> > +                     bufferReturnCounters[info->rawBuffer] += 1;
> > +
> > +                     imgu0_->param_->queueBuffer(info->paramBuffer);
> > +                     imgu0_->stat_->queueBuffer(info->statBuffer);
> > +                     imgu0_->input_->queueBuffer(info->rawBuffer);
> > +             }
> >       } else {
> >               info->paramDequeued = true;
> >               info->metadataProcessed = true;
> >       }
> >
> >       if (hasCapture) {
> > -             bufferReturnCounters[info->rawBuffer] += 1;
> > +             lastRequestSequence_ = info->request->sequence();
> >
> > -             imgu1_->param_->queueBuffer(info->captureParamBuffer);
> > -             imgu1_->input_->queueBuffer(info->rawBuffer);
> > +             for (int i = 0; i < stillCount; ++i) {
> > +                     bufferReturnCounters[info->captureParamBuffer] +=
> 1;
> > +                     bufferReturnCounters[info->rawBuffer] += 1;
> > +
> > +
>  imgu1_->param_->queueBuffer(info->captureParamBuffer);
> > +                     imgu1_->input_->queueBuffer(info->rawBuffer);
> > +             }
> >       } else {
> >               info->captureParamDequeued = true;
> >       }
> > @@ -1503,6 +1534,11 @@ void IPU3CameraData::tryReturnBuffer(FrameBuffer
> *buffer)
> >    */
> >   void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >   {
> > +     if (--bufferReturnCounters[buffer] > 0)
> > +             return;
> > +
> > +     bufferReturnCounters.erase(buffer);
> > +
> >       IPU3Frames::Info *info = frameInfos_.find(buffer);
> >       if (!info)
> >               return;
> > @@ -1569,6 +1605,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer
> *buffer)
> >
> >   void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> >   {
> > +     if (--bufferReturnCounters[buffer] > 0)
> > +             return;
> > +
> > +     bufferReturnCounters.erase(buffer);
> > +
> >       IPU3Frames::Info *info = frameInfos_.find(buffer);
> >       if (!info)
> >               return;
> > @@ -1589,6 +1630,11 @@ void IPU3CameraData::paramBufferReady(FrameBuffer
> *buffer)
> >
> >   void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer)
> >   {
> > +     if (--bufferReturnCounters[buffer] > 0)
> > +             return;
> > +
> > +     bufferReturnCounters.erase(buffer);
> > +
> >       IPU3Frames::Info *info = frameInfos_.find(buffer);
> >       if (!info)
> >               return;
> > @@ -1609,6 +1655,11 @@ void
> IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer)
> >
> >   void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >   {
> > +     if (--bufferReturnCounters[buffer] > 0)
> > +             return;
> > +
> > +     bufferReturnCounters.erase(buffer);
> > +
> >       IPU3Frames::Info *info = frameInfos_.find(buffer);
> >       if (!info)
> >               return;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220802/e811148d/attachment.htm>