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

[Laurent Pinchart]

Hello,

On Thu, Sep 26, 2019 at 04:18:45PM +0100, Kieran Bingham wrote:
> 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 ....

Good names are difficult to create. I agree with Kieran here, list is
more descriptive of the purpose of the class than map in this case. We
could also call it ControlSet if we wanted to emphasise that the order
doesn't matter, but that name would generate confusion with the
operation of setting controls in my opinion.

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

While I have a tendency to like operators, I would side a bit with
Jacopo on this, snapshot.set(Exposure, 5) or snapshot.add(Exposure, 5)
may be more readable. I know what operator[] does as I'm familiar with
the code, but for someone without knowledge of the implementation, the
operator may not be that intuitive. Among other things, operator[] needs
to define what happens when the element doesn't exist in the container.
One may suspect that it would get added, but .set() or .add() would make
that more explicit. Similarly, it is clear that .get() will do something
when the element doesn't exist (even though the function name doesn't
really tell what will happen), while operator[] may leave the user
wondering whether it could crash or lead to other unexpected behaviour.

Let's remember the core rule: in a good API, methods operate as you
would expect from their name, and are named as you would expect from
their purpose.

> 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

-- 
Regards,

Laurent Pinchart