[libcamera-devel] [PATCH 0/5] libcamera: Control framework backend rework

[Kieran Bingham]

Hi Jacopo,

On 18/09/2019 11:34, Jacopo Mondi wrote:
> Hello,
>    this series is the first half of the currently in progress serialization
> support work.
> 
> The basic idea is that to be able to serialize Controls and V4L2Controls
> using the same API, their back-end storage should be unified to use the same
> polymorphic data storage class. This was the initial plan to be honest, but it
> didn't match the urgent need to get controls support in at the time, but it's
> now required for serialization, so it's now a good time to fix this long
> standing issue.
> 
> The series addresses the issue taking the following steps.
> 1) Break out the DataValue class from the ControlValue class and associate it
>    with a DataInfo class, that is meant to support constant validation
>    information associated with a value.
> 
>    The mapping will be
>    V4L2Control = Control = DataValue
>    V4L2ControlInfo = ControlInfo = DataInfo
> 
>    With V4L2ControlList being a list of V4L2Controls
>    and ControlList being a map of Controls associated to a map of
>        ControlInfo both indexed by ControlId
> 
>    [ later considerations: when writing below about the API of ControlList and
>      V4L2ControlList I realized they're effectively maps of ids to values, and
>      naming them "Map" would make the syntax "ControlMap[id] = value" more
>      natural. What do you think? ]


Yes, they are a map - but the map represents a list of controls ... I
kinda like ControlList (/V4L2ControlList) as that better describes what
the content of the object is.

The fact that it's internally represented as a map, is implementation.
The 'high-level' requirement is to provide a list of controls to set (or
read?)


In fact, 'ControlList' is our API. I don't think we should list the type
in the API, i.e. if it were a vector it shouldn't be ControlVector.

It's a list. - the storage is up to the implementation.
Of course that affects how users interact with it ....


>    All of this in patches 1, 2, 3
> 
> 2) Serialization implies de-serialization, and to construct a ControlList we
>    need a camera. Break this dependency in patch 4/5 to be able to reconstruct
>    a ControlList from serialized data later
> 
> 3) Bikeshedders heaven: rewording of comments and rename of data types in
>    Control framework. As explained in the commit message of 5/5, the rewording
>    tries to enforce the following concepts: control metadata, control info,
>    and control values.
> 
> On top of this I have patches and tests for Serialization, but not yet for
> de-serialization. I hope not, but it might be that implementing it
> require changes in this series, so be prepared.
> 
> All of this for the back-end storage.
> 
> The user APIs: there is work there too, as both V4L2Controls and Controls might
> need re-thinking of their user API. The fundamental difference between the two
> (and it took me a while to realize the implication of this in the API
> definition) it's where validation information are kept.
> 
> ControlList is created with a reference to a ControlInfoMap used for validation
> and produced by pipeline handlers at camera registration time. This allows to
> validate a control at the moment it is added to the list, as doing so when
> queuing requests is indeed too late for a safe recovery.

Hrm ... so we have a ControlInfoMap ... Well that kind of goes a bit
against what I said earlier. But in that case, it's not really an
arbitrary list of ControlInfo is it ? I wonder if that makes the term
'map' more applicable?

Or maybe it's just because in my head, a Control is always an
association of an ID and a value.

(I think in my original implementations I aimed to have an object called
Control which stored both the ID and the value in a single class).


> V4L2Control list only contains values. The validation informations are kept in
> the device the list is applied on. I think this still makes sense, as V4L2
> controls are meant to be validate at the time they are immediately applied to
> the device by the driver exposing them, and they will not be exposed to
> applications. Anyway, creating a V4L2ControlList from a device might make
> serialization easier because both values and information would then be kept
> in the same class, as it happens now for Controls.
> 
> The other difference in the API is a matter of tastes mostly. Controls use the
> overloaded operator[] to set/get controls, V4L2Controls have explicit get/set.
> Having implemented them, it's clear that my preference goes toward explicit
> get/set, as operator overloading makes me always a bit unconfortable and I
> prefer to type in 3 letters more for set/get but have an explicit indication of
> what's happening. Anyway, the use of [] made in ControlList is pretty natural
> and does what you would expect it to do (maybe if it was a ControlMap it would
> feel more natural to do controls[x] = y ? Now that I wrote it here, I'll suggest
> it in the above comment too) and seems to be also in line with

I find the [] operator quite natural, and allows short clean code ?

  ControlList snapshot;

  snapshot[Exposure] = 5;


Aha - That was it - Now I remember why we had the ControlInfo comparator
overloads. It was to allow indexing the ControlList maps with either an
enum of the control ID, or by indexing with a ControlInfo.

But if we don't use it with the ControlInfo then I guess they aren't needed.


> https://google.github.io/styleguide/cppguide.html "Operator Overloading"
> section. So I'll go with popular preference, but in any case I think the two
> should be unified.
> 
> As said, serialization will come on top of this work, so please comment to avoid
> me too many re-bases :)
> 
> Thanks
>    j
> 
> Jacopo Mondi (5):
>   libcamera: Create DataValue and DataInfo
>   libcamera: controls: Use DataType and DataValue
>   libcamera: v4l2_controls: Use DataValue and DataInfo
>   libcamera: controls: De-couple Controls from Camera
>   libcamera: controls: Control framework refresh
> 
>  include/libcamera/controls.h                  |  94 +----
>  include/libcamera/data_value.h                |  84 ++++
>  include/libcamera/meson.build                 |   1 +
>  src/libcamera/controls.cpp                    | 398 +++++-------------
>  src/libcamera/data_value.cpp                  | 317 ++++++++++++++
>  src/libcamera/gen-controls.awk                |   4 +-
>  src/libcamera/include/v4l2_controls.h         |  22 +-
>  src/libcamera/include/v4l2_device.h           |   1 -
>  src/libcamera/meson.build                     |   7 +-
>  src/libcamera/pipeline/uvcvideo.cpp           |   2 +-
>  src/libcamera/pipeline/vimc.cpp               |   2 +-
>  src/libcamera/request.cpp                     |   4 +-
>  src/libcamera/v4l2_controls.cpp               |  49 +--
>  src/libcamera/v4l2_device.cpp                 |  25 +-
>  test/controls/control_info.cpp                |   4 +-
>  test/controls/control_list.cpp                |   6 +-
>  test/controls/meson.build                     |   1 -
>  .../data_value.cpp}                           |  24 +-
>  test/data_value/meson.build                   |  12 +
>  test/meson.build                              |   1 +
>  20 files changed, 593 insertions(+), 465 deletions(-)
>  create mode 100644 include/libcamera/data_value.h
>  create mode 100644 src/libcamera/data_value.cpp
>  rename test/{controls/control_value.cpp => data_value/data_value.cpp} (68%)
>  create mode 100644 test/data_value/meson.build
> 
> --
> 2.23.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran