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

[Laurent Pinchart]

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()).

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