[libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store timestamp in the correct Request metadata

[Kieran Bingham]

Hi Naush,

On 12/05/2021 09:47, Naushir Patuck wrote:
> Write the controls::SensorTimestamp value in the Request metadata when
> the request is popped from the queue ready to run the pipeline. This
> ensures that the timestamp is written to the correct Request item,
> which may not be at the top of the queue when the Unicam buffer dequeue
> occurs.
> 
> Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp")
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 29 +++++++++++--------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 6fbdba0487bf..eb6d31670567 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -221,6 +221,8 @@ public:
>  
>  private:
>  	void checkRequestCompleted();
> +	void fillRequestMetadata(const ControlList &bufferControls,
> +				 Request *request);
>  	void tryRunPipeline();
>  	bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer);
>  
> @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  			<< ", timestamp: " << buffer->metadata().timestamp;
>  
>  	if (stream == &unicam_[Unicam::Image]) {
> -		/*
> -		 * Record the sensor timestamp in the Request.
> -		 *
> -		 * \todo Do not assume the request in the front of the queue
> -		 * is the correct one
> -		 */
> -		Request *request = requestQueue_.front();
> -		ASSERT(request);
> -
> -		request->metadata().set(controls::SensorTimestamp,
> -					buffer->metadata().timestamp);
> -
>  		/*
>  		 * Lookup the sensor controls used for this frame sequence from
>  		 * DelayedControl and queue them along with the frame buffer.
> @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)
>  	}
>  }
>  
> +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
> +					Request *request)
> +{
> +	request->metadata().set(controls::SensorTimestamp,
> +				bufferControls.get(controls::SensorTimestamp));
> +}
> +
>  void RPiCameraData::tryRunPipeline()
>  {
>  	FrameBuffer *embeddedBuffer;
> @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()
>  	/* See if a new ScalerCrop value needs to be applied. */
>  	applyScalerCrop(request->controls());
>  
> +	/*
> +	 * Clear the request metadata and fill it with some initial non-IPA
> +	 * related controls. We clear it first because the request metadata
> +	 * may have been populated if we have dropped the previous frame.
> +	 */

Aha, I was going to say is this the right place to clear this, and then
I re-read that comment and now I understand why it's here.

So I think this is fine.

Is there anything else that would have to be cleared down if a request
gets 're-used' internally?

I suspect not at the moment, but it may be that we need a specific call
on a request to clean it up more generically.

This should be fine though

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



Hrm, thinking about it more, i think this comes down to the
ControlList->merge() doesn't it ... if the entry is already there it
won't be updated? I almost feel like in this case - it should be
updated, because it's newer and more correct information...



> +	request->metadata().clear();
> +	fillRequestMetadata(bayerFrame.controls, request);
> +
>  	/*
>  	 * Process all the user controls by the IPA. Once this is complete, we
>  	 * queue the ISP output buffer listed in the request to start the HW
> 

-- 
Regards
--
Kieran