[libcamera-devel] [PATCH 1/3] libcamera: controls: Destage 'SensorTimestamp'

[Laurent Pinchart]

Hi Jacopo,

Nice to see controls become mature :-)

On Fri, Apr 09, 2021 at 11:22:55AM +0200, Jacopo Mondi wrote:
> On Fri, Apr 09, 2021 at 11:18:33AM +0200, Niklas Söderlund wrote:
> > On 2021-04-07 18:06:42 +0200, Jacopo Mondi wrote:
> > > Destage the 'SensorTimestamp' control, which is used by pipeline
> > > handlers to report the time when the first active line of the sensor's
> > > pixel array is exposed.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/control_ids.yaml | 21 ++++++++++++++-------
> > >  1 file changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index b4771f9def89..f025819aedfd 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -360,6 +360,20 @@ controls:
> > >
> > >        size: [2]
> > >
> > > +  - SensorTimestamp:
> > > +      type: int64_t
> > > +      description: |
> > > +        The time when the first row of the image sensor active array is exposed,

s/exposed,/exposed./

> > > +        The timestamp represents a monotonically increasing counter since the
> > > +        system boot time expressed in nanoseconds.
> >
> > Is it wise to state that time time counter is increasing since system
> > boot?  To me that reads as the counter would start at 0 at system boot
> > and that I could calculate uptime from it. Even if this is true on some
> 
> If anyone wants to use a frame timestamp to calculate the system
> uptime I think he deserves horrible bugs, but I see your point..
> 
> > platforms I'm sure there are others where it's not. How about,
> >
> >     A timestamp when the first row of the image sensor active array is
> >     exposed. The timestamp represents a monotonically increasing counter
> >     expressed in nanoseconds.
> 
> I'll drop the "system boot" part

I think it's important to define which clock the timestamp relates to.
The only two options that seem to make sense are CLOCK_MONOTONIC and
CLOCK_BOOTTIME. If we were to pick a single one, I'd pick the latter.

> > > +
> > > +        Pipeline handlers set this control in a completed Request metadata to
> > > +        allow application to compute the sensor's frame period duration by
> > > +        comparing consecutive capture results.

I'd simply state "The SensorTimestamp control can only be returned in
metadata." to match other controls. What applications will do with the
timestamps is up to them.

> > > +
> > > +        \todo Define how the sensor timestamp has to be used in the reprocessing
> > > +        use case.
> > > +
> > >    # ----------------------------------------------------------------------------
> > >    # Draft controls section
> > >
> > > @@ -547,13 +561,6 @@ controls:
> > >            value: 3
> > >            description: The AWB algorithm is locked.
> > >
> > > -  - SensorTimestamp:
> > > -      type: int64_t
> > > -      draft: true
> > > -      description: |
> > > -       Control to report the start of exposure of the first row of the captured
> > > -       image. Currently identical to ANDROID_SENSOR_TIMESTAMP.
> > > -
> > >    - SensorRollingShutterSkew:
> > >        type: int64_t
> > >        draft: true

-- 
Regards,

Laurent Pinchart