[libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Update sensor's RED/BLUE balance controls when present
Naushir Patuck
naush at raspberrypi.com
Thu Apr 8 11:44:26 CEST 2021
Hi David,
On Thu, 8 Apr 2021 at 10:20, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> Thanks for the feedback!
>
> On Wed, 7 Apr 2021 at 11:32, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > Hi David,
> >
> > Thank you for your patch.
> >
> > On Wed, 24 Mar 2021 at 11:44, David Plowman <
> david.plowman at raspberrypi.com> wrote:
> >>
> >> If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE
> >> controls, assume it wants to be told the colour gains. We will need to
> >> add these to the list of delayed controls we can apply.
> >>
> >> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> >> ---
> >> src/ipa/raspberrypi/raspberrypi.cpp | 17 ++++++++++++++---
> >> .../pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++
> >> 2 files changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> >> index 1c928b72..7437a77e 100644
> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> @@ -1030,13 +1030,24 @@ void IPARPi::processStats(unsigned int bufferId)
> >> RPiController::StatisticsPtr statistics =
> std::make_shared<bcm2835_isp_stats>(*stats);
> >> controller_.Process(statistics, &rpiMetadata_);
> >>
> >> + ControlList ctrls(sensorCtrls_);
> >> +
> >> struct AgcStatus agcStatus;
> >> - if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> >> - ControlList ctrls(sensorCtrls_);
> >> + if (rpiMetadata_.Get("agc.status", agcStatus) == 0)
> >> applyAGC(&agcStatus, ctrls);
> >>
> >> - setDelayedControls.emit(ctrls);
> >> + struct AwbStatus awbStatus;
> >> + if (rpiMetadata_.Get("awb.status", awbStatus) == 0 &&
> >> + sensorCtrls_.find(V4L2_CID_RED_BALANCE) !=
> sensorCtrls_.end() &&
> >> + sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) !=
> sensorCtrls_.end()) {
> >> + ctrls.set(V4L2_CID_RED_BALANCE,
> >> + static_cast<int32_t>(awbStatus.gain_r * 256));
> >> + ctrls.set(V4L2_CID_BLUE_BALANCE,
> >> + static_cast<int32_t>(awbStatus.gain_b * 256));
> >
> >
> > The 256 const bothers me slightly. Is this FP multiplier going to be
> true for all sensors?
> > Any way we can deduce this (perhaps from the min value)?
>
> For the sensor I have, there doesn't seem to be a documented min
> value, so the driver will basically accept anything. Values less than
> 256 (1.0x) might seem unlikely, but it's perhaps not reasonable to
> rule them out completely.
>
> My other suggestion was to add a CamHelper::ColourGainCode method.
> What do you make of that? It would also cover the theoretical
> possibility that the value we give to the driver is not a simple
> linear gain. (Note also that there's no need for the reverse mapping -
> code back to gain - as we don't need it.)
>
This sounds like a good idea to me.
>
> >
> >>
> >> }
> >> +
> >> + if (!ctrls.empty())
> >> + setDelayedControls.emit(ctrls);
> >> }
> >>
> >> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls)
> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> index 2cac802c..7bac1503 100644
> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> @@ -1027,6 +1027,15 @@ bool PipelineHandlerRPi::match(DeviceEnumerator
> *enumerator)
> >> { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay,
> false } },
> >> { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }
> >> };
> >> +
> >> + /* If the sensor exposes red/blue balance controls, we will
> update them. */
> >> + const ControlInfoMap &sensorControls =
> data->unicam_[Unicam::Image].dev()->controls();
> >> + if (sensorControls.find(V4L2_CID_RED_BALANCE) !=
> sensorControls.end() &&
> >> + sensorControls.find(V4L2_CID_BLUE_BALANCE) !=
> sensorControls.end()) {
> >> + params[V4L2_CID_RED_BALANCE] = {
> sensorConfig.vblankDelay, false };
> >> + params[V4L2_CID_BLUE_BALANCE] = {
> sensorConfig.vblankDelay, false };
> >
> >
> > Should the delay value be 1 here, as the change likely happens on the
> very next frame?
>
> Aha, interesting point! So my thinking was:
>
> 1. We never read these values back, so I don't care about when they're
> reported as having taken effect, and
> 2. I want the value to be applied immediately, not after several
> frames of delay. As such, the same delay as "the worst" of the other
> controls should be optimal.
>
> To some extent, perhaps this is revealing a minor abuse of the delayed
> control system for something we didn't really intend. But I don't
> think we provide any other way of getting these values back to the PH.
>
> As a minimum though, I guess a comment would be helpful. Do you think
> we need to do something more/different?
>
I wonder if we should bypass DelayedControls for this entirely?
If we create a new Signal called SetSensorControls in the IPA interface
that pushes the
ctrl values directly to the sensor, that should be enough. Hopefully not a
big change, but
may make this bit a bit neater.
Regards,
Naush
>
> Thanks!
> David
>
> >
> > Regards,
> > Naush
> >
> >
> >>
> >> + }
> >> +
> >> data->delayedCtrls_ =
> std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(),
> params);
> >> data->sensorMetadata_ = sensorConfig.sensorMetadata;
> >>
> >> --
> >> 2.20.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/20210408/9b9af29f/attachment.htm>
More information about the libcamera-devel
mailing list