[libcamera-devel] [PATCH v5 0/9] Introduce camera properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Feb 17 01:53:37 CET 2020
Hi Jacopo,
On Fri, Feb 14, 2020 at 09:24:29AM +0100, Jacopo Mondi wrote:
> On Fri, Feb 14, 2020 at 12:06:00AM +0200, Laurent Pinchart wrote:
> > 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.
Hence the "slightly easier", I didn't say difficult :-)
> > 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.
I agree with you, I think we should apply the same logic for both, but
I'm tempted to think that
- name: AeEnable
type: bool
description: |
Enable or disable the AE.
is the right option.
> >> 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
More information about the libcamera-devel
mailing list