[libcamera-devel] [PATCH 5/5] libcamera: controls: Control framework refresh
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 23 13:31:57 CEST 2019
Hi Kieran
On Fri, Sep 20, 2019 at 01:56:42PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 18/09/2019 11:34, Jacopo Mondi wrote:
> > Now that the control data value and info storage have been unified with
> > V4L2 control a refresh of naming and minor bits had to be performed.
> > The control framework implementation uses the word 'control' in types
> > and definition to refer to different things, making it harder to follow.
> >
> > Perform a rename of elements defined by the control framework to enforce
> > the following concepts:
> > - control metadata:
> > static control metadata information such as id, type and name, defined by
> > libcamera
> > - control information:
> > constant information associated with a control used for validation of
> > control values, provided by pipeline handlers and defined by the
> > capabilities of the device the control applies to
> > - control:
> > a control identifier with an associated value, provided by
> > applications when setting a control to a specific value
>
> You've defined a 'Control' as an alias to DataValue. I suspect that
> perhaps ControlValue would have been more correct, because Control does
>
> Thus a 'Control' should be a pair of both a ControlID and a
> ControlValue... ?
>
> Or maybe this makes me think that DataValue should be renamed to just
> Value then there's no need to express a ControlValue separately...
I tried to shorten names, as this is application API to and it's quite
cumbersome to write.
>
>
> >
> > Update the framework documentation trying to use those concepts
> > consistently.
>
> I think this patch needs a bit of cleanup, and there are definitely some
> unrelated arbitrary changes to separate out. There's probably not even
> that many in fact.
>
> Anyway - various discussion points below. (Oh, and now above too)
>
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/controls.h | 19 ++--
> > src/libcamera/controls.cpp | 129 +++++++++++++++++-----------
> > src/libcamera/gen-controls.awk | 2 +-
> > src/libcamera/meson.build | 6 +-
> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc.cpp | 2 +-
> > test/controls/control_list.cpp | 2 +-
> > 7 files changed, 97 insertions(+), 65 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index d3065c0f3f16..174bc92732aa 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -19,7 +19,7 @@ namespace libcamera {
> >
> > class Camera;
> >
> > -struct ControlIdentifier {
> > +struct ControlMetadata {
> > ControlId id;
> > const char *name;
> > DataType type;
> > @@ -37,21 +37,22 @@ public:
> > std::string toString() const;
> >
> > private:
> > - const struct ControlIdentifier *ident_;
> > + const struct ControlMetadata *ident_;
>
> Shouldn't ident_ be renamed to meta_ as well?
>
Possibly, yes
> > };
> >
> > using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>;
> >
> > +using Control = DataValue;
>
> I'm initially hesitant here, but I can't actually find a way to disagree :-D
>
> I think https://google.github.io/styleguide/cppguide.html#Aliases makes
> it clear that aliases are acceptable, but should be clearly documented.
>
> Not sure what documentation we should provide, but we should likely have
> something explaining what a Control is used for at a high level, and
> that it maps to the underlying data type of a DataValue.
>
>
> <Also perhaps a newline here>
>
>
> > class ControlList
> > {
> > private:
> > - using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;
> > + using ControlMap = std::unordered_map<const ControlInfo *, Control>;
> >
> > public:
> > ControlList(const ControlInfoMap &infoMap);
> >
> > - using iterator = ControlListMap::iterator;
> > - using const_iterator = ControlListMap::const_iterator;
> > + using iterator = ControlMap::iterator;
> > + using const_iterator = ControlMap::const_iterator;
> >
> > iterator begin() { return controls_.begin(); }
> > iterator end() { return controls_.end(); }
> > @@ -64,14 +65,14 @@ public:
> > std::size_t size() const { return controls_.size(); }
> > void clear() { controls_.clear(); }
> >
> > - DataValue &operator[](ControlId id);
> > - DataValue &operator[](const ControlInfo *info) { return controls_[info]; }
> > + Control &operator[](ControlId id);
> > + Control &operator[](const ControlInfo *info) { return controls_[info]; }
> >
> > - void update(const ControlList &list);
> > + bool merge(const ControlList &list);
> >
> > private:
> > const ControlInfoMap &infoMap_;
> > - ControlListMap controls_;
> > + ControlMap controls_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 184d544c05bc..028ffc1e80b9 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -7,9 +7,6 @@
> >
> > #include <libcamera/controls.h>
> >
> > -#include <sstream>
> > -#include <string>
> > -
> > #include <libcamera/camera.h>
> >
> > #include "log.h"
> > @@ -17,7 +14,7 @@
> >
> > /**
> > * \file controls.h
> > - * \brief Describes control framework and controls supported by a camera
> > + * \brief Control framework implementation
> > */
> >
> > namespace libcamera {
> > @@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls)
> > */
> >
> > /**
> > - * \struct ControlIdentifier
> > - * \brief Describe a ControlId with control specific constant meta-data
> > + * \struct ControlMetadata
> > + * \brief Describe the control static meta-data
> > + *
> > + * Defines the Control static meta-data information, auto-generated from the
> > + * ControlId documentation.
> > + *
> > + * The control information are defined in the control_id.h file, parsed by
>
> s/are/is/
>
> > + * the gen-controls.awk script to produce a control_metadata.cpp file that
>
> I'm not sure that gen-controls.awk should be part of public documentation.
>
> That's an internal implementation detail, and gen-controls.awk is not
> distributed with the public-headers.
I see. I wanted to document it as the first time I had to deal with
it, I had quite an hard time to find it out.
>
>
>
> > + * provides a static list of control id with an associated type and name. The
> > + * file is not for public use, and so no suitable header exists as
> > + * its sole usage is for the ControlMetadata definition.
>
> Aha, as stated here :-D
>
> So I think this documentation for struct ControlMetadata should focus on
> documenting ControlMetadata.
>
> > *
> > - * Defines a Control with a unique ID, a name, and a type.
> > - * This structure is used as static part of the auto-generated control
> > - * definitions, which are generated from the ControlId documentation.
> > + * \todo Expand the control meta-data definition to support more complex
> > + * control types and describe their characteristics (ie. Number of deta elements
>
> s/deta/data/
>
>
> > + * for compound control types, versioning etc).
> > *
> > - * \var ControlIdentifier::id
> > + * \var ControlMetadata::id
> > * The unique ID for a control
> > - * \var ControlIdentifier::name
> > + * \var ControlMetadata::name
> > * The string representation of the control
> > - * \var ControlIdentifier::type
> > + * \var ControlMetadata::type
> > * The ValueType required to represent the control value
> > */
> >
> > /*
> > - * The controlTypes are automatically generated to produce a control_types.cpp
> > - * output. This file is not for public use, and so no suitable header exists
> > - * for this sole usage of the controlTypes reference. As such the extern is
> > - * only defined here for use during the ControlInfo constructor and should not
> > + * The ControlTypes map is defined by the generated control_metadata.cpp file
> > + * and only used here during the ControlInfo construction and should not
> > * be referenced directly elsewhere.
>
> You've removed key information here, which I'm not sure should have been
> removed. It explained /why/ this extern is defined here, and not in a
> non-existent control_types.h file.
I moved it up, in the comment that referred to gen-controls.awka to
explain where ControlMetadata come from.
And to be precise "extern is only defined" did not made much sense. If
a variable is defined with extern it is expected to be defined
elsewhere, and I explained where it is defined, and that it is only
used here.
>
>
> > */
> > -extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> > +extern const std::unordered_map<ControlId, ControlMetadata> controlTypes;
> >
> > /**
> > * \class ControlInfo
> > * \brief Describe the information and capabilities of a Control
> > *
> > - * The ControlInfo represents control specific meta-data which is constant on a
> > - * per camera basis. ControlInfo classes are constructed by pipeline handlers
> > - * to expose the controls they support and the metadata needed to utilise those
> > - * controls.
> > + * The ControlInfo class associates the control's constant metadata defined by
> > + * libcamera and collected in the ControlMetadata class, with its static
> > + * information, such the range of supported values and the control size,
> > + * described by the DataInfo base class and defined by the capabilities of
> > + * the Camera device that implements the control.
> > + *
> > + * ControlInfo instances are constructed by pipeline handlers and associated
> > + * with the Camera devices they register to expose the list of controls they
> > + * support and the static information required to use those controls.
> > */
> >
> > /**
> > @@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min,
> > {
> > auto iter = controlTypes.find(id);
> > if (iter == controlTypes.end()) {
> > - LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> > + LOG(Controls, Fatal) << "Control " << id << " not defined";
>
> Reporting the id here is indeed more helpful :-)
>
> > return;
> > }
> >
> > @@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min,
> >
> > /**
> > * \fn ControlInfo::id()
> > - * \brief Retrieve the control ID
> > + * \brief Retrieve the control ID from the control constant metadata
> > * \return The control ID
> > */
> >
> > /**
> > * \fn ControlInfo::name()
> > - * \brief Retrieve the control name string
> > + * \brief Retrieve the control name string from the control constant metadata
> > * \return The control name string
> > */
> >
> > /**
> > * \fn ControlInfo::type()
> > - * \brief Retrieve the control designated type
> > + * \brief Retrieve the control designated type from the control constant metadata
> > * \return The control type
> > */
> >
> > @@ -161,21 +170,37 @@ std::string ControlInfo::toString() const
> > * \brief A map of ControlId to ControlInfo
> > */
> >
> > +/**
> > + * \typedef Control
>
> Aha - here's the documentation I asked for earlier.
>
> I thought I looked for it but didn't find it in my first pass :D
>
>
> > + * \brief Use 'Control' in place of DataValue in the ControlList class
>
> I would instead change the brief to what a Control is used for.
>
> The fact that a Control is a DataValue is an implementation detail.
>
> Something like:
> "\brief A Control defines the value ..." Ah ...
>
> Ok - so I've just hit a logic fail. A "Control" is a pairing of the
> Control ID and a value isn't it?
>
> But here - it's just the value... - Moving that discussion up to the top
> commit message discussion on what a Control is.
>
For appliation a Control is a value associated to a control. To reduce
bikeshedding I would rather change this back to ControlValue if you
feel more confortable, but to me it's just a longer name.
>
>
> > + *
> > + * \todo If more data and operations than the ones defined by DataValue
> > + * will need to be implemented for controls, make this typedef a class that
> > + * inherits from DataValue.
>
> I'm not sure that todo helps. It's more of a private comment than a
> public-documentation todo action.
>
Yes, it's an implementation todo. Should I record it outside of /** ?
> > + */
> > +
> > /**
> > * \class ControlList
> > - * \brief Associate a list of ControlId with their values for a camera
> > + * \brief List of controls values
> > *
> > - * A ControlList wraps a map of ControlId to DataValue and provide
> > - * additional validation against the control information exposed by a Camera.
> > + * A ControlList wraps a map of ControlId to Control instances and provide
> > + * additional validation against the control information reported by a Camera.
> > *
> > - * A list is only valid for as long as the camera it refers to is valid. After
> > - * that calling any method of the ControlList class other than its destructor
> > - * will cause undefined behaviour.
> > + * ControlList instances are initially created empty and should be populated by
> > + * assigning a value to each control added to the list. The added control is
> > + * validated to verify it is supported by the Camera the list refers to, and
> > + * validate the provided value against the static information provided by the
>
> s/ / / (double space between against "and" "the")
>
> > + * ControlInfo instance associated with the control.
>
> Now that we've converted from storing a camera * to a ControlList
> reference from the Camera, is it still possible that someone could
> delete a Camera - and if so - what happens to the validity of the
> ControlList associated with it, as we have taken a reference.
I think that if a camera is removed:
1) The IPA module that uses the controlInfoMap is removed to
2) it is not possible to queue request to that camera
But yes, you could keep adding controls to a control list that refers to a
camera that has been removed, and the ControlInfoMap there referenced
would be invalid. I think the right way to handle this would be using
a shared pointer, to keep the InfoMap around as long as there are
control lists using it. Or simply keep the warning for now.
>
> I don't think it magically became safe, so do we need to keep the
> warning regarding the validity ?
>
>
> > */
> >
> > /**
> > - * \brief Construct a ControlList with a reference to the Camera it applies on
> > + * \brief Construct a ControlList with a map of control information
>
> Hrm ... that should probably be moved to the previous patch which
> changes the Control * to a ControlInfoMap reference.
>
>
> > * \param[in] infoMap The ControlInfoMap of the camera the control list refers to
> > + *
> > + * The provided \a infoMap collects the control id and the associated static
> > + * information for each control that can be set on the list. \a infoMap is
> > + * provided by the Camera the list of controls refers to and it is used to make
>
>
>
> > + * sure only controls supported by the camera can be added to the list.
> > */
> > ControlList::ControlList(const ControlInfoMap &infoMap)
> > : infoMap_(infoMap)
> > @@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap)
> > /**
> > * \brief Check if the list contains a control with the specified \a id
> > * \param[in] id The control ID
> > - *
> > - * The behaviour is undefined if the control \a id is not supported by the
> > - * camera that the ControlList refers to.
> > - *
>
> Is this no longer true? I guess so.
According to the above comment, yes, it might happen
>
>
> > * \return True if the list contains a matching control, false otherwise
> > */
> > bool ControlList::contains(ControlId id) const
> > @@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const
> > /**
> > * \fn ControlList::size()
> > * \brief Retrieve the number of controls in the list
> > - * \return The number of DataValue entries stored in the list
> > + * \return The number of Control entries stored in the list
> > */
> >
> > /**
> > @@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const
> > * The behaviour is undefined if the control \a id is not supported by the
>
> I see another undefined behaviour reference there... this needs to be
> consistent (and correct).
>
>
> > * camera that the ControlList refers to.
> > *
> > - * \return A reference to the value of the control identified by \a id
> > + * \return A reference to the value of the control identified by \a id if the
> > + * control is supported, an empty control otherwise
>
> Is the change from undefined behaviour to returning an emtpy control in
> this series? If so this documentation change should go in the relevant
> patch.
>
>
> > */
> > -DataValue &ControlList::operator[](ControlId id)
> > +Control &ControlList::operator[](ControlId id)
> > {
> > const auto info = infoMap_.find(id);
> > if (info == infoMap_.end()) {
> > LOG(Controls, Error) << "Control " << id << " not supported";
> >
> > - static DataValue empty;
> > + static Control empty;
> > return empty;
> > }
> >
> > @@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id)
> > * This method returns a reference to the control identified by \a info,
> > * inserting it in the list if the info is not already present.
> > *
> > - * \return A reference to the value of the control identified by \a info
> > + * The behaviour is undefined if the control \a info is not supported by the
> > + * camera that the ControlList refers to.
>
> I'm confused... What's the difference between an empty control, and one
> that is not supported by the camera...
I agree the undefined behaviour here is not accurate. But the empty
control is what is returned IF the control is not supported by the
camera.
>
>
> > + *
> > + * \return A reference to the value of the control identified by \a info if the
> > + * control is supported, an mpty control otherwise
>
> s/mpty/empty/
>
>
> > */
> >
> > /**
> > - * \brief Update the list with a union of itself and \a other
> > - * \param other The other list
> > + * \brief Merge the control in the list with controls from \a other
> > + * \param other The control list to merge with
> > *
> > * Update the control list to include all values from the \a other list.
> > - * Elements in the list whose control IDs are contained in \a other are updated
> > + * Elements in the list whose control IDs are contained in \a other are merged
> > * with the value from \a other. Elements in the \a other list that have no
> > * corresponding element in the list are added to the list with their value.
> > *
> > - * The behaviour is undefined if the two lists refer to different Camera
> > - * instances.
> > + * All controls in \a other should be supported by the list, otherwise no
> > + * control is updated.
> > + *
> > + * \return True if control list have been merged, false otherwise
> > + *
> > */
> > -void ControlList::update(const ControlList &other)
> > +bool ControlList::merge(const ControlList &other)
>
> Why this rename? This seems like an arbitrary API change. If it's
> suitable, then it's probably it's own patch.
Because the operation is a merge, it is also commented in tests with
"merge two lists".
Our API is floating, I don't this this change is worth a patch by
itself. If you really care so much of having this operation called
update() because you named it so, I can drop it to reduce discussions.
>
>
> > {
> > /*
> > * Make sure if all controls in other's list are supported by this
> > @@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other)
> >
> > if (infoMap_.find(id) == infoMap_.end()) {
> > LOG(Controls, Error)
> > - << "Failed to update control list with control: "
> > + << "Failed to merge control list with control: "
> > << id;
> > - return;
> > + return false;
> > }
> > }
> >
> > for (auto &control : other)
> > controls_[control.first] = control.second;
> > +
> > + return true;
> > }
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> > index abeb0218546b..b7f6532dfd43 100755
> > --- a/src/libcamera/gen-controls.awk
> > +++ b/src/libcamera/gen-controls.awk
> > @@ -89,7 +89,7 @@ function GenerateTable(file) {
> >
> > EnterNameSpace(file)
> >
> > - print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> > + print "extern const std::unordered_map<ControlId, ControlMetadata>" > file
> > print "controlTypes {" > file
> > for (i=1; i <= id; ++i) {
> > printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index c3100a1709e0..c4fcd0569bd7 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -60,13 +60,13 @@ endif
> >
> > gen_controls = files('gen-controls.awk')
> >
> > -control_types_cpp = custom_target('control_types_cpp',
> > +control_metadata_cpp = custom_target('control_metadata_cpp',
> > input : files('controls.cpp'),
> > - output : 'control_types.cpp',
> > + output : 'control_metadata.cpp',
> > depend_files : gen_controls,
> > command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> >
> > -libcamera_sources += control_types_cpp
> > +libcamera_sources += control_metadata_cpp
> >
> > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index dd253f94ca3d..d7f7fb0d6322 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> >
> > for (auto it : request->controls()) {
> > const ControlInfo *ci = it.first;
> > - DataValue &value = it.second;
> > + Control &value = it.second;
> >
> > switch (ci->id()) {
> > case Brightness:
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 3df239bdb277..1d36ec54bc3b 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> >
> > for (auto it : request->controls()) {
> > const ControlInfo *ci = it.first;
> > - DataValue &value = it.second;
> > + Control &value = it.second;
> >
> > switch (ci->id()) {
> > case Brightness:
> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> > index 3c6eb40c091b..2a90cffe5106 100644
> > --- a/test/controls/control_list.cpp
> > +++ b/test/controls/control_list.cpp
> > @@ -159,7 +159,7 @@ protected:
> >
> > newList[Brightness] = 128;
> > newList[Saturation] = 255;
> > - newList.update(list);
> > + newList.merge(list);
>
> You've changed the merge operation to return a value. Perhaps it's worth
> checking it if you think that change was warranted.
Correct.
Thanks
j
>
>
> >
> > list.clear();
> >
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
-------------- 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/20190923/8aeea757/attachment-0001.sig>
More information about the libcamera-devel
mailing list