| Message ID | 20220623232210.18742-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit : > The buffer pts and the pad latency are computed from the framebuffer > timestamp, separately for each pad. Use the sensor timestamp provided > through the request metadata instead, to compute the values once outside > of the pads loop. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 34 ++++++++++++++++++++++--------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 700bee2bf877..a1fab71d4f09 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -34,6 +34,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > +#include <libcamera/control_ids.h> > > #include <gst/base/base.h> > > @@ -164,22 +165,35 @@ GstLibcameraSrcState::requestCompleted(Request *request) > return; > } > > + GstClockTime latency; > + GstClockTime pts; > + > + if (GST_ELEMENT_CLOCK(src_)) { > + int64_t timestamp = request->metadata().get(controls::SensorTimestamp); > + > + GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; > + GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_time */ > + GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); > + pts = timestamp - sys_base_time; > + latency = sys_now - timestamp; > + } else { > + latency = 0; > + pts = 0; I would like to suggest: pts = GST_CLOCK_TIME_NONE; > + } > + > for (GstPad *srcpad : srcpads_) { > Stream *stream = gst_libcamera_pad_get_stream(srcpad); > GstBuffer *buffer = wrap->detachBuffer(stream); > > FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > > - if (GST_ELEMENT_CLOCK(src_)) { > - GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; > - GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_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); > + if (pts) { And then: if (GST_CLOCK_TIME_IS_VALID(pts)) { Conceptually, pts can be 0 even if it will never happen in practice. I'm fine with the change otherwise. This entire block seems complex enough that it could be its own helper function if you feel like it. In long term, when we start supporting "desync" streams, we'll be requesting some pads independently, and then we'd start having per pad latency that do differ. Not sure why, but it looks like I prepared the field for bunch of future use cases, and I fail to add all the todos. > + GST_BUFFER_PTS(buffer) = pts; > + gst_libcamera_pad_set_latency(srcpad, latency); > } else { > GST_BUFFER_PTS(buffer) = 0; > }
Hi Nicolas, On Mon, Jun 27, 2022 at 05:00:19PM -0400, Nicolas Dufresne wrote: > Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit : > > The buffer pts and the pad latency are computed from the framebuffer > > timestamp, separately for each pad. Use the sensor timestamp provided > > through the request metadata instead, to compute the values once outside > > of the pads loop. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 34 ++++++++++++++++++++++--------- > > 1 file changed, 24 insertions(+), 10 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 700bee2bf877..a1fab71d4f09 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -34,6 +34,7 @@ > > > > #include <libcamera/camera.h> > > #include <libcamera/camera_manager.h> > > +#include <libcamera/control_ids.h> > > > > #include <gst/base/base.h> > > > > @@ -164,22 +165,35 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > return; > > } > > > > + GstClockTime latency; > > + GstClockTime pts; > > + > > + if (GST_ELEMENT_CLOCK(src_)) { > > + int64_t timestamp = request->metadata().get(controls::SensorTimestamp); > > + > > + GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; > > + GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_time */ > > + GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); > > + pts = timestamp - sys_base_time; > > + latency = sys_now - timestamp; > > + } else { > > + latency = 0; > > + pts = 0; > > I would like to suggest: > > pts = GST_CLOCK_TIME_NONE; > > > + } > > + > > for (GstPad *srcpad : srcpads_) { > > Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > GstBuffer *buffer = wrap->detachBuffer(stream); > > > > FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > > > > - if (GST_ELEMENT_CLOCK(src_)) { > > - GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; > > - GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_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); > > + if (pts) { > > And then: > if (GST_CLOCK_TIME_IS_VALID(pts)) { > > Conceptually, pts can be 0 even if it will never happen in practice. That was my reasoning too :-) I didn't know about GST_CLOCK_TIME_NONE and GST_CLOCK_TIME_IS_VALID, I'll use those. > I'm fine > with the change otherwise. This entire block seems complex enough that it could > be its own helper function if you feel like it. In long term, when we start > supporting "desync" streams, we'll be requesting some pads independently, and > then we'd start having per pad latency that do differ. Not sure why, but it > looks like I prepared the field for bunch of future use cases, and I fail to add > all the todos. That will be interesting to implement. > > + GST_BUFFER_PTS(buffer) = pts; > > + gst_libcamera_pad_set_latency(srcpad, latency); > > } else { > > GST_BUFFER_PTS(buffer) = 0; > > }
Hi Laurent, On 6/28/22 03:52, Laurent Pinchart via libcamera-devel wrote: > Hi Nicolas, > > On Mon, Jun 27, 2022 at 05:00:19PM -0400, Nicolas Dufresne wrote: >> Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit : >>> The buffer pts and the pad latency are computed from the framebuffer >>> timestamp, separately for each pad. Use the sensor timestamp provided >>> through the request metadata instead, to compute the values once outside >>> of the pads loop. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/gstreamer/gstlibcamerasrc.cpp | 34 ++++++++++++++++++++++--------- >>> 1 file changed, 24 insertions(+), 10 deletions(-) >>> >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp >>> index 700bee2bf877..a1fab71d4f09 100644 >>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>> @@ -34,6 +34,7 @@ >>> >>> #include <libcamera/camera.h> >>> #include <libcamera/camera_manager.h> >>> +#include <libcamera/control_ids.h> >>> >>> #include <gst/base/base.h> >>> >>> @@ -164,22 +165,35 @@ GstLibcameraSrcState::requestCompleted(Request *request) >>> return; >>> } >>> >>> + GstClockTime latency; >>> + GstClockTime pts; >>> + >>> + if (GST_ELEMENT_CLOCK(src_)) { >>> + int64_t timestamp = request->metadata().get(controls::SensorTimestamp); >>> + >>> + GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; >>> + GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_time */ >>> + GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); >>> + pts = timestamp - sys_base_time; >>> + latency = sys_now - timestamp; >>> + } else { >>> + latency = 0; >>> + pts = 0; >> I would like to suggest: >> >> pts = GST_CLOCK_TIME_NONE; >> >>> + } >>> + >>> for (GstPad *srcpad : srcpads_) { >>> Stream *stream = gst_libcamera_pad_get_stream(srcpad); >>> GstBuffer *buffer = wrap->detachBuffer(stream); >>> >>> FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); >>> >>> - if (GST_ELEMENT_CLOCK(src_)) { >>> - GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; >>> - GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_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); >>> + if (pts) { >> And then: >> if (GST_CLOCK_TIME_IS_VALID(pts)) { >> >> Conceptually, pts can be 0 even if it will never happen in practice. > That was my reasoning too :-) I didn't know about GST_CLOCK_TIME_NONE > and GST_CLOCK_TIME_IS_VALID, I'll use those. > >> I'm fine >> with the change otherwise. This entire block seems complex enough that it could >> be its own helper function if you feel like it. In long term, when we start Agreed. Can be done on top as well. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> supporting "desync" streams, we'll be requesting some pads independently, and >> then we'd start having per pad latency that do differ. Not sure why, but it >> looks like I prepared the field for bunch of future use cases, and I fail to add >> all the todos. > That will be interesting to implement. > >>> + GST_BUFFER_PTS(buffer) = pts; >>> + gst_libcamera_pad_set_latency(srcpad, latency); >>> } else { >>> GST_BUFFER_PTS(buffer) = 0; >>> }
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 700bee2bf877..a1fab71d4f09 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -34,6 +34,7 @@ #include <libcamera/camera.h> #include <libcamera/camera_manager.h> +#include <libcamera/control_ids.h> #include <gst/base/base.h> @@ -164,22 +165,35 @@ GstLibcameraSrcState::requestCompleted(Request *request) return; } + GstClockTime latency; + GstClockTime pts; + + if (GST_ELEMENT_CLOCK(src_)) { + int64_t timestamp = request->metadata().get(controls::SensorTimestamp); + + GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; + GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_time */ + GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); + pts = timestamp - sys_base_time; + latency = sys_now - timestamp; + } else { + latency = 0; + pts = 0; + } + for (GstPad *srcpad : srcpads_) { Stream *stream = gst_libcamera_pad_get_stream(srcpad); GstBuffer *buffer = wrap->detachBuffer(stream); FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); - if (GST_ELEMENT_CLOCK(src_)) { - GstClockTime gst_base_time = GST_ELEMENT(src_)->base_time; - GstClockTime gst_now = gst_clock_get_time(GST_ELEMENT_CLOCK(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_base_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); + if (pts) { + GST_BUFFER_PTS(buffer) = pts; + gst_libcamera_pad_set_latency(srcpad, latency); } else { GST_BUFFER_PTS(buffer) = 0; }
The buffer pts and the pad latency are computed from the framebuffer timestamp, separately for each pad. Use the sensor timestamp provided through the request metadata instead, to compute the values once outside of the pads loop. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 34 ++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 10 deletions(-)