| Message ID | 20200309123319.630-5-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 09/03/2020 12:33, Naushir Patuck wrote: > AwbMode is a new enum type to specify operating mode of the AWB > algorithm. All modes may not be supported by all platforms. > > ManualWbGains is a new float arary type used to specify manual red /arary/array/ > and blue (in that order) colour channel gains when AWB is disabled. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index da1a7b43..9a33094a 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -143,6 +143,53 @@ controls: > description: | > Enable or disable the AWB. > > + - AwbMode: > + type: int32_t > + description: | > + Specify the range of illumants to use for the AWB algorihtm. The modes /illumants/illuminance/ or /illumants/luminance/ ? I'm not quite sure what word you were going for with illumants. s/algorihtm/algorithm/ > + supported are platform specific, and not all modes may be supported. > + enum: > + - name: AwbAuto > + value: 0 > + description: Search over the whole CT range. > + - name: AwbIncandescent > + value: 1 > + description: Incandescent AWB lamp mode. > + - name: AwbTungsten > + value: 2 > + description: Tungsten AWB lamp mode. > + - name: AwbFluorescent > + value: 3 > + description: Fluorescent AWB lamp mode. > + - name: AwbIndoor > + value: 4 > + description: Indoor AWB lighting mode. > + - name: AwbDaylight > + value: 5 > + description: Daylight AWB lighting mode. > + - name: AwbCloudy > + value: 6 > + description: Cloudy AWB lighting mode. I wonder what sort order we should apply to control values like this? I'm sure Laurent would love anything to be alphabetically sorted by a default if nothing else applies, but would there be an expected order to this list otherwise? > + - name: AwbCustom1 > + value: 7 > + description: Custom AWB mode 1. > + - name: AwbCustom2 > + value: 8 > + description: Custom AWB mode 2. > + - name: AwbCustom3 > + value: 9 > + description: Custom AWB mode 3. > + - name: AwbModeMax > + value: 9 > + description: Maximum allowed value (place any new values above here). Only the same response as the others here, > + > + - ManualWbGains: > + type: float > + description: | > + Specify a fixed gain parameter for the Red and Blue colour channels, > + in that order. > + size: [2] > + > - Brightness: > type: int32_t > description: Specify a fixed brightness parameter >
On 20/03/2020 15:11, Kieran Bingham wrote: > Hi Naush, > > On 09/03/2020 12:33, Naushir Patuck wrote: >> AwbMode is a new enum type to specify operating mode of the AWB >> algorithm. All modes may not be supported by all platforms. >> >> ManualWbGains is a new float arary type used to specify manual red > > /arary/array/ > >> and blue (in that order) colour channel gains when AWB is disabled. >> >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> --- >> src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml >> index da1a7b43..9a33094a 100644 >> --- a/src/libcamera/control_ids.yaml >> +++ b/src/libcamera/control_ids.yaml >> @@ -143,6 +143,53 @@ controls: >> description: | >> Enable or disable the AWB. >> >> + - AwbMode: >> + type: int32_t >> + description: | >> + Specify the range of illumants to use for the AWB algorihtm. The modes > > /illumants/illuminance/ > or > /illumants/luminance/ ? > I'm not quite sure what word you were going for with illumants. Aha, I think 'illuminants' might be the right spelling (though my spell checker still doesn't like that one either :D) Oh well... > > s/algorihtm/algorithm/ > >> + supported are platform specific, and not all modes may be supported. >> + enum: >> + - name: AwbAuto >> + value: 0 >> + description: Search over the whole CT range. >> + - name: AwbIncandescent >> + value: 1 >> + description: Incandescent AWB lamp mode. >> + - name: AwbTungsten >> + value: 2 >> + description: Tungsten AWB lamp mode. >> + - name: AwbFluorescent >> + value: 3 >> + description: Fluorescent AWB lamp mode. >> + - name: AwbIndoor >> + value: 4 >> + description: Indoor AWB lighting mode. >> + - name: AwbDaylight >> + value: 5 >> + description: Daylight AWB lighting mode. >> + - name: AwbCloudy >> + value: 6 >> + description: Cloudy AWB lighting mode. > > I wonder what sort order we should apply to control values like this? > I'm sure Laurent would love anything to be alphabetically sorted by a > default if nothing else applies, but would there be an expected order to > this list otherwise? > >> + - name: AwbCustom1 >> + value: 7 >> + description: Custom AWB mode 1. >> + - name: AwbCustom2 >> + value: 8 >> + description: Custom AWB mode 2. >> + - name: AwbCustom3 >> + value: 9 >> + description: Custom AWB mode 3. >> + - name: AwbModeMax >> + value: 9 >> + description: Maximum allowed value (place any new values above here). > > Only the same response as the others here, > >> + >> + - ManualWbGains: >> + type: float >> + description: | >> + Specify a fixed gain parameter for the Red and Blue colour channels, >> + in that order. >> + size: [2] >> + >> - Brightness: >> type: int32_t >> description: Specify a fixed brightness parameter >> >
Yes, "illuminants" is correct. The plural of an "illuminant". It doesn't seem to be an everyday sort of a word... David On Fri, 20 Mar 2020 at 15:50, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > On 20/03/2020 15:11, Kieran Bingham wrote: > > Hi Naush, > > > > On 09/03/2020 12:33, Naushir Patuck wrote: > >> AwbMode is a new enum type to specify operating mode of the AWB > >> algorithm. All modes may not be supported by all platforms. > >> > >> ManualWbGains is a new float arary type used to specify manual red > > > > /arary/array/ > > > >> and blue (in that order) colour channel gains when AWB is disabled. > >> > >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >> --- > >> src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ > >> 1 file changed, 47 insertions(+) > >> > >> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > >> index da1a7b43..9a33094a 100644 > >> --- a/src/libcamera/control_ids.yaml > >> +++ b/src/libcamera/control_ids.yaml > >> @@ -143,6 +143,53 @@ controls: > >> description: | > >> Enable or disable the AWB. > >> > >> + - AwbMode: > >> + type: int32_t > >> + description: | > >> + Specify the range of illumants to use for the AWB algorihtm. The modes > > > > /illumants/illuminance/ > > or > > /illumants/luminance/ ? > > I'm not quite sure what word you were going for with illumants. > > Aha, I think 'illuminants' might be the right spelling (though my spell > checker still doesn't like that one either :D) > > Oh well... > > > > > s/algorihtm/algorithm/ > > > >> + supported are platform specific, and not all modes may be supported. > >> + enum: > >> + - name: AwbAuto > >> + value: 0 > >> + description: Search over the whole CT range. > >> + - name: AwbIncandescent > >> + value: 1 > >> + description: Incandescent AWB lamp mode. > >> + - name: AwbTungsten > >> + value: 2 > >> + description: Tungsten AWB lamp mode. > >> + - name: AwbFluorescent > >> + value: 3 > >> + description: Fluorescent AWB lamp mode. > >> + - name: AwbIndoor > >> + value: 4 > >> + description: Indoor AWB lighting mode. > >> + - name: AwbDaylight > >> + value: 5 > >> + description: Daylight AWB lighting mode. > >> + - name: AwbCloudy > >> + value: 6 > >> + description: Cloudy AWB lighting mode. > > > > I wonder what sort order we should apply to control values like this? > > I'm sure Laurent would love anything to be alphabetically sorted by a > > default if nothing else applies, but would there be an expected order to > > this list otherwise? > > > >> + - name: AwbCustom1 > >> + value: 7 > >> + description: Custom AWB mode 1. > >> + - name: AwbCustom2 > >> + value: 8 > >> + description: Custom AWB mode 2. > >> + - name: AwbCustom3 > >> + value: 9 > >> + description: Custom AWB mode 3. > >> + - name: AwbModeMax > >> + value: 9 > >> + description: Maximum allowed value (place any new values above here). > > > > Only the same response as the others here, > > > >> + > >> + - ManualWbGains: > >> + type: float > >> + description: | > >> + Specify a fixed gain parameter for the Red and Blue colour channels, > >> + in that order. > >> + size: [2] > >> + > >> - Brightness: > >> type: int32_t > >> description: Specify a fixed brightness parameter > >> > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, Thank you for the patch. On Fri, Mar 20, 2020 at 03:11:42PM +0000, Kieran Bingham wrote: > On 09/03/2020 12:33, Naushir Patuck wrote: > > AwbMode is a new enum type to specify operating mode of the AWB > > algorithm. All modes may not be supported by all platforms. > > > > ManualWbGains is a new float arary type used to specify manual red > > /arary/array/ > > > and blue (in that order) colour channel gains when AWB is disabled. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index da1a7b43..9a33094a 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -143,6 +143,53 @@ controls: > > description: | > > Enable or disable the AWB. > > > > + - AwbMode: > > + type: int32_t > > + description: | > > + Specify the range of illumants to use for the AWB algorihtm. The modes > > /illumants/illuminance/ > or > /illumants/luminance/ ? > I'm not quite sure what word you were going for with illumants. > > s/algorihtm/algorithm/ > > > + supported are platform specific, and not all modes may be supported. > > + enum: > > + - name: AwbAuto > > + value: 0 > > + description: Search over the whole CT range. > > + - name: AwbIncandescent > > + value: 1 > > + description: Incandescent AWB lamp mode. > > + - name: AwbTungsten > > + value: 2 > > + description: Tungsten AWB lamp mode. > > + - name: AwbFluorescent > > + value: 3 > > + description: Fluorescent AWB lamp mode. > > + - name: AwbIndoor > > + value: 4 > > + description: Indoor AWB lighting mode. > > + - name: AwbDaylight > > + value: 5 > > + description: Daylight AWB lighting mode. > > + - name: AwbCloudy > > + value: 6 > > + description: Cloudy AWB lighting mode. > > I wonder what sort order we should apply to control values like this? > I'm sure Laurent would love anything to be alphabetically sorted by a > default if nothing else applies, but would there be an expected order to > this list otherwise? I like auto being first, and it fortunately starts with an A ;-) Jokes aside, this is a pretty standard configuration parameter for cameras, would there be any published standard that we could follow for possible values ? > > + - name: AwbCustom1 > > + value: 7 > > + description: Custom AWB mode 1. > > + - name: AwbCustom2 > > + value: 8 > > + description: Custom AWB mode 2. > > + - name: AwbCustom3 > > + value: 9 > > + description: Custom AWB mode 3. > > + - name: AwbModeMax > > + value: 9 > > + description: Maximum allowed value (place any new values above here). > > Only the same response as the others here, > > > + > > + - ManualWbGains: > > + type: float > > + description: | > > + Specify a fixed gain parameter for the Red and Blue colour channels, > > + in that order. > > + size: [2] How do these controls interact with ManualGain ? I wonder if we shouldn't merge them all in a 2x2 matrix of red, 2 x green and blue gains. > > + > > - Brightness: > > type: int32_t > > description: Specify a fixed brightness parameter
Hi Laurent, On Thu, 26 Mar 2020 at 15:46, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Fri, Mar 20, 2020 at 03:11:42PM +0000, Kieran Bingham wrote: > > On 09/03/2020 12:33, Naushir Patuck wrote: > > > AwbMode is a new enum type to specify operating mode of the AWB > > > algorithm. All modes may not be supported by all platforms. > > > > > > ManualWbGains is a new float arary type used to specify manual red > > > > /arary/array/ > > > > > and blue (in that order) colour channel gains when AWB is disabled. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 47 insertions(+) > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > index da1a7b43..9a33094a 100644 > > > --- a/src/libcamera/control_ids.yaml > > > +++ b/src/libcamera/control_ids.yaml > > > @@ -143,6 +143,53 @@ controls: > > > description: | > > > Enable or disable the AWB. > > > > > > + - AwbMode: > > > + type: int32_t > > > + description: | > > > + Specify the range of illumants to use for the AWB algorihtm. The modes > > > > /illumants/illuminance/ > > or > > /illumants/luminance/ ? > > I'm not quite sure what word you were going for with illumants. > > > > s/algorihtm/algorithm/ > > > > > + supported are platform specific, and not all modes may be supported. > > > + enum: > > > + - name: AwbAuto > > > + value: 0 > > > + description: Search over the whole CT range. > > > + - name: AwbIncandescent > > > + value: 1 > > > + description: Incandescent AWB lamp mode. > > > + - name: AwbTungsten > > > + value: 2 > > > + description: Tungsten AWB lamp mode. > > > + - name: AwbFluorescent > > > + value: 3 > > > + description: Fluorescent AWB lamp mode. > > > + - name: AwbIndoor > > > + value: 4 > > > + description: Indoor AWB lighting mode. > > > + - name: AwbDaylight > > > + value: 5 > > > + description: Daylight AWB lighting mode. > > > + - name: AwbCloudy > > > + value: 6 > > > + description: Cloudy AWB lighting mode. > > > > I wonder what sort order we should apply to control values like this? > > I'm sure Laurent would love anything to be alphabetically sorted by a > > default if nothing else applies, but would there be an expected order to > > this list otherwise? > > I like auto being first, and it fortunately starts with an A ;-) Jokes > aside, this is a pretty standard configuration parameter for cameras, > would there be any published standard that we could follow for possible > values ? > The only one that comes to mind would be the Android Camera HAL 3 values: https://source.android.com/devices/camera/camera3_3Amodes We have already diverged slightly from that list :) > > > + - name: AwbCustom1 > > > + value: 7 > > > + description: Custom AWB mode 1. > > > + - name: AwbCustom2 > > > + value: 8 > > > + description: Custom AWB mode 2. > > > + - name: AwbCustom3 > > > + value: 9 > > > + description: Custom AWB mode 3. > > > + - name: AwbModeMax > > > + value: 9 > > > + description: Maximum allowed value (place any new values above here). > > > > Only the same response as the others here, > > > > > + > > > + - ManualWbGains: > > > + type: float > > > + description: | > > > + Specify a fixed gain parameter for the Red and Blue colour channels, > > > + in that order. > > > + size: [2] > > How do these controls interact with ManualGain ? I wonder if we > shouldn't merge them all in a 2x2 matrix of red, 2 x green and blue > gains. My intention was that ManualGain would be the analogue (and possibly some digital) gain applied by the sensor in manual AE modes. Perhaps to be more explicit I should rename ManualAnalgoueGain...? In any case, this gain value are applied to all bayer colour channels at the sensor level. ManualWbGains is used in Manual AWB mode. If you do not use one of the presets (AwbIncandescent, AwbTungsten, etc.), you can specify your own gain to apply (in the ISP pipeline) to fix the white point. I don't know if this is a standard thing done by other vendors, but the white balance is computed as gains applied on the red and blue channels only. The green channel(s) is fixed at 1x gain for white balance adjustment. If you did want to add further gain to the signal (i.e. digital gain), it would be then applied equally to all RGB channels, but in addition to any white balance gains that were applied to the R and B channels. However.... to cause more confusion, there is an ISO control that will likely be added at some point. I think the convention is that ISO is a combination of all gains (analogue and digital) applied in the full imaging pipeline, but not accounting for the white balance gain adjustments to the R/B channels. So to finally answer your question, these operations all seem independent to me, so I do not think we would be able to merge the controls into one 2x2 matrix. > > > > + > > > - Brightness: > > > type: int32_t > > > description: Specify a fixed brightness parameter > > -- > Regards, > > Laurent Pinchart Regards, Naush
Hi Naush, On Thu, Mar 26, 2020 at 07:29:42PM +0000, Naushir Patuck wrote: > On Thu, 26 Mar 2020 at 15:46, Laurent Pinchart wrote: > > On Fri, Mar 20, 2020 at 03:11:42PM +0000, Kieran Bingham wrote: > >> On 09/03/2020 12:33, Naushir Patuck wrote: > >>> AwbMode is a new enum type to specify operating mode of the AWB > >>> algorithm. All modes may not be supported by all platforms. > >>> > >>> ManualWbGains is a new float arary type used to specify manual red > >> > >> /arary/array/ > >> > >>> and blue (in that order) colour channel gains when AWB is disabled. > >>> > >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >>> --- > >>> src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 47 insertions(+) > >>> > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > >>> index da1a7b43..9a33094a 100644 > >>> --- a/src/libcamera/control_ids.yaml > >>> +++ b/src/libcamera/control_ids.yaml > >>> @@ -143,6 +143,53 @@ controls: > >>> description: | > >>> Enable or disable the AWB. > >>> > >>> + - AwbMode: > >>> + type: int32_t > >>> + description: | > >>> + Specify the range of illumants to use for the AWB algorihtm. The modes > >> > >> /illumants/illuminance/ > >> or > >> /illumants/luminance/ ? > >> I'm not quite sure what word you were going for with illumants. > >> > >> s/algorihtm/algorithm/ > >> > >>> + supported are platform specific, and not all modes may be supported. > >>> + enum: > >>> + - name: AwbAuto > >>> + value: 0 > >>> + description: Search over the whole CT range. > >>> + - name: AwbIncandescent > >>> + value: 1 > >>> + description: Incandescent AWB lamp mode. > >>> + - name: AwbTungsten > >>> + value: 2 > >>> + description: Tungsten AWB lamp mode. > >>> + - name: AwbFluorescent > >>> + value: 3 > >>> + description: Fluorescent AWB lamp mode. > >>> + - name: AwbIndoor > >>> + value: 4 > >>> + description: Indoor AWB lighting mode. > >>> + - name: AwbDaylight > >>> + value: 5 > >>> + description: Daylight AWB lighting mode. > >>> + - name: AwbCloudy > >>> + value: 6 > >>> + description: Cloudy AWB lighting mode. > >> > >> I wonder what sort order we should apply to control values like this? > >> I'm sure Laurent would love anything to be alphabetically sorted by a > >> default if nothing else applies, but would there be an expected order to > >> this list otherwise? > > > > I like auto being first, and it fortunately starts with an A ;-) Jokes > > aside, this is a pretty standard configuration parameter for cameras, > > would there be any published standard that we could follow for possible > > values ? > > The only one that comes to mind would be the Android Camera HAL 3 values: > https://source.android.com/devices/camera/camera3_3Amodes > > We have already diverged slightly from that list :) :-) The list is interesting, as it also mentions the corresponding temperatures. I however assume that it probably isn't just a matter of temperature, is it ? The other interesting part in that list is that the manual modes are documented as "Fixed white balance settings for ...". I wonder if it's really fixed, or if it restricts the search range of the AWB algorithm. > >>> + - name: AwbCustom1 > >>> + value: 7 > >>> + description: Custom AWB mode 1. > >>> + - name: AwbCustom2 > >>> + value: 8 > >>> + description: Custom AWB mode 2. > >>> + - name: AwbCustom3 > >>> + value: 9 > >>> + description: Custom AWB mode 3. > >>> + - name: AwbModeMax > >>> + value: 9 > >>> + description: Maximum allowed value (place any new values above here). > >> > >> Only the same response as the others here, > >> > >>> + > >>> + - ManualWbGains: > >>> + type: float > >>> + description: | > >>> + Specify a fixed gain parameter for the Red and Blue colour channels, > >>> + in that order. > >>> + size: [2] > > > > How do these controls interact with ManualGain ? I wonder if we > > shouldn't merge them all in a 2x2 matrix of red, 2 x green and blue > > gains. > > My intention was that ManualGain would be the analogue (and possibly > some digital) gain applied by the sensor in manual AE modes. Perhaps > to be more explicit I should rename ManualAnalgoueGain...? In any > case, this gain value are applied to all bayer colour channels at the > sensor level. Do you think there could be a need to apply different gains for individual Bayer components when operating in manual mode, or would that be a sensor feature that would only be used by the IPA for AWB ? I'd suspect the latter (or even no use of separate analog gains at all by IPAs in many cases), but that claim is not backed by personal experience. > ManualWbGains is used in Manual AWB mode. If you do not use one of > the presets (AwbIncandescent, AwbTungsten, etc.), you can specify your > own gain to apply (in the ISP pipeline) to fix the white point. Thanks for the clarification. Let's then document AnalogGain and ManualWbGains to make the distinction clear. As we're dropping the Manual prefix, I wonder what a good name for ManualWbGains would be. DigitalGains ? ColourGains ? RGBGains ? DigitalColourGains ? > I don't know if this is a standard thing done by other vendors, but the > white balance is computed as gains applied on the red and blue > channels only. The green channel(s) is fixed at 1x gain for white > balance adjustment. If you did want to add further gain to the signal > (i.e. digital gain), it would be then applied equally to all RGB > channels, but in addition to any white balance gains that were applied > to the R and B channels. Just to clarify, does this mean that there's no digital gain for the green channels in the ISP, or that it's not used by the IPA ? As this is a manual mode control, do you think it would be simpler to expose it as a set of Bayer gains and transform it into red gain, blue gain and global gain for the hardware that implement the gain control that way ? A red+blue gains control as propose here plus a global digital gain is another model that could also work, but it wouldn't allow specifying different gains for the two green channels (when the hardware supports that). I don't know how useful that feature would be in practice. It's supported (but optional) in the Android camera HAL, but maybe it's unused :-) > However.... to cause more confusion, there is an ISO control that will > likely be added at some point. I think the convention is that ISO is > a combination of all gains (analogue and digital) applied in the full > imaging pipeline, but not accounting for the white balance gain > adjustments to the R/B channels. Such a lovely mess :-) We already have an analog gain (let's assume we won't split it in per-channel gains), and we could add a global digital gain too in addition to the R+B WB gains (or the full 4 channels WB gains if we decide to go that way). Adding a global ISO on top of that would mean yet another layer, with interactions between the controls that need to be clearly defined. Should we instead have separate analog and digital gains, and let applications decide how to split their ISO gain between them ? Maybe with a helper (similarly to what I've proposed in another e-mail in this thread for brightness, contrast and saturation) ? > So to finally answer your question, these operations all seem > independent to me, so I do not think we would be able to merge the > controls into one 2x2 matrix. Thank you for the information. It's much clearer to me now, and I agree a single control isn't enough. > >>> + > >>> - Brightness: > >>> type: int32_t > >>> description: Specify a fixed brightness parameter
Hi Laurent, On Fri, 27 Mar 2020 at 14:01, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Thu, Mar 26, 2020 at 07:29:42PM +0000, Naushir Patuck wrote: > > On Thu, 26 Mar 2020 at 15:46, Laurent Pinchart wrote: > > > On Fri, Mar 20, 2020 at 03:11:42PM +0000, Kieran Bingham wrote: > > >> On 09/03/2020 12:33, Naushir Patuck wrote: > > >>> AwbMode is a new enum type to specify operating mode of the AWB > > >>> algorithm. All modes may not be supported by all platforms. > > >>> > > >>> ManualWbGains is a new float arary type used to specify manual red > > >> > > >> /arary/array/ > > >> > > >>> and blue (in that order) colour channel gains when AWB is disabled. > > >>> > > >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > >>> --- > > >>> src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ > > >>> 1 file changed, 47 insertions(+) > > >>> > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > >>> index da1a7b43..9a33094a 100644 > > >>> --- a/src/libcamera/control_ids.yaml > > >>> +++ b/src/libcamera/control_ids.yaml > > >>> @@ -143,6 +143,53 @@ controls: > > >>> description: | > > >>> Enable or disable the AWB. > > >>> > > >>> + - AwbMode: > > >>> + type: int32_t > > >>> + description: | > > >>> + Specify the range of illumants to use for the AWB algorihtm. The modes > > >> > > >> /illumants/illuminance/ > > >> or > > >> /illumants/luminance/ ? > > >> I'm not quite sure what word you were going for with illumants. > > >> > > >> s/algorihtm/algorithm/ > > >> > > >>> + supported are platform specific, and not all modes may be supported. > > >>> + enum: > > >>> + - name: AwbAuto > > >>> + value: 0 > > >>> + description: Search over the whole CT range. > > >>> + - name: AwbIncandescent > > >>> + value: 1 > > >>> + description: Incandescent AWB lamp mode. > > >>> + - name: AwbTungsten > > >>> + value: 2 > > >>> + description: Tungsten AWB lamp mode. > > >>> + - name: AwbFluorescent > > >>> + value: 3 > > >>> + description: Fluorescent AWB lamp mode. > > >>> + - name: AwbIndoor > > >>> + value: 4 > > >>> + description: Indoor AWB lighting mode. > > >>> + - name: AwbDaylight > > >>> + value: 5 > > >>> + description: Daylight AWB lighting mode. > > >>> + - name: AwbCloudy > > >>> + value: 6 > > >>> + description: Cloudy AWB lighting mode. > > >> > > >> I wonder what sort order we should apply to control values like this? > > >> I'm sure Laurent would love anything to be alphabetically sorted by a > > >> default if nothing else applies, but would there be an expected order to > > >> this list otherwise? > > > > > > I like auto being first, and it fortunately starts with an A ;-) Jokes > > > aside, this is a pretty standard configuration parameter for cameras, > > > would there be any published standard that we could follow for possible > > > values ? > > > > The only one that comes to mind would be the Android Camera HAL 3 values: > > https://source.android.com/devices/camera/camera3_3Amodes > > > > We have already diverged slightly from that list :) > > :-) The list is interesting, as it also mentions the corresponding > temperatures. I however assume that it probably isn't just a matter of > temperature, is it ? > > The other interesting part in that list is that the manual modes are > documented as "Fixed white balance settings for ...". I wonder if it's > really fixed, or if it restricts the search range of the AWB algorithm. > I suppose both of these are implementation dependent. For our AWB, the mode restricts the search range for the algorithm, but I suppose there is nothing stopping you from just having fixed gains for each mode. > > >>> + - name: AwbCustom1 > > >>> + value: 7 > > >>> + description: Custom AWB mode 1. > > >>> + - name: AwbCustom2 > > >>> + value: 8 > > >>> + description: Custom AWB mode 2. > > >>> + - name: AwbCustom3 > > >>> + value: 9 > > >>> + description: Custom AWB mode 3. > > >>> + - name: AwbModeMax > > >>> + value: 9 > > >>> + description: Maximum allowed value (place any new values above here). > > >> > > >> Only the same response as the others here, > > >> > > >>> + > > >>> + - ManualWbGains: > > >>> + type: float > > >>> + description: | > > >>> + Specify a fixed gain parameter for the Red and Blue colour channels, > > >>> + in that order. > > >>> + size: [2] > > > > > > How do these controls interact with ManualGain ? I wonder if we > > > shouldn't merge them all in a 2x2 matrix of red, 2 x green and blue > > > gains. > > > > My intention was that ManualGain would be the analogue (and possibly > > some digital) gain applied by the sensor in manual AE modes. Perhaps > > to be more explicit I should rename ManualAnalgoueGain...? In any > > case, this gain value are applied to all bayer colour channels at the > > sensor level. > > Do you think there could be a need to apply different gains for > individual Bayer components when operating in manual mode, or would that > be a sensor feature that would only be used by the IPA for AWB ? I'd > suspect the latter (or even no use of separate analog gains at all by > IPAs in many cases), but that claim is not backed by personal > experience. Sensor Analogue gain can only be applied globally to all colour channels. If there is a need to apply different R/G/B gains (for manual WB correction), you would have to use digital gain, either in the sensor if it supports it, or the ISP pipeline. Can't think of any other reason to use different R/B/G gains apart for WB correction. > > > ManualWbGains is used in Manual AWB mode. If you do not use one of > > the presets (AwbIncandescent, AwbTungsten, etc.), you can specify your > > own gain to apply (in the ISP pipeline) to fix the white point. > > > Thanks for the clarification. Let's then document AnalogGain and > ManualWbGains to make the distinction clear. As we're dropping the > Manual prefix, I wonder what a good name for ManualWbGains would be. > DigitalGains ? ColourGains ? RGBGains ? DigitalColourGains ? > ColourGains seems like a sensible one to me. > > I don't know if this is a standard thing done by other vendors, but the > > white balance is computed as gains applied on the red and blue > > channels only. The green channel(s) is fixed at 1x gain for white > > balance adjustment. If you did want to add further gain to the signal > > (i.e. digital gain), it would be then applied equally to all RGB > > channels, but in addition to any white balance gains that were applied > > to the R and B channels. > > Just to clarify, does this mean that there's no digital gain for the > green channels in the ISP, or that it's not used by the IPA ? In our hardware, there's no digital gain control for the green channels on its own. Of course, this may be different for different vendors, but I do not see a use for adjusting G gain independently from R/B. > As this is > a manual mode control, do you think it would be simpler to expose it as > a set of Bayer gains and transform it into red gain, blue gain and > global gain for the hardware that implement the gain control that way ? It is possible to combine all these into one control. However, I did want the separation as the R/B gains are adjusted as part of the AWB algorithm, and the Analog + Digital Gain are adjusted as part of the AE algorithm. So I wanted to keep these operations separate. In truth though, the most hardware will probably apply R/B + global digital gains as a single stage operation. > A red+blue gains control as propose here plus a global digital gain is > another model that could also work, but it wouldn't allow specifying > different gains for the two green channels (when the hardware supports > that). I don't know how useful that feature would be in practice. It's > supported (but optional) in the Android camera HAL, but maybe it's > unused :-) > You are correct, with this set of controls we do not specify different gains for different green channels. My view on that one is that if it is needed, it is for a very specific IQ tuning operation (probably green equalistion?). I can't see an application level control being of any use here. > > However.... to cause more confusion, there is an ISO control that will > > likely be added at some point. I think the convention is that ISO is > > a combination of all gains (analogue and digital) applied in the full > > imaging pipeline, but not accounting for the white balance gain > > adjustments to the R/B channels. > > Such a lovely mess :-) We already have an analog gain (let's assume we > won't split it in per-channel gains), and we could add a global digital > gain too in addition to the R+B WB gains (or the full 4 channels WB > gains if we decide to go that way). Adding a global ISO on top of that > would mean yet another layer, with interactions between the controls > that need to be clearly defined. Should we instead have separate analog > and digital gains, and let applications decide how to split their ISO > gain between them ? Maybe with a helper (similarly to what I've proposed > in another e-mail in this thread for brightness, contrast and > saturation) ? It gets worse :) You would hope that ISO would be normalised to something standard (e.g. total gain of 1.0 == ISO 100), but it's not. Each sensor type has its own ISO normalisation that we must follow. I really don't like ISO! Having separate analogue and digital gain controls can be a useful thing, our existing library does this. A helper could be used for ISO, but it will have to fetch out the sensor ISO normalisation and adjust between analogue and digital gain appropriately. There may be other implications for AGC that I have not thought of yet. Let's worry about ISO another day :) > > > So to finally answer your question, these operations all seem > > independent to me, so I do not think we would be able to merge the > > controls into one 2x2 matrix. > > Thank you for the information. It's much clearer to me now, and I agree > a single control isn't enough. > > > >>> + > > >>> - Brightness: > > >>> type: int32_t > > >>> description: Specify a fixed brightness parameter > > -- > Regards, > > Laurent Pinchart Regards, Naush
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index da1a7b43..9a33094a 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -143,6 +143,53 @@ controls: description: | Enable or disable the AWB. + - AwbMode: + type: int32_t + description: | + Specify the range of illumants to use for the AWB algorihtm. The modes + supported are platform specific, and not all modes may be supported. + enum: + - name: AwbAuto + value: 0 + description: Search over the whole CT range. + - name: AwbIncandescent + value: 1 + description: Incandescent AWB lamp mode. + - name: AwbTungsten + value: 2 + description: Tungsten AWB lamp mode. + - name: AwbFluorescent + value: 3 + description: Fluorescent AWB lamp mode. + - name: AwbIndoor + value: 4 + description: Indoor AWB lighting mode. + - name: AwbDaylight + value: 5 + description: Daylight AWB lighting mode. + - name: AwbCloudy + value: 6 + description: Cloudy AWB lighting mode. + - name: AwbCustom1 + value: 7 + description: Custom AWB mode 1. + - name: AwbCustom2 + value: 8 + description: Custom AWB mode 2. + - name: AwbCustom3 + value: 9 + description: Custom AWB mode 3. + - name: AwbModeMax + value: 9 + description: Maximum allowed value (place any new values above here). + + - ManualWbGains: + type: float + description: | + Specify a fixed gain parameter for the Red and Blue colour channels, + in that order. + size: [2] + - Brightness: type: int32_t description: Specify a fixed brightness parameter
AwbMode is a new enum type to specify operating mode of the AWB algorithm. All modes may not be supported by all platforms. ManualWbGains is a new float arary type used to specify manual red and blue (in that order) colour channel gains when AWB is disabled. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/control_ids.yaml | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)