[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: Update sensor's RED/BLUE balance controls when present
David Plowman
david.plowman at raspberrypi.com
Wed Apr 14 10:44:37 CEST 2021
HI Kieran
Thanks for the review!
On Wed, 14 Apr 2021 at 09:39, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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.
Yes, I think that seems reasonable to me too. I'll update the patch shortly.
Best regards
David
>
>
> 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