[libcamera-devel] [PATCH v3 3/3] libcamera: pipeline: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAINS control

[David Plowman]

Thanks, Laurent!

On Thu, 6 Jan 2022 at 14:37, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> On Thu, Jan 06, 2022 at 02:26:13PM +0000, David Plowman wrote:
> > On Wed, 5 Jan 2022 at 19:47, Laurent Pinchart wrote:
> > > On Wed, Jan 05, 2022 at 03:55:39PM +0000, David Plowman wrote:
> > > > If the sensor exposes the V4L2_CID_NOTIFY_GAINS control, assume it
> > > > means the sensor wants to be told the latest colour gains.
> > > >
> > > > We store whether the control exists and if so its default value, to
> > > > save us checking for it on every frame.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 +++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index b5c687da..4adef952 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -264,6 +264,12 @@ public:
> > > >
> > > >       unsigned int dropFrameCount_;
> > > >
> > > > +     /*
> > > > +      * If set, this stores the value that represets a gain of one for
> > > > +      * the V4L2_CID_NOTIFY_GAINS control.
> > > > +      */
> > > > +     std::optional<int32_t> notifyGainsUnity_;
> > >
> > > A plain int32_t would be enough, as a zero value isn't valid. Up to
> > > you.
> >
> > And I was trying so hard to avoid using the value 0 to have a
> > "special" meaning... :)
>
> I thought std::optional was nice in that it avoided considering any
> specific value as special, but then realized that 0 is invalid anyway.
>
> > I think at this point I'm inclined to leave it as it is, unless anyone
> > else objects, or I might change it if I need to do another revision.
>
> Fine with me.
>
> > > > +
> > > >  private:
> > > >       void checkRequestCompleted();
> > > >       void fillRequestMetadata(const ControlList &bufferControls,
> > > > @@ -1191,6 +1197,15 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
> > > >       /* Initialize the camera properties. */
> > > >       data->properties_ = data->sensor_->properties();
> > > >
> > > > +     /*
> > > > +      * The V4L2_CID_NOTIFY_GAINS control, if present, is used to inform the
> > > > +      * sensor of the colour gains. It is defined to be a linear gain where
> > > > +      * the default value represents a gain of exactly one.
> > > > +      */
> > > > +     auto it = data->sensor_->controls().find(V4L2_CID_NOTIFY_GAINS);
> > > > +     if (it != data->sensor_->controls().end())
> > > > +             data->notifyGainsUnity_ = it->second.def().get<int32_t>();
> > > > +
> > > >       /*
> > > >        * Set a default value for the ScalerCropMaximum property to show
> > > >        * that we support its use, however, initialise it to zero because
> > > > @@ -1495,6 +1510,22 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> > > >       Request *request = requestQueue_.front();
> > > >       request->metadata().merge(controls);
> > > >
> > > > +     /*
> > > > +      * Inform the sensor of the latest colour gains if it has the
> > > > +      * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
> > > > +      */
> > > > +     if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) {
> > > > +             libcamera::Span<const float> colourGains = controls.get(libcamera::controls::ColourGains);
> > > > +             /* The control wants linear gains in the order B, Gb, Gr, R. */
> > > > +             ControlList ctrls(sensor_->controls());
> > > > +             std::array<int32_t, 4> gains = { static_cast<int32_t>(colourGains[1] * *notifyGainsUnity_),
> > > > +                                              *notifyGainsUnity_, *notifyGainsUnity_,
> > > > +                                              static_cast<int32_t>(colourGains[0] * *notifyGainsUnity_) };
> > >
> > > Let's shorten the lines a bit:
> > >
> > >                 std::array<int32_t, 4> gains{
> > >                         static_cast<int32_t>(colourGains[1] * *notifyGainsUnity_),
> > >                         *notifyGainsUnity_,
> > >                         *notifyGainsUnity_,
> > >                         static_cast<int32_t>(colourGains[0] * *notifyGainsUnity_)
> > >                 };
> >
> > Is that fix-while-applying-able, do you think? Otherwise I'll do
> > another version.
>
> I can handle this locally, no problem.
>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > > +             ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const int32_t>{ gains });
> > > > +
> > > > +             sensor_->setControls(&ctrls);
> > > > +     }
> > > > +
> > > >       state_ = State::IpaComplete;
> > > > +             ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const int32_t>{ gains });
> > > > +
> > > > +             sensor_->setControls(&ctrls);
> > > > +     }
> > > > +
> > > >       state_ = State::IpaComplete;
> > > >       handleState();
> > > >  }
>
> --
> Regards,
>
> Laurent Pinchart