[libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable with AwbMode
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Jul 2 11:52:44 CEST 2021
Hi Laurent,
On Mon, Jun 28, 2021 at 04:22:46AM +0300, Laurent Pinchart wrote:
> Hi Paul,
>
> A related question, will there be a similar patch for AE ?
The android's AE modes besides off/auto are all just variations of
"needs flash", and when we discussed this last iirc the conclusion was
that we don't need them, so off/on is sufficient.
Paul
>
> On Mon, Jun 28, 2021 at 04:06:30AM +0300, Laurent Pinchart wrote:
> > 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;
> > > > }
> > > >
More information about the libcamera-devel
mailing list