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

[Hirokazu Honda]

Hi Laurent,

On Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> 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
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.

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