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

[Laurent Pinchart]

Hi Nicolas,

On Sat, Feb 29, 2020 at 10:32:52AM -0500, Nicolas Dufresne wrote:
> Le samedi 29 février 2020 à 16:59 +0200, Laurent Pinchart a écrit :
> > 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).

I was thinking of getting V4L2 to produce BOOTTIME timestamps :-)

My question was whether the BOOTTIME clock would be appropriate for
GStreamer, or if there would be a clock that would be more suitable.

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

There are so many ways to get clock conversion wrong that I think it
should always be implemented in helpers, regardless of whether they're
KISS or very complex.

> > > +			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);
> > >  	}
> > >  

-- 
Regards,

Laurent Pinchart