[libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add ControlTypeMenu

[Hirokazu Honda]

On Tue, Apr 27, 2021 at 12:56 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Tue, Apr 27, 2021 at 12:46:30PM +0900, Hirokazu Honda wrote:
> > On Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart wrote:
> > > On Wed, Apr 21, 2021 at 01:23:41PM +0900, Hirokazu Honda wrote:
> > > > This adds control type for v4l2 menu.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > >  include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++
> > > >  src/libcamera/controls.cpp   |  7 +++++++
> > > >  2 files changed, 35 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > index 1a5690a5..357819e7 100644
> > > > --- a/include/libcamera/controls.h
> > > > +++ b/include/libcamera/controls.h
> > > > @@ -9,6 +9,7 @@
> > > >  #define __LIBCAMERA_CONTROLS_H__
> > > >
> > > >  #include <assert.h>
> > > > +#include <sstream>
> > > >  #include <stdint.h>
> > > >  #include <string>
> > > >  #include <unordered_map>
> > > > @@ -32,6 +33,28 @@ enum ControlType {
> > > >       ControlTypeString,
> > > >       ControlTypeRectangle,
> > > >       ControlTypeSize,
> > > > +     ControlTypeMenu,
> > > > +};
> > > > +
> > > > +struct ControlMenu {
> > > > +     uint32_t index;
> > > > +     bool isName = false;
> > > > +
> > > > +     union {
> > > > +             char name[28];
> > > > +             int64_t value;
> > > > +     };
> > > > +
> > > > +     std::string toString() const
> > > > +     {
> > > > +             std::stringstream ss;
> > > > +             ss << "index: " << index;
> > > > +             if (isName)
> > > > +                     ss << "name: " << name;
> > > > +             else
> > > > +                     ss << "value: " << value;
> > > > +             return ss.str();
> > > > +     }
> > > >  };
> > >
> > > As this is used for V4L2 controls only, I'm really not keen on adding
> > > support for named menu entries :-( Especially given that 28 is a
> > > completely arbitrary limit. I'd like to instead store the value as an
> > > integer, and, if we really need support for named menu entries, to add
> > > them to the ControlInfo class instead ? The mapping between numerical
> > > values and names should be constant, it shouldn't be a property of the
> > > ControlValue.
> >
> > I see. Actually the named menu is necessary for test pattern mode.
> > We need a class for string. The problem is ControlValue uses memcpy
>
> Why do we need strings, why can't we use the numerical values ?
>

I misunderstood you don't like to have the 28 length limitation.
By the way, for v4l2 menu, we need to store index and either string or integer.
Are you okay to introduce two new control types, std::pair<int32_t,
uint8_t[28]> and std::pair<int32_t, uint32_t>?

-Hiro

> > for copy, but it doesn't obviously work for std::string.
> > Could you tell me your prefered solution?
> > My idea is only to replace memcpy by copy assignment operator.
>
> I don't know, I likely won't have time to try and design a solution for
> this in the near future, all I know is that we should do better :-)
>
> > > >  namespace details {
> > > > @@ -85,6 +108,11 @@ struct control_type<Size> {
> > > >       static constexpr ControlType value = ControlTypeSize;
> > > >  };
> > > >
> > > > +template<>
> > > > +struct control_type<ControlMenu> {
> > > > +     static constexpr ControlType value = ControlTypeMenu;
> > > > +};
> > > > +
> > > >  template<typename T, std::size_t N>
> > > >  struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > > >  };
> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > > index c58ed394..8ab5ac92 100644
> > > > --- a/src/libcamera/controls.cpp
> > > > +++ b/src/libcamera/controls.cpp
> > > > @@ -60,6 +60,7 @@ static constexpr size_t ControlValueSize[] = {
> > > >       [ControlTypeString]             = sizeof(char),
> > > >       [ControlTypeRectangle]          = sizeof(Rectangle),
> > > >       [ControlTypeSize]               = sizeof(Size),
> > > > +     [ControlTypeMenu]               = sizeof(ControlMenu),
> > > >  };
> > > >
> > > >  } /* namespace */
> > > > @@ -254,6 +255,12 @@ std::string ControlValue::toString() const
> > > >                       str += value->toString();
> > > >                       break;
> > > >               }
> > > > +             case ControlTypeMenu: {
> > > > +                     const ControlMenu *value =
> > > > +                             reinterpret_cast<const ControlMenu *>(data);
> > > > +                     str += value->toString();
> > > > +                     break;
> > > > +             }
> > > >               case ControlTypeNone:
> > > >               case ControlTypeString:
> > > >                       break;
>
> --
> Regards,
>
> Laurent Pinchart