[libcamera-devel] [PATCH 1/2] android: camera_device: Fix sensor frame duration

[Laurent Pinchart]

Hello everybody,

On Fri, May 21, 2021 at 01:08:37PM +0100, Naushir Patuck wrote:
> On Fri, 21 May 2021 at 12:53, Jacopo Mondi wrote:
> 
> > Hi Paul,
> >    the Fix in subject is probably not required, as the
> > ANDROID_SENSOR_FRAME_DURATION property was not registered at all.
> >
> > On Fri, May 21, 2021 at 07:55:33PM +0900, Paul Elder wrote:
> > > The sensor frame duration should be set by IPA. Get the information for
> > > the result metadata from libcamera.
> >
> > how about:
> >
> > "Libcamera reports the frame duration through the
> > controls::FrameDuration controls. Populate the
> > android.sensor.frameDuration result metadata using the libcamera
> > control value"
> >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  src/android/camera_device.cpp | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index b32e8be5..779ce554 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> > >               resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
> > >       }
> > >
> > > +     if (metadata.contains(controls::FrameDurations)) {
> > > +             Span<const int64_t> durations =
> > > +                     metadata.get(controls::FrameDurations);
> > > +             if (durations[0] == durations[1]) {
> >
> > Yes, we have a bit of an unspecified behaviour here.
> >
> > What is the use case for reporting two different frame durations ?
> >
> > Indeed I see this one:
> > src/ipa/raspberrypi/raspberrypi.cpp:
> > libcameraMetadata_.set(controls::FrameDurations,
> > src/ipa/raspberrypi/raspberrypi.cpp:                           {
> > static_cast<int64_t>(minFrameDuration_),
> > src/ipa/raspberrypi/raspberrypi.cpp:
> >  static_cast<int64_t>(maxFrameDuration_) });
> >
> > Where the RPi IPA reports the min/max frame durations received as
> > controls part of a Request before using them to clamp the AE computed
> > exposure time and blankings.
> >
> > +Naush, +David
> >
> > Wouldn't it be better to report the actual duration by knowing the
> > actual exposure+blanking values once AGC has run ?
> 
> This was discussed some time back, and I think we loosely agreed that the values
> returned here in the Request metadata are the min/max frame duration values that
> are actually going to be used, as the may have been clipped based on sensor
> limitations.

I don't recall if you had actual use cases to consume this in
applications, do you remember ?

> Note that this gets put into the metadata only when a user request comes
> through, and not on every frame.

What metadata should be reported every frame or only in specific
circumstances is still to be discussed, but that's a topic for later :-)

> Here is the description I worded in the yaml file:
> 
> > When reported in metadata, the control expresses the minimum and maximum frame
> > durations used after being clipped to the sensor provided frame duration limits.
> 
> Our IPA does not return controls::FrameDurations per-frame, instead we return
> controls::SensorTimestamp that (almost) gives you the inter-frame duration after
> exposure+blanking.

That's subject to jitter. The timestamp is important, but reporting the
nominal frame duration is also important.

How about renaming FrameDurations to FrameDurationLimits, and adding a
new FrameDuration control to report the nominal duration for the frame
in the metadata ?

> > > +                     int64_t duration = durations[0] * 1000;
> > > +
> >  resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
> > > +                                              duration);
> > > +             }
> > > +     }
> > > +
> > >       if (metadata.contains(controls::ScalerCrop)) {
> > >               Rectangle crop = metadata.get(controls::ScalerCrop);
> > >               int32_t cropRect[] = {

-- 
Regards,

Laurent Pinchart