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

[Kieran Bingham]

Hi Jacopo,

On 06/02/2020 01:25, 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

I would be happy to merge this patch to master for the following:

 - You have already posted the corresponding change upstream.

 - If anything changes in the meanwhile, you'll know first and fast.

 - libcamera is not yet 'stable' so if we have to revert or change this
   this further it's not that big a deal?

 - IIUC - Running this on a kernel which does not have those features
   should not 'explode' but the feature just won't work.

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

 - Keeping code out of tree is really painful.

So I would vote for adding the patch to get this series merged.

I would however probably put a comment above the addition saying that
this has been added before it hits mainline linux.

And that comment would simply be automatically removed when we update to
a set of headers that has an upstream implementation.


Anyway, that's just my 5 cents.
--
Kieran


>    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.
> 
> Anyway, I have not dropped your tags, but I didn't feel like merging without
> pointing out the above.
> 
> Thanks
>     j
> 
> 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
> 
> --
> 2.24.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran