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

[Naushir Patuck]

Hi all,



On Sun, 23 May 2021, 1:14 am Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:

> 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 ?
>

Sounds good to me.

We will need to sync the change with our libcamera-apps as they do use this
control, but I'm sure that's manageable.

Naush



> > > > +                     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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210523/da3c1476/attachment.htm>