[libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable with AwbMode
David Plowman
david.plowman at raspberrypi.com
Mon Jun 28 12:54:35 CEST 2021
Just to add that I think it's wrong to fix the colour gains when a preset
AWB mode is chosen. Illuminants can differ in other ways besides colour
temperature - for example, some lamps are greener than others, especially
true of some modern fluorescent lights. The RPi approach will still take
care of this, but algorithms that simply use fixed colour gains can leave
you with greenish or purplish images. (Of course, we still let you set
explicit values using the control mechanism.)
David
On Mon, 28 Jun 2021 at 10:13, Naushir Patuck <naush at raspberrypi.com> wrote:
> 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/3d442f26/attachment.htm>
More information about the libcamera-devel
mailing list