[libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add control of sensor vblanking
Jacopo Mondi
jacopo at jmondi.org
Thu Dec 17 17:25:48 CET 2020
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 :)
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.
>
> 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
> >>
> >
More information about the libcamera-devel
mailing list