[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: Update sensor's RED/BLUE balance controls when present
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Apr 14 10:39:32 CEST 2021
Hi David,
On 08/04/2021 18:12, David Plowman wrote:
> Hi Jacopo
>
> Thanks very much for the comments.
>
> On Thu, 8 Apr 2021 at 16:59, Jacopo Mondi <jacopo at jmondi.org> wrote:
>>
>> Hi David,
>>
>> On Thu, Apr 08, 2021 at 02:36:34PM +0100, David Plowman 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 want these to be applied as soon as possible so we add a new
>>
>> Can you be patient with me and explain why these controls are special
>> and qualify for a fast-tracked method ?
>
> Yes of course! So I'm looking at some sensors that do some "clever"
> processing on the sensor itself, for which they need to know what the
> red and blue gains are. Some examples:
>
> * A "quad Bayer" or "tetracell" sensor may have the ability to output
> an HDR Bayer pattern. In order to combine all the different exposures
> it needs to have the colour gains.
> * The same sensor may have the ability to turn the "quad" pattern of
> pixels into a standard full resolution Bayer pattern. This needs the
> colour gains too.
> * Sensors with non-standard patterns often support conversion to
> regular Bayer - which can work better with the colour gains.
>
> In all these cases it's good to get the latest colour gains to the
> sensor as fast as we can, to limit the artefacts produced if the white
> balance is changing. Note that we never need to try and read them
> back, as we do with exposure and analogue gain.
>
> The first version of this patch used the delayed control mechanism. I
> think that was OK too, but it felt like a slight abuse of that
> mechanism - we don't need the complexity of trying to align it with
> other sensor updates. And it was confusing - I gave it the same delay
> as the vblank control so that it would get written immediately, yet it
> actually happens on the very next frame which would normally be a
> delay of 1, so it all seemed a bit strange...
>
>>
>>> setSensorControls signal to the IPA which passes these back to the
>>> pipeline handler without using the DelayedControls mechanism.
>>>
>>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
>>> ---
>>> include/libcamera/ipa/raspberrypi.mojom | 1 +
>>> src/ipa/raspberrypi/raspberrypi.cpp | 13 +++++++++++++
>>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++
>>> 3 files changed, 24 insertions(+)
>>>
>>> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
>>> index f38c2261..ebaf0a04 100644
>>> --- a/include/libcamera/ipa/raspberrypi.mojom
>>> +++ b/include/libcamera/ipa/raspberrypi.mojom
>>> @@ -117,6 +117,7 @@ interface IPARPiEventInterface {
>>> statsMetadataComplete(uint32 bufferId, ControlList controls);
>>> runIsp(uint32 bufferId);
>>> embeddedComplete(uint32 bufferId);
>>> + setSensorControls(ControlList controls);
>>> setIspControls(ControlList controls);
>>> setDelayedControls(ControlList controls);
>>> };
>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>> index dad6395f..f95896d2 100644
>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>> @@ -1036,6 +1036,19 @@ void IPARPi::processStats(unsigned int bufferId)
>>>
>>> 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()) {
>>> + ControlList ctrls(sensorCtrls_);
>>> + ctrls.set(V4L2_CID_RED_BALANCE,
>>> + static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_r)));
>>> + ctrls.set(V4L2_CID_BLUE_BALANCE,
>>> + static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_b)));
>>
>> Won't this trigger the assert(0) in the previous patch ?
>
> Hopefully not - the idea is that you must override the default method
> if your sensor wants the RED/BLUE_BALANCE numbers. For sensors that
> don't need them, everything is fine as it is.
Yes, I can see that the ColourGainCode is only called if the sensor has
a control for boht RED and BLUE balance.
And if that control is available on the sensor, then the CamHelper
*must* override the ColourGainCode().
I think that makes me more convinced that the assertion in the previous
patch should be a more verbose message explaining what must be done if
it is hit, as it could be hit by a user just changing to a new (not yet
supported) sensor... but otherwise it seems fine.
I'm a bit weary of having two routes to set controls on the sensor
through setSensorControls() and through setDelayedControls(), but given
the namings of both show that one is 'delayed', I think the reasoning is
clear ...
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> I hope that all makes sense?
>
> Thanks!
> David
>
>>
>> Thanks
>> j
>>
>>> +
>>> + setSensorControls.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 f22e286e..8dae4912 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> @@ -152,6 +152,7 @@ public:
>>> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
>>> void runIsp(uint32_t bufferId);
>>> void embeddedComplete(uint32_t bufferId);
>>> + void setSensorControls(const ControlList &controls);
>>> void setIspControls(const ControlList &controls);
>>> void setDelayedControls(const ControlList &controls);
>>>
>>> @@ -1219,6 +1220,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>>> ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
>>> ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>>> ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
>>> + ipa_->setSensorControls.connect(this, &RPiCameraData::setSensorControls);
>>> ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>>> ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>>>
>>> @@ -1361,6 +1363,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
>>> handleState();
>>> }
>>>
>>> +void RPiCameraData::setSensorControls(const ControlList &controls)
>>> +{
>>> + ControlList ctrls = controls;
>>> +
>>> + unicam_[Unicam::Image].dev()->setControls(&ctrls);
>>> + handleState();
>>> +}
>>> +
>>> void RPiCameraData::setIspControls(const ControlList &controls)
>>> {
>>> ControlList ctrls = controls;
>>> --
>>> 2.20.1
>>>
>>> _______________________________________________
>>> libcamera-devel mailing list
>>> libcamera-devel at lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list