| Message ID | 20210421042346.312854-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. > > 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;
Hi Laurent, On Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart <laurent.pinchart@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@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
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@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;
On Tue, Apr 27, 2021 at 12:56 PM Laurent Pinchart <laurent.pinchart@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@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
Hi Hiro, On Tue, Apr 27, 2021 at 01:13:48PM +0900, Hirokazu Honda wrote: > On Tue, Apr 27, 2021 at 12:56 PM Laurent Pinchart wrote: > > 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@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>? In ControlValue I would like to store the index only. If we need to store strings in ControlInfo, I think we'll need to extend the ControlInfo class to store other information than just a ControlValue. I want to avoid as much as possible extending the types of ControlValue we support, as you would otherwise need to plumb them through the IPC serialization code too. ControlValue is designed as the container to be stored in ControlList. It's reused in ControlInfo, but that's not what it's optimized for, it's main target is ControlList. > > > 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;
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(); + } }; 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;
This adds control type for v4l2 menu. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++ src/libcamera/controls.cpp | 7 +++++++ 2 files changed, 35 insertions(+)