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

[Umang Jain]

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


Agreed. Can be done on top as well.

Reviewed-by: Umang Jain <umang.jain at 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;
>>>   		}