[libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report sensor timestamp

[Jacopo Mondi]

Hello

On Wed, Apr 21, 2021 at 11:19:13AM +0100, Naushir Patuck wrote:

[snip]

> > > > > > >
> > > > > > > > +               Request *request = requestQueue_.front();
> > > > >
> > > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete
> > here
> > > > > while the ISP is busy processing the previous frame, wouldn't
> > > > > requestQueue_.front() then correspond to the frame processed by the
> > ISP
> > > > > ? Maybe you could use buffer->request() instead (assuming the RPi
> > > > > pipeline handler calls setRequest()).
> > >
> > > Quite right, because of the queueing, there is a possibility that the
> > top of
> > > the request queue may lag behind and will could return out a wrong
> > > timestamp with the above.
> > >
> > > > This has required me quite some mumbling, as I was a bit confused too
> > > > on how better associate a buffer with a request in this pipeline.
> > > >
> > > > So, RPi does not use anything similar to FrameInfo, which I think we
> > > > should bring to the core framework level soon, and have ph opt-in.
> > > > It does not call setRequest() on buffers, and I failed to identify
> > > > where to do so, given the ph design. The rest of the ph code when
> > > > it has to retrieve the Request * goes and assume it's the first one
> > > > (ie the oldest one) it's the one to chose. I have been wondering too
> > > > if that's correct. Maybe I got lost into something trivial, Naush do
> > > > you have opinions to share ?
> > >
> > > We don't use FrameInfo like the other pipeline handlers, but we do have
> > > a similar mechanism to store frame controls with a buffer.
> > >
> > > Coincidentally, I do have a patch that stores the timestamp for my IPA
> > > rate-control series [1].  This almost does what we need, the only thing
> > > missing is some code to transfer the timestamp from this ControlList
> > > to the request metadata.
> > >
> > > To get this change, we can do one of the following:
> > >
> > > 1) I can backport part of the change in [1] that we care about here, and
> > > add code to transfer to the request metadata.  I can either provide a
> > patch
> > > to add to this series or provide it on top of this series.
> > >
> > > 2) I can make this change in my patch series fairly easily as well.
> > Given this
> > > seems to be related to Android, I doubt anybody would miss this feature
> > > from the Raspberry Pi platform until that series is merged.
> >
> > The timestamp metadata isn't limited to Android support, but nobody else
> > uses it now, so the risk of breakage is fairly limited. Jacopo's series
> > is nearly ready to get merged so I would avoid delaying it if possible.
> > Jacopo, how about keeping this patch as-is with a \todo comment to tell
> > that the front of the queue may not be the right buffer, and fixing it
> > on top once both this series and Naush's series get merged ? Naush,
> > would you be able to then submit a patch on top of the combination of
> > both ? I'll review v5 of the rate limitation now.
> >
>
> Sounds good, we fix this up once Jacopo's series and my rate-control have
> been merged.  Note that my series is rebased on top of David's work at:
> https://patchwork.libcamera.org/patch/11730/)
>
> so I will also have to wait for that to be submitted.

Great, I'll record that with a \todo then

Thanks
  j

>
> Regards,
> Naush
>
>
>
> >
> > > Let me know what you think.
> > >
> > > [1] https://patchwork.libcamera.org/patch/12006/
> > >
> > > > > > > > +               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.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >