| Message ID | 20200227200407.490616-24-nicolas.dufresne@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + 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@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); > } >
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@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@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); > > } > >
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@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@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); > > > } > > >
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. */ + GstClockTime sys_now = g_get_monotonic_time() * 1000; + + /* Deduced from: sys_now - sys_base_time == gst_now - gst_bae_time */ + 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); }
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@collabora.com> --- src/gstreamer/gstlibcamerapad.cpp | 23 +++++++++++++++++++++++ src/gstreamer/gstlibcamerapad.h | 2 ++ src/gstreamer/gstlibcamerasrc.cpp | 20 ++++++++++++++++++++ 3 files changed, 45 insertions(+)