[libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable with AwbMode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 28 03:06:29 CEST 2021
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 ?
> > 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
More information about the libcamera-devel
mailing list