[libcamera-devel] [PATCH 05/13] gstreamer: Move timestamp calculation out of pad loop

[Laurent Pinchart]

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 at 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;
> >  		}

-- 
Regards,

Laurent Pinchart