<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 21 Apr 2021 at 10:44, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">Hello,<br>
<br>
On Wed, Apr 21, 2021 at 10:14:57AM +0100, Naushir Patuck wrote:<br>
> On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote:<br>
> > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:<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 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>
> Quite right, because of the queueing, there is a possibility that the top of<br>
> the request queue may lag behind and will could return out a wrong<br>
> timestamp with the above.<br>
> <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>
> <br>
> We don't use FrameInfo like the other pipeline handlers, but we do have<br>
> a similar mechanism to store frame controls with a buffer.<br>
> <br>
> Coincidentally, I do have a patch that stores the timestamp for my IPA<br>
> rate-control series [1]. This almost does what we need, the only thing<br>
> missing is some code to transfer the timestamp from this ControlList<br>
> to the request metadata.<br>
> <br>
> To get this change, we can do one of the following:<br>
> <br>
> 1) I can backport part of the change in [1] that we care about here, and<br>
> add code to transfer to the request metadata. I can either provide a patch<br>
> to add to this series or provide it on top of this series.<br>
> <br>
> 2) I can make this change in my patch series fairly easily as well. Given this<br>
> seems to be related to Android, I doubt anybody would miss this feature<br>
> from the Raspberry Pi platform until that series is merged.<br>
<br>
The timestamp metadata isn't limited to Android support, but nobody else<br>
uses it now, so the risk of breakage is fairly limited. Jacopo's series<br>
is nearly ready to get merged so I would avoid delaying it if possible.<br>
Jacopo, how about keeping this patch as-is with a \todo comment to tell<br>
that the front of the queue may not be the right buffer, and fixing it<br>
on top once both this series and Naush's series get merged ? Naush,<br>
would you be able to then submit a patch on top of the combination of<br>
both ? I'll review v5 of the rate limitation now.<br></blockquote><div><br></div><div>Sounds good, we fix this up once Jacopo's series and my rate-control have</div><div> been merged. Note that my series is rebased on top of David's work at:</div><div><a href="https://patchwork.libcamera.org/patch/11730/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/11730/</a>)<br></div><div><br></div><div>so I will also have to wait for that to be submitted.</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>
> Let me know what you think.<br>
> <br>
> [1] <a href="https://patchwork.libcamera.org/patch/12006/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/12006/</a><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>