| Message ID | 20210428073617.373422-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, + Laurent for a digression at the end of the email On Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote: > The v4l2 menu contains not only index (int32_t) but also either > name (string) or value (int64_t). To support it keeping > ControlValue simple, this adds the extra ControlValues to > ControlInfo. With the ControlInfo, indeices are stored in > ControlInfo::values, and names (or values) are stored in > ControlInfo::extraValues. > I feel it will be hard to assign a proper semantic to what "values" and "extra values" are for a control. Let's take a step back and look at the issue at hand and ask ourselves if we really care about strings that are used to describe a test pattern to users. What do we care about ? 1) Being able to enumerate all the test patter modes supported by a sensor 2) Map the sensor test patterns to a libcamera test pattern 3) Map the libcamera test pattern to the Android one Looking at the rest of the series it seems the string test pattern description is used to map the sensor's test pattern mode to the libcamera one. Is this necessary ? Can't we map indexes to indexes and leave strings out ? This would allow us to: 1) Augment V4L2Device::listControls() to add support for menu controls by only collecting indexes. This would work for menu and int_menu controls. The indexes can be collected one by one calls to VIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo can be augmented with a contructor similar to ControlInfo::ControlInfo(Span<const ControlValue> values, const ControlValue &def) So that it can transport a list of int32 (the indexes) 2) The sensor db requires a bit more work, not that much actually. In your series you add entries like: { "Disabled", controls::draft::TestPatternModeOff }, Could this just be { 0, controls::draft::TestPatternModeOff }, ? Of course we are now tying the driver's patter listing order to the libcamera's control, if a driver changes the ordering (why would it ?) the sensor db will have to be updated. But this is not different.. if the driver changes a pattern name (why would it?) we'll have to update the db anyhow (possibly with versioning \o/ ) Setting a menu control goes by index anyhow, so it won't really make a difference (when and if we'll have to support setting menu controls). This might work for test pattern modes, as we don't actually care about the mode name, if not for debugging purposes. It won't work that well with INT_MENU controls, the most notable being CID_LINK_FREQ (even if its usage from userspace is limited, even not required possibly). In that case the index is less representitive than its content, so might chose for INT_MENU to collect the actual values instead of the indexes, and in that case we'll need a way to reverse the value->index association when we'll need to set an INT_MENU control as I assume ph will want to say "set LINK_FREQ to 192000000" and not "set LINK_FREQ to index 0") --------- Digressions not related to this patch ---------------------- --------- Mostly for discussions and not blocking ------------------- A question remains when setting a menu control: where does the value->index reversing happens ? Applications/Android will tell "set TEST_PATTERN to libcamera::controls::TestPatternXYZ" to the pipeline handler that will receive a control list with { TestPatternMode, TestPatternModeOff } and it will have to set it in the video device or the camera sensor. For video devices, we have a pure v4l2 controls based interface, so the ph will have to do the reversing: TestPatternModeOff = index 0 by itself as it knows its video devices and can create a map for that purpose. For camera sensor we have a v4l2 controls based interface, but that's not really finalized, and if we want the ph to do the value->index reversing it mean we'll have to expose the sensor db. Otherwise we introduce a libcamera::Controls based interface in CameraSensor and keep the reversing internal, but this ofc has other drawbacks (the most notable will be the fact we'll have two interfaces (one v4l2, the other libcamera) or we drop the v4l2 interface from CameraSensor (which will require IPAs to speak libcamera::controls when they send sensor's controls to the ph). What do you think ? > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/controls.h | 5 +++++ > src/libcamera/controls.cpp | 22 ++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 1a5690a5..a8deb16a 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -271,11 +271,15 @@ public: > const ControlValue &def = 0); > explicit ControlInfo(Span<const ControlValue> values, > const ControlValue &def = {}); > + explicit ControlInfo(Span<const ControlValue> values, > + Span<const ControlValue> extraValues, > + const ControlValue &def = {}); > > const ControlValue &min() const { return min_; } > const ControlValue &max() const { return max_; } > const ControlValue &def() const { return def_; } > const std::vector<ControlValue> &values() const { return values_; } > + const std::vector<ControlValue> &extraValues() const { return extraValues_; } > > std::string toString() const; > > @@ -294,6 +298,7 @@ private: > ControlValue max_; > ControlValue def_; > std::vector<ControlValue> values_; > + std::vector<ControlValue> extraValues_; > }; > > using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>; > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index c58ed394..e2e8619a 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > values_.push_back(value); > } > > +/** > + * \brief Construct a ControlInfo from the list of valid values and extra values > + * \param[in] values The control valid values > + * \param[in] extraValues The control valid extra values associated with \a values > + * \param[in] def The control default value > + * > + * Construct a ControlInfo from a list of valid values and extra values. The > + * ControlInfo minimum and maximum values are set to the first and last members > + * of the values list respectively. The default value is set to \a def if > + * provided, or to the minimum value otherwise. The extra values are associated > + * with \a values and in the same order as \a values. > + * > + */ > +ControlInfo::ControlInfo(Span<const ControlValue> values, > + Span<const ControlValue> extraValues, > + const ControlValue &def) > + : ControlInfo(values, def) > +{ > + for (const ControlValue &extraValue : extraValues) > + extraValues_.push_back(extraValue); > +} > + > /** > * \fn ControlInfo::min() > * \brief Retrieve the minimum value of the control > -- > 2.31.1.498.g6c1eba8ee3d-goog > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Fri, Apr 30, 2021 at 10:58:10AM +0200, Jacopo Mondi wrote: > Hi Hiro, > + Laurent for a digression at the end of the email > > On Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote: > > The v4l2 menu contains not only index (int32_t) but also either > > name (string) or value (int64_t). To support it keeping > > ControlValue simple, this adds the extra ControlValues to > > ControlInfo. With the ControlInfo, indeices are stored in > > ControlInfo::values, and names (or values) are stored in > > ControlInfo::extraValues. > > > > I feel it will be hard to assign a proper semantic to what "values" > and "extra values" are for a control. > > Let's take a step back and look at the issue at hand and ask ourselves > if we really care about strings that are used to describe a test > pattern to users. > > What do we care about ? > 1) Being able to enumerate all the test patter modes supported by a > sensor > 2) Map the sensor test patterns to a libcamera test pattern > 3) Map the libcamera test pattern to the Android one > > Looking at the rest of the series it seems the string test pattern > description is used to map the sensor's test pattern mode to the > libcamera one. Is this necessary ? Can't we map indexes to indexes and > leave strings out ? Agreed on all that. The semantics of "extraValues()" is very ill-defined, and it would be hard to fix that. It's a hack, and we don't want hacks in the public API, we want a properly designed solution. Menu strings are not needed in ControlList, and ControlValue is mostly meant as the storage for variant entries in ControlList. It is also reused in ControlInfo, but that's not its main purpose. If we want to handle menu strings, they should go to ControlInfo directly. Even then, it would need to be done in a clean way from a public API point of view. In particular, the issue of internationalisation needs to be taken into account, so I'm really, really not keen to have menu strings, anywhere. > This would allow us to: > 1) Augment V4L2Device::listControls() to add support for menu controls > by only collecting indexes. This would work for menu and int_menu > controls. The indexes can be collected one by one calls to > VIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo > can be augmented with a contructor similar to > > ControlInfo::ControlInfo(Span<const ControlValue> values, > const ControlValue &def) > > So that it can transport a list of int32 (the indexes) > > 2) The sensor db requires a bit more work, not that much actually. > In your series you add entries like: > > { "Disabled", controls::draft::TestPatternModeOff }, > > Could this just be > > { 0, controls::draft::TestPatternModeOff }, > ? > > Of course we are now tying the driver's patter listing order to the > libcamera's control, if a driver changes the ordering (why would it ?) > the sensor db will have to be updated. But this is not different.. if > the driver changes a pattern name (why would it?) we'll have to update > the db anyhow (possibly with versioning \o/ ) > > Setting a menu control goes by index anyhow, so it won't really make a > difference (when and if we'll have to support setting menu controls). > > This might work for test pattern modes, as we don't actually care > about the mode name, if not for debugging purposes. It won't work that > well with INT_MENU controls, the most notable being CID_LINK_FREQ > (even if its usage from userspace is limited, even not required > possibly). In that case the index is less representitive than its > content, so might chose for INT_MENU to collect the actual values > instead of the indexes, and in that case we'll need a way to reverse > the value->index association when we'll need to set an INT_MENU > control as I assume ph will want to say "set LINK_FREQ to 192000000" > and not "set LINK_FREQ to index 0") > > --------- Digressions not related to this patch ---------------------- > --------- Mostly for discussions and not blocking ------------------- > > A question remains when setting a menu control: where does the > value->index reversing happens ? > > Applications/Android will tell "set TEST_PATTERN to > libcamera::controls::TestPatternXYZ" to the pipeline > handler that will receive a control list with > > { TestPatternMode, TestPatternModeOff } > > and it will have to set it in the video device or the camera sensor. > > For video devices, we have a pure v4l2 controls based interface, so > the ph will have to do the reversing: > > TestPatternModeOff = index 0 > > by itself as it knows its video devices and can create a map for that > purpose. > > For camera sensor we have a v4l2 controls based interface, but that's > not really finalized, and if we want the ph to do the value->index > reversing it mean we'll have to expose the sensor db. Otherwise we > introduce a libcamera::Controls based interface in CameraSensor and > keep the reversing internal, but this ofc has other drawbacks (the > most notable will be the fact we'll have two interfaces (one v4l2, the > other libcamera) or we drop the v4l2 interface from CameraSensor > (which will require IPAs to speak libcamera::controls when they send > sensor's controls to the ph). > > What do you think ? > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/controls.h | 5 +++++ > > src/libcamera/controls.cpp | 22 ++++++++++++++++++++++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index 1a5690a5..a8deb16a 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -271,11 +271,15 @@ public: > > const ControlValue &def = 0); > > explicit ControlInfo(Span<const ControlValue> values, > > const ControlValue &def = {}); > > + explicit ControlInfo(Span<const ControlValue> values, > > + Span<const ControlValue> extraValues, > > + const ControlValue &def = {}); > > > > const ControlValue &min() const { return min_; } > > const ControlValue &max() const { return max_; } > > const ControlValue &def() const { return def_; } > > const std::vector<ControlValue> &values() const { return values_; } > > + const std::vector<ControlValue> &extraValues() const { return extraValues_; } > > > > std::string toString() const; > > > > @@ -294,6 +298,7 @@ private: > > ControlValue max_; > > ControlValue def_; > > std::vector<ControlValue> values_; > > + std::vector<ControlValue> extraValues_; > > }; > > > > using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index c58ed394..e2e8619a 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > > values_.push_back(value); > > } > > > > +/** > > + * \brief Construct a ControlInfo from the list of valid values and extra values > > + * \param[in] values The control valid values > > + * \param[in] extraValues The control valid extra values associated with \a values > > + * \param[in] def The control default value > > + * > > + * Construct a ControlInfo from a list of valid values and extra values. The > > + * ControlInfo minimum and maximum values are set to the first and last members > > + * of the values list respectively. The default value is set to \a def if > > + * provided, or to the minimum value otherwise. The extra values are associated > > + * with \a values and in the same order as \a values. > > + * > > + */ > > +ControlInfo::ControlInfo(Span<const ControlValue> values, > > + Span<const ControlValue> extraValues, > > + const ControlValue &def) > > + : ControlInfo(values, def) > > +{ > > + for (const ControlValue &extraValue : extraValues) > > + extraValues_.push_back(extraValue); > > +} > > + > > /** > > * \fn ControlInfo::min() > > * \brief Retrieve the minimum value of the control
Hi Jacopo and Laurent, On Fri, Apr 30, 2021 at 8:44 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Fri, Apr 30, 2021 at 10:58:10AM +0200, Jacopo Mondi wrote: > > Hi Hiro, > > + Laurent for a digression at the end of the email > > > > On Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote: > > > The v4l2 menu contains not only index (int32_t) but also either > > > name (string) or value (int64_t). To support it keeping > > > ControlValue simple, this adds the extra ControlValues to > > > ControlInfo. With the ControlInfo, indeices are stored in > > > ControlInfo::values, and names (or values) are stored in > > > ControlInfo::extraValues. > > > > > > > I feel it will be hard to assign a proper semantic to what "values" > > and "extra values" are for a control. > > > > Let's take a step back and look at the issue at hand and ask ourselves > > if we really care about strings that are used to describe a test > > pattern to users. > > > > What do we care about ? > > 1) Being able to enumerate all the test patter modes supported by a > > sensor > > 2) Map the sensor test patterns to a libcamera test pattern > > 3) Map the libcamera test pattern to the Android one > > > > Looking at the rest of the series it seems the string test pattern > > description is used to map the sensor's test pattern mode to the > > libcamera one. Is this necessary ? Can't we map indexes to indexes and > > leave strings out ? > > Agreed on all that. The semantics of "extraValues()" is very > ill-defined, and it would be hard to fix that. It's a hack, and we don't > want hacks in the public API, we want a properly designed solution. > > Menu strings are not needed in ControlList, and ControlValue is mostly > meant as the storage for variant entries in ControlList. It is also > reused in ControlInfo, but that's not its main purpose. If we want to > handle menu strings, they should go to ControlInfo directly. Even then, > it would need to be done in a clean way from a public API point of view. > In particular, the issue of internationalisation needs to be taken into > account, so I'm really, really not keen to have menu strings, anywhere. > > Laurent, is the implementation of this patch what you suggested me? I will abandon this and re-implement with Jacopo's idea. But I would like to make sure if I didn't miss something. -Hiro > > This would allow us to: > > 1) Augment V4L2Device::listControls() to add support for menu controls > > by only collecting indexes. This would work for menu and int_menu > > controls. The indexes can be collected one by one calls to > > VIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo > > can be augmented with a contructor similar to > > > > ControlInfo::ControlInfo(Span<const ControlValue> values, > > const ControlValue &def) > > > > So that it can transport a list of int32 (the indexes) > > > > 2) The sensor db requires a bit more work, not that much actually. > > In your series you add entries like: > > > > { "Disabled", controls::draft::TestPatternModeOff }, > > > > Could this just be > > > > { 0, controls::draft::TestPatternModeOff }, > > ? > > > > Of course we are now tying the driver's patter listing order to the > > libcamera's control, if a driver changes the ordering (why would it ?) > > the sensor db will have to be updated. But this is not different.. if > > the driver changes a pattern name (why would it?) we'll have to update > > the db anyhow (possibly with versioning \o/ ) > > > > Setting a menu control goes by index anyhow, so it won't really make a > > difference (when and if we'll have to support setting menu controls). > > > > This might work for test pattern modes, as we don't actually care > > about the mode name, if not for debugging purposes. It won't work that > > well with INT_MENU controls, the most notable being CID_LINK_FREQ > > (even if its usage from userspace is limited, even not required > > possibly). In that case the index is less representitive than its > > content, so might chose for INT_MENU to collect the actual values > > instead of the indexes, and in that case we'll need a way to reverse > > the value->index association when we'll need to set an INT_MENU > > control as I assume ph will want to say "set LINK_FREQ to 192000000" > > and not "set LINK_FREQ to index 0") > > > > --------- Digressions not related to this patch ---------------------- > > --------- Mostly for discussions and not blocking ------------------- > > > > A question remains when setting a menu control: where does the > > value->index reversing happens ? > > > > Applications/Android will tell "set TEST_PATTERN to > > libcamera::controls::TestPatternXYZ" to the pipeline > > handler that will receive a control list with > > > > { TestPatternMode, TestPatternModeOff } > > > > and it will have to set it in the video device or the camera sensor. > > > > For video devices, we have a pure v4l2 controls based interface, so > > the ph will have to do the reversing: > > > > TestPatternModeOff = index 0 > > > > by itself as it knows its video devices and can create a map for that > > purpose. > > > > For camera sensor we have a v4l2 controls based interface, but that's > > not really finalized, and if we want the ph to do the value->index > > reversing it mean we'll have to expose the sensor db. Otherwise we > > introduce a libcamera::Controls based interface in CameraSensor and > > keep the reversing internal, but this ofc has other drawbacks (the > > most notable will be the fact we'll have two interfaces (one v4l2, the > > other libcamera) or we drop the v4l2 interface from CameraSensor > > (which will require IPAs to speak libcamera::controls when they send > > sensor's controls to the ph). > > > > What do you think ? > > I agreed. It sounds like the current control implementation is enough for menu because we should only store the index. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > include/libcamera/controls.h | 5 +++++ > > > src/libcamera/controls.cpp | 22 ++++++++++++++++++++++ > > > 2 files changed, 27 insertions(+) > > > > > > diff --git a/include/libcamera/controls.h > b/include/libcamera/controls.h > > > index 1a5690a5..a8deb16a 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -271,11 +271,15 @@ public: > > > const ControlValue &def = 0); > > > explicit ControlInfo(Span<const ControlValue> values, > > > const ControlValue &def = {}); > > > + explicit ControlInfo(Span<const ControlValue> values, > > > + Span<const ControlValue> extraValues, > > > + const ControlValue &def = {}); > > > > > > const ControlValue &min() const { return min_; } > > > const ControlValue &max() const { return max_; } > > > const ControlValue &def() const { return def_; } > > > const std::vector<ControlValue> &values() const { return values_; } > > > + const std::vector<ControlValue> &extraValues() const { return > extraValues_; } > > > > > > std::string toString() const; > > > > > > @@ -294,6 +298,7 @@ private: > > > ControlValue max_; > > > ControlValue def_; > > > std::vector<ControlValue> values_; > > > + std::vector<ControlValue> extraValues_; > > > }; > > > > > > using ControlIdMap = std::unordered_map<unsigned int, const ControlId > *>; > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index c58ed394..e2e8619a 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span<const ControlValue> > values, > > > values_.push_back(value); > > > } > > > > > > +/** > > > + * \brief Construct a ControlInfo from the list of valid values and > extra values > > > + * \param[in] values The control valid values > > > + * \param[in] extraValues The control valid extra values associated > with \a values > > > + * \param[in] def The control default value > > > + * > > > + * Construct a ControlInfo from a list of valid values and extra > values. The > > > + * ControlInfo minimum and maximum values are set to the first and > last members > > > + * of the values list respectively. The default value is set to \a > def if > > > + * provided, or to the minimum value otherwise. The extra values are > associated > > > + * with \a values and in the same order as \a values. > > > + * > > > + */ > > > +ControlInfo::ControlInfo(Span<const ControlValue> values, > > > + Span<const ControlValue> extraValues, > > > + const ControlValue &def) > > > + : ControlInfo(values, def) > > > +{ > > > + for (const ControlValue &extraValue : extraValues) > > > + extraValues_.push_back(extraValue); > > > +} > > > + > > > /** > > > * \fn ControlInfo::min() > > > * \brief Retrieve the minimum value of the control > > -- > Regards, > > Laurent Pinchart >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 1a5690a5..a8deb16a 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -271,11 +271,15 @@ public: const ControlValue &def = 0); explicit ControlInfo(Span<const ControlValue> values, const ControlValue &def = {}); + explicit ControlInfo(Span<const ControlValue> values, + Span<const ControlValue> extraValues, + const ControlValue &def = {}); const ControlValue &min() const { return min_; } const ControlValue &max() const { return max_; } const ControlValue &def() const { return def_; } const std::vector<ControlValue> &values() const { return values_; } + const std::vector<ControlValue> &extraValues() const { return extraValues_; } std::string toString() const; @@ -294,6 +298,7 @@ private: ControlValue max_; ControlValue def_; std::vector<ControlValue> values_; + std::vector<ControlValue> extraValues_; }; using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index c58ed394..e2e8619a 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, values_.push_back(value); } +/** + * \brief Construct a ControlInfo from the list of valid values and extra values + * \param[in] values The control valid values + * \param[in] extraValues The control valid extra values associated with \a values + * \param[in] def The control default value + * + * Construct a ControlInfo from a list of valid values and extra values. The + * ControlInfo minimum and maximum values are set to the first and last members + * of the values list respectively. The default value is set to \a def if + * provided, or to the minimum value otherwise. The extra values are associated + * with \a values and in the same order as \a values. + * + */ +ControlInfo::ControlInfo(Span<const ControlValue> values, + Span<const ControlValue> extraValues, + const ControlValue &def) + : ControlInfo(values, def) +{ + for (const ControlValue &extraValue : extraValues) + extraValues_.push_back(extraValue); +} + /** * \fn ControlInfo::min() * \brief Retrieve the minimum value of the control
The v4l2 menu contains not only index (int32_t) but also either name (string) or value (int64_t). To support it keeping ControlValue simple, this adds the extra ControlValues to ControlInfo. With the ControlInfo, indeices are stored in ControlInfo::values, and names (or values) are stored in ControlInfo::extraValues. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/controls.h | 5 +++++ src/libcamera/controls.cpp | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+)