[libcamera-devel] [RFC 2/6] libcamera: property_ids: Define draft properties
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 7 17:11:27 CEST 2020
Hi Jacopo,
On 07/10/2020 15:40, Jacopo Mondi wrote:
> 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.
Well - 'just dropping the DRAFT_' wasn't quite the point ;-) The name
can always be changed at the same time as updating the draft status (or
it could be done in two stages as well, I don't care).
The point of the namespacing in my head was to kind of group, and
clearly mark the controls by nature of their usage.
The namespace really highlights that it's a 'property' of the name, and
not a part of the name.
(I'm getting extreme to try to highlight my angle of course)
'DRAFT_EXCLUDE_RANGE'
The 'draft exclude' there could be interpreted as a single term (a draft
excluder).
Whereas:
DRAFT::EXCLUDE_RANGE
Clearly defines the difference/separation ;-0
--
Kieran
> 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.
Indeed, I think for now - it's not something I'm worried about.
>
>>
>> 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
Aha - I see - those are the actual enum values that get assigned to the
properties! I see now ;-)
>
>>
>> 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.
>
I think we need to be consistent in the way we style these properties in
our namespace though ...
>>> + - 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
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list