<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 17 Dec 2020 at 16:25, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
On Thu, Dec 17, 2020 at 04:12:08PM +0000, Naushir Patuck wrote:<br>
> Hi Jacopo,<br>
> >><br>
> >> > +                     libcameraMetadata_.set(controls::FrameDurations,<br>
> >> > +                                            {<br>
> >> static_cast<int64_t>(minFrameDuration_),<br>
> >> > +<br>
> >> static_cast<int64_t>(maxFrameDuration_) });<br>
> >><br>
> >> I don't think this matches the control description though:<br>
> >><br>
> >> +          When reported by pipelines, the control expresses the duration<br>
> >> of the<br>
> >> +          sensor frame used to produce streams part of the completed<br>
> >> Request.<br>
> >> +          The minimum and maximum values shall then be the same, as the<br>
> >> sensor<br>
> >> +          frame duration is a fixed parameter.<br>
> >><br>
> >> Should you record the exposure time calculated by GetVBlanking() and<br>
> >> report it here ?<br>
> >><br>
> ><br>
> > Yes, this should be the "adjusted" frame durations. I do have that fix in<br>
> > my next set of patches, but perhaps I apply them here instead?  Note that I<br>
> > do not use exposure directly to calculate this, rather I use the vblank<br>
> > limits provided by the sensor.  There is a sensor dependent offset between<br>
> > exposure lines and VTS, so the numbers may be close, but not fully<br>
> > accurate, when using exposure time to calculate frame durations.<br>
> ><br>
><br>
> I'm really sorry, my above comment is complete rubbish, please disregard it<br>
> (but see below first) :-) I didn't fully understand your comment, I do now.<br>
><br>
> Did you expect controls::FrameDurations to be updated on every frame, even<br>
> if no custom frame durations was set?  I suspect the answer would be yes,<br>
> otherwise it may not make much sense.  However, we already return exposure<br>
> times per frame through controls::ExposureTime in the metadata, and<br>
> together with the buffer timestamps, this control in the metadata may seem<br>
> a bit redundant.<br>
<br>
I was expecting this to be returned per-frame yes.<br>
<br>
><br>
> I'm circling around a bit now, but my original intention for<br>
> controls::FrameDurations returned in the metadata was to report the actual<br>
> frame durations used, if they were clipped by the sensor mode limits (given<br>
> by vblank min() and max() v4l2 ctrls).  Would returning these clipped<br>
> values back in metadata if a user set custom frame durations be more<br>
> suitable here?  Or maybe not bother returning frame durations in the<br>
> metadata at all, as this is already conveyed in controls::ExposureTime?<br>
<br>
Mmm, as ExposureTime is directly connected to manual exposure setting,<br>
I was expecting it to be returned only when the application provides<br>
it. You seem to thinkg the same about FrameDuration :)<br></blockquote><div><br></div><div>Exact opposite thoughts! :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
To be honest, my preference of having FrameDuration returned<br>
unconditionally is only because ExposureTime has a relationship with<br>
the AEG routine being enabled or not. That's a weak preference though,<br>
and if you have other reasons to go in one direction or another I've no<br>
strong preference.<br></blockquote><div><br></div><div>I've viewed ExposureTime and AnalogueGain as intrinsic properties of a frame, regardless of whether manual values were used or not.  Hence why I was setting it every frame.  Of course, the exact same thing could be said for Frame Duration!  Perhaps go with what we currently do, and if Laurent and others have a strong preference the other way, I can make the necessary updates?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
><br>
> ><br>
> ><br>
> >><br>
> >> > +                     break;<br>
> >> > +             }<br>
> >> > +<br>
> >> >               default:<br>
> >> >                       LOG(IPARPI, Warning)<br>
> >> >                               << "Ctrl " << controls::<a href="http://controls.at" rel="noreferrer" target="_blank">controls.at</a><br>
> >> (ctrl.first)->name()<br>
> >> > @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus<br>
> >> *awbStatus, ControlList &ctrls)<br>
> >> >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList<br>
> >> &ctrls)<br>
> >> >  {<br>
> >> >       int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<br>
> >> > -     int32_t exposureLines =<br>
> >> helper_->ExposureLines(agcStatus->shutter_time);<br>
> >> ><br>
> >> > -     LOG(IPARPI, Debug) << "Applying AGC Exposure: " <<<br>
> >> agcStatus->shutter_time<br>
> >> > -                        << " (Shutter lines: " << exposureLines << ")<br>
> >> Gain: "<br>
> >> > +     /* GetVBlanking might clip exposure time to the fps limits. */<br>
> >> > +     double exposure = agcStatus->shutter_time;<br>
> >> > +     int32_t vblanking = helper_->GetVBlanking(exposure,<br>
> >> minFrameDuration_,<br>
> >> > +                                               maxFrameDuration_);<br>
> >> > +     int32_t exposureLines = helper_->ExposureLines(exposure);<br>
> >> > +<br>
> >> > +     LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure<br>
> >> > +                        << " (Shutter lines: " << exposureLines << ",<br>
> >> AGC requested "<br>
> >> > +                        << agcStatus->shutter_time << ") Gain: "<br>
> >> >                          << agcStatus->analogue_gain << " (Gain Code: "<br>
> >> >                          << gainCode << ")";<br>
> >> ><br>
> >> > -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);<br>
> >> > +     /*<br>
> >> > +      * Due to the behavior of V4L2, the current value of VBLANK could<br>
> >> clip the<br>
> >> > +      * exposure time without us knowing. The next time though this<br>
> >> function should<br>
> >> > +      * clip exposure correctly.<br>
> >><br>
> >> Replying to myself: we skip one frame in case VBLANK clips exposure.<br>
> >> I guess setting VBLANK one frame in advance compared to exposure won't<br>
> >> help, as it could clip the previous frame. The only solution is to<br>
> >> prioritize VBLANK when setting controls to the device I guess.<br>
> >><br>
> ><br>
> > In my next patch set, I address this by allowing the staggered write<br>
> > controls to write VBLANK before any other controls.  Depending on the<br>
> > timing of DelayedControls being merged, I can easily port it if needed, or<br>
> > just submit my change to staggered write.<br>
> ><br>
> ><br>
> >><br>
> >> > +      */<br>
> >> > +     ctrls.set(V4L2_CID_VBLANK, vblanking);<br>
> >> >       ctrls.set(V4L2_CID_EXPOSURE, exposureLines);<br>
> >> > +     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);<br>
> >> >  }<br>
> >> ><br>
> >> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList<br>
> >> &ctrls)<br>
> >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > index 7a5f5881..252cab64 100644<br>
> >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> >> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const<br>
> >> CameraConfiguration *config)<br>
> >> >               if (!staggeredCtrl_) {<br>
> >> >                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),<br>
> >> >                                           { { V4L2_CID_ANALOGUE_GAIN,<br>
> >> result.data[resultIdx++] },<br>
> >> > -                                           { V4L2_CID_EXPOSURE,<br>
> >> result.data[resultIdx++] } });<br>
> >> > +                                           { V4L2_CID_EXPOSURE,<br>
> >> result.data[resultIdx++] },<br>
> >> > +                                           { V4L2_CID_VBLANK,<br>
> >> result.data[resultIdx++] } });<br>
> >><br>
> >> A few issues apart (metadata handling and manual exposure clipping<br>
> >> apart) the rest is all minor stuff. With these fixed/clarified<br>
> >><br>
> ><br>
> > I will update with a v10 to address the metadata and typo.  The manual<br>
> > exposure clipping ought to be part of the next patch set as it is part of<br>
> > the bigger change where the AGC applies the limits as well.  Is that ok<br>
> > with you?<br>
> ><br>
> > Regards,<br>
> > Naush<br>
> ><br>
> ><br>
> ><br>
> >><br>
> >> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> >><br>
> >> Thanks<br>
> >>   j<br>
> >><br>
> >> >                       sensorMetadata_ = result.data[resultIdx++];<br>
> >> >               }<br>
> >> >       }<br>
> >> > --<br>
> >> > 2.25.1<br>
> >> ><br>
> >> > _______________________________________________<br>
> >> > libcamera-devel mailing list<br>
> >> > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> >> > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> >><br>
> ><br>
</blockquote></div></div>