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

[Jacopo Mondi]

Hi Laurent,

On Fri, Feb 14, 2020 at 12:06:00AM +0200, Laurent Pinchart wrote:
> 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'].

Parsing it's not an issue, it's easy enough to access the key.

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

The only counter-argument I could have is that we don't define controls
and properties as:

  - name: AeEnable:
    type: bool
    description: |
      Enable or disable the AE.

but rather as

  - AeEnable:
      type: bool
      description: |
        Enable or disable the AE.

If the control name is the key there, the enumeration entry should as
well be the key, as it has a set of associated properties, in the same
way a control has.


>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200214/30ec7da9/attachment-0001.sig>