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

[Laurent Pinchart]

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 ?

> 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