[libcamera-devel] [PATCH 03/10] libcamera: property_ids: Define draft properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Oct 09, 2020 at 01:26:00PM +0100, Kieran Bingham wrote:
> On 09/10/2020 13:20, Jacopo Mondi wrote:
> > Libcamera is in the process of defining its own set of properties, to

s/Libcamera/libcamera/

> > advertise the camera capabilities.
> > 
> > To temporary close the gap in the Android camera HAL and support all
> 
> s/temporary/temporarily/
> 
> > static metadata required in the LIMITED hw level, define a set of Draft

s/hw/hardware/ ?

> > properties whose values are taken from their Android definition, in order
> > to allow pipeline handlers to support them.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/property_ids.yaml | 74 +++++++++++++++++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> > 
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 7261263a9ba3..e524a0718dc3 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -663,4 +663,78 @@ controls:
> >          \todo Rename this property to ActiveAreas once we will have property
> >                categories (i.e. Properties::PixelArray::ActiveAreas)
> >  
> > +  - AvailableNoiseReductionModes:
> > +      type: int32_t
> > +      draft: true
> > +      size: [n]
> > +      description: |
> > +        Draft property to report the list of supported noise reduction modes.
> > +        Currently identical to
> > +        ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES.
> 
> I don't think it's necessary to say "Draft property" in the
> descriptions, and that's defined by the draft flag.
> 
> However - I interpret "draft: true", to be a statement that all/any of
> the implementation of the control will/can be updated, and therefore
> that includes the description - and thus ... you can put in anything you
> like that you feel is appropriate to get started ;-)
> 
> For me - the parts I would check on draft controls are that the styles
> are consistent and no glaring errors to tackle - so that makes adding
> draft controls easy.
> 
> (And getting out of draft status is where the important stage is).
> 
> So -
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

I would also drop draft in the description, but both option are fine
with me.

> > +      enum:
> > +        - name: NoiseReductionModeOff
> > +          value: 0
> > +          description: No noise reduction is applied
> > +        - name: NoiseReductionModeFast
> > +          value: 1
> > +          description: |
> > +            Noise reduction is applied without reducing the frame rate.
> > +        - name: NoiseReductionModeHighQuality
> > +          value: 2
> > +          description: |
> > +            High quality noise reduction at the expense of frame rate.
> > +        - name: NoiseReductionModeMinimal
> > +          value: 3
> > +          description: |
> > +            Minimal noise reduction is applied without reducing the frame rate.
> > +        - name: NoiseReductionModeZSL
> > +          value: 4
> > +          description: |
> > +            Noise reduction is applied at different levels to different streams.

This one will likely be handled using per-stream controls, which will
then influence the other values.

> > +
> > +  - AvailableColorCorrectionAberrationModes:
> > +      type: int32_t
> > +      draft: true
> > +      size: [n]
> > +      description: |
> > +        Draft property to report the list of supported color correction
> > +        aberration modes. Currently identical to
> > +        ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES.
> > +      enum:
> > +        - name: ColorCorrectionAberrationOff
> > +          value: 0
> > +          description: No aberration correction is applied.
> > +        - name: ColorCorrectionAberrationFast
> > +          value: 1
> > +          description: Aberration correction will not slow down the frame rate.
> > +        - name: ColorCorrectionAberrationHighQuality
> > +          value: 2
> > +          description: |
> > +            High quality aberration correction which might reduce the frame
> > +            rate.
> > +
> > +  - AvailableLensShadingMapModes:
> > +      type: int32_t
> > +      draft: true
> > +      size: [n]
> > +      description: |
> > +        Draft property to report the list of supported lens shading map modes.
> > +        Currently identical to
> > +        ANDROID_STATISTICS_INFO_AVAILABLE_LENS_SHADING_MAP_MODES.
> > +      enum:
> > +        - name: LensShadingMapModeOff
> > +          value: 0
> > +          description: No lens shading map mode is available.
> > +        - name: LensShadingMapModeOn
> > +          value: 1
> > +          description: The lens shading map mode is available.

Note that these three properties should be handled through ControlInfo
instead of being manually defined. For instance, the LensShadingMapMode
control will have a ControlInfo that should list the supported values.
We don't have this yet in ControlInfo for enum controls, so it's not a
candidate for this patch series, but let's keep the direction in mind.

Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +
> > +  - PipelineMaxDepth:
> > +      type: int32_t
> > +      draft: true
> > +      description: |
> > +        Draft control to report the maximum number of pipeline stages a frame
> > +        has to go through from when it is exposed to when it is available to
> > +        applications. Currently identical to ANDROID_REQUEST_PIPELINE_MAX_DEPTH.
> > +
> >  ...
> > 

-- 
Regards,

Laurent Pinchart