[libcamera-devel,v1,4/5] libcamera: controls: Use vendor tags for draft controls and properties

19197 diff mbox series

Label draft controls and properties through the "draft" vendor tag
and deprecate the existing "draft: true" mechanism. This uses the new
vendor tags mechanism to place draft controls in the same
libcamera::controls::draft namespace and provide a defined control id
range for these controls.

One breaking change in this commit is that draft control ids also move
to the libcamera::controls::draft namespace from the existing
libcamera::controls namespace. This is desirable to avoid API breakages
when adding new libcamera controls. So, for example, the use of
controls::NOISE_REDUCTION_MODE will need to be replaced with
controls::draft::NOISE_REDUCTION_MODE.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/control_ids.h.in            |  6 ---
 include/libcamera/property_ids.h.in           |  6 ---
 src/ipa/rpi/common/ipa_base.cpp               |  2 +-
 src/ipa/rpi/vc4/vc4.cpp                       |  2 +-
 src/libcamera/control_ids.cpp.in              | 16 +-------
 src/libcamera/control_ids.yaml                | 20 +++++-----
 src/libcamera/property_ids.cpp.in             | 16 +-------
 src/libcamera/property_ids.yaml               |  2 +-
 src/py/libcamera/gen-py-controls.py           |  5 +--
 src/py/libcamera/py_controls_generated.cpp.in |  5 ---
 .../libcamera/py_properties_generated.cpp.in  |  1 -
 utils/gen-controls.py                         | 39 +++++--------------
 12 files changed, 27 insertions(+), 93 deletions(-)

Hi Naush,

Thank you for the patch.

On Fri, Nov 10, 2023 at 11:00:01AM +0000, Naushir Patuck via libcamera-devel wrote:
> Label draft controls and properties through the "draft" vendor tag
> and deprecate the existing "draft: true" mechanism. This uses the new
> vendor tags mechanism to place draft controls in the same
> libcamera::controls::draft namespace and provide a defined control id
> range for these controls.
> 
> One breaking change in this commit is that draft control ids also move
> to the libcamera::controls::draft namespace from the existing
> libcamera::controls namespace. This is desirable to avoid API breakages
> when adding new libcamera controls. So, for example, the use of
> controls::NOISE_REDUCTION_MODE will need to be replaced with
> controls::draft::NOISE_REDUCTION_MODE.

The idea looks good to me, but I'll hold off reviewing the changes in
details as discussions on previous poatches may result in a different
implementation here.

> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/control_ids.h.in            |  6 ---
>  include/libcamera/property_ids.h.in           |  6 ---
>  src/ipa/rpi/common/ipa_base.cpp               |  2 +-
>  src/ipa/rpi/vc4/vc4.cpp                       |  2 +-
>  src/libcamera/control_ids.cpp.in              | 16 +-------
>  src/libcamera/control_ids.yaml                | 20 +++++-----
>  src/libcamera/property_ids.cpp.in             | 16 +-------
>  src/libcamera/property_ids.yaml               |  2 +-
>  src/py/libcamera/gen-py-controls.py           |  5 +--
>  src/py/libcamera/py_controls_generated.cpp.in |  5 ---
>  .../libcamera/py_properties_generated.cpp.in  |  1 -
>  utils/gen-controls.py                         | 39 +++++--------------
>  12 files changed, 27 insertions(+), 93 deletions(-)
> 
> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> index c97b09a82450..d53b1cf7beb2 100644
> --- a/include/libcamera/control_ids.h.in
> +++ b/include/libcamera/control_ids.h.in
> @@ -26,12 +26,6 @@ ${controls}
>  
>  extern const ControlIdMap controls;
>  
> -namespace draft {
> -
> -${draft_controls}
> -
> -} /* namespace draft */
> -
>  ${vendor_controls}
>  
>  } /* namespace controls */
> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> index 47c5d6bf2e28..43372c718fc9 100644
> --- a/include/libcamera/property_ids.h.in
> +++ b/include/libcamera/property_ids.h.in
> @@ -23,12 +23,6 @@ ${ids}
>  
>  ${controls}
>  
> -namespace draft {
> -
> -${draft_controls}
> -
> -} /* namespace draft */
> -
>  extern const ControlIdMap properties;
>  
>  ${vendor_controls}
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index a1fec3aa3dd1..6ac9d5de2f88 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -1024,7 +1024,7 @@ void IpaBase::applyControls(const ControlList &controls)
>  			break;
>  		}
>  
> -		case controls::NOISE_REDUCTION_MODE:
> +		case controls::draft::NOISE_REDUCTION_MODE:
>  			/* Handled below in handleControls() */
>  			libcameraMetadata_.set(controls::draft::NoiseReductionMode,
>  					       ctrl.second.get<int32_t>());
> diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp
> index c4baf04fb1e7..c165a5b8b0b6 100644
> --- a/src/ipa/rpi/vc4/vc4.cpp
> +++ b/src/ipa/rpi/vc4/vc4.cpp
> @@ -260,7 +260,7 @@ void IpaVc4::handleControls(const ControlList &controls)
>  
>  	for (auto const &ctrl : controls) {
>  		switch (ctrl.first) {
> -		case controls::NOISE_REDUCTION_MODE: {
> +		case controls::draft::NOISE_REDUCTION_MODE: {
>  			RPiController::DenoiseAlgorithm *sdn = dynamic_cast<RPiController::DenoiseAlgorithm *>(
>  				controller_.getAlgorithm("SDN"));
>  			/* Some platforms may have a combined "denoise" algorithm instead. */
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index d26eb930b30c..284589567cfb 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -24,15 +24,6 @@ namespace controls {
>  
>  ${controls_doc}
>  
> -/**
> - * \brief Namespace for libcamera draft controls
> - */
> -namespace draft {
> -
> -${draft_controls_doc}
> -
> -} /* namespace draft */
> -
>  ${vendor_controls_doc}
>  
>  #ifndef __DOXYGEN__
> @@ -42,13 +33,8 @@ ${vendor_controls_doc}
>   */
>  ${controls_def}
>  
> -namespace draft {
> -
> -${draft_controls_def}
> -
> -} /* namespace draft */
> -
>  ${vendor_controls_def}
> +
>  #endif
>  
>  /**
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 5827d7ecef49..ce65522e1b71 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -869,7 +869,7 @@ controls:
>  
>    - AePrecaptureTrigger:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>          Control for AE metering trigger. Currently identical to
>          ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER.
> @@ -891,7 +891,7 @@ controls:
>  
>    - NoiseReductionMode:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>         Control to select the noise reduction algorithm mode. Currently
>         identical to ANDROID_NOISE_REDUCTION_MODE.
> @@ -920,7 +920,7 @@ controls:
>  
>    - ColorCorrectionAberrationMode:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>         Control to select the color correction aberration mode. Currently
>         identical to ANDROID_COLOR_CORRECTION_ABERRATION_MODE.
> @@ -941,7 +941,7 @@ controls:
>  
>    - AeState:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>         Control to report the current AE algorithm state. Currently identical to
>         ANDROID_CONTROL_AE_STATE.
> @@ -971,7 +971,7 @@ controls:
>  
>    - AwbState:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>         Control to report the current AWB algorithm state. Currently identical
>         to ANDROID_CONTROL_AWB_STATE.
> @@ -993,7 +993,7 @@ controls:
>  
>    - SensorRollingShutterSkew:
>        type: int64_t
> -      draft: true
> +      vendor: draft
>        description: |
>         Control to report the time between the start of exposure of the first
>         row and the start of exposure of the last row. Currently identical to
> @@ -1001,7 +1001,7 @@ controls:
>  
>    - LensShadingMapMode:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>         Control to report if the lens shading map is available. Currently
>         identical to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE.
> @@ -1015,7 +1015,7 @@ controls:
>  
>    - PipelineDepth:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>          Specifies the number of pipeline stages the frame went through from when
>          it was exposed to when the final completed result was available to the
> @@ -1030,7 +1030,7 @@ controls:
>  
>    - MaxLatency:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>          The maximum number of frames that can occur after a request (different
>          than the previous) has been submitted, and before the result's state
> @@ -1040,7 +1040,7 @@ controls:
>  
>    - TestPatternMode:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>          Control to select the test pattern mode. Currently identical to
>          ANDROID_SENSOR_TEST_PATTERN_MODE.
> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> index ddbe714c3f00..b72e12e4cc70 100644
> --- a/src/libcamera/property_ids.cpp.in
> +++ b/src/libcamera/property_ids.cpp.in
> @@ -23,14 +23,7 @@ namespace properties {
>  
>  ${controls_doc}
>  
> -/**
> - * \brief Namespace for libcamera draft properties
> - */
> -namespace draft {
> -
> -${draft_controls_doc}
> -
> -} /* namespace draft */
> +${vendor_controls_doc}
>  
>  ${vendor_controls_doc}
>  
> @@ -41,13 +34,8 @@ ${vendor_controls_doc}
>   */
>  ${controls_def}
>  
> -namespace draft {
> -
> -${draft_controls_def}
> -
> -} /* namespace draft */
> -
>  ${vendor_controls_def}
> +
>  #endif
>  
>  /**
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index f35563842a5a..622d0a8cf26f 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -705,7 +705,7 @@ controls:
>  
>    - ColorFilterArrangement:
>        type: int32_t
> -      draft: true
> +      vendor: draft
>        description: |
>          The arrangement of color filters on sensor; represents the colors in the
>          top-left 2x2 section of the sensor, in reading order. Currently
> diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py
> index a32ef09c2f48..9416be6e611d 100755
> --- a/src/py/libcamera/gen-py-controls.py
> +++ b/src/py/libcamera/gen-py-controls.py
> @@ -36,10 +36,7 @@ def generate_py(controls, mode):
>              vendor_defs.append('\tauto {} = py::class_<Py{}Controls>(controls, \"{}\");'.format(vendor, vendor, vendor))
>              vendors.append(vendor)
>  
> -        if ctrl.get('draft'):
> -            ns = 'libcamera::{}::draft::'.format(mode)
> -            container = 'draft'
> -        elif vendor:
> +        if vendor:
>              ns = 'libcamera::{}::{}::'.format(mode, vendor)
>              container = vendor
>          else:
> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in
> index ec4b55ef2011..8d282ce51856 100644
> --- a/src/py/libcamera/py_controls_generated.cpp.in
> +++ b/src/py/libcamera/py_controls_generated.cpp.in
> @@ -17,16 +17,11 @@ class PyControls
>  {
>  };
>  
> -class PyDraftControls
> -{
> -};
> -
>  ${vendors_class_def}
>  
>  void init_py_controls_generated(py::module& m)
>  {
>  	auto controls = py::class_<PyControls>(m, "controls");
> -	auto draft = py::class_<PyDraftControls>(controls, "draft");
>  ${vendors_defs}
>  
>  ${controls}
> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in
> index 87bc5e5d937e..1fa1967a9672 100644
> --- a/src/py/libcamera/py_properties_generated.cpp.in
> +++ b/src/py/libcamera/py_properties_generated.cpp.in
> @@ -26,7 +26,6 @@ ${vendors_class_def}
>  void init_py_properties_generated(py::module& m)
>  {
>  	auto controls = py::class_<PyProperties>(m, "properties");
> -	auto draft = py::class_<PyDraftProperties>(controls, "draft");
>  ${vendors_defs}	
>  
>  ${controls}
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index a10b9fdc19ee..3ae48ca3e0bc 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -84,11 +84,6 @@ class Control(object):
>          """Is the control an enumeration"""
>          return self.__enum_values is not None
>  
> -    @property
> -    def is_draft(self):
> -        """Is the control a draft control"""
> -        return self.__data.get('draft') is not None
> -
>      @property
>      def vendor(self):
>          """The vendor string, or None"""
> @@ -99,12 +94,6 @@ class Control(object):
>          """The control name (CamelCase)"""
>          return self.__name
>  
> -    @property
> -    def q_name(self):
> -        """The control name, qualified with a namespace"""
> -        ns = 'draft::' if self.is_draft else ''
> -        return ns + self.__name
> -
>      @property
>      def type(self):
>          typ = self.__data.get('type')
> @@ -159,7 +148,7 @@ ${description}
>  
>          vendor = ctrl.vendor
>          if vendor is None:
> -            vendor = 'draft' if ctrl.is_draft else 'libcamera'
> +            vendor = 'libcamera'
>  
>          if vendor not in ctrls_doc:
>              ctrls_doc[vendor] = []
> @@ -211,7 +200,7 @@ ${description}
>          target_doc.append(doc_template.substitute(info))
>          target_def.append(def_template.substitute(info))
>  
> -        target_ctrls_map.append('\t{ ' + id_name + ', &' + ctrl.q_name + ' },')
> +        target_ctrls_map.append('\t{ ' + id_name + ', &' + ctrl.name + ' },')
>  
>      vendor_ctrl_doc_sub = []
>      vendor_ctrl_template = string.Template('''
> @@ -221,11 +210,11 @@ ${vendor_controls_str}
>  
>  } /* namespace ${vendor} */''')
>  
> -    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:
> +    for vendor in [v for v in ctrls_map.keys() if v != 'libcamera']:
>          vendor_ctrl_doc_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\n\n'.join(ctrls_doc[vendor])}))
>  
>      vendor_ctrl_def_sub = []
> -    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:
> +    for vendor in [v for v in ctrls_map.keys() if v != 'libcamera']:
>          vendor_ctrl_def_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\n'.join(ctrls_def[vendor])}))
>  
>      vendor_ctrl_map_sub = []
> @@ -243,16 +232,14 @@ ${vendor_controls_map}
>  } /* namespace ${vendor} */
>  ''')
>  
> -    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:
> +    for vendor in [v for v in ctrls_map.keys() if v != 'libcamera']:
>          vendor_ctrl_map_sub.append(vendor_ctrl_template.substitute({'vendor': vendor,
>                                                                      'vendor_controls_map': '\n'.join(ctrls_map[vendor])}))
>  
>      return {
>          'controls_doc': '\n\n'.join(ctrls_doc['libcamera']),
>          'controls_def': '\n'.join(ctrls_def['libcamera']),
> -        'draft_controls_doc': '\n\n'.join(ctrls_doc['draft']),
> -        'draft_controls_def': '\n\n'.join(ctrls_def['draft']),
> -        'controls_map': '\n'.join(ctrls_map['libcamera'] + ctrls_map['draft']),
> +        'controls_map': '\n'.join(ctrls_map['libcamera']),
>          'vendor_controls_doc': '\n'.join(vendor_ctrl_doc_sub),
>          'vendor_controls_def': '\n'.join(vendor_ctrl_def_sub),
>          'vendor_controls_map': '\n'.join(vendor_ctrl_map_sub),
> @@ -274,7 +261,7 @@ def generate_h(controls, mode, ranges):
>  
>          vendor = ctrl.vendor
>          if vendor is None:
> -            vendor = 'draft' if ctrl.is_draft else 'libcamera'
> +            vendor = 'libcamera'
>  
>          if vendor not in ctrls:
>              if vendor not in ranges.keys():
> @@ -283,8 +270,7 @@ def generate_h(controls, mode, ranges):
>              ids[vendor] = []
>              ctrls[vendor] = []
>  
> -        # Core and draft controls use the same ID value
> -        target_ids = ids['libcamera'] if vendor in ['libcamera', 'draft'] else ids[vendor]
> +        target_ids = ids[vendor]
>          target_ids.append('\t' + id_name + ' = ' + str(id_value[vendor]) + ',')
>  
>          info = {
> @@ -292,11 +278,7 @@ def generate_h(controls, mode, ranges):
>              'type': ctrl.type,
>          }
>  
> -        target_ctrls = ctrls['libcamera']
> -        if ctrl.is_draft:
> -            target_ctrls = ctrls['draft']
> -        elif vendor != 'libcamera':
> -            target_ctrls = ctrls[vendor]
> +        target_ctrls = ctrls[vendor]
>  
>          if ctrl.is_enum:
>              target_ctrls.append(enum_template_start.substitute(info))
> @@ -337,7 +319,7 @@ ${vendor_controls}
>  ''')
>  
>      vendor_sub = []
> -    for vendor in [v for v in ctrls.keys() if v not in ['libcamera', 'draft']]:
> +    for vendor in [v for v in ctrls.keys() if v != 'libcamera']:
>          vendor_sub.append(vendor_template.substitute({'mode': mode.upper(),
>                                                        'vendor': vendor,
>                                                        'vendor_def': vendor.upper(),
> @@ -347,7 +329,6 @@ ${vendor_controls}
>      return {
>          'ids': '\n'.join(ids['libcamera']),
>          'controls': '\n'.join(ctrls['libcamera']),
> -        'draft_controls': '\n'.join(ctrls['draft']),
>          'vendor_controls': '\n'.join(vendor_sub)
>      }
>