| Message ID | 20200309123319.630-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 09/03/2020 12:33, Naushir Patuck wrote: > AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type > controls used to specify operating modes in the AE algorithm. All modes > may not be supported by all platforms. > > ExposureValue is a new double type control used to set the log2 exposure > adjustment for the AE algorithm. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- > 1 file changed, 109 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index 5bbe65ae..da1a7b43 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -23,24 +23,104 @@ controls: > > \sa AeEnable > > - - AwbEnable: > - type: bool > + - AeMeteringMode: > + type: int32_t > description: | > - Enable or disable the AWB. > - > - \sa ManualGain > + Specify a metering mode for the AE algorithm to use. The metering > + modes determine which parts of the image are used to determine the > + scene brightness. Metering modes may be platform-specific and not Only caught because of the spelling error below, but in the other instances you do not hyphenate platform-specific. Maybe the simple option is to not hyphenate here either :-) > + all metering modes may be supported. > + enum: > + - name: MeteringCentreWeighted > + value: 0 > + description: Centre-weighted metering mode. > + - name: MeteringSpot > + value: 1 > + description: Spot metering mode. > + - name: MeteringMatrix > + value: 2 > + description: Matrix metering mode. > + - name: MeteringCustom1 > + value: 3 This feels a bit painful if we want to add more (non-custom, or custom) modes. We would only be able to append to prevent ABI breakage - Then the enums could seem a bit ugly: enum { MeteringCentreWeighted, MeteringSpot, MeteringMatrix, MeteringCustom1, MeteringCustom2, MeteringCustom3, MeteringMadeUpGeneralControl, MeteringCustom4, MeteringMadeupButGeneralControl2, } But I fear that may not be something we can easily avoid unless we know *all* of the metering mode names in advance... Of course we're intentionally not yet ABI stable anyway, so this could still be an intermediate step... One option could be to have MeteringCustom, with the actual 'custom' choice provided by another means ('yet another' control?), but maybe that doesn't scale well either. I'm not sure yet... > + description: Custom metering mode 1. > + - name: MeteringCustom2 > + value: 4 > + description: Custom metering mode 2. > + - name: MeteringCustom3 > + value: 5 > + description: Custom metering mode 3. Are you able to express what you currently envisage the 3 custom modes would be used for? > + - name: MeteringModeMax > + value: 5 It's a shame we have to express the enum max value manually here :-( I haven't looked at if it could be identified through code though. > + description: Maximum allowed value (place any new values above here). > > - - Brightness: > + - AeConstraintMode: > type: int32_t > - description: Specify a fixed brightness parameter > + description: | > + Specify a constraint mode for the AE algorithm to use. These determine > + how the measured scene brightness is adjusted to reach the desired > + target exposure. Constraint modes may be platform specific, and not > + all constaint modes may be supported. /constaint/constraint/ > + enum: > + - name: ConstraintNormal > + value: 0 > + description: Default constraint mode. > + - name: ConstraintHighlight > + value: 1 > + description: Highlight constraint mode. > + - name: ConstraintCustom1 > + value: 2 > + description: Custom constraint mode 1. > + - name: ConstraintCustom2 > + value: 3 > + description: Custom constraint mode 2. > + - name: ConstraintCustom3 > + value: 4 > + description: Custom constraint mode 3. > + - name: ConstraintModeMax > + value: 4 > + description: Maximum allowed value (place any new values above here). Same comments apply here of course. Perhaps we should expand the initial set from the modes/constraints that we will need for supporting the Android HAL... > > - - Contrast: > + - AeExposureMode: > type: int32_t > - description: Specify a fixed contrast parameter > + description: | > + Specify an exposure mode for the AE algorithm to use. These specify > + how the desired total exposure is divided between the shutter time > + and the sensor's analogue gain. The exposure modes are platform > + spcific, and not all exposure modes may be supported. s/spcific/specific/ > + enum: > + - name: ExposureNormal > + value: 0 > + description: Default metering mode. > + - name: ExposureSport > + value: 1 > + description: Sport metering mode (favouring short shutter times). > + - name: ExposureLong > + value: 2 > + description: Exposure mode allowing long exposure times. > + - name: ExposureCustom1 > + value: 3 > + description: Custom exposure mode 1. > + - name: ExposureCustom2 > + value: 4 > + description: Custom exposure mode 2. > + - name: ExposureCustom3 > + value: 5 > + description: Custom exposure mode 3. > + - name: ExposureModeMax > + value: 5 > + description: Maximum allowed value (place any new values above here). > > - - Saturation: > - type: int32_t > - description: Specify a fixed saturation parameter > + - ExposureValue: > + type: float > + description: | > + Specify an Exposure Value (EV) parameter. The EV parameter will only be > + applied if the AE alogrithm is currently enabled. > + > + By convention EV adjusts the exposure as log2. For example > + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment > + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. > + > + \sa AeEnable > > - ManualExposure: > type: int32_t > @@ -57,4 +137,21 @@ controls: > applied to all colour channels. > > \sa ManualExposure > + What changes were made to the properties below ? (I don't see anything in particular) or is that just git being a pain and adding churn? > + - AwbEnable: > + type: bool > + description: | > + Enable or disable the AWB. > + > + - Brightness: > + type: int32_t > + description: Specify a fixed brightness parameter > + > + - Contrast: > + type: int32_t > + description: Specify a fixed contrast parameter > + > + - Saturation: > + type: int32_t > + description: Specify a fixed saturation parameter > ... >
Hi Kieran, On Fri, 20 Mar 2020 at 14:58, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Naush, > > On 09/03/2020 12:33, Naushir Patuck wrote: > > AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type > > controls used to specify operating modes in the AE algorithm. All modes > > may not be supported by all platforms. > > > > ExposureValue is a new double type control used to set the log2 exposure > > adjustment for the AE algorithm. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- > > 1 file changed, 109 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index 5bbe65ae..da1a7b43 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -23,24 +23,104 @@ controls: > > > > \sa AeEnable > > > > - - AwbEnable: > > - type: bool > > + - AeMeteringMode: > > + type: int32_t > > description: | > > - Enable or disable the AWB. > > - > > - \sa ManualGain > > + Specify a metering mode for the AE algorithm to use. The metering > > + modes determine which parts of the image are used to determine the > > + scene brightness. Metering modes may be platform-specific and not > > Only caught because of the spelling error below, but in the other > instances you do not hyphenate platform-specific. Maybe the simple > option is to not hyphenate here either :-) > > > + all metering modes may be supported. > > + enum: > > + - name: MeteringCentreWeighted > > + value: 0 > > + description: Centre-weighted metering mode. > > + - name: MeteringSpot > > + value: 1 > > + description: Spot metering mode. > > + - name: MeteringMatrix > > + value: 2 > > + description: Matrix metering mode. > > + - name: MeteringCustom1 > > + value: 3 > > This feels a bit painful if we want to add more (non-custom, or custom) > modes. > > We would only be able to append to prevent ABI breakage - Then the enums > could seem a bit ugly: > > enum { > MeteringCentreWeighted, > MeteringSpot, > MeteringMatrix, > MeteringCustom1, > MeteringCustom2, > MeteringCustom3, > MeteringMadeUpGeneralControl, > MeteringCustom4, > MeteringMadeupButGeneralControl2, > } > > But I fear that may not be something we can easily avoid unless we know > *all* of the metering mode names in advance... > > Of course we're intentionally not yet ABI stable anyway, so this could > still be an intermediate step... > > > One option could be to have MeteringCustom, with the actual 'custom' > choice provided by another means ('yet another' control?), but maybe > that doesn't scale well either. I'm not sure yet... > > > + description: Custom metering mode 1. > > + - name: MeteringCustom2 > > + value: 4 > > + description: Custom metering mode 2. > > + - name: MeteringCustom3 > > + value: 5 > > + description: Custom metering mode 3. > > Are you able to express what you currently envisage the 3 custom modes > would be used for? > The intention here was to allow users to add new modes to the algorithm, perhaps something very specific to their application. If the tuning parameters for the mode could be added/tweaked via a text/config file, then the user could directly use the new mode without ever having to download/re-compile any code. Initially (as in patch v1), these modes were defined as std::string types, but we concluded that added too much vagueness to the list of options. Perhaps 3 custom modes is too much, and we can make do with just one? > > + - name: MeteringModeMax > > + value: 5 > It's a shame we have to express the enum max value manually here :-( > I haven't looked at if it could be identified through code though. > This is primarily needed for setting up ControlRange for the control. It should be entirely possible for the gen_controls script to add the Max item to the end of the enum? > > + description: Maximum allowed value (place any new values above here). > > > > - - Brightness: > > + - AeConstraintMode: > > type: int32_t > > - description: Specify a fixed brightness parameter > > + description: | > > + Specify a constraint mode for the AE algorithm to use. These determine > > + how the measured scene brightness is adjusted to reach the desired > > + target exposure. Constraint modes may be platform specific, and not > > + all constaint modes may be supported. > > /constaint/constraint/ > > > + enum: > > + - name: ConstraintNormal > > + value: 0 > > + description: Default constraint mode. > > + - name: ConstraintHighlight > > + value: 1 > > + description: Highlight constraint mode. > > + - name: ConstraintCustom1 > > + value: 2 > > + description: Custom constraint mode 1. > > + - name: ConstraintCustom2 > > + value: 3 > > + description: Custom constraint mode 2. > > + - name: ConstraintCustom3 > > + value: 4 > > + description: Custom constraint mode 3. > > + - name: ConstraintModeMax > > + value: 4 > > + description: Maximum allowed value (place any new values above here). > > Same comments apply here of course. > > Perhaps we should expand the initial set from the modes/constraints that > we will need for supporting the Android HAL... > > > > > > - - Contrast: > > + - AeExposureMode: > > type: int32_t > > - description: Specify a fixed contrast parameter > > + description: | > > + Specify an exposure mode for the AE algorithm to use. These specify > > + how the desired total exposure is divided between the shutter time > > + and the sensor's analogue gain. The exposure modes are platform > > + spcific, and not all exposure modes may be supported. > > s/spcific/specific/ > > > + enum: > > + - name: ExposureNormal > > + value: 0 > > + description: Default metering mode. > > + - name: ExposureSport > > + value: 1 > > + description: Sport metering mode (favouring short shutter times). > > + - name: ExposureLong > > + value: 2 > > + description: Exposure mode allowing long exposure times. > > + - name: ExposureCustom1 > > + value: 3 > > + description: Custom exposure mode 1. > > + - name: ExposureCustom2 > > + value: 4 > > + description: Custom exposure mode 2. > > + - name: ExposureCustom3 > > + value: 5 > > + description: Custom exposure mode 3. > > + - name: ExposureModeMax > > + value: 5 > > + description: Maximum allowed value (place any new values above here). > > > > - - Saturation: > > - type: int32_t > > - description: Specify a fixed saturation parameter > > + - ExposureValue: > > + type: float > > + description: | > > + Specify an Exposure Value (EV) parameter. The EV parameter will only be > > + applied if the AE alogrithm is currently enabled. > > + > > + By convention EV adjusts the exposure as log2. For example > > + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment > > + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. > > + > > + \sa AeEnable > > > > - ManualExposure: > > type: int32_t > > @@ -57,4 +137,21 @@ controls: > > applied to all colour channels. > > > > \sa ManualExposure > > + > > What changes were made to the properties below ? (I don't see anything > in particular) or is that just git being a pain and adding churn? > I only moved them around - so all the controls in the file are ordered in appropriate groups (AGC, AWB, General...). > > > > + - AwbEnable: > > + type: bool > > + description: | > > + Enable or disable the AWB. > > + > > + - Brightness: > > + type: int32_t > > + description: Specify a fixed brightness parameter > > + > > + - Contrast: > > + type: int32_t > > + description: Specify a fixed contrast parameter > > + > > + - Saturation: > > + type: int32_t > > + description: Specify a fixed saturation parameter > > ... > > > > -- > Regards > -- > Kieran Regards, Naush
Hi Naush, On 23/03/2020 15:29, Naushir Patuck wrote: > Hi Kieran, > > On Fri, 20 Mar 2020 at 14:58, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Naush, >> >> On 09/03/2020 12:33, Naushir Patuck wrote: >>> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type >>> controls used to specify operating modes in the AE algorithm. All modes >>> may not be supported by all platforms. >>> >>> ExposureValue is a new double type control used to set the log2 exposure >>> adjustment for the AE algorithm. >>> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>> --- >>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- >>> 1 file changed, 109 insertions(+), 12 deletions(-) >>> >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml >>> index 5bbe65ae..da1a7b43 100644 >>> --- a/src/libcamera/control_ids.yaml >>> +++ b/src/libcamera/control_ids.yaml >>> @@ -23,24 +23,104 @@ controls: >>> >>> \sa AeEnable >>> >>> - - AwbEnable: >>> - type: bool >>> + - AeMeteringMode: >>> + type: int32_t >>> description: | >>> - Enable or disable the AWB. >>> - >>> - \sa ManualGain >>> + Specify a metering mode for the AE algorithm to use. The metering >>> + modes determine which parts of the image are used to determine the >>> + scene brightness. Metering modes may be platform-specific and not >> >> Only caught because of the spelling error below, but in the other >> instances you do not hyphenate platform-specific. Maybe the simple >> option is to not hyphenate here either :-) >> >>> + all metering modes may be supported. >>> + enum: >>> + - name: MeteringCentreWeighted >>> + value: 0 >>> + description: Centre-weighted metering mode. >>> + - name: MeteringSpot >>> + value: 1 >>> + description: Spot metering mode. >>> + - name: MeteringMatrix >>> + value: 2 >>> + description: Matrix metering mode. >>> + - name: MeteringCustom1 >>> + value: 3 >> >> This feels a bit painful if we want to add more (non-custom, or custom) >> modes. >> >> We would only be able to append to prevent ABI breakage - Then the enums >> could seem a bit ugly: >> >> enum { >> MeteringCentreWeighted, >> MeteringSpot, >> MeteringMatrix, >> MeteringCustom1, >> MeteringCustom2, >> MeteringCustom3, >> MeteringMadeUpGeneralControl, >> MeteringCustom4, >> MeteringMadeupButGeneralControl2, >> } >> >> But I fear that may not be something we can easily avoid unless we know >> *all* of the metering mode names in advance... >> >> Of course we're intentionally not yet ABI stable anyway, so this could >> still be an intermediate step... >> >> >> One option could be to have MeteringCustom, with the actual 'custom' >> choice provided by another means ('yet another' control?), but maybe >> that doesn't scale well either. I'm not sure yet... >> >>> + description: Custom metering mode 1. >>> + - name: MeteringCustom2 >>> + value: 4 >>> + description: Custom metering mode 2. >>> + - name: MeteringCustom3 >>> + value: 5 >>> + description: Custom metering mode 3. >> >> Are you able to express what you currently envisage the 3 custom modes >> would be used for? >> > > The intention here was to allow users to add new modes to the > algorithm, perhaps something very specific to their application. If > the tuning parameters for the mode could be added/tweaked via a > text/config file, then the user could directly use the new mode > without ever having to download/re-compile any code. Initially (as in > patch v1), these modes were defined as std::string types, but we > concluded that added too much vagueness to the list of options. > Perhaps 3 custom modes is too much, and we can make do with just one? Maybe one is enough I don't know. That's why I wondered about one custom control which has a secondary option to give more flexibility. Otherwise '3' seems like a very arbitrary definition ... > >>> + - name: MeteringModeMax >>> + value: 5 >> It's a shame we have to express the enum max value manually here :-( >> I haven't looked at if it could be identified through code though. >> > > This is primarily needed for setting up ControlRange for the control. > It should be entirely possible for the gen_controls script to add the > Max item to the end of the enum? Aha, then indeed perhaps we should automate the creation somehow > >>> + description: Maximum allowed value (place any new values above here). >>> >>> - - Brightness: >>> + - AeConstraintMode: >>> type: int32_t >>> - description: Specify a fixed brightness parameter >>> + description: | >>> + Specify a constraint mode for the AE algorithm to use. These determine >>> + how the measured scene brightness is adjusted to reach the desired >>> + target exposure. Constraint modes may be platform specific, and not >>> + all constaint modes may be supported. >> >> /constaint/constraint/ >> >>> + enum: >>> + - name: ConstraintNormal >>> + value: 0 >>> + description: Default constraint mode. >>> + - name: ConstraintHighlight >>> + value: 1 >>> + description: Highlight constraint mode. >>> + - name: ConstraintCustom1 >>> + value: 2 >>> + description: Custom constraint mode 1. >>> + - name: ConstraintCustom2 >>> + value: 3 >>> + description: Custom constraint mode 2. >>> + - name: ConstraintCustom3 >>> + value: 4 >>> + description: Custom constraint mode 3. >>> + - name: ConstraintModeMax >>> + value: 4 >>> + description: Maximum allowed value (place any new values above here). >> >> Same comments apply here of course. >> >> Perhaps we should expand the initial set from the modes/constraints that >> we will need for supporting the Android HAL... >> >> >>> >>> - - Contrast: >>> + - AeExposureMode: >>> type: int32_t >>> - description: Specify a fixed contrast parameter >>> + description: | >>> + Specify an exposure mode for the AE algorithm to use. These specify >>> + how the desired total exposure is divided between the shutter time >>> + and the sensor's analogue gain. The exposure modes are platform >>> + spcific, and not all exposure modes may be supported. >> >> s/spcific/specific/ >> >>> + enum: >>> + - name: ExposureNormal >>> + value: 0 >>> + description: Default metering mode. >>> + - name: ExposureSport >>> + value: 1 >>> + description: Sport metering mode (favouring short shutter times). >>> + - name: ExposureLong >>> + value: 2 >>> + description: Exposure mode allowing long exposure times. >>> + - name: ExposureCustom1 >>> + value: 3 >>> + description: Custom exposure mode 1. >>> + - name: ExposureCustom2 >>> + value: 4 >>> + description: Custom exposure mode 2. >>> + - name: ExposureCustom3 >>> + value: 5 >>> + description: Custom exposure mode 3. >>> + - name: ExposureModeMax >>> + value: 5 >>> + description: Maximum allowed value (place any new values above here). >>> >>> - - Saturation: >>> - type: int32_t >>> - description: Specify a fixed saturation parameter >>> + - ExposureValue: >>> + type: float >>> + description: | >>> + Specify an Exposure Value (EV) parameter. The EV parameter will only be >>> + applied if the AE alogrithm is currently enabled. >>> + >>> + By convention EV adjusts the exposure as log2. For example >>> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment >>> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. >>> + >>> + \sa AeEnable >>> >>> - ManualExposure: >>> type: int32_t >>> @@ -57,4 +137,21 @@ controls: >>> applied to all colour channels. >>> >>> \sa ManualExposure >>> + >> >> What changes were made to the properties below ? (I don't see anything >> in particular) or is that just git being a pain and adding churn? >> > > I only moved them around - so all the controls in the file are ordered > in appropriate groups (AGC, AWB, General...). Aha - in that case, it would generally be better to commit any non-functional-change code move as a separate patch, so that it's clear and distinct on it's own. >>> + - AwbEnable: >>> + type: bool >>> + description: | >>> + Enable or disable the AWB. >>> + >>> + - Brightness: >>> + type: int32_t >>> + description: Specify a fixed brightness parameter >>> + >>> + - Contrast: >>> + type: int32_t >>> + description: Specify a fixed contrast parameter >>> + >>> + - Saturation: >>> + type: int32_t >>> + description: Specify a fixed saturation parameter >>> ... >>> >> >> -- >> Regards >> -- >> Kieran > > Regards, > Naush >
Hi Naush, Thank you for the patch. On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote: > On 23/03/2020 15:29, Naushir Patuck wrote: > > On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote: > >> On 09/03/2020 12:33, Naushir Patuck wrote: > >>> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type > >>> controls used to specify operating modes in the AE algorithm. All modes > >>> may not be supported by all platforms. > >>> > >>> ExposureValue is a new double type control used to set the log2 exposure > >>> adjustment for the AE algorithm. > >>> > >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >>> --- > >>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- > >>> 1 file changed, 109 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > >>> index 5bbe65ae..da1a7b43 100644 > >>> --- a/src/libcamera/control_ids.yaml > >>> +++ b/src/libcamera/control_ids.yaml > >>> @@ -23,24 +23,104 @@ controls: > >>> > >>> \sa AeEnable > >>> > >>> - - AwbEnable: > >>> - type: bool > >>> + - AeMeteringMode: > >>> + type: int32_t > >>> description: | > >>> - Enable or disable the AWB. > >>> - > >>> - \sa ManualGain > >>> + Specify a metering mode for the AE algorithm to use. The metering > >>> + modes determine which parts of the image are used to determine the > >>> + scene brightness. Metering modes may be platform-specific and not > >> > >> Only caught because of the spelling error below, but in the other > >> instances you do not hyphenate platform-specific. Maybe the simple > >> option is to not hyphenate here either :-) > >> > >>> + all metering modes may be supported. > >>> + enum: > >>> + - name: MeteringCentreWeighted > >>> + value: 0 > >>> + description: Centre-weighted metering mode. > >>> + - name: MeteringSpot > >>> + value: 1 > >>> + description: Spot metering mode. > >>> + - name: MeteringMatrix > >>> + value: 2 > >>> + description: Matrix metering mode. > >>> + - name: MeteringCustom1 > >>> + value: 3 > >> > >> This feels a bit painful if we want to add more (non-custom, or custom) > >> modes. > >> > >> We would only be able to append to prevent ABI breakage - Then the enums > >> could seem a bit ugly: > >> > >> enum { > >> MeteringCentreWeighted, > >> MeteringSpot, > >> MeteringMatrix, > >> MeteringCustom1, > >> MeteringCustom2, > >> MeteringCustom3, > >> MeteringMadeUpGeneralControl, > >> MeteringCustom4, > >> MeteringMadeupButGeneralControl2, > >> } > >> > >> But I fear that may not be something we can easily avoid unless we know > >> *all* of the metering mode names in advance... > >> > >> Of course we're intentionally not yet ABI stable anyway, so this could > >> still be an intermediate step... > >> > >> One option could be to have MeteringCustom, with the actual 'custom' > >> choice provided by another means ('yet another' control?), but maybe > >> that doesn't scale well either. I'm not sure yet... > >> > >>> + description: Custom metering mode 1. > >>> + - name: MeteringCustom2 > >>> + value: 4 > >>> + description: Custom metering mode 2. > >>> + - name: MeteringCustom3 > >>> + value: 5 > >>> + description: Custom metering mode 3. > >> > >> Are you able to express what you currently envisage the 3 custom modes > >> would be used for? > > > > The intention here was to allow users to add new modes to the > > algorithm, perhaps something very specific to their application. If > > the tuning parameters for the mode could be added/tweaked via a > > text/config file, then the user could directly use the new mode > > without ever having to download/re-compile any code. Initially (as in > > patch v1), these modes were defined as std::string types, but we > > concluded that added too much vagueness to the list of options. > > Perhaps 3 custom modes is too much, and we can make do with just one? > > Maybe one is enough I don't know. That's why I wondered about one custom > control which has a secondary option to give more flexibility. > > Otherwise '3' seems like a very arbitrary definition ... This is indeed an issue, I'll try to sleep over it. Having a single value wouldn't be too bad when it comes to extending the enum, but that may not be enough. That being said, for this specific control, I wonder if we should make it a bit more finer-grained by specifying a list of weighted regions. Just brainstorming, the ControlInfo (previously known as ControlRange) class could then possibly offer a set of defaults based on an enum. > >>> + - name: MeteringModeMax > >>> + value: 5 > >> > >> It's a shame we have to express the enum max value manually here :-( > >> I haven't looked at if it could be identified through code though. > > > > This is primarily needed for setting up ControlRange for the control. > > It should be entirely possible for the gen_controls script to add the > > Max item to the end of the enum? > > Aha, then indeed perhaps we should automate the creation somehow Yes, we should do that. > >>> + description: Maximum allowed value (place any new values above here). > >>> > >>> - - Brightness: > >>> + - AeConstraintMode: > >>> type: int32_t > >>> - description: Specify a fixed brightness parameter > >>> + description: | > >>> + Specify a constraint mode for the AE algorithm to use. These determine > >>> + how the measured scene brightness is adjusted to reach the desired > >>> + target exposure. Constraint modes may be platform specific, and not > >>> + all constaint modes may be supported. > >> > >> /constaint/constraint/ > >> > >>> + enum: > >>> + - name: ConstraintNormal > >>> + value: 0 > >>> + description: Default constraint mode. > >>> + - name: ConstraintHighlight > >>> + value: 1 > >>> + description: Highlight constraint mode. Would it be possible to give a more detailed description of those modes ? > >>> + - name: ConstraintCustom1 > >>> + value: 2 > >>> + description: Custom constraint mode 1. > >>> + - name: ConstraintCustom2 > >>> + value: 3 > >>> + description: Custom constraint mode 2. > >>> + - name: ConstraintCustom3 > >>> + value: 4 > >>> + description: Custom constraint mode 3. > >>> + - name: ConstraintModeMax > >>> + value: 4 > >>> + description: Maximum allowed value (place any new values above here). > >> > >> Same comments apply here of course. > >> > >> Perhaps we should expand the initial set from the modes/constraints that > >> we will need for supporting the Android HAL... > >> > >>> - - Contrast: > >>> + - AeExposureMode: > >>> type: int32_t > >>> - description: Specify a fixed contrast parameter > >>> + description: | > >>> + Specify an exposure mode for the AE algorithm to use. These specify > >>> + how the desired total exposure is divided between the shutter time > >>> + and the sensor's analogue gain. The exposure modes are platform > >>> + spcific, and not all exposure modes may be supported. > >> > >> s/spcific/specific/ > >> > >>> + enum: > >>> + - name: ExposureNormal > >>> + value: 0 > >>> + description: Default metering mode. > >>> + - name: ExposureSport > >>> + value: 1 > >>> + description: Sport metering mode (favouring short shutter times). Reading the description, I wonder if this should be turned into a scene mode control, as it could also have an effect on other algorithms (auto-focus comes to mind for instance). > >>> + - name: ExposureLong > >>> + value: 2 > >>> + description: Exposure mode allowing long exposure times. > >>> + - name: ExposureCustom1 > >>> + value: 3 > >>> + description: Custom exposure mode 1. > >>> + - name: ExposureCustom2 > >>> + value: 4 > >>> + description: Custom exposure mode 2. > >>> + - name: ExposureCustom3 > >>> + value: 5 > >>> + description: Custom exposure mode 3. > >>> + - name: ExposureModeMax > >>> + value: 5 > >>> + description: Maximum allowed value (place any new values above here). > >>> > >>> - - Saturation: > >>> - type: int32_t > >>> - description: Specify a fixed saturation parameter > >>> + - ExposureValue: > >>> + type: float > >>> + description: | > >>> + Specify an Exposure Value (EV) parameter. The EV parameter will only be > >>> + applied if the AE alogrithm is currently enabled. > >>> + > >>> + By convention EV adjusts the exposure as log2. For example > >>> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment > >>> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. > >>> + > >>> + \sa AeEnable > >>> > >>> - ManualExposure: > >>> type: int32_t > >>> @@ -57,4 +137,21 @@ controls: > >>> applied to all colour channels. > >>> > >>> \sa ManualExposure > >>> + > >> > >> What changes were made to the properties below ? (I don't see anything > >> in particular) or is that just git being a pain and adding churn? > > > > I only moved them around - so all the controls in the file are ordered > > in appropriate groups (AGC, AWB, General...). > > Aha - in that case, it would generally be better to commit any > non-functional-change code move as a separate patch, so that it's clear > and distinct on it's own. > > >>> + - AwbEnable: > >>> + type: bool > >>> + description: | > >>> + Enable or disable the AWB. > >>> + > >>> + - Brightness: > >>> + type: int32_t > >>> + description: Specify a fixed brightness parameter > >>> + > >>> + - Contrast: > >>> + type: int32_t > >>> + description: Specify a fixed contrast parameter > >>> + > >>> + - Saturation: > >>> + type: int32_t > >>> + description: Specify a fixed saturation parameter > >>> ...
Hi Laurent, On Thu, 26 Mar 2020 at 16:26, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote: > > On 23/03/2020 15:29, Naushir Patuck wrote: > > > On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote: > > >> On 09/03/2020 12:33, Naushir Patuck wrote: > > >>> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type > > >>> controls used to specify operating modes in the AE algorithm. All modes > > >>> may not be supported by all platforms. > > >>> > > >>> ExposureValue is a new double type control used to set the log2 exposure > > >>> adjustment for the AE algorithm. > > >>> > > >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > >>> --- > > >>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- > > >>> 1 file changed, 109 insertions(+), 12 deletions(-) > > >>> > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > >>> index 5bbe65ae..da1a7b43 100644 > > >>> --- a/src/libcamera/control_ids.yaml > > >>> +++ b/src/libcamera/control_ids.yaml > > >>> @@ -23,24 +23,104 @@ controls: > > >>> > > >>> \sa AeEnable > > >>> > > >>> - - AwbEnable: > > >>> - type: bool > > >>> + - AeMeteringMode: > > >>> + type: int32_t > > >>> description: | > > >>> - Enable or disable the AWB. > > >>> - > > >>> - \sa ManualGain > > >>> + Specify a metering mode for the AE algorithm to use. The metering > > >>> + modes determine which parts of the image are used to determine the > > >>> + scene brightness. Metering modes may be platform-specific and not > > >> > > >> Only caught because of the spelling error below, but in the other > > >> instances you do not hyphenate platform-specific. Maybe the simple > > >> option is to not hyphenate here either :-) > > >> > > >>> + all metering modes may be supported. > > >>> + enum: > > >>> + - name: MeteringCentreWeighted > > >>> + value: 0 > > >>> + description: Centre-weighted metering mode. > > >>> + - name: MeteringSpot > > >>> + value: 1 > > >>> + description: Spot metering mode. > > >>> + - name: MeteringMatrix > > >>> + value: 2 > > >>> + description: Matrix metering mode. > > >>> + - name: MeteringCustom1 > > >>> + value: 3 > > >> > > >> This feels a bit painful if we want to add more (non-custom, or custom) > > >> modes. > > >> > > >> We would only be able to append to prevent ABI breakage - Then the enums > > >> could seem a bit ugly: > > >> > > >> enum { > > >> MeteringCentreWeighted, > > >> MeteringSpot, > > >> MeteringMatrix, > > >> MeteringCustom1, > > >> MeteringCustom2, > > >> MeteringCustom3, > > >> MeteringMadeUpGeneralControl, > > >> MeteringCustom4, > > >> MeteringMadeupButGeneralControl2, > > >> } > > >> > > >> But I fear that may not be something we can easily avoid unless we know > > >> *all* of the metering mode names in advance... > > >> > > >> Of course we're intentionally not yet ABI stable anyway, so this could > > >> still be an intermediate step... > > >> > > >> One option could be to have MeteringCustom, with the actual 'custom' > > >> choice provided by another means ('yet another' control?), but maybe > > >> that doesn't scale well either. I'm not sure yet... > > >> > > >>> + description: Custom metering mode 1. > > >>> + - name: MeteringCustom2 > > >>> + value: 4 > > >>> + description: Custom metering mode 2. > > >>> + - name: MeteringCustom3 > > >>> + value: 5 > > >>> + description: Custom metering mode 3. > > >> > > >> Are you able to express what you currently envisage the 3 custom modes > > >> would be used for? > > > > > > The intention here was to allow users to add new modes to the > > > algorithm, perhaps something very specific to their application. If > > > the tuning parameters for the mode could be added/tweaked via a > > > text/config file, then the user could directly use the new mode > > > without ever having to download/re-compile any code. Initially (as in > > > patch v1), these modes were defined as std::string types, but we > > > concluded that added too much vagueness to the list of options. > > > Perhaps 3 custom modes is too much, and we can make do with just one? > > > > Maybe one is enough I don't know. That's why I wondered about one custom > > control which has a secondary option to give more flexibility. > > > > Otherwise '3' seems like a very arbitrary definition ... > > This is indeed an issue, I'll try to sleep over it. Having a single > value wouldn't be too bad when it comes to extending the enum, but that > may not be enough. > > That being said, for this specific control, I wonder if we should make > it a bit more finer-grained by specifying a list of weighted regions. > Just brainstorming, the ControlInfo (previously known as ControlRange) > class could then possibly offer a set of defaults based on an enum. > This is possible, but it might get tricky trying to interoperate with different vendor IPAs where the HW/SW capabilities may not be the same. > > >>> + - name: MeteringModeMax > > >>> + value: 5 > > >> > > >> It's a shame we have to express the enum max value manually here :-( > > >> I haven't looked at if it could be identified through code though. > > > > > > This is primarily needed for setting up ControlRange for the control. > > > It should be entirely possible for the gen_controls script to add the > > > Max item to the end of the enum? > > > > Aha, then indeed perhaps we should automate the creation somehow > > Yes, we should do that. As mentioned previously, the max value is only used for ControlInfo (previously known as ControlRange). I do wonder if ControlInfo is a sensible thing to have for an enum though. Suppose an IPA allows only metering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be specified as (min, max) == (0, 5), but a value of 3 is invalid. > > > >>> + description: Maximum allowed value (place any new values above here). > > >>> > > >>> - - Brightness: > > >>> + - AeConstraintMode: > > >>> type: int32_t > > >>> - description: Specify a fixed brightness parameter > > >>> + description: | > > >>> + Specify a constraint mode for the AE algorithm to use. These determine > > >>> + how the measured scene brightness is adjusted to reach the desired > > >>> + target exposure. Constraint modes may be platform specific, and not > > >>> + all constaint modes may be supported. > > >> > > >> /constaint/constraint/ > > >> > > >>> + enum: > > >>> + - name: ConstraintNormal > > >>> + value: 0 > > >>> + description: Default constraint mode. > > >>> + - name: ConstraintHighlight > > >>> + value: 1 > > >>> + description: Highlight constraint mode. > > Would it be possible to give a more detailed description of those modes > ? Will do. > > > >>> + - name: ConstraintCustom1 > > >>> + value: 2 > > >>> + description: Custom constraint mode 1. > > >>> + - name: ConstraintCustom2 > > >>> + value: 3 > > >>> + description: Custom constraint mode 2. > > >>> + - name: ConstraintCustom3 > > >>> + value: 4 > > >>> + description: Custom constraint mode 3. > > >>> + - name: ConstraintModeMax > > >>> + value: 4 > > >>> + description: Maximum allowed value (place any new values above here). > > >> > > >> Same comments apply here of course. > > >> > > >> Perhaps we should expand the initial set from the modes/constraints that > > >> we will need for supporting the Android HAL... > > >> > > >>> - - Contrast: > > >>> + - AeExposureMode: > > >>> type: int32_t > > >>> - description: Specify a fixed contrast parameter > > >>> + description: | > > >>> + Specify an exposure mode for the AE algorithm to use. These specify > > >>> + how the desired total exposure is divided between the shutter time > > >>> + and the sensor's analogue gain. The exposure modes are platform > > >>> + spcific, and not all exposure modes may be supported. > > >> > > >> s/spcific/specific/ > > >> > > >>> + enum: > > >>> + - name: ExposureNormal > > >>> + value: 0 > > >>> + description: Default metering mode. > > >>> + - name: ExposureSport > > >>> + value: 1 > > >>> + description: Sport metering mode (favouring short shutter times). > > Reading the description, I wonder if this should be turned into a scene > mode control, as it could also have an effect on other algorithms > (auto-focus comes to mind for instance). > I always looked as scene mode as a higher level construct within the camera application. The application bunches a group of settings for a particular scene mode (e.g. for Sport scene mode, AE turned to sport, focus turned to fast converge, flash turned to fast sync) and sets them individually. This way it could tailor them exactly to what it needs. > > >>> + - name: ExposureLong > > >>> + value: 2 > > >>> + description: Exposure mode allowing long exposure times. > > >>> + - name: ExposureCustom1 > > >>> + value: 3 > > >>> + description: Custom exposure mode 1. > > >>> + - name: ExposureCustom2 > > >>> + value: 4 > > >>> + description: Custom exposure mode 2. > > >>> + - name: ExposureCustom3 > > >>> + value: 5 > > >>> + description: Custom exposure mode 3. > > >>> + - name: ExposureModeMax > > >>> + value: 5 > > >>> + description: Maximum allowed value (place any new values above here). > > >>> > > >>> - - Saturation: > > >>> - type: int32_t > > >>> - description: Specify a fixed saturation parameter > > >>> + - ExposureValue: > > >>> + type: float > > >>> + description: | > > >>> + Specify an Exposure Value (EV) parameter. The EV parameter will only be > > >>> + applied if the AE alogrithm is currently enabled. > > >>> + > > >>> + By convention EV adjusts the exposure as log2. For example > > >>> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment > > >>> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. > > >>> + > > >>> + \sa AeEnable > > >>> > > >>> - ManualExposure: > > >>> type: int32_t > > >>> @@ -57,4 +137,21 @@ controls: > > >>> applied to all colour channels. > > >>> > > >>> \sa ManualExposure > > >>> + > > >> > > >> What changes were made to the properties below ? (I don't see anything > > >> in particular) or is that just git being a pain and adding churn? > > > > > > I only moved them around - so all the controls in the file are ordered > > > in appropriate groups (AGC, AWB, General...). > > > > Aha - in that case, it would generally be better to commit any > > non-functional-change code move as a separate patch, so that it's clear > > and distinct on it's own. > > > > >>> + - AwbEnable: > > >>> + type: bool > > >>> + description: | > > >>> + Enable or disable the AWB. > > >>> + > > >>> + - Brightness: > > >>> + type: int32_t > > >>> + description: Specify a fixed brightness parameter > > >>> + > > >>> + - Contrast: > > >>> + type: int32_t > > >>> + description: Specify a fixed contrast parameter > > >>> + > > >>> + - Saturation: > > >>> + type: int32_t > > >>> + description: Specify a fixed saturation parameter > > >>> ... > > -- > Regards, > > Laurent Pinchart Regards, Naush
Hi Naush, On Fri, Mar 27, 2020 at 10:56:14AM +0000, Naushir Patuck wrote: > On Thu, 26 Mar 2020 at 16:26, Laurent Pinchart wrote: > > On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote: > >> On 23/03/2020 15:29, Naushir Patuck wrote: > >>> On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote: > >>>> On 09/03/2020 12:33, Naushir Patuck wrote: > >>>>> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type > >>>>> controls used to specify operating modes in the AE algorithm. All modes > >>>>> may not be supported by all platforms. > >>>>> > >>>>> ExposureValue is a new double type control used to set the log2 exposure > >>>>> adjustment for the AE algorithm. > >>>>> > >>>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >>>>> --- > >>>>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- > >>>>> 1 file changed, 109 insertions(+), 12 deletions(-) > >>>>> > >>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > >>>>> index 5bbe65ae..da1a7b43 100644 > >>>>> --- a/src/libcamera/control_ids.yaml > >>>>> +++ b/src/libcamera/control_ids.yaml > >>>>> @@ -23,24 +23,104 @@ controls: > >>>>> > >>>>> \sa AeEnable > >>>>> > >>>>> - - AwbEnable: > >>>>> - type: bool > >>>>> + - AeMeteringMode: > >>>>> + type: int32_t > >>>>> description: | > >>>>> - Enable or disable the AWB. > >>>>> - > >>>>> - \sa ManualGain > >>>>> + Specify a metering mode for the AE algorithm to use. The metering > >>>>> + modes determine which parts of the image are used to determine the > >>>>> + scene brightness. Metering modes may be platform-specific and not > >>>> > >>>> Only caught because of the spelling error below, but in the other > >>>> instances you do not hyphenate platform-specific. Maybe the simple > >>>> option is to not hyphenate here either :-) > >>>> > >>>>> + all metering modes may be supported. > >>>>> + enum: > >>>>> + - name: MeteringCentreWeighted > >>>>> + value: 0 > >>>>> + description: Centre-weighted metering mode. > >>>>> + - name: MeteringSpot > >>>>> + value: 1 > >>>>> + description: Spot metering mode. > >>>>> + - name: MeteringMatrix > >>>>> + value: 2 > >>>>> + description: Matrix metering mode. > >>>>> + - name: MeteringCustom1 > >>>>> + value: 3 > >>>> > >>>> This feels a bit painful if we want to add more (non-custom, or custom) > >>>> modes. > >>>> > >>>> We would only be able to append to prevent ABI breakage - Then the enums > >>>> could seem a bit ugly: > >>>> > >>>> enum { > >>>> MeteringCentreWeighted, > >>>> MeteringSpot, > >>>> MeteringMatrix, > >>>> MeteringCustom1, > >>>> MeteringCustom2, > >>>> MeteringCustom3, > >>>> MeteringMadeUpGeneralControl, > >>>> MeteringCustom4, > >>>> MeteringMadeupButGeneralControl2, > >>>> } > >>>> > >>>> But I fear that may not be something we can easily avoid unless we know > >>>> *all* of the metering mode names in advance... > >>>> > >>>> Of course we're intentionally not yet ABI stable anyway, so this could > >>>> still be an intermediate step... > >>>> > >>>> One option could be to have MeteringCustom, with the actual 'custom' > >>>> choice provided by another means ('yet another' control?), but maybe > >>>> that doesn't scale well either. I'm not sure yet... > >>>> > >>>>> + description: Custom metering mode 1. > >>>>> + - name: MeteringCustom2 > >>>>> + value: 4 > >>>>> + description: Custom metering mode 2. > >>>>> + - name: MeteringCustom3 > >>>>> + value: 5 > >>>>> + description: Custom metering mode 3. > >>>> > >>>> Are you able to express what you currently envisage the 3 custom modes > >>>> would be used for? > >>> > >>> The intention here was to allow users to add new modes to the > >>> algorithm, perhaps something very specific to their application. If > >>> the tuning parameters for the mode could be added/tweaked via a > >>> text/config file, then the user could directly use the new mode > >>> without ever having to download/re-compile any code. Initially (as in > >>> patch v1), these modes were defined as std::string types, but we > >>> concluded that added too much vagueness to the list of options. > >>> Perhaps 3 custom modes is too much, and we can make do with just one? > >> > >> Maybe one is enough I don't know. That's why I wondered about one custom > >> control which has a secondary option to give more flexibility. > >> > >> Otherwise '3' seems like a very arbitrary definition ... > > > > This is indeed an issue, I'll try to sleep over it. Having a single > > value wouldn't be too bad when it comes to extending the enum, but that > > may not be enough. > > > > That being said, for this specific control, I wonder if we should make > > it a bit more finer-grained by specifying a list of weighted regions. > > Just brainstorming, the ControlInfo (previously known as ControlRange) > > class could then possibly offer a set of defaults based on an enum. > > This is possible, but it might get tricky trying to interoperate with > different vendor IPAs where the HW/SW capabilities may not be the > same. I agree, that's why I was wondering what would be a good way to express metering (which will apply to AF too, and potentially AWB ?) in a flexible way that could give finer-grained control when the hardware and IPA support it, but wouldn't be awkward to use for devices that have more limited capabilities. We don't have to fix all of this now, it could come as a later rework of the controls, but I wanted to raise the issue already in case we could figure out a solution in the short term. > >>>>> + - name: MeteringModeMax > >>>>> + value: 5 > >>>> > >>>> It's a shame we have to express the enum max value manually here :-( > >>>> I haven't looked at if it could be identified through code though. > >>> > >>> This is primarily needed for setting up ControlRange for the control. > >>> It should be entirely possible for the gen_controls script to add the > >>> Max item to the end of the enum? > >> > >> Aha, then indeed perhaps we should automate the creation somehow > > > > Yes, we should do that. > > As mentioned previously, the max value is only used for ControlInfo > (previously known as ControlRange). I do wonder if ControlInfo is a > sensible thing to have for an enum though. Suppose an IPA allows only > metering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be > specified as (min, max) == (0, 5), but a value of 3 is invalid. I agree with you. We need ControlInfo to report the valid values for enumerated controls. Another item on our todo list :-) > >>>>> + description: Maximum allowed value (place any new values above here). > >>>>> > >>>>> - - Brightness: > >>>>> + - AeConstraintMode: > >>>>> type: int32_t > >>>>> - description: Specify a fixed brightness parameter > >>>>> + description: | > >>>>> + Specify a constraint mode for the AE algorithm to use. These determine > >>>>> + how the measured scene brightness is adjusted to reach the desired > >>>>> + target exposure. Constraint modes may be platform specific, and not > >>>>> + all constaint modes may be supported. > >>>> > >>>> /constaint/constraint/ > >>>> > >>>>> + enum: > >>>>> + - name: ConstraintNormal > >>>>> + value: 0 > >>>>> + description: Default constraint mode. > >>>>> + - name: ConstraintHighlight > >>>>> + value: 1 > >>>>> + description: Highlight constraint mode. > > > > Would it be possible to give a more detailed description of those modes > > ? > > Will do. > > >>>>> + - name: ConstraintCustom1 > >>>>> + value: 2 > >>>>> + description: Custom constraint mode 1. > >>>>> + - name: ConstraintCustom2 > >>>>> + value: 3 > >>>>> + description: Custom constraint mode 2. > >>>>> + - name: ConstraintCustom3 > >>>>> + value: 4 > >>>>> + description: Custom constraint mode 3. > >>>>> + - name: ConstraintModeMax > >>>>> + value: 4 > >>>>> + description: Maximum allowed value (place any new values above here). > >>>> > >>>> Same comments apply here of course. > >>>> > >>>> Perhaps we should expand the initial set from the modes/constraints that > >>>> we will need for supporting the Android HAL... > >>>> > >>>>> - - Contrast: > >>>>> + - AeExposureMode: > >>>>> type: int32_t > >>>>> - description: Specify a fixed contrast parameter > >>>>> + description: | > >>>>> + Specify an exposure mode for the AE algorithm to use. These specify > >>>>> + how the desired total exposure is divided between the shutter time > >>>>> + and the sensor's analogue gain. The exposure modes are platform > >>>>> + spcific, and not all exposure modes may be supported. > >>>> > >>>> s/spcific/specific/ > >>>> > >>>>> + enum: > >>>>> + - name: ExposureNormal > >>>>> + value: 0 > >>>>> + description: Default metering mode. > >>>>> + - name: ExposureSport > >>>>> + value: 1 > >>>>> + description: Sport metering mode (favouring short shutter times). > > > > Reading the description, I wonder if this should be turned into a scene > > mode control, as it could also have an effect on other algorithms > > (auto-focus comes to mind for instance). > > I always looked as scene mode as a higher level construct within the > camera application. The application bunches a group of settings for a > particular scene mode (e.g. for Sport scene mode, AE turned to sport, > focus turned to fast converge, flash turned to fast sync) and sets > them individually. This way it could tailor them exactly to what it > needs. That makes sense to me, but should the lower-level settings then use scene mode names ? We could for instance name this ExposureShort (very easy to mistype ExposureSport :-)) for instance (or any other name that would convey the underlying behaviour). I trust your expertise on this topic. > >>>>> + - name: ExposureLong > >>>>> + value: 2 > >>>>> + description: Exposure mode allowing long exposure times. > >>>>> + - name: ExposureCustom1 > >>>>> + value: 3 > >>>>> + description: Custom exposure mode 1. > >>>>> + - name: ExposureCustom2 > >>>>> + value: 4 > >>>>> + description: Custom exposure mode 2. > >>>>> + - name: ExposureCustom3 > >>>>> + value: 5 > >>>>> + description: Custom exposure mode 3. > >>>>> + - name: ExposureModeMax > >>>>> + value: 5 > >>>>> + description: Maximum allowed value (place any new values above here). > >>>>> > >>>>> - - Saturation: > >>>>> - type: int32_t > >>>>> - description: Specify a fixed saturation parameter > >>>>> + - ExposureValue: > >>>>> + type: float > >>>>> + description: | > >>>>> + Specify an Exposure Value (EV) parameter. The EV parameter will only be > >>>>> + applied if the AE alogrithm is currently enabled. > >>>>> + > >>>>> + By convention EV adjusts the exposure as log2. For example > >>>>> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment > >>>>> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. > >>>>> + > >>>>> + \sa AeEnable > >>>>> > >>>>> - ManualExposure: > >>>>> type: int32_t > >>>>> @@ -57,4 +137,21 @@ controls: > >>>>> applied to all colour channels. > >>>>> > >>>>> \sa ManualExposure > >>>>> + > >>>> > >>>> What changes were made to the properties below ? (I don't see anything > >>>> in particular) or is that just git being a pain and adding churn? > >>> > >>> I only moved them around - so all the controls in the file are ordered > >>> in appropriate groups (AGC, AWB, General...). > >> > >> Aha - in that case, it would generally be better to commit any > >> non-functional-change code move as a separate patch, so that it's clear > >> and distinct on it's own. > >> > >>>>> + - AwbEnable: > >>>>> + type: bool > >>>>> + description: | > >>>>> + Enable or disable the AWB. > >>>>> + > >>>>> + - Brightness: > >>>>> + type: int32_t > >>>>> + description: Specify a fixed brightness parameter > >>>>> + > >>>>> + - Contrast: > >>>>> + type: int32_t > >>>>> + description: Specify a fixed contrast parameter > >>>>> + > >>>>> + - Saturation: > >>>>> + type: int32_t > >>>>> + description: Specify a fixed saturation parameter > >>>>> ...
HI Laurent, On Fri, 27 Mar 2020 at 11:03, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Fri, Mar 27, 2020 at 10:56:14AM +0000, Naushir Patuck wrote: > > On Thu, 26 Mar 2020 at 16:26, Laurent Pinchart wrote: > > > On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote: > > >> On 23/03/2020 15:29, Naushir Patuck wrote: > > >>> On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote: > > >>>> On 09/03/2020 12:33, Naushir Patuck wrote: > > >>>>> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type > > >>>>> controls used to specify operating modes in the AE algorithm. All modes > > >>>>> may not be supported by all platforms. > > >>>>> > > >>>>> ExposureValue is a new double type control used to set the log2 exposure > > >>>>> adjustment for the AE algorithm. > > >>>>> > > >>>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > >>>>> --- > > >>>>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- > > >>>>> 1 file changed, 109 insertions(+), 12 deletions(-) > > >>>>> > > >>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > >>>>> index 5bbe65ae..da1a7b43 100644 > > >>>>> --- a/src/libcamera/control_ids.yaml > > >>>>> +++ b/src/libcamera/control_ids.yaml > > >>>>> @@ -23,24 +23,104 @@ controls: > > >>>>> > > >>>>> \sa AeEnable > > >>>>> > > >>>>> - - AwbEnable: > > >>>>> - type: bool > > >>>>> + - AeMeteringMode: > > >>>>> + type: int32_t > > >>>>> description: | > > >>>>> - Enable or disable the AWB. > > >>>>> - > > >>>>> - \sa ManualGain > > >>>>> + Specify a metering mode for the AE algorithm to use. The metering > > >>>>> + modes determine which parts of the image are used to determine the > > >>>>> + scene brightness. Metering modes may be platform-specific and not > > >>>> > > >>>> Only caught because of the spelling error below, but in the other > > >>>> instances you do not hyphenate platform-specific. Maybe the simple > > >>>> option is to not hyphenate here either :-) > > >>>> > > >>>>> + all metering modes may be supported. > > >>>>> + enum: > > >>>>> + - name: MeteringCentreWeighted > > >>>>> + value: 0 > > >>>>> + description: Centre-weighted metering mode. > > >>>>> + - name: MeteringSpot > > >>>>> + value: 1 > > >>>>> + description: Spot metering mode. > > >>>>> + - name: MeteringMatrix > > >>>>> + value: 2 > > >>>>> + description: Matrix metering mode. > > >>>>> + - name: MeteringCustom1 > > >>>>> + value: 3 > > >>>> > > >>>> This feels a bit painful if we want to add more (non-custom, or custom) > > >>>> modes. > > >>>> > > >>>> We would only be able to append to prevent ABI breakage - Then the enums > > >>>> could seem a bit ugly: > > >>>> > > >>>> enum { > > >>>> MeteringCentreWeighted, > > >>>> MeteringSpot, > > >>>> MeteringMatrix, > > >>>> MeteringCustom1, > > >>>> MeteringCustom2, > > >>>> MeteringCustom3, > > >>>> MeteringMadeUpGeneralControl, > > >>>> MeteringCustom4, > > >>>> MeteringMadeupButGeneralControl2, > > >>>> } > > >>>> > > >>>> But I fear that may not be something we can easily avoid unless we know > > >>>> *all* of the metering mode names in advance... > > >>>> > > >>>> Of course we're intentionally not yet ABI stable anyway, so this could > > >>>> still be an intermediate step... > > >>>> > > >>>> One option could be to have MeteringCustom, with the actual 'custom' > > >>>> choice provided by another means ('yet another' control?), but maybe > > >>>> that doesn't scale well either. I'm not sure yet... > > >>>> > > >>>>> + description: Custom metering mode 1. > > >>>>> + - name: MeteringCustom2 > > >>>>> + value: 4 > > >>>>> + description: Custom metering mode 2. > > >>>>> + - name: MeteringCustom3 > > >>>>> + value: 5 > > >>>>> + description: Custom metering mode 3. > > >>>> > > >>>> Are you able to express what you currently envisage the 3 custom modes > > >>>> would be used for? > > >>> > > >>> The intention here was to allow users to add new modes to the > > >>> algorithm, perhaps something very specific to their application. If > > >>> the tuning parameters for the mode could be added/tweaked via a > > >>> text/config file, then the user could directly use the new mode > > >>> without ever having to download/re-compile any code. Initially (as in > > >>> patch v1), these modes were defined as std::string types, but we > > >>> concluded that added too much vagueness to the list of options. > > >>> Perhaps 3 custom modes is too much, and we can make do with just one? > > >> > > >> Maybe one is enough I don't know. That's why I wondered about one custom > > >> control which has a secondary option to give more flexibility. > > >> > > >> Otherwise '3' seems like a very arbitrary definition ... > > > > > > This is indeed an issue, I'll try to sleep over it. Having a single > > > value wouldn't be too bad when it comes to extending the enum, but that > > > may not be enough. > > > > > > That being said, for this specific control, I wonder if we should make > > > it a bit more finer-grained by specifying a list of weighted regions. > > > Just brainstorming, the ControlInfo (previously known as ControlRange) > > > class could then possibly offer a set of defaults based on an enum. > > > > This is possible, but it might get tricky trying to interoperate with > > different vendor IPAs where the HW/SW capabilities may not be the > > same. > > I agree, that's why I was wondering what would be a good way to express > metering (which will apply to AF too, and potentially AWB ?) in a > flexible way that could give finer-grained control when the hardware and > IPA support it, but wouldn't be awkward to use for devices that have > more limited capabilities. We don't have to fix all of this now, it > could come as a later rework of the controls, but I wanted to raise the > issue already in case we could figure out a solution in the short term. > > > >>>>> + - name: MeteringModeMax > > >>>>> + value: 5 > > >>>> > > >>>> It's a shame we have to express the enum max value manually here :-( > > >>>> I haven't looked at if it could be identified through code though. > > >>> > > >>> This is primarily needed for setting up ControlRange for the control. > > >>> It should be entirely possible for the gen_controls script to add the > > >>> Max item to the end of the enum? > > >> > > >> Aha, then indeed perhaps we should automate the creation somehow > > > > > > Yes, we should do that. > > > > As mentioned previously, the max value is only used for ControlInfo > > (previously known as ControlRange). I do wonder if ControlInfo is a > > sensible thing to have for an enum though. Suppose an IPA allows only > > metering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be > > specified as (min, max) == (0, 5), but a value of 3 is invalid. > > I agree with you. We need ControlInfo to report the valid values for > enumerated controls. Another item on our todo list :-) > > > >>>>> + description: Maximum allowed value (place any new values above here). > > >>>>> > > >>>>> - - Brightness: > > >>>>> + - AeConstraintMode: > > >>>>> type: int32_t > > >>>>> - description: Specify a fixed brightness parameter > > >>>>> + description: | > > >>>>> + Specify a constraint mode for the AE algorithm to use. These determine > > >>>>> + how the measured scene brightness is adjusted to reach the desired > > >>>>> + target exposure. Constraint modes may be platform specific, and not > > >>>>> + all constaint modes may be supported. > > >>>> > > >>>> /constaint/constraint/ > > >>>> > > >>>>> + enum: > > >>>>> + - name: ConstraintNormal > > >>>>> + value: 0 > > >>>>> + description: Default constraint mode. > > >>>>> + - name: ConstraintHighlight > > >>>>> + value: 1 > > >>>>> + description: Highlight constraint mode. > > > > > > Would it be possible to give a more detailed description of those modes > > > ? > > > > Will do. > > > > >>>>> + - name: ConstraintCustom1 > > >>>>> + value: 2 > > >>>>> + description: Custom constraint mode 1. > > >>>>> + - name: ConstraintCustom2 > > >>>>> + value: 3 > > >>>>> + description: Custom constraint mode 2. > > >>>>> + - name: ConstraintCustom3 > > >>>>> + value: 4 > > >>>>> + description: Custom constraint mode 3. > > >>>>> + - name: ConstraintModeMax > > >>>>> + value: 4 > > >>>>> + description: Maximum allowed value (place any new values above here). > > >>>> > > >>>> Same comments apply here of course. > > >>>> > > >>>> Perhaps we should expand the initial set from the modes/constraints that > > >>>> we will need for supporting the Android HAL... > > >>>> > > >>>>> - - Contrast: > > >>>>> + - AeExposureMode: > > >>>>> type: int32_t > > >>>>> - description: Specify a fixed contrast parameter > > >>>>> + description: | > > >>>>> + Specify an exposure mode for the AE algorithm to use. These specify > > >>>>> + how the desired total exposure is divided between the shutter time > > >>>>> + and the sensor's analogue gain. The exposure modes are platform > > >>>>> + spcific, and not all exposure modes may be supported. > > >>>> > > >>>> s/spcific/specific/ > > >>>> > > >>>>> + enum: > > >>>>> + - name: ExposureNormal > > >>>>> + value: 0 > > >>>>> + description: Default metering mode. > > >>>>> + - name: ExposureSport > > >>>>> + value: 1 > > >>>>> + description: Sport metering mode (favouring short shutter times). > > > > > > Reading the description, I wonder if this should be turned into a scene > > > mode control, as it could also have an effect on other algorithms > > > (auto-focus comes to mind for instance). > > > > I always looked as scene mode as a higher level construct within the > > camera application. The application bunches a group of settings for a > > particular scene mode (e.g. for Sport scene mode, AE turned to sport, > > focus turned to fast converge, flash turned to fast sync) and sets > > them individually. This way it could tailor them exactly to what it > > needs. > > That makes sense to me, but should the lower-level settings then use > scene mode names ? We could for instance name this ExposureShort (very > easy to mistype ExposureSport :-)) for instance (or any other name that > would convey the underlying behaviour). I trust your expertise on this > topic. Yes, that seems reasonable. > > > >>>>> + - name: ExposureLong > > >>>>> + value: 2 > > >>>>> + description: Exposure mode allowing long exposure times. > > >>>>> + - name: ExposureCustom1 > > >>>>> + value: 3 > > >>>>> + description: Custom exposure mode 1. > > >>>>> + - name: ExposureCustom2 > > >>>>> + value: 4 > > >>>>> + description: Custom exposure mode 2. > > >>>>> + - name: ExposureCustom3 > > >>>>> + value: 5 > > >>>>> + description: Custom exposure mode 3. > > >>>>> + - name: ExposureModeMax > > >>>>> + value: 5 > > >>>>> + description: Maximum allowed value (place any new values above here). > > >>>>> > > >>>>> - - Saturation: > > >>>>> - type: int32_t > > >>>>> - description: Specify a fixed saturation parameter > > >>>>> + - ExposureValue: > > >>>>> + type: float > > >>>>> + description: | > > >>>>> + Specify an Exposure Value (EV) parameter. The EV parameter will only be > > >>>>> + applied if the AE alogrithm is currently enabled. > > >>>>> + > > >>>>> + By convention EV adjusts the exposure as log2. For example > > >>>>> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment > > >>>>> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. > > >>>>> + > > >>>>> + \sa AeEnable > > >>>>> > > >>>>> - ManualExposure: > > >>>>> type: int32_t > > >>>>> @@ -57,4 +137,21 @@ controls: > > >>>>> applied to all colour channels. > > >>>>> > > >>>>> \sa ManualExposure > > >>>>> + > > >>>> > > >>>> What changes were made to the properties below ? (I don't see anything > > >>>> in particular) or is that just git being a pain and adding churn? > > >>> > > >>> I only moved them around - so all the controls in the file are ordered > > >>> in appropriate groups (AGC, AWB, General...). > > >> > > >> Aha - in that case, it would generally be better to commit any > > >> non-functional-change code move as a separate patch, so that it's clear > > >> and distinct on it's own. > > >> > > >>>>> + - AwbEnable: > > >>>>> + type: bool > > >>>>> + description: | > > >>>>> + Enable or disable the AWB. > > >>>>> + > > >>>>> + - Brightness: > > >>>>> + type: int32_t > > >>>>> + description: Specify a fixed brightness parameter > > >>>>> + > > >>>>> + - Contrast: > > >>>>> + type: int32_t > > >>>>> + description: Specify a fixed contrast parameter > > >>>>> + > > >>>>> + - Saturation: > > >>>>> + type: int32_t > > >>>>> + description: Specify a fixed saturation parameter > > >>>>> ... > > -- > Regards, > > Laurent Pinchart Regards, Naush
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index 5bbe65ae..da1a7b43 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -23,24 +23,104 @@ controls: \sa AeEnable - - AwbEnable: - type: bool + - AeMeteringMode: + type: int32_t description: | - Enable or disable the AWB. - - \sa ManualGain + Specify a metering mode for the AE algorithm to use. The metering + modes determine which parts of the image are used to determine the + scene brightness. Metering modes may be platform-specific and not + all metering modes may be supported. + enum: + - name: MeteringCentreWeighted + value: 0 + description: Centre-weighted metering mode. + - name: MeteringSpot + value: 1 + description: Spot metering mode. + - name: MeteringMatrix + value: 2 + description: Matrix metering mode. + - name: MeteringCustom1 + value: 3 + description: Custom metering mode 1. + - name: MeteringCustom2 + value: 4 + description: Custom metering mode 2. + - name: MeteringCustom3 + value: 5 + description: Custom metering mode 3. + - name: MeteringModeMax + value: 5 + description: Maximum allowed value (place any new values above here). - - Brightness: + - AeConstraintMode: type: int32_t - description: Specify a fixed brightness parameter + description: | + Specify a constraint mode for the AE algorithm to use. These determine + how the measured scene brightness is adjusted to reach the desired + target exposure. Constraint modes may be platform specific, and not + all constaint modes may be supported. + enum: + - name: ConstraintNormal + value: 0 + description: Default constraint mode. + - name: ConstraintHighlight + value: 1 + description: Highlight constraint mode. + - name: ConstraintCustom1 + value: 2 + description: Custom constraint mode 1. + - name: ConstraintCustom2 + value: 3 + description: Custom constraint mode 2. + - name: ConstraintCustom3 + value: 4 + description: Custom constraint mode 3. + - name: ConstraintModeMax + value: 4 + description: Maximum allowed value (place any new values above here). - - Contrast: + - AeExposureMode: type: int32_t - description: Specify a fixed contrast parameter + description: | + Specify an exposure mode for the AE algorithm to use. These specify + how the desired total exposure is divided between the shutter time + and the sensor's analogue gain. The exposure modes are platform + spcific, and not all exposure modes may be supported. + enum: + - name: ExposureNormal + value: 0 + description: Default metering mode. + - name: ExposureSport + value: 1 + description: Sport metering mode (favouring short shutter times). + - name: ExposureLong + value: 2 + description: Exposure mode allowing long exposure times. + - name: ExposureCustom1 + value: 3 + description: Custom exposure mode 1. + - name: ExposureCustom2 + value: 4 + description: Custom exposure mode 2. + - name: ExposureCustom3 + value: 5 + description: Custom exposure mode 3. + - name: ExposureModeMax + value: 5 + description: Maximum allowed value (place any new values above here). - - Saturation: - type: int32_t - description: Specify a fixed saturation parameter + - ExposureValue: + type: float + description: | + Specify an Exposure Value (EV) parameter. The EV parameter will only be + applied if the AE alogrithm is currently enabled. + + By convention EV adjusts the exposure as log2. For example + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x]. + + \sa AeEnable - ManualExposure: type: int32_t @@ -57,4 +137,21 @@ controls: applied to all colour channels. \sa ManualExposure + + - AwbEnable: + type: bool + description: | + Enable or disable the AWB. + + - Brightness: + type: int32_t + description: Specify a fixed brightness parameter + + - Contrast: + type: int32_t + description: Specify a fixed contrast parameter + + - Saturation: + type: int32_t + description: Specify a fixed saturation parameter ...
AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type controls used to specify operating modes in the AE algorithm. All modes may not be supported by all platforms. ExposureValue is a new double type control used to set the log2 exposure adjustment for the AE algorithm. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++---- 1 file changed, 109 insertions(+), 12 deletions(-)