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

[Jacopo Mondi]

Hi Laurent,
   + Naush

On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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 <jacopo at jmondi.org> 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()).
>

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 ?

Thanks
   j

> > > > +               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