| Message ID | 20200309123319.630-7-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 09/03/2020 12:33, Naushir Patuck wrote: > Add controls to report the current frame's exposure and gain, and an > estimate of the current lux level from the AE algorithm. > > Add controls to report the red and blue colour gains, and an estimate of > the colour temperature from the AWB algorithm. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index 3d1b058f..ee93975e 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -138,6 +138,22 @@ controls: > > \sa ManualExposure > > + - CurrentExposure: > + type: int32_t > + description: Report the exposure time in micro-seconds of this frame. > + > + \sa CurrentAnalogueGain > + > + - CurrentAnalogueGain: > + type: float > + description: Report the analogue gain parameter for the current frame. > + > + \sa CurrentExposure > + > + - CurrentLux: > + type: float > + description: Report an estimate of the current lux level. > + > - AwbEnable: > type: bool > description: | > @@ -190,6 +206,18 @@ controls: > in that order. > size: [2] > > + - CurrentWbGains: > + type: float > + description: | > + Report the current gain AWB gain values for the Red and Blue colour > + channels, in that order. > + size: [2] > + > + - CurrentTemperature: > + type: float > + description: | > + Report the current estimate of the colour temperature for this frame. Do we need to explicitly define the scale/units of the temperature? (Presumably people would assume kelvins, but perhaps people could misinterpret this somehow if we're not explicit) > + > - Brightness: > type: int32_t > description: | >
Yes, we return the colour temperature in Kelvin. But you're right, and I think there's a general point here. How is code going to get less platform specific if we don't define what all these numbers mean, by which I mean both the controls that you set and the values that may get reported back? David On Fri, 20 Mar 2020 at 15:45, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Naush, > > On 09/03/2020 12:33, Naushir Patuck wrote: > > Add controls to report the current frame's exposure and gain, and an > > estimate of the current lux level from the AE algorithm. > > > > Add controls to report the red and blue colour gains, and an estimate of > > the colour temperature from the AWB algorithm. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index 3d1b058f..ee93975e 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -138,6 +138,22 @@ controls: > > > > \sa ManualExposure > > > > + - CurrentExposure: > > + type: int32_t > > + description: Report the exposure time in micro-seconds of this frame. > > + > > + \sa CurrentAnalogueGain > > + > > + - CurrentAnalogueGain: > > + type: float > > + description: Report the analogue gain parameter for the current frame. > > + > > + \sa CurrentExposure > > + > > + - CurrentLux: > > + type: float > > + description: Report an estimate of the current lux level. > > + > > - AwbEnable: > > type: bool > > description: | > > @@ -190,6 +206,18 @@ controls: > > in that order. > > size: [2] > > > > + - CurrentWbGains: > > + type: float > > + description: | > > + Report the current gain AWB gain values for the Red and Blue colour > > + channels, in that order. > > + size: [2] > > + > > + - CurrentTemperature: > > + type: float > > + description: | > > + Report the current estimate of the colour temperature for this frame. > > Do we need to explicitly define the scale/units of the temperature? > (Presumably people would assume kelvins, but perhaps people could > misinterpret this somehow if we're not explicit) > > > + > > - Brightness: > > type: int32_t > > description: | > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Fri, Mar 20, 2020 at 04:25:49PM +0000, David Plowman wrote: > Yes, we return the colour temperature in Kelvin. But you're right, and > I think there's a general point here. How is code going to get less > platform specific if we don't define what all these numbers mean, by > which I mean both the controls that you set and the values that may > get reported back? I think defining the units and ranges is the best approach when possible. Controls with custom units should be an exception. For ranges, they could certainly be platform specific for some controls, but that's less of an issue if the units are well-defined. > On Fri, 20 Mar 2020 at 15:45, Kieran Bingham wrote: > > On 09/03/2020 12:33, Naushir Patuck wrote: > > > Add controls to report the current frame's exposure and gain, and an > > > estimate of the current lux level from the AE algorithm. > > > > > > Add controls to report the red and blue colour gains, and an estimate of > > > the colour temperature from the AWB algorithm. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > index 3d1b058f..ee93975e 100644 > > > --- a/src/libcamera/control_ids.yaml > > > +++ b/src/libcamera/control_ids.yaml > > > @@ -138,6 +138,22 @@ controls: > > > > > > \sa ManualExposure > > > > > > + - CurrentExposure: > > > + type: int32_t > > > + description: Report the exposure time in micro-seconds of this frame. > > > + > > > + \sa CurrentAnalogueGain > > > + I don't think we need a separate control for this. We have ManualExposure already (which I think we'll rename to ExposureTime), and you can use it in metadata. It's best to use the same control for the request control and metadata when they relate to the same value. > > > + - CurrentAnalogueGain: > > > + type: float > > > + description: Report the analogue gain parameter for the current frame. > > > + > > > + \sa CurrentExposure Does this one correspond to any of the other controls ? > > > + > > > + - CurrentLux: > > > + type: float > > > + description: Report an estimate of the current lux level. I don't think we need the 'Current' prefix. Reporting a control in the request metadata implies it's the current value. > > > + > > > - AwbEnable: > > > type: bool > > > description: | > > > @@ -190,6 +206,18 @@ controls: > > > in that order. > > > size: [2] > > > > > > + - CurrentWbGains: > > > + type: float > > > + description: | > > > + Report the current gain AWB gain values for the Red and Blue colour > > > + channels, in that order. > > > + size: [2] For this one we already have a control. > > > + > > > + - CurrentTemperature: > > > + type: float > > > + description: | > > > + Report the current estimate of the colour temperature for this frame. > > > > Do we need to explicitly define the scale/units of the temperature? > > (Presumably people would assume kelvins, but perhaps people could > > misinterpret this somehow if we're not explicit) And here we can drop the Current prefix too. I'd call it ColorTemperature (or ColourTemperature ?) though, to avoid confusion with the temperature of the camera sensor that we may decide to report later on. > > > > > + > > > - Brightness: > > > type: int32_t > > > description: |
Hi Laurent, On Thu, 26 Mar 2020 at 16:34, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Fri, Mar 20, 2020 at 04:25:49PM +0000, David Plowman wrote: > > Yes, we return the colour temperature in Kelvin. But you're right, and > > I think there's a general point here. How is code going to get less > > platform specific if we don't define what all these numbers mean, by > > which I mean both the controls that you set and the values that may > > get reported back? > > I think defining the units and ranges is the best approach when > possible. Controls with custom units should be an exception. For ranges, > they could certainly be platform specific for some controls, but that's > less of an issue if the units are well-defined. > > > On Fri, 20 Mar 2020 at 15:45, Kieran Bingham wrote: > > > On 09/03/2020 12:33, Naushir Patuck wrote: > > > > Add controls to report the current frame's exposure and gain, and an > > > > estimate of the current lux level from the AE algorithm. > > > > > > > > Add controls to report the red and blue colour gains, and an estimate of > > > > the colour temperature from the AWB algorithm. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ > > > > 1 file changed, 28 insertions(+) > > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > > index 3d1b058f..ee93975e 100644 > > > > --- a/src/libcamera/control_ids.yaml > > > > +++ b/src/libcamera/control_ids.yaml > > > > @@ -138,6 +138,22 @@ controls: > > > > > > > > \sa ManualExposure > > > > > > > > + - CurrentExposure: > > > > + type: int32_t > > > > + description: Report the exposure time in micro-seconds of this frame. > > > > + > > > > + \sa CurrentAnalogueGain > > > > + > > I don't think we need a separate control for this. We have > ManualExposure already (which I think we'll rename to ExposureTime), and > you can use it in metadata. It's best to use the same control for the > request control and metadata when they relate to the same value. > That seems like a good option. We can rename ManualExposure to ExposureTime and reuse for the return metadata. > > > > + - CurrentAnalogueGain: > > > > + type: float > > > > + description: Report the analogue gain parameter for the current frame. > > > > + > > > > + \sa CurrentExposure > > Does this one correspond to any of the other controls ? CurrentAnalogueGain has the corresponding control ManualGain (which we are having a separate discussion about) so we should be able to reuse that control. > > > > > + > > > > + - CurrentLux: > > > > + type: float > > > > + description: Report an estimate of the current lux level. > > I don't think we need the 'Current' prefix. Reporting a control in the > request metadata implies it's the current value. Will remove the Current prefix. > > > > > + > > > > - AwbEnable: > > > > type: bool > > > > description: | > > > > @@ -190,6 +206,18 @@ controls: > > > > in that order. > > > > size: [2] > > > > > > > > + - CurrentWbGains: > > > > + type: float > > > > + description: | > > > > + Report the current gain AWB gain values for the Red and Blue colour > > > > + channels, in that order. > > > > + size: [2] > > For this one we already have a control. > > > > > + > > > > + - CurrentTemperature: > > > > + type: float > > > > + description: | > > > > + Report the current estimate of the colour temperature for this frame. > > > > > > Do we need to explicitly define the scale/units of the temperature? > > > (Presumably people would assume kelvins, but perhaps people could > > > misinterpret this somehow if we're not explicit) > > And here we can drop the Current prefix too. I'd call it > ColorTemperature (or ColourTemperature ?) though, to avoid confusion > with the temperature of the camera sensor that we may decide to report > later on. Will do. > > > > > > > + > > > > - Brightness: > > > > type: int32_t > > > > description: | > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel Regards, Naush
Hi Naush, On Fri, Mar 27, 2020 at 10:59:56AM +0000, Naushir Patuck wrote: > On Thu, 26 Mar 2020 at 16:34, Laurent Pinchart wrote: > > On Fri, Mar 20, 2020 at 04:25:49PM +0000, David Plowman wrote: > > > Yes, we return the colour temperature in Kelvin. But you're right, and > > > I think there's a general point here. How is code going to get less > > > platform specific if we don't define what all these numbers mean, by > > > which I mean both the controls that you set and the values that may > > > get reported back? > > > > I think defining the units and ranges is the best approach when > > possible. Controls with custom units should be an exception. For ranges, > > they could certainly be platform specific for some controls, but that's > > less of an issue if the units are well-defined. > > > > > On Fri, 20 Mar 2020 at 15:45, Kieran Bingham wrote: > > > > On 09/03/2020 12:33, Naushir Patuck wrote: > > > > > Add controls to report the current frame's exposure and gain, and an > > > > > estimate of the current lux level from the AE algorithm. > > > > > > > > > > Add controls to report the red and blue colour gains, and an estimate of > > > > > the colour temperature from the AWB algorithm. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ > > > > > 1 file changed, 28 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > > > index 3d1b058f..ee93975e 100644 > > > > > --- a/src/libcamera/control_ids.yaml > > > > > +++ b/src/libcamera/control_ids.yaml > > > > > @@ -138,6 +138,22 @@ controls: > > > > > > > > > > \sa ManualExposure > > > > > > > > > > + - CurrentExposure: > > > > > + type: int32_t > > > > > + description: Report the exposure time in micro-seconds of this frame. > > > > > + > > > > > + \sa CurrentAnalogueGain > > > > > + > > > > I don't think we need a separate control for this. We have > > ManualExposure already (which I think we'll rename to ExposureTime), and > > you can use it in metadata. It's best to use the same control for the > > request control and metadata when they relate to the same value. > > That seems like a good option. We can rename ManualExposure to > ExposureTime and reuse for the return metadata. > > > > > > + - CurrentAnalogueGain: > > > > > + type: float > > > > > + description: Report the analogue gain parameter for the current frame. > > > > > + > > > > > + \sa CurrentExposure > > > > Does this one correspond to any of the other controls ? > > CurrentAnalogueGain has the corresponding control ManualGain (which we > are having a separate discussion about) so we should be able to reuse > that control. Should we then rename ManualGain to AnalogGain ? > > > > > + > > > > > + - CurrentLux: > > > > > + type: float > > > > > + description: Report an estimate of the current lux level. > > > > I don't think we need the 'Current' prefix. Reporting a control in the > > request metadata implies it's the current value. > > Will remove the Current prefix. > > > > > > + > > > > > - AwbEnable: > > > > > type: bool > > > > > description: | > > > > > @@ -190,6 +206,18 @@ controls: > > > > > in that order. > > > > > size: [2] > > > > > > > > > > + - CurrentWbGains: > > > > > + type: float > > > > > + description: | > > > > > + Report the current gain AWB gain values for the Red and Blue colour > > > > > + channels, in that order. > > > > > + size: [2] > > > > For this one we already have a control. > > > > > > > + > > > > > + - CurrentTemperature: > > > > > + type: float > > > > > + description: | > > > > > + Report the current estimate of the colour temperature for this frame. > > > > > > > > Do we need to explicitly define the scale/units of the temperature? > > > > (Presumably people would assume kelvins, but perhaps people could > > > > misinterpret this somehow if we're not explicit) > > > > And here we can drop the Current prefix too. I'd call it > > ColorTemperature (or ColourTemperature ?) though, to avoid confusion > > with the temperature of the camera sensor that we may decide to report > > later on. > > Will do. > > > > > > > > > > + > > > > > - Brightness: > > > > > type: int32_t > > > > > description: |
HI Laurent, On Fri, 27 Mar 2020 at 11:04, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Fri, Mar 27, 2020 at 10:59:56AM +0000, Naushir Patuck wrote: > > On Thu, 26 Mar 2020 at 16:34, Laurent Pinchart wrote: > > > On Fri, Mar 20, 2020 at 04:25:49PM +0000, David Plowman wrote: > > > > Yes, we return the colour temperature in Kelvin. But you're right, and > > > > I think there's a general point here. How is code going to get less > > > > platform specific if we don't define what all these numbers mean, by > > > > which I mean both the controls that you set and the values that may > > > > get reported back? > > > > > > I think defining the units and ranges is the best approach when > > > possible. Controls with custom units should be an exception. For ranges, > > > they could certainly be platform specific for some controls, but that's > > > less of an issue if the units are well-defined. > > > > > > > On Fri, 20 Mar 2020 at 15:45, Kieran Bingham wrote: > > > > > On 09/03/2020 12:33, Naushir Patuck wrote: > > > > > > Add controls to report the current frame's exposure and gain, and an > > > > > > estimate of the current lux level from the AE algorithm. > > > > > > > > > > > > Add controls to report the red and blue colour gains, and an estimate of > > > > > > the colour temperature from the AWB algorithm. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ > > > > > > 1 file changed, 28 insertions(+) > > > > > > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > > > > index 3d1b058f..ee93975e 100644 > > > > > > --- a/src/libcamera/control_ids.yaml > > > > > > +++ b/src/libcamera/control_ids.yaml > > > > > > @@ -138,6 +138,22 @@ controls: > > > > > > > > > > > > \sa ManualExposure > > > > > > > > > > > > + - CurrentExposure: > > > > > > + type: int32_t > > > > > > + description: Report the exposure time in micro-seconds of this frame. > > > > > > + > > > > > > + \sa CurrentAnalogueGain > > > > > > + > > > > > > I don't think we need a separate control for this. We have > > > ManualExposure already (which I think we'll rename to ExposureTime), and > > > you can use it in metadata. It's best to use the same control for the > > > request control and metadata when they relate to the same value. > > > > That seems like a good option. We can rename ManualExposure to > > ExposureTime and reuse for the return metadata. > > > > > > > > + - CurrentAnalogueGain: > > > > > > + type: float > > > > > > + description: Report the analogue gain parameter for the current frame. > > > > > > + > > > > > > + \sa CurrentExposure > > > > > > Does this one correspond to any of the other controls ? > > > > CurrentAnalogueGain has the corresponding control ManualGain (which we > > are having a separate discussion about) so we should be able to reuse > > that control. > > Should we then rename ManualGain to AnalogGain ? > Yes, that's certainly a better name. > > > > > > + > > > > > > + - CurrentLux: > > > > > > + type: float > > > > > > + description: Report an estimate of the current lux level. > > > > > > I don't think we need the 'Current' prefix. Reporting a control in the > > > request metadata implies it's the current value. > > > > Will remove the Current prefix. > > > > > > > > + > > > > > > - AwbEnable: > > > > > > type: bool > > > > > > description: | > > > > > > @@ -190,6 +206,18 @@ controls: > > > > > > in that order. > > > > > > size: [2] > > > > > > > > > > > > + - CurrentWbGains: > > > > > > + type: float > > > > > > + description: | > > > > > > + Report the current gain AWB gain values for the Red and Blue colour > > > > > > + channels, in that order. > > > > > > + size: [2] > > > > > > For this one we already have a control. > > > > > > > > > + > > > > > > + - CurrentTemperature: > > > > > > + type: float > > > > > > + description: | > > > > > > + Report the current estimate of the colour temperature for this frame. > > > > > > > > > > Do we need to explicitly define the scale/units of the temperature? > > > > > (Presumably people would assume kelvins, but perhaps people could > > > > > misinterpret this somehow if we're not explicit) > > > > > > And here we can drop the Current prefix too. I'd call it > > > ColorTemperature (or ColourTemperature ?) though, to avoid confusion > > > with the temperature of the camera sensor that we may decide to report > > > later on. > > > > Will do. > > > > > > > > > > > > > + > > > > > > - Brightness: > > > > > > type: int32_t > > > > > > description: | > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index 3d1b058f..ee93975e 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -138,6 +138,22 @@ controls: \sa ManualExposure + - CurrentExposure: + type: int32_t + description: Report the exposure time in micro-seconds of this frame. + + \sa CurrentAnalogueGain + + - CurrentAnalogueGain: + type: float + description: Report the analogue gain parameter for the current frame. + + \sa CurrentExposure + + - CurrentLux: + type: float + description: Report an estimate of the current lux level. + - AwbEnable: type: bool description: | @@ -190,6 +206,18 @@ controls: in that order. size: [2] + - CurrentWbGains: + type: float + description: | + Report the current gain AWB gain values for the Red and Blue colour + channels, in that order. + size: [2] + + - CurrentTemperature: + type: float + description: | + Report the current estimate of the colour temperature for this frame. + - Brightness: type: int32_t description: |
Add controls to report the current frame's exposure and gain, and an estimate of the current lux level from the AE algorithm. Add controls to report the red and blue colour gains, and an estimate of the colour temperature from the AWB algorithm. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)