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