[libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store timestamp in the correct Request metadata

[Naushir Patuck]

Hi Kieran,

Thank you for your feedback.

On Mon, 17 May 2021 at 13:40, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
> On 12/05/2021 09:47, Naushir Patuck wrote:
> > Write the controls::SensorTimestamp value in the Request metadata when
> > the request is popped from the queue ready to run the pipeline. This
> > ensures that the timestamp is written to the correct Request item,
> > which may not be at the top of the queue when the Unicam buffer dequeue
> > occurs.
> >
> > Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp")
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 29 +++++++++++--------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 6fbdba0487bf..eb6d31670567 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -221,6 +221,8 @@ public:
> >
> >  private:
> >       void checkRequestCompleted();
> > +     void fillRequestMetadata(const ControlList &bufferControls,
> > +                              Request *request);
> >       void tryRunPipeline();
> >       bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer
> *&embeddedBuffer);
> >
> > @@ -1416,18 +1418,6 @@ void
> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >                       << ", timestamp: " << buffer->metadata().timestamp;
> >
> >       if (stream == &unicam_[Unicam::Image]) {
> > -             /*
> > -              * Record the sensor timestamp in the Request.
> > -              *
> > -              * \todo Do not assume the request in the front of the
> queue
> > -              * is the correct one
> > -              */
> > -             Request *request = requestQueue_.front();
> > -             ASSERT(request);
> > -
> > -             request->metadata().set(controls::SensorTimestamp,
> > -                                     buffer->metadata().timestamp);
> > -
> >               /*
> >                * Lookup the sensor controls used for this frame sequence
> from
> >                * DelayedControl and queue them along with the frame
> buffer.
> > @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const
> ControlList &controls)
> >       }
> >  }
> >
> > +void RPiCameraData::fillRequestMetadata(const ControlList
> &bufferControls,
> > +                                     Request *request)
> > +{
> > +     request->metadata().set(controls::SensorTimestamp,
> > +
>  bufferControls.get(controls::SensorTimestamp));
> > +}
> > +
> >  void RPiCameraData::tryRunPipeline()
> >  {
> >       FrameBuffer *embeddedBuffer;
> > @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()
> >       /* See if a new ScalerCrop value needs to be applied. */
> >       applyScalerCrop(request->controls());
> >
> > +     /*
> > +      * Clear the request metadata and fill it with some initial non-IPA
> > +      * related controls. We clear it first because the request metadata
> > +      * may have been populated if we have dropped the previous frame.
> > +      */
>
> Aha, I was going to say is this the right place to clear this, and then
> I re-read that comment and now I understand why it's here.
>
> So I think this is fine.
>
> Is there anything else that would have to be cleared down if a request
> gets 're-used' internally?
>

I think that's it.... for now :-)


>
> I suspect not at the moment, but it may be that we need a specific call
> on a request to clean it up more generically.
>
> This should be fine though
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
> Hrm, thinking about it more, i think this comes down to the
> ControlList->merge() doesn't it ... if the entry is already there it
> won't be updated? I almost feel like in this case - it should be
> updated, because it's newer and more correct information...
>

Correct.  It's a consequence of doing the ControlList::merge() now.
I think  ControlList::merge() follows the convention of std::map::merge()
where it does not merge if duplicate keys exist between the two maps.

Regards,
Naush



>
>
>
> > +     request->metadata().clear();
> > +     fillRequestMetadata(bayerFrame.controls, request);
> > +
> >       /*
> >        * Process all the user controls by the IPA. Once this is
> complete, we
> >        * queue the ISP output buffer listed in the request to start the
> HW
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210517/8edb8d74/attachment-0001.htm>