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