[libcamera-devel] [PATCH v5 0/9] Introduce camera properties

[Laurent Pinchart]

Hi Jacopo,

On Thu, Feb 06, 2020 at 02:25:52AM +0100, Jacopo Mondi wrote:
> Hello,
>    this series contains the first 9 patches sent as part of the
> "Properties and compound controls" series.
> I'm mixing up a bit version numbers, I know..
> 
> All patches here are tagged, but I have not merged them yet for the following
> reasons:
> 
> 1) what to do with [1/9] ?
>    I'm a bit in two minds here, as I see two possible things to happen
>    1) we merge the series knowing we'll have to change the header with the
>       version that will hit mainline. This should be safe as long as the
>       only drivers using this definitions are our downstream devices for
>       testing. Although having code in master that we don't want to be used
>       even if for a short time makes me feel uncomfortable
>    2) we keep the series warm until we don't get kernel support and some users
>       in mainline. We would delay this features too long imho.
> 
>    Unless you have other ideas, 1 seems to be unavoidable in order to merge
>    properties support and start building the definitions of others on top.
> 
> 2) I have updated the "Rotation" definition with Laurent's suggestions. It
>    is now reviewed.
> 
> 3) I have changed the way enum values are defined in yaml.
> 
>    We previously had:
> 
>    enum:
>      - entry:
>        value: x
>        description: ".."
> 
>    Which is parsed as
> 
>    {
>         "entry": ,
>   	"value" : x,
> 	"description": "...",
>    }
> 
>    This required to extract the "entry" value by accessing the first of the
>    dictionaries keys. This triggered an error I started noticing when building
>    for CrOS, but could have happened earlier, as dictionaries keys are not
>    sorted.
> 
>    What we actually want is instead
> 
>    {
>        "entry": {
>   	   "value" : x,
> 	    "description": "...",
>        }
>    }
> 
>    Which is represented in yaml as:
> 
>    enum:
>      - entry:
>          value: x
>          description: ".."
> 
>    Which is probably also more semantically correct.

I wonder if we should instead go fo

    enum:
      - name: entry
        value: x
	description: ".."

The rationale is that the key ('entry' in your example) is of no
significance to the parser, and should thus likely not be a key. Parsing
becomes also slightly easier, as the key doesn't have to be extracted,
but the 'entry' name can be access through element['name'].

It's probably no big deal, both options can work, but it seems to be
more aligned with how yaml is used in the Linux kernel for DT bindings.
I've asked Maxime Ripard (my go to expert in this domain) about his
opinion, and the only rationale he had to offer was the one I already
mentioned, he had no more compeling argument to go one way or the other.

> Anyway, I have not dropped your tags, but I didn't feel like merging without
> pointing out the above.
> 
> Jacopo Mondi (9):
>   [TEMP] include: linux: Update v4l2-controls.h
>   libcamera: controls: Parse 'enum' in gen-controls.py
>   libcamera: properties: Add location property
>   libcamera: properties: Add rotation property
>   libcamera: controls: Add default to ControlRange
>   libcamera: camera_sensor: Parse camera properties
>   libcamera: pipeline_handler: Add Camera properties
>   libcamera: camera: Add Camera properties
>   android: camera_device: Use Camera properties for static Metadata
> 
>  include/libcamera/camera.h               |   1 +
>  include/libcamera/controls.h             |   5 +-
>  include/libcamera/meson.build            |  26 +-
>  include/libcamera/property_ids.h.in      |  33 +++
>  include/linux/v4l2-controls.h            |   7 +
>  src/android/camera_device.cpp            |  29 +-
>  src/libcamera/camera.cpp                 |  16 +-
>  src/libcamera/camera_sensor.cpp          |  49 +++-
>  src/libcamera/controls.cpp               |  12 +-
>  src/libcamera/gen-controls.py            |  50 +++-
>  src/libcamera/include/camera_sensor.h    |   7 +-
>  src/libcamera/include/pipeline_handler.h |   2 +
>  src/libcamera/meson.build                |  21 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
>  src/libcamera/pipeline/vimc.cpp          |   4 +
>  src/libcamera/pipeline_handler.cpp       |  19 ++
>  src/libcamera/property_ids.cpp.in        |  43 +++
>  src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
>  src/libcamera/v4l2_controls.cpp          |   9 +-
>  20 files changed, 662 insertions(+), 34 deletions(-)
>  create mode 100644 include/libcamera/property_ids.h.in
>  create mode 100644 src/libcamera/property_ids.cpp.in
>  create mode 100644 src/libcamera/property_ids.yaml

-- 
Regards,

Laurent Pinchart