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

[Laurent Pinchart]

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.

> 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