[libcamera-devel] [PATCH v3 06/14] libcamera: controls: Generate a vector of enumerated values

[Laurent Pinchart]

Hi Jacopo,

On Fri, Oct 23, 2020 at 01:11:42PM +0200, Jacopo Mondi wrote:
> On Thu, Oct 22, 2020 at 05:41:13AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 21, 2020 at 04:36:27PM +0200, Jacopo Mondi wrote:
> > > For each Control that support enumerated values generate a vector
> > > of ControlValues which contains the full list of values.
> > >
> > > At the expense of a slight increase in memory occupation this change
> > > allows the construction of the ControlInfo associated with a Control
> > > from the values list, defaulting the minimum and maximum values
> > > reported by the ControlInfo.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/control_ids.cpp.in |  2 ++
> > >  utils/gen-controls.py            | 10 ++++++++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > > index 056645cfbdfb..ca0b5b22f899 100644
> > > --- a/src/libcamera/control_ids.cpp.in
> > > +++ b/src/libcamera/control_ids.cpp.in
> > > @@ -6,8 +6,10 @@
> > >   *
> > >   * This file is auto-generated. Do not edit.
> > >   */
> > > +#include <vector>
> >
> > This should go to control_ids.h.in as that's where you add
> > std::vector<>.
> >
> > >  #include <libcamera/control_ids.h>
> > > +#include <libcamera/controls.h>
> > >
> > >  /**
> > >   * \file control_ids.h
> > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > > index bf681503f86a..23aace50f666 100755
> > > --- a/utils/gen-controls.py
> > > +++ b/utils/gen-controls.py
> > > @@ -100,6 +100,8 @@ ${description}
> > >  def generate_h(controls):
> > >      enum_template_start = string.Template('''enum ${name}Enum {''')
> > >      enum_value_template = string.Template('''\t${name} = ${value},''')
> > > +    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}Values = {''')
> > > +    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
> >
> > Shouldn't you declare the vector as extern const here, and define it in
> > control_ids.cpp ? Otherwise you'll duplicate the values in every
> > compilation unit that includes control_ids.h.
> 
> Right right right!
> 
> I used a static const as I was wrongly generating the vector
> definition in the .h file as extern const, as I got a rightfull linker
> error for a redefinition.
> 
> What I do have now looks like (for AeMeteringControl)
> 
> control_ids.h:
> enum AeMeteringModeEnum {
> 	MeteringCentreWeighted = 0,
> 	MeteringSpot = 1,
> 	MeteringMatrix = 2,
> 	MeteringCustom = 3,
> };
> extern const std::vector<ControlValue> AeMeteringModeValues;
> extern const Control<int32_t> AeMeteringMode;
> 
> control_ids.cpp:
> extern const std::vector<ControlValue> AeMeteringModeValues = {
> 	static_cast<int32_t>(MeteringCentreWeighted),
> 	static_cast<int32_t>(MeteringSpot),
> 	static_cast<int32_t>(MeteringMatrix),
> 	static_cast<int32_t>(MeteringCustom),
> };
> extern const Control<int32_t> AeMeteringMode(AE_METERING_MODE, "AeMeteringMode");
> 
> Which sounds about right to me.

Seems much more than about right to me :-)

> > I would also make it an std::array, to avoid global objects that have
> > non-trivial constructors. But the ControlValue stored in the vector has
> > a non-trivial constructor :-/ Do you expect enumerated controls with
> > other types than Integer32 ? If not we could make the API more specific,
> > but maybe that's not a very good idea in case we need to extend it
> > later.
> >
> > std::array may be best anyway, even with ControlValue, as it will be
> > simpler. Given that we know here how many enumerated values we generate,
> > setting the number of elements in the std::array template parameter
> > shouldn't be a problem. Then, you would need to modify the ControlInfo
> > constructor to take a Span<ControlValue> instead of a std::vector<>.
> 
> I struggled for the last hour to have a construct like this one work
> using std::array<> and Span<>
> 
> Using std::array<> is fine, I can easily generate that.
> Using Span<> to transport the array content in the ControlInfo is not
> as passing a new Span<ControlValue> contructed from an array<ControlValue, N>
> calls the ControlValue contructor
> 
> 	ControlValue(const T &value)
> 		: type_(ControlTypeNone), numElements_(0)
> 	{
> 		set(details::control_type<std::remove_cv_t<T>>::value, true,
> 		    value.data(), value.size(), sizeof(typename T::value_type));
> 	}
> 
> and not the copy constructor. The construction than fails at
>         details::control_type<std::remove_cv_t<T>>::value
>
> As no specialization is provided for control_type<ControlValue>
> (rightfully.

If someone wants to introduce this, I think I'll give up on computers
and go plant tomatoes in a warm country :-)

> I got away with:
>         ControlInfo(ControlValue *data, unsigned int size);
> Which can be easily retrieved from std::array::data() and std::array::size()
> 
> In example (raspberrypi.h)
> 	{
>           &controls::AeMeteringMode,
>           ControlInfo(controls::AeMeteringModeValues.data(),
>           controls::AeMeteringModeValues.size())
>         }
> 
> Which is rather ugly but can be shortened with some macro magic
> 
> To sum up:
> - array: yes
> - Span<ControlValue>: no
> - so using array I access the raw data and transport them to the
>   ControlInfo (copying them in an class member vector at the moment,
>   but it can be improved)
> 
> Sincerely, I don't see much gain in all this churn compared to using a
> plain std::container but I might be missing something.

If it's too much churn then it's probably not worth it indeed. In
general it's best to avoid non-trivial types for global variables as
much as possible, but not to the extent that would require us to rewrite
the C++ standard and implement our own compiler of course :-)

> > >      template = string.Template('''extern const Control<${type}> ${name};''')
> > >
> > >      ctrls = []
> > > @@ -140,6 +142,14 @@ def generate_h(controls):
> > >                  target_ctrls.append(enum_value_template.substitute(value_info))
> > >              target_ctrls.append("};")
> > >
> > > +            target_ctrls.append(enum_list_start.substitute(info))
> > > +            for entry in enum:
> > > +                value_info = {
> > > +                    'name': entry['name'],
> > > +                }
> > > +                target_ctrls.append(enum_list_values.substitute(value_info))
> > > +            target_ctrls.append("};")
> > > +
> >
> > Not a candidate for this series, but given that we will soon depend on
> > jinja at build time (for IPA IPC and for tracing), I think we could
> > upgrade the control and property templates to jinja templates, it should
> > simplify this code.
> >
> > >          target_ctrls.append(template.substitute(info))
> > >          id_value += 1
> > >

-- 
Regards,

Laurent Pinchart