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

[Naushir Patuck]

Hi Jacopo and Laurent,

On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi <jacopo at jmondi.org> wrote:

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

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.

Let me know what you think.

Regards,
Naush


[1] https://patchwork.libcamera.org/patch/12006/



>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210421/6568d0a6/attachment-0001.htm>