<div dir="ltr"><div dir="ltr">Hi Jacopo and Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</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 Laurent,<br>
+ Naush<br>
<br>
On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:<br>
> Hi Jacopo,<br>
><br>
> Thank you for the patch.<br>
><br>
> On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:<br>
> > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:<br>
> > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
> > > > Report the sensor's timestamp in the Request metadata by using the<br>
> > > > Unicam::Image buffer timestamp as an initial approximation.<br>
> > > ><br>
> > > > The buffer's timestamp is recorded at DMA-transfer time, and it does not<br>
> > > > theoretically matches the 'start of exposure' definition, but when used<br>
> > > > to compare two consecutive frames it gives an acceptable estimation of<br>
> > > > the sensor frame period duration.<br>
> > > ><br>
> > > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > > > ---<br>
> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++<br>
> > > > 1 file changed, 12 insertions(+)<br>
> > > ><br>
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > index d1902bfc3393..7dc92acf3c4f 100644<br>
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > @@ -1415,6 +1415,18 @@ 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 The sensor timestamp should be better estimated by<br>
> > > > + * sampling the V4L2Device::frameStart signal.<br>
> > > > + */<br>
> > ><br>
> > > Just a minor correction to this comment - the VB2 buffer timestamp that<br>
> > > is provided by the device driver is actually sampled at the CSI2 frame start<br>
> > > event, so is pretty much as close to the start of exposure as we can get<br>
> > > without dead reckoning. This sample is also before the DMA to memory<br>
> > > and V4L2Device::frameStart signal.<br>
><br>
> I wish more devices and drivers would do so :-)<br>
><br>
> > Ah you see! That's great, I wrongly assumed the raw buffer timestamp<br>
> > was sampled at DMA transfer time end, as it happens on IPU3. It's<br>
> > great if I could drop that todo item!<br>
> ><br>
> > > Other than that,<br>
> > > Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > ><br>
> > > > + Request *request = requestQueue_.front();<br>
><br>
> Don't we queue multiple buffers to Unicam ? Can't a buffer complete here<br>
> while the ISP is busy processing the previous frame, wouldn't<br>
> requestQueue_.front() then correspond to the frame processed by the ISP<br>
> ? Maybe you could use buffer->request() instead (assuming the RPi<br>
> pipeline handler calls setRequest()).<br>
><br></blockquote><div><br></div><div>Quite right, because of the queueing, there is a possibility that the top of</div><div>the request queue may lag behind and will could return out a wrong</div><div>timestamp with the above.</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>
This has required me quite some mumbling, as I was a bit confused too<br>
on how better associate a buffer with a request in this pipeline.<br>
<br>
So, RPi does not use anything similar to FrameInfo, which I think we<br>
should bring to the core framework level soon, and have ph opt-in.<br>
It does not call setRequest() on buffers, and I failed to identify<br>
where to do so, given the ph design. The rest of the ph code when<br>
it has to retrieve the Request * goes and assume it's the first one<br>
(ie the oldest one) it's the one to chose. I have been wondering too<br>
if that's correct. Maybe I got lost into something trivial, Naush do<br>
you have opinions to share ?<br></blockquote><div><br></div><div>We don't use FrameInfo like the other pipeline handlers, but we do have</div><div>a similar mechanism to store frame controls with a buffer.</div><div><br></div><div>Coincidentally, I do have a patch that stores the timestamp for my IPA</div><div>rate-control series [1]. This almost does what we need, the only thing</div><div>missing is some code to transfer the timestamp from this ControlList</div><div>to the request metadata.</div><div><br></div><div>To get this change, we can do one of the following:</div><div><br></div><div>1) I can backport part of the change in [1] that we care about here, and</div><div>add code to transfer to the request metadata. I can either provide a patch</div><div>to add to this series or provide it on top of this series.</div><div><br></div><div>2) I can make this change in my patch series fairly easily as well. Given this</div><div>seems to be related to Android, I doubt anybody would miss this feature</div><div>from the Raspberry Pi platform until that series is merged.</div><div><br></div><div>Let me know what you think.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div><br></div><div>[1] <a href="https://patchwork.libcamera.org/patch/12006/">https://patchwork.libcamera.org/patch/12006/</a></div><div> </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>
Thanks<br>
j<br>
<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>
><br>
> --<br>
> Regards,<br>
><br>
> Laurent Pinchart<br>
</blockquote></div></div>