[libcamera-devel] [RFC 2/6] libcamera: property_ids: Define draft properties

[Jacopo Mondi]

Hi Kieran,

On Wed, Oct 07, 2020 at 03:23:12PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 11/09/2020 17:20, Jacopo Mondi wrote:
> > Libcamera is in the process of defining its own set of properties, to
> > 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
> > properties whose values are taken from their Android definition, in order
> > to allow pipeline handlers to support Android.
> >
>
> I had it in my head that these draft controls were going to be
> namespaced such that it was accessible through:
>
> switch(props.get(properties::draft::AvailableNoiseReductionModes) {
> 	case properties::draft::NOISE_REDUCTION_MODE_OFF:
> 	case properties::draft::NOISE_REDUCTION_MODE_FAST:
> 		// We don't do noise reduction
> 		break;
> 	case properties::draft::NOISE_REDUCTION_MODE_HIGH_QUALITY:
> DRAFT_NOISE_REDUCTION_MODE_MINIMAL
> }
>
> And that inserting (or removing) a control to/from that draft namespace
> would be about setting a property on the control definition like:
>
> +  - AvailableNoiseReductionModes:
> +      draft: true
> +      type: int32_t
> +      size: [n]
> +      description: |
>
> Or of course it could be:
> 	status: draft
> or
> 	status: staging
>
> But that's all bikeshed colours.
>
>
> Is the reason you've used a prefix (on every value) because that's the
> way you want it? or because you haven't looked at updating the control
> generation scripts to support the 'draft' property?
>
> If it's the latter, I'll tackle that now for you.
> (I'm going to have a quick go to see how hard it would be to implement).
>

I don't think it's worth as I suspect very few draft controls will be made
libcamera controls by just dropping 'draft' from their name.

There are Android defined controls, plain and simple. For each of them
there will be corresponding libcamera control.

Instrumenting the control generation script for this purpose seems a
bit an unjustified effort.

>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/property_ids.yaml | 70 +++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 74ad0195d631..62bed9f3844d 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -640,4 +640,74 @@ controls:
> >          \todo Rename this property to ActiveAreas once we will have property
> >                categories (i.e. Properties::PixelArray::ActiveAreas)
> >
> > +  - DraftAvailableNoiseReductionModes:
> > +      type: int32_t
> > +      size: [n]
> > +      description: |
> > +        Draft property to report the list of supported noise reduction modes.
> > +        Currently identical to
> > +        ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES.
> > +      enum:
> > +        - name: DRAFT_NOISE_REDUCTION_MODE_OFF
> > +          value: 0
> > +          description: No noise reduction is applied
> > +        - name: DRAFT_NOISE_REDUCTION_MODE_FAST
> > +          value: 1
> > +          description: |
> > +            Noise reduction is applied without reducing the frame rate.
> > +        - name: DRAFT_NOISE_REDUCTION_MODE_HIGH_QUALITY
> > +          value: 2
> > +          description: |
> > +            High quality noise reduction at the expense of frame rate.
> > +        - name: DRAFT_NOISE_REDUCTION_MODE_MINIMAL
> > +          value: 3
> > +          description: |
> > +            Minimal noise reduction is applied without reducing the frame rate.
> > +        - name: DRAFT_NOISE_REDUCTION_MODE_ZSL
> > +          value: 4
> > +          description: |
> > +            Noise reduction is applied at different levels to different streams.
>
> This implies that different streams can process noise reduction
> separately? but it's confusing because it's referencing ZeroShutterLag?
> So - I don't (yet) understand how that corresponds to the noise
> reduction. Maybe something for me to learn soon.

Not sure. Just imported those values from the Android metadata
description.

>
> Anyway, especially as these are 'DRAFT/::draft::' whatever - I don't
> think we need to dwell 'too' much on the detail, as the point of being a
> draft is to allow the plumbing to progress, and fix the detail before
> moving out of draft status.
>
>
> The other point I'd like to discuss here, is that you have used
> DRAFT_ALL_CAPS_ENUM style for all the enums, which matches the
> annonymous enum in
> include/libcamera/property_ids.h
> enum {
>         LOCATION = 1,
>         ROTATION = 2,
>         MODEL = 3,
>         UNIT_CELL_SIZE = 4,
>         PIXEL_ARRAY_SIZE = 5,
>         PIXEL_ARRAY_OPTICAL_BLACK_RECTANGLES = 6,
>         PIXEL_ARRAY_ACTIVE_AREAS = 7,
> };
>

These are controls/properties identifiers

>
> but differs to the LocationValues enum:
> enum LocationValues {
>         CameraLocationFront = 0,
>         CameraLocationBack = 1,
>         CameraLocationExternal = 2,
> };
>
> My personal preference would be to follow the LocationValues style, as
> it feels more C++/libcamery ;-)
>
> But - it's just an open question for the moment.

Again, just because Android named them in this way and staying
consistent with their definition has value to me.

>
>
>
> > +
> > +  - DraftAvailableColorCorrectionAberrationModes:
> > +      type: int32_t
> > +      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: DRAFT_COLOR_CORRECTION_ABERRATION_OFF
> > +          value: 0
> > +          description: No aberration correction is applied.
> > +        - name: DRAFT_COLOR_CORRECTION_ABERRATION_FAST
> > +          value: 1
> > +          description: Aberration correction will not slow down the frame rate.
> > +        - name: DRAFT_COLOR_CORRECTION_ABERRATION_HIGH_QUALITY
> > +          value: 2
> > +          description: |
> > +            High quality aberration correction which might reduce the frame
> > +            rate.
> > +
> > +  - DraftAvailableLensShadingMapModes:
> > +      type: int32_t
> > +      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: DRAFT_LENS_SHADING_MAP_MODE_OFF
> > +          value: 0
> > +          description: No lens shading map mode is available.
> > +        - name: DRAFT_LENS_SHADING_MAP_MODE_ON
> > +          value: 1
> > +          description: The lens shading map mode is available.
> > +
> > +  - DraftPipelineMaxDepth:
> > +      type: int32_t
> > +      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
> --
> Kieran