[libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable with AwbMode
Naushir Patuck
naush at raspberrypi.com
Mon Jun 28 11:13:35 CEST 2021
Hi Laurent,
On Mon, 28 Jun 2021 at 02:06, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Paul,
>
> Thank you for your patch.
>
> On Fri, Jun 18, 2021 at 01:23:03PM +0100, Naushir Patuck wrote:
> > On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:
> > > Previously it was possible to have AwbEnable set to false, yet have
> > > AwbMode on anything. This caused a confusion situation, so merge the
> two
> > > into AwbMode. While at it, pull in the android AWB modes.
>
> I'd say "pull in additional AWB modes defined by Android" (or s/pull
> in/add).
>
> > >
> > > Adjust the previous users of AwbEnable accordingly.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > > include/libcamera/ipa/raspberrypi.h | 1 -
> > > src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------
> > > src/libcamera/control_ids.yaml | 32 +++++++++++++++--------------
> > > test/controls/control_list.cpp | 6 +++---
> > > 4 files changed, 29 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> > > index a8790000..63392a26 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {
> > > { &controls::AeConstraintMode,
> ControlInfo(controls::AeConstraintModeValues) },
> > > { &controls::AeExposureMode,
> ControlInfo(controls::AeExposureModeValues) },
> > > { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > > - { &controls::AwbEnable, ControlInfo(false, true) },
> > > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > > { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > > { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index ad0132c0..ed5f1250 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList
> &controls)
> > > break;
> > > }
> > >
> > > - case controls::AWB_ENABLE: {
> > > - RPiController::Algorithm *awb =
> controller_.GetAlgorithm("awb");
> > > - if (!awb) {
> > > - LOG(IPARPI, Warning)
> > > - << "Could not set AWB_ENABLE -
> no AWB algorithm";
> > > - break;
> > > - }
> > > -
> > > - if (ctrl.second.get<bool>() == false)
> > > - awb->Pause();
> > > - else
> > > - awb->Resume();
> > > -
> > > - libcameraMetadata_.set(controls::AwbEnable,
> > > -
> ctrl.second.get<bool>());
> > > - break;
> > > - }
> > > -
> > > case controls::AWB_MODE: {
> > > RPiController::AwbAlgorithm *awb =
> dynamic_cast<RPiController::AwbAlgorithm *>(
> > > controller_.GetAlgorithm("awb"));
> > > @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList
> &controls)
> > > }
> > >
> > > int32_t idx = ctrl.second.get<int32_t>();
> > > +
> > > + if (idx == controls::AwbOff) {
> > > + awb->Pause();
> > > + break;
> > > + }
> > > +
> > > + if (idx == controls::AwbAuto)
> > > + awb->Resume();
> >
> > I think the logic here may need adjusting such that any state that is not
> > controls::AwbOff will need to call awb->Resume(), or conditionally call
> > resume if adb->IsPaused() returns true.
>
> Agreed. The RPi AWB implementation differs from the behaviour specified
> by Android in that the RPi AWB algorithm isn't disabled when the mode is
> set to one of the presets. The presets instead serve to limit the search
> range of the AWB algorithm, instead of setting hardcoded manual values
> as in Android.
>
> Naush, what would happen if the tuning file specified a fixed colour
> temperature (by setting the min and max to the same value) for AWB
> presets ? Would the AWB algorithm then always produce fixed values for
> the colour gains ?
>
In this case the RPi AWB will (or at least should, I've never tried :))
will still
search a small patch around the region. So you may not necessarily get the
exact same gain values for every frame, but they will be close.
Regards,
Naush
>
> > > if (AwbModeTable.count(idx)) {
> > > awb->SetMode(AwbModeTable.at(idx));
> > >
> libcameraMetadata_.set(controls::AwbMode, idx);
> > > diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> > > index 5717bc1f..2e62f61b 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -229,13 +229,6 @@ controls:
> > > Report an estimate of the current illuminance level in lux.
> The Lux
> > > control can only be returned in metadata.
> > >
> > > - - AwbEnable:
> > > - type: bool
> > > - description: |
> > > - Enable or disable the AWB.
> > > -
> > > - \sa ColourGains
> > > -
> > > # AwbMode needs further attention:
> > > # - Auto-generate max enum value.
> > > # - Better handling of custom types.
> > > @@ -245,29 +238,38 @@ controls:
> > > Specify the range of illuminants to use for the AWB
> algorithm. The modes
> > > supported are platform specific, and not all modes may be
> supported.
> > > enum:
> > > - - name: AwbAuto
> > > + - name: AwbOff
> > > value: 0
> > > + description: The AWB routune is disabled.
>
> s/routine/
>
> > > + - name: AwbAuto
> > > + value: 1
> > > description: Search over the whole colour temperature range.
> > > - name: AwbIncandescent
> > > - value: 1
> > > - description: Incandescent AWB lamp mode.
> > > - - name: AwbTungsten
> > > value: 2
> > > - description: Tungsten AWB lamp mode.
> > > + description: Incandescent AWB lamp mode.
> > > - name: AwbFluorescent
> > > value: 3
> > > description: Fluorescent AWB lamp mode.
> > > - - name: AwbIndoor
> > > + - name: AwbWarmFluorescent
> > > value: 4
> > > - description: Indoor AWB lighting mode.
> > > + description: Warm fluorescent AWB lamp mode.
> > > - name: AwbDaylight
> > > value: 5
> > > description: Daylight AWB lighting mode.
> > > - name: AwbCloudy
> > > value: 6
> > > description: Cloudy AWB lighting mode.
> > > - - name: AwbCustom
> > > + - name: AwbTwilight
> > > value: 7
> > > + description: Twilight AWB lamp mode.
> > > + - name: AwbTungsten
> > > + value: 8
> > > + description: Tungsten AWB lamp mode.
> > > + - name: AwbIndoor
> > > + value: 9
> > > + description: Indoor AWB lighting mode.
> > > + - name: AwbCustom
> > > + value: 10
> > > description: Custom AWB mode.
> > >
> > > - AwbLocked:
> > > diff --git a/test/controls/control_list.cpp
> b/test/controls/control_list.cpp
> > > index 70cf61b8..ce55d09b 100644
> > > --- a/test/controls/control_list.cpp
> > > +++ b/test/controls/control_list.cpp
> > > @@ -143,10 +143,10 @@ protected:
> > > * Attempt to set an invalid control and verify that
> the
> > > * operation failed.
> > > */
> > > - list.set(controls::AwbEnable, true);
> > > + list.set(controls::AwbMode, true);
> > >
> > > - if (list.contains(controls::AwbEnable)) {
> > > - cout << "List shouldn't contain AwbEnable
> control" << endl;
> > > + if (list.contains(controls::AwbMode)) {
> > > + cout << "List shouldn't contain AwbMode
> control" << endl;
> > > return TestFail;
> > > }
> > >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210628/1c879634/attachment-0001.htm>
More information about the libcamera-devel
mailing list