[libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report sensor timestamp

Naushir Patuck naush at raspberrypi.com
Wed Apr 21 12:19:13 CEST 2021 

On Wed, 21 Apr 2021 at 10:44, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hello,
>
> On Wed, Apr 21, 2021 at 10:14:57AM +0100, Naushir Patuck wrote:
> > On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote:
> > > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:
> > > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:
> > > > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:
> > > > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:
> > > > > > > Report the sensor's timestamp in the Request metadata by using
> the
> > > > > > > Unicam::Image buffer timestamp as an initial approximation.
> > > > > > >
> > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and
> it does not
> > > > > > > theoretically matches the 'start of exposure' definition, but
> when used
> > > > > > > to compare two consecutive frames it gives an acceptable
> estimation of
> > > > > > > the sensor frame period duration.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > > ---
> > > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12
> ++++++++++++
> > > > > > >  1 file changed, 12 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > index d1902bfc3393..7dc92acf3c4f 100644
> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > @@ -1415,6 +1415,18 @@ void
> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > > > > > >                         << ", timestamp: " <<
> buffer->metadata().timestamp;
> > > > > > >
> > > > > > >         if (stream == &unicam_[Unicam::Image]) {
> > > > > > > +               /*
> > > > > > > +                * Record the sensor timestamp in the Request.
> > > > > > > +                *
> > > > > > > +                * \todo The sensor timestamp should be better
> estimated by
> > > > > > > +                * sampling the V4L2Device::frameStart signal.
> > > > > > > +                */
> > > > > >
> > > > > > Just a minor correction to this comment - the VB2 buffer
> timestamp that
> > > > > > is provided by the device driver is actually sampled at the CSI2
> frame start
> > > > > > event, so is pretty much as close to the start of exposure as we
> can get
> > > > > > without dead reckoning.  This sample is also before the DMA to
> memory
> > > > > > and V4L2Device::frameStart signal.
> > > >
> > > > I wish more devices and drivers would do so :-)
> > > >
> > > > > Ah you see! That's great, I wrongly assumed the raw buffer
> timestamp
> > > > > was sampled at DMA transfer time end, as it happens on IPU3. It's
> > > > > great if I could drop that todo item!
> > > > >
> > > > > > Other than that,
> > > > > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > >
> > > > > > > +               Request *request = requestQueue_.front();
> > > >
> > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete
> here
> > > > while the ISP is busy processing the previous frame, wouldn't
> > > > requestQueue_.front() then correspond to the frame processed by the
> ISP
> > > > ? Maybe you could use buffer->request() instead (assuming the RPi
> > > > pipeline handler calls setRequest()).
> >
> > Quite right, because of the queueing, there is a possibility that the
> top of
> > the request queue may lag behind and will could return out a wrong
> > timestamp with the above.
> >
> > > This has required me quite some mumbling, as I was a bit confused too
> > > on how better associate a buffer with a request in this pipeline.
> > >
> > > So, RPi does not use anything similar to FrameInfo, which I think we
> > > should bring to the core framework level soon, and have ph opt-in.
> > > It does not call setRequest() on buffers, and I failed to identify
> > > where to do so, given the ph design. The rest of the ph code when
> > > it has to retrieve the Request * goes and assume it's the first one
> > > (ie the oldest one) it's the one to chose. I have been wondering too
> > > if that's correct. Maybe I got lost into something trivial, Naush do
> > > you have opinions to share ?
> >
> > We don't use FrameInfo like the other pipeline handlers, but we do have
> > a similar mechanism to store frame controls with a buffer.
> >
> > Coincidentally, I do have a patch that stores the timestamp for my IPA
> > rate-control series [1].  This almost does what we need, the only thing
> > missing is some code to transfer the timestamp from this ControlList
> > to the request metadata.
> >
> > To get this change, we can do one of the following:
> >
> > 1) I can backport part of the change in [1] that we care about here, and
> > add code to transfer to the request metadata.  I can either provide a
> patch
> > to add to this series or provide it on top of this series.
> >
> > 2) I can make this change in my patch series fairly easily as well.
> Given this
> > seems to be related to Android, I doubt anybody would miss this feature
> > from the Raspberry Pi platform until that series is merged.
>
> The timestamp metadata isn't limited to Android support, but nobody else
> uses it now, so the risk of breakage is fairly limited. Jacopo's series
> is nearly ready to get merged so I would avoid delaying it if possible.
> Jacopo, how about keeping this patch as-is with a \todo comment to tell
> that the front of the queue may not be the right buffer, and fixing it
> on top once both this series and Naush's series get merged ? Naush,
> would you be able to then submit a patch on top of the combination of
> both ? I'll review v5 of the rate limitation now.
>

Sounds good, we fix this up once Jacopo's series and my rate-control have
been merged.  Note that my series is rebased on top of David's work at:
https://patchwork.libcamera.org/patch/11730/)

so I will also have to wait for that to be submitted.

Regards,
Naush



>
> > Let me know what you think.
> >
> > [1] https://patchwork.libcamera.org/patch/12006/
> >
> > > > > > > +               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.
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210421/6581fdd2/attachment.htm>

More information about the libcamera-devel mailing list