<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 17 May 2021 at 13:40, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 12/05/2021 09:47, Naushir Patuck wrote:<br>
> Write the controls::SensorTimestamp value in the Request metadata when<br>
> the request is popped from the queue ready to run the pipeline. This<br>
> ensures that the timestamp is written to the correct Request item,<br>
> which may not be at the top of the queue when the Unicam buffer dequeue<br>
> occurs.<br>
> <br>
> Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp")<br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 29 +++++++++++--------<br>
>  1 file changed, 17 insertions(+), 12 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 6fbdba0487bf..eb6d31670567 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -221,6 +221,8 @@ public:<br>
>  <br>
>  private:<br>
>       void checkRequestCompleted();<br>
> +     void fillRequestMetadata(const ControlList &bufferControls,<br>
> +                              Request *request);<br>
>       void tryRunPipeline();<br>
>       bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer);<br>
>  <br>
> @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
>                       << ", timestamp: " << buffer->metadata().timestamp;<br>
>  <br>
>       if (stream == &unicam_[Unicam::Image]) {<br>
> -             /*<br>
> -              * Record the sensor timestamp in the Request.<br>
> -              *<br>
> -              * \todo Do not assume the request in the front of the queue<br>
> -              * is the correct one<br>
> -              */<br>
> -             Request *request = requestQueue_.front();<br>
> -             ASSERT(request);<br>
> -<br>
> -             request->metadata().set(controls::SensorTimestamp,<br>
> -                                     buffer->metadata().timestamp);<br>
> -<br>
>               /*<br>
>                * Lookup the sensor controls used for this frame sequence from<br>
>                * DelayedControl and queue them along with the frame buffer.<br>
> @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)<br>
>       }<br>
>  }<br>
>  <br>
> +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,<br>
> +                                     Request *request)<br>
> +{<br>
> +     request->metadata().set(controls::SensorTimestamp,<br>
> +                             bufferControls.get(controls::SensorTimestamp));<br>
> +}<br>
> +<br>
>  void RPiCameraData::tryRunPipeline()<br>
>  {<br>
>       FrameBuffer *embeddedBuffer;<br>
> @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()<br>
>       /* See if a new ScalerCrop value needs to be applied. */<br>
>       applyScalerCrop(request->controls());<br>
>  <br>
> +     /*<br>
> +      * Clear the request metadata and fill it with some initial non-IPA<br>
> +      * related controls. We clear it first because the request metadata<br>
> +      * may have been populated if we have dropped the previous frame.<br>
> +      */<br>
<br>
Aha, I was going to say is this the right place to clear this, and then<br>
I re-read that comment and now I understand why it's here.<br>
<br>
So I think this is fine.<br>
<br>
Is there anything else that would have to be cleared down if a request<br>
gets 're-used' internally?<br></blockquote><div><br></div><div>I think that's it.... for now :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I suspect not at the moment, but it may be that we need a specific call<br>
on a request to clean it up more generically.<br>
<br>
This should be fine though<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
<br>
Hrm, thinking about it more, i think this comes down to the<br>
ControlList->merge() doesn't it ... if the entry is already there it<br>
won't be updated? I almost feel like in this case - it should be<br>
updated, because it's newer and more correct information...<br></blockquote><div><br></div><div>Correct.  It's a consequence of doing the ControlList::merge() now.</div><div>I think  ControlList::merge() follows the convention of std::map::merge()</div><div>where it does not merge if duplicate keys exist between the two maps.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
<br>
> +     request->metadata().clear();<br>
> +     fillRequestMetadata(bayerFrame.controls, request);<br>
> +<br>
>       /*<br>
>        * Process all the user controls by the IPA. Once this is complete, we<br>
>        * queue the ISP output buffer listed in the request to start the HW<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>