[libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather than buffer metadata

Thu Dec 9 10:23:03 CET 2021

Hi all,

On Wed, 8 Dec 2021 at 21:13, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Kieran,
>
> On Wed, Dec 08, 2021 at 08:59:29PM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2021-12-08 18:57:10)
> > > On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:
> > > > On 12/7/21 6:51 PM, Umang Jain wrote:
> > > > > On 12/7/21 5:39 AM, Laurent Pinchart wrote:
> > > > >> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:
> > > > >>> The SensorTimestamp is defined to be the time of capture of the
> image.
> > > > >>> While all streams should have the same timestamp, this is not
> always
> > > > >>> defined or guaranteed as ISP drivers may not forward sequence
> numbers
> > > > >>> and timestamps from their input buffer.
> > > > >>
> > > > >> That should then bo solved by the pipeline handler, which should
> store
> > > > >> the correct timestamp in the buffer metadata.
> > > > >>
> > > > >>> Use the Request metadata to get the SensorTimestamp which must be
> > > > >>> set by the pipeline handlers according to the data from the
> capture
> > > > >>> device.
> > > > >>>
> > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > >>> ---
> > > > >>>   src/cam/camera_session.cpp | 7 ++++---
> > > > >>>   1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >>>
> > > > >>> diff --git a/src/cam/camera_session.cpp
> b/src/cam/camera_session.cpp
> > > > >>> index 1bf460fa3fb7..50170723c30f 100644
> > > > >>> --- a/src/cam/camera_session.cpp
> > > > >>> +++ b/src/cam/camera_session.cpp
> > > > >>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request
> > > > >>> *request)
> > > > >>>       const Request::BufferMap &buffers = request->buffers();
> > > > >>>         /*
> > > > >>> -     * Compute the frame rate. The timestamp is arbitrarily
> retrieved from
> > > > >>> -     * the first buffer, as all buffers should have matching
> timestamps.
> > > > >>> +     * Compute the frame rate. The timestamp is retrieved from
> the
> > > > >>> +     * SensorTimestamp property, though all streams should have
> the
> > > > >>> +     * same timestamp.
> > > > >>>        */
> > > > >>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;
> > > > >>> +    uint64_t ts =
> request->metadata().get(controls::SensorTimestamp);
> > > > >>
> > > > >> This seems reasonable. Why do we have timestamps in the buffer
> metadata
> > > > >> ? :-)
> > > >
> > > > Strong chance I have mis-understood the question, I later realized
> this
> > > > is a cam-related patch.
> > > >
> > > > to me, the question translated :
> > > >
> > > >      why do we have timestamps in the
> FrameBuffer.metadata_.timestamp ?
> > >
> > > To clarify, I meant to ask why we have both controls::SensorTimestamp
> > > *and* timestamps in individual buffers
> > > (FrameBuffer::metadata_.timestamp). I suppose it's historical, the
> > > timestamp member comes from
> > >
> > > commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2
> > > Author: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > Date:   Thu Jan 24 23:34:51 2019 +0100
> > >
> > >     libcamera: v4l2_device: Update dequeued buffer information
> > >
> > > while controls::SensorTimestamp has been introduced in
> > >
> > > commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d
> > > Author: Jacopo Mondi <jacopo at jmondi.org>
> > > Date:   Thu Jul 30 15:18:50 2020 +0200
> > >
> > >     libcamera: control_ids: Define draft controls
> > >
> > > > So ignore the discussion (if you want) :-P
> > > >
> > > > > Because there is no buffer assigned on the time-point where we
> want to
> > > > > capture the timestamp.
> > > > >
> > > > > The exact location of the timestamp is a \todo
> > > > >
> > > > >              * \todo The sensor timestamp should be better
> estimated
> > > > > by connecting
> > > > >              * to the V4L2Device::frameStart signal.
> > > > >
> > > > > in all the pipeline-handlers as of now.
> > > > >
> > > > > We *want* to capture at frameStart signal, which emits in response
> to
> > > > > VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)
> > > > >
> > > > > We probably need to assign a container taking care of timestamps
> with
> > > > > sequence numbers until a buffer is assigned down-the-line and then
> set
> > > > > the buffer metadata by reading seq# & timestamp from that
> container.
> > >
> > > Should we just drop FrameBuffer::metadata_.timestamp in favour of
> > > controls::SensorTimestamp ? What would be the drawbacks, if any ?
> >
> > Drawbacks for using Metadata control:
> >  - Have to parse the metadata control list to get timestamps
> >    (/sequences)
>
> The request metadata is also not available before the request fully
> completes, while buffers will complete individually before. This is
> something we may want to address in a generic way though, I suspect
> there will be use cases for providing request metadata early.
>
> > Pro's for Metadata control:
> >  - Only one place for it, all buffers in the same request are defined as
> >    having the same value (sequence and timestamp).
>
> That's the part I like :-)
>
> > Drawbacks for per-buffer metadata:
> >  - Must be explictily assigned to keep them in sync even though they are
> >    the output of multiple video devices perhaps.
> >
> > Pro's for for per-buffer metadata:
> >  - Quicker to retrieve the values? (defined structure?)
>
> Possibly a bit quicker, but that seems marginal to me.
>
> > Any more on either side?
> >
> >
> > Personally, I'm currently thinking these are better defined by the
> > metadata() control list. After all, that's the purpose of the list
> > right?
>
> I think so. I see very little value today in providing buffer completion
> timestamps that would be different than the sensor timestamp (this could
> possibly help measuring the processing time, but if that's the goal, it
> could also be done by capturing the system timestamp in the buffer
> completion handler on the application side, which would provide
> statistics about the full processing time, including both the hardware
> processing and the software processing).
>

The above reason was exactly my rationale for keeping both :-)
However, it's not something we rely on, or even use really, so
I am happy to get rid of the buffer timestamps.

Naush

>
> Let's wait for more feedback, but if there's a general agreement that
> this is the direction we want to take, we could remove
> FrameBuffer::metadata_.timestamp independently of addressing the sensor
> sequence issue.
>
> > > > >>>       double fps = ts - last_;
> > > > >>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;
> > > > >>>       last_ = ts;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211209/56c02cac/attachment-0001.htm>