Message ID | 20200217142609.22837-4-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Naush, Thank you for the patch. On Mon, Feb 17, 2020 at 02:26:09PM +0000, Naushir Patuck wrote: > AeMode is a new std::string type control used to set the AE algorithm > mode. The mode may not always be supported by all platforms. > > AwbMode is a new std::string type control used to set the AWB algorithm > illuminant mode. The mode may not always be supported by all platforms. > > EV is a new double type control used to set the log2 exposure adjustment > for the AE algorithm. > > ManualGainR is a new double type control used to set the Red channel > gain in manual AWB mode. > > ManualGainB is a new double type control used to set the Blue channel > gain in manual AWB mode. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.org> > --- > src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index 33062d6..21d2065 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -50,4 +50,44 @@ controls: > type: double > description: Specify a fixed gain parameter. > > + - AeMode: > + type: std::string > + description: | > + Specify an exposure mode for the AE algorithm to use. The exposure modes > + supported are platform spcific, and not all modes may be supported. s/spcific/specific/ > + > + Examples include "normal", "sport", "low light", etc. > + > + - AwbMode: > + type: std::string > + description: | > + Specify the range of illuminant to use for the AWB algorihtm. The modes > + supported are platform specific, and not all modes may be supported. > + > + Examples include "auto", "incandescent", "daylight", etc. I think we should use enumerated values instead of strings for both controls. I do agree that strings have the nice benefit of being easily extensible, but they will be much more difficult to use for applications. A typical end-user camera application will display icons for the different modes, and will thus need to map strings to icons. In order to allow doing so, we would need to specify what mode strings are supported in this spec, which will remove the extensibility feature of strings. Strings would then have no advantage over numerical values anymore. I do however agree that an extension mechanism can be useful, but to design it properly, I think we need to discuss how such extensions would be used by applications. > + > + - EV: Would it make sense to spell it out to ExposureValue ? > + type: double > + description: | > + Specify an Exposure Value (EV) parameter. > + > + By convention, EV adjusts the exposure by a factor of 2^EV. 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]. You should also explain here how this interacts with the AeEnable control. One option would be to mention that the control only applies when AE is enabled, and that if AE is disabled, the EV value being set will take effect next time AE is enabled. I'm open to discussing alternative proposals. I would also like to know what values you envision being supported for this control. Would an IPA typically support a continuous range, or discrete values ? > + - ManualGainR: > + type: double > + description: | > + Specify a fixed gain parameter for the Red colour channel. This must be > + set together with ManualGainB to be applied. > + > + \sa ManualGainB > + > + - ManualGainB: > + type: double > + description: | > + Specify a fixed gain parameter for the Green colour channel. This must > + bet together with AwbManualGainR to be applied. > + > + \sa ManualGainR How do those two controls interact with ManualGain ? And where in the pipeline do they apply ? > ...
Hi Laurent, On Tue, 18 Feb 2020 at 00:59, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Mon, Feb 17, 2020 at 02:26:09PM +0000, Naushir Patuck wrote: > > AeMode is a new std::string type control used to set the AE algorithm > > mode. The mode may not always be supported by all platforms. > > > > AwbMode is a new std::string type control used to set the AWB algorithm > > illuminant mode. The mode may not always be supported by all platforms. > > > > EV is a new double type control used to set the log2 exposure adjustment > > for the AE algorithm. > > > > ManualGainR is a new double type control used to set the Red channel > > gain in manual AWB mode. > > > > ManualGainB is a new double type control used to set the Blue channel > > gain in manual AWB mode. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.org> > > --- > > src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index 33062d6..21d2065 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -50,4 +50,44 @@ controls: > > type: double > > description: Specify a fixed gain parameter. > > > > + - AeMode: > > + type: std::string > > + description: | > > + Specify an exposure mode for the AE algorithm to use. The exposure modes > > + supported are platform spcific, and not all modes may be supported. > > s/spcific/specific/ > > > + > > + Examples include "normal", "sport", "low light", etc. > > + > > + - AwbMode: > > + type: std::string > > + description: | > > + Specify the range of illuminant to use for the AWB algorihtm. The modes > > + supported are platform specific, and not all modes may be supported. > > + > > + Examples include "auto", "incandescent", "daylight", etc. > > I think we should use enumerated values instead of strings for both > controls. I do agree that strings have the nice benefit of being easily > extensible, but they will be much more difficult to use for > applications. A typical end-user camera application will display icons > for the different modes, and will thus need to map strings to icons. In > order to allow doing so, we would need to specify what mode strings are > supported in this spec, which will remove the extensibility feature of > strings. Strings would then have no advantage over numerical values > anymore. > > I do however agree that an extension mechanism can be useful, but to > design it properly, I think we need to discuss how such extensions would > be used by applications. Yes, this is a tricky one, and the choice here will not please everyone. I will switch to using enums in the next version of the patchset. One strong argument for using strings is to allow users (who may not be technically inclined) to add new modes to the tuning and be able to use them without having the need to every recompile any code. To that end, what if I add, for example, {Custom1 Custom2 Custom3} enum values to the AE Mode? This will allow the user to set the mode in the tuning configuration, and select it in the application without needing to ever recompile. What do you think? On a related note, do you expect any mechanism where the IPAs advertise which enums values are supported - this will be entirely platform/sensor/tuning dependent. > > > + > > + - EV: > > Would it make sense to spell it out to ExposureValue ? > Yes, that sounds better. > > + type: double > > + description: | > > + Specify an Exposure Value (EV) parameter. > > + > > + By convention, EV adjusts the exposure by a factor of 2^EV. 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]. > > You should also explain here how this interacts with the AeEnable > control. One option would be to mention that the control only applies > when AE is enabled, and that if AE is disabled, the EV value being set > will take effect next time AE is enabled. I'm open to discussing > alternative proposals. > I think having EV applied only when AE is enabled makes sense - and indeed, this is what our platform will do. I will update the description. > I would also like to know what values you envision being supported for > this control. Would an IPA typically support a continuous range, or > discrete values ? > Our platform supports a continuous range, I expect others will do as well. Normally I would expect discretizing the allowable values should be handled by the camera application. > > + - ManualGainR: > > + type: double > > + description: | > > + Specify a fixed gain parameter for the Red colour channel. This must be > > + set together with ManualGainB to be applied. > > + > > + \sa ManualGainB > > + > > + - ManualGainB: > > + type: double > > + description: | > > + Specify a fixed gain parameter for the Green colour channel. This must > > + bet together with AwbManualGainR to be applied. > > + > > + \sa ManualGainR > > How do those two controls interact with ManualGain ? And where in the > pipeline do they apply ? > I took ManualGain to be the sensor analogue gain value (although, it could equally be applied to sensor digital gain I suppose). This pairs with the ManualExposure sensor ctrl. ManualGainR and ManualGainB are specific to the white balance control when AWB is off and we provide these fixed gains to apply in the ISP pipeline. So there is no link between ManualGain and ManualGainR/ManualGainB. Does this seem reasonable? > > ... > > -- > Regards, > > Laurent Pinchart Regards, Naush
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index 33062d6..21d2065 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -50,4 +50,44 @@ controls: type: double description: Specify a fixed gain parameter. + - AeMode: + type: std::string + description: | + Specify an exposure mode for the AE algorithm to use. The exposure modes + supported are platform spcific, and not all modes may be supported. + + Examples include "normal", "sport", "low light", etc. + + - AwbMode: + type: std::string + description: | + Specify the range of illuminant to use for the AWB algorihtm. The modes + supported are platform specific, and not all modes may be supported. + + Examples include "auto", "incandescent", "daylight", etc. + + - EV: + type: double + description: | + Specify an Exposure Value (EV) parameter. + + By convention, EV adjusts the exposure by a factor of 2^EV. 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]. + + - ManualGainR: + type: double + description: | + Specify a fixed gain parameter for the Red colour channel. This must be + set together with ManualGainB to be applied. + + \sa ManualGainB + + - ManualGainB: + type: double + description: | + Specify a fixed gain parameter for the Green colour channel. This must + bet together with AwbManualGainR to be applied. + + \sa ManualGainR ...
AeMode is a new std::string type control used to set the AE algorithm mode. The mode may not always be supported by all platforms. AwbMode is a new std::string type control used to set the AWB algorithm illuminant mode. The mode may not always be supported by all platforms. EV is a new double type control used to set the log2 exposure adjustment for the AE algorithm. ManualGainR is a new double type control used to set the Red channel gain in manual AWB mode. ManualGainB is a new double type control used to set the Blue channel gain in manual AWB mode. Signed-off-by: Naushir Patuck <naush@raspberrypi.org> --- src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)