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

[Jacopo Mondi]

Hi Laurent,

On Thu, Oct 22, 2020 at 05:41:13AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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.

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

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.


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