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

[Kieran Bingham]

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).


> 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.

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,
};


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.



> +
> +  - 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