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

[Nicolas Dufresne]

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