<div dir="auto"><div>Hi all,<div dir="auto"><br></div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 23 May 2021, 1:14 am Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello everybody,<br>
<br>
On Fri, May 21, 2021 at 01:08:37PM +0100, Naushir Patuck wrote:<br>
> On Fri, 21 May 2021 at 12:53, Jacopo Mondi wrote:<br>
> <br>
> > Hi Paul,<br>
> >    the Fix in subject is probably not required, as the<br>
> > ANDROID_SENSOR_FRAME_DURATION property was not registered at all.<br>
> ><br>
> > On Fri, May 21, 2021 at 07:55:33PM +0900, Paul Elder wrote:<br>
> > > The sensor frame duration should be set by IPA. Get the information for<br>
> > > the result metadata from libcamera.<br>
> ><br>
> > how about:<br>
> ><br>
> > "Libcamera reports the frame duration through the<br>
> > controls::FrameDuration controls. Populate the<br>
> > android.sensor.frameDuration result metadata using the libcamera<br>
> > control value"<br>
> ><br>
> > > Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank" rel="noreferrer">paul.elder@ideasonboard.com</a>><br>
> > > ---<br>
> > >  src/android/camera_device.cpp | 10 ++++++++++<br>
> > >  1 file changed, 10 insertions(+)<br>
> > ><br>
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> > > index b32e8be5..779ce554 100644<br>
> > > --- a/src/android/camera_device.cpp<br>
> > > +++ b/src/android/camera_device.cpp<br>
> > > @@ -2252,6 +2252,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons<br>
> > >               resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);<br>
> > >       }<br>
> > ><br>
> > > +     if (metadata.contains(controls::FrameDurations)) {<br>
> > > +             Span<const int64_t> durations =<br>
> > > +                     metadata.get(controls::FrameDurations);<br>
> > > +             if (durations[0] == durations[1]) {<br>
> ><br>
> > Yes, we have a bit of an unspecified behaviour here.<br>
> ><br>
> > What is the use case for reporting two different frame durations ?<br>
> ><br>
> > Indeed I see this one:<br>
> > src/ipa/raspberrypi/raspberrypi.cpp:<br>
> > libcameraMetadata_.set(controls::FrameDurations,<br>
> > src/ipa/raspberrypi/raspberrypi.cpp:                           {<br>
> > static_cast<int64_t>(minFrameDuration_),<br>
> > src/ipa/raspberrypi/raspberrypi.cpp:<br>
> >  static_cast<int64_t>(maxFrameDuration_) });<br>
> ><br>
> > Where the RPi IPA reports the min/max frame durations received as<br>
> > controls part of a Request before using them to clamp the AE computed<br>
> > exposure time and blankings.<br>
> ><br>
> > +Naush, +David<br>
> ><br>
> > Wouldn't it be better to report the actual duration by knowing the<br>
> > actual exposure+blanking values once AGC has run ?<br>
> <br>
> This was discussed some time back, and I think we loosely agreed that the values<br>
> returned here in the Request metadata are the min/max frame duration values that<br>
> are actually going to be used, as the may have been clipped based on sensor<br>
> limitations.<br>
<br>
I don't recall if you had actual use cases to consume this in<br>
applications, do you remember ?<br>
<br>
> Note that this gets put into the metadata only when a user request comes<br>
> through, and not on every frame.<br>
<br>
What metadata should be reported every frame or only in specific<br>
circumstances is still to be discussed, but that's a topic for later :-)<br>
<br>
> Here is the description I worded in the yaml file:<br>
> <br>
> > When reported in metadata, the control expresses the minimum and maximum frame<br>
> > durations used after being clipped to the sensor provided frame duration limits.<br>
> <br>
> Our IPA does not return controls::FrameDurations per-frame, instead we return<br>
> controls::SensorTimestamp that (almost) gives you the inter-frame duration after<br>
> exposure+blanking.<br>
<br>
That's subject to jitter. The timestamp is important, but reporting the<br>
nominal frame duration is also important.<br>
<br>
How about renaming FrameDurations to FrameDurationLimits, and adding a<br>
new FrameDuration control to report the nominal duration for the frame<br>
in the metadata ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Sounds good to me.</div><div dir="auto"><br></div><div dir="auto">We will need to sync the change with our libcamera-apps as they do use this control, but I'm sure that's manageable.</div><div dir="auto"><br></div><div dir="auto">Naush</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> > > +                     int64_t duration = durations[0] * 1000;<br>
> > > +<br>
> >  resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,<br>
> > > +                                              duration);<br>
> > > +             }<br>
> > > +     }<br>
> > > +<br>
> > >       if (metadata.contains(controls::ScalerCrop)) {<br>
> > >               Rectangle crop = metadata.get(controls::ScalerCrop);<br>
> > >               int32_t cropRect[] = {<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div></div>