[libcamera-devel] [PATCH v2 23/27] gst: libcamerasrc: Implement timestamp support

[Nicolas Dufresne]

Le samedi 29 février 2020 à 16:59 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Thu, Feb 27, 2020 at 03:04:03PM -0500, Nicolas Dufresne wrote:
> > This is an experimental patch adding timestamp support to the libcamerasrc
> > element. This patch currently assume that the driver timestamp are relative to
> > the system monotonic clock. Without a reference clock source, the timestamp are
> > otherwise unusable, and without timestamp only minor use case can be achieved.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> >  src/gstreamer/gstlibcamerapad.cpp | 23 +++++++++++++++++++++++
> >  src/gstreamer/gstlibcamerapad.h   |  2 ++
> >  src/gstreamer/gstlibcamerasrc.cpp | 20 ++++++++++++++++++++
> >  3 files changed, 45 insertions(+)
> > 
> > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp
> > index 2cf1630..31d3edd 100644
> > --- a/src/gstreamer/gstlibcamerapad.cpp
> > +++ b/src/gstreamer/gstlibcamerapad.cpp
> > @@ -62,9 +62,24 @@ gst_libcamera_pad_get_property(GObject *object, guint prop_id, GValue *value,
> >  	}
> >  }
> >  
> > +static gboolean
> > +gst_libcamera_pad_query(GstPad *pad, GstObject *parent, GstQuery *query)
> > +{
> > +	auto *self = GST_LIBCAMERA_PAD(pad);
> > +
> > +	if (query->type != GST_QUERY_LATENCY)
> > +		return gst_pad_query_default(pad, parent, query);
> > +
> > +	/* TRUE here means live, we assumes that max latency is the same as min
> > +	 * as we have no idea that duration of frames. */
> 
> 	/*
> 	 * ...
> 	 */
> 
> > +	gst_query_set_latency(query, TRUE, self->latency, self->latency);
> > +	return TRUE;
> > +}
> > +
> >  static void
> >  gst_libcamera_pad_init(GstLibcameraPad *self)
> >  {
> > +	GST_PAD_QUERYFUNC(self) = gst_libcamera_pad_query;
> >  }
> >  
> >  static GType
> > @@ -172,3 +187,11 @@ gst_libcamera_pad_has_pending(GstPad *pad)
> >  	GLibLocker lock(GST_OBJECT(self));
> >  	return (self->pending_buffers.length > 0);
> >  }
> > +
> > +void
> > +gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency)
> > +{
> > +	auto *self = GST_LIBCAMERA_PAD(pad);
> > +	GLibLocker lock(GST_OBJECT(self));
> > +	self->latency = latency;
> > +}
> > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h
> > index eb24000..a3291e8 100644
> > --- a/src/gstreamer/gstlibcamerapad.h
> > +++ b/src/gstreamer/gstlibcamerapad.h
> > @@ -32,4 +32,6 @@ GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);
> >  
> >  bool gst_libcamera_pad_has_pending(GstPad *pad);
> >  
> > +void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency);
> > +
> >  #endif /* __GST_LIBCAMERA_PAD_H__ */
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 70f2048..73a1360 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -151,6 +151,26 @@ GstLibcameraSrcState::requestCompleted(Request *request)
> >  	for (GstPad *srcpad : this->srcpads) {
> >  		Stream *stream = gst_libcamera_pad_get_stream(srcpad);
> >  		buffer = wrap->detachBuffer(stream);
> > +
> > +		FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);
> > +
> > +		if (GST_ELEMENT_CLOCK(this->src)) {
> > +			GstClockTime gst_base_time = GST_ELEMENT(this->src)->base_time;
> > +			GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(this->src));
> > +			/* \todo Need to expose which reference clock the timestamp relates to. */
> 
> Something we need to do indeed. We're considering standardizing on
> CLOCK_BOOTTIME, would that be good for gstreamer ?

Can we do like DRM and just pass back the clock ID ? I've followed a
little bit the discussion and haven't found enough rationale to show
that BOOTTIME is the right approach. It felt very usecase specific and
to make things worst, drivers will have to support both for backward
compatibility reason.

As you can see, this code path is a userland translation from one clock
to another. This the type of thing you want to avoid like the peste in
fact, since it's really error prone due to scheduling latency. So
forcing BOOTIME would imply that we'd have two of these translation
going on per frame for V4L2 devices (at least in current state of v4l2
drivers).

That being said, I'll add a \todo if someone want to do future
enhancement, since we could try to use GstClock derivation mechanism.
It's a mechanism to which you pass samples of the match, and GstClock
will average and smooth a slope for derivation. Last time I tried, it
was a bit inconvenient to use and the result tend to be historic, hence
the reason I went for a KISS implementation. I would need to dig into
this 15 years old piece of code in GStreamer and find out what I'm
doing wrong, or what its doing wrong.

> 
> > +			GstClockTime sys_now = g_get_monotonic_time() * 1000;
> > +
> > +			/* Deduced from: sys_now - sys_base_time == gst_now - gst_bae_time */
> 
> s/gst_bae_time/gst_base_time/
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > +			GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time);
> > +			GST_BUFFER_PTS(buffer) = fb->metadata().timestamp - sys_base_time;
> > +			gst_libcamera_pad_set_latency(srcpad, sys_now - fb->metadata().timestamp);
> > +		} else {
> > +			GST_BUFFER_PTS(buffer) = 0;
> > +		}
> > +
> > +		GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence;
> > +		GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence;
> > +
> >  		gst_libcamera_pad_queue_buffer(srcpad, buffer);
> >  	}
> >  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200229/65af422b/attachment.sig>