[libcamera-devel] [PATCH 2/8] libcamera: pipeline: Set the Sensor sequence number for all pipelines

[Umang Jain]

Hi Kieran,

On 12/7/21 5:09 AM, Kieran Bingham wrote:
> The SensorSequence metadata provides the sequence number associated with
> the image capture directly.
>
> While we already report sequence numbers in the metadata of the buffers,
> this can lead to mis-leading parsing of those sequence numbers which may
> be purely monotonic sequences from an ISP, and not represent the true
> sequence counts from a capture device.
>
> Use the newly added SensorSequence metadata control and populate
> completed requests with the sequence number from the appropriate stream.
> For ISP based devices, such as the IPU3, RPi, and RkISP1, this comes
> from the CSI2 receiver capture device, while for the Simple pipeline and
> the UVC Video pipeline, this comes from the single video capture device.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>   src/libcamera/pipeline/ipu3/ipu3.cpp               |  4 +++-
>   src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  3 +++
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
>   src/libcamera/pipeline/simple/simple.cpp           | 12 ++++++++----
>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       |  2 ++
>   5 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 313220624aff..a7f35840afbb 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1372,13 +1372,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>   	}
>   
>   	/*
> -	 * Record the sensor's timestamp in the request metadata.
> +	 * Record the sensor's timestamp and sequence in the request metadata.
>   	 *
>   	 * \todo The sensor timestamp should be better estimated by connecting
>   	 * to the V4L2Device::frameStart signal.
>   	 */
>   	request->metadata().set(controls::SensorTimestamp,
>   				buffer->metadata().timestamp);
> +	request->metadata().set(controls::SensorSequence,
> +				buffer->metadata().sequence);


I vaguely remember looking into consolidating SensorTimestamp with 
frameStart signal(turned out to be non-trivial), as mentioned in the 
\todo above.

Does sequence number also  seems a good candidate to be emitted from the 
signal, from that angle? If yes, I would add that in the \todo as well.

For the patch

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>


>   
>   	info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);
>   
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 321b72adbbf7..f458587a8920 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1549,6 +1549,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>   		 * as it does not receive the FrameBuffer object.
>   		 */
>   		ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp);
> +		ctrl.set(controls::SensorSequence, buffer->metadata().sequence);
>   		bayerQueue_.push({ buffer, std::move(ctrl) });
>   	} else {
>   		embeddedQueue_.push(buffer);
> @@ -1786,6 +1787,8 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
>   {
>   	request->metadata().set(controls::SensorTimestamp,
>   				bufferControls.get(controls::SensorTimestamp));
> +	request->metadata().set(controls::SensorSequence,
> +				bufferControls.get(controls::SensorSequence));
>   
>   	request->metadata().set(controls::ScalerCrop, scalerCrop_);
>   }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 36ef6a02ae90..27592e473c14 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1077,13 +1077,15 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>   	Request *request = buffer->request();
>   
>   	/*
> -	 * Record the sensor's timestamp in the request metadata.
> +	 * Record the sensor's timestamp and sequence in the request metadata.
>   	 *
>   	 * \todo The sensor timestamp should be better estimated by connecting
>   	 * to the V4L2Device::frameStart signal.
>   	 */
>   	request->metadata().set(controls::SensorTimestamp,
>   				buffer->metadata().timestamp);
> +	request->metadata().set(controls::SensorSequence,
> +				buffer->metadata().sequence);
>   
>   	completeBuffer(request, buffer);
>   	tryCompleteRequest(request);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 701fb4be0b71..4c5d9c0b9919 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -667,9 +667,10 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>   	}
>   
>   	/*
> -	 * Record the sensor's timestamp in the request metadata. The request
> -	 * needs to be obtained from the user-facing buffer, as internal
> -	 * buffers are free-wheeling and have no request associated with them.
> +	 * Record the sensor's timestamp and sequence in the request metadata.
> +	 * The request needs to be obtained from the user-facing buffer, as
> +	 * internal buffers are free-wheeling and have no request associated
> +	 * with them.
>   	 *
>   	 * \todo The sensor timestamp should be better estimated by connecting
>   	 * to the V4L2Device::frameStart signal if the platform provides it.
> @@ -686,9 +687,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>   		}
>   	}
>   
> -	if (request)
> +	if (request) {
>   		request->metadata().set(controls::SensorTimestamp,
>   					buffer->metadata().timestamp);
> +		request->metadata().set(controls::SensorSequence,
> +					buffer->metadata().sequence);
> +	}
>   
>   	/*
>   	 * Queue the captured and the request buffer to the converter if format
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 264f5370cf32..f5fab3b69e6d 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -666,6 +666,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)
>   	/* \todo Use the UVC metadata to calculate a more precise timestamp */
>   	request->metadata().set(controls::SensorTimestamp,
>   				buffer->metadata().timestamp);
> +	request->metadata().set(controls::SensorSequence,
> +				buffer->metadata().sequence);
>   
>   	pipe()->completeBuffer(request, buffer);
>   	pipe()->completeRequest(request);