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