[libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Update sensor's RED/BLUE balance controls when present

[David Plowman]

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.)

>
>>
>>         }
>> +
>> +       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?

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