[libcamera-devel] [PATCH] libcamera: controls: Use template in the id-based interface

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jan 02, 2020 at 12:04:42PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 02, 2020 at 12:40:23PM +0200, Laurent Pinchart wrote:
> > On Thu, Jan 02, 2020 at 09:10:38AM +0100, Jacopo Mondi wrote:
> >> On Tue, Dec 31, 2019 at 02:32:50PM +0200, Laurent Pinchart wrote:
> >>> On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote:
> >>>> The ControlList Control<> and id based interfaces to set or get controls
> >>>> differ in the way they return or receive values. The Control<> based
> >>>> interface allows the usage of raw values, while the id-based interface
> >>>> always goes through ControlValue, resulting in one additional function
> >>>> call in the caller when accessing controls by numerical id.
> >>>>
> >>>> Align the two implementations by providing access to raw values for the
> >>>> id-based interface.
> >>>>
> >>>> -       controls.get(V4L2_CID_CONTRAST).get<int32_t>()
> >>>> +	controls.get<int32_t>(V4L2_CID_CONTRAST)
> >>>
> >>> The commit message doesn't explain why this is desirable :-) This patch
> >>> may make sense as part of a bigger set of changes, but in isolation I
> >>> don't see anything wrong with accessing the ControlValue.
> >>
> >> Well, "The ControlList Control<> and id based interfaces to set or get controls
> >> differ in the way they return or receive values" explains that the API
> >> differs based on the usage of Control or numerical id and to me
> >> controls.get<int32_t>(V4L2_CID_SATURATION) is not only nicer to type than
> >> controls.get(V4L2_CID_SATURATION).get<int32_t>() but it's also
> >> consistent with controls.get<int32_t>(Controls::Brightness)
> >>
> >> I'll resend as part of a longer series if it makes more sense, but to
> >> me this discrepancy in the interface is worth being changed
> >> considering all the effort we put in having a single ControlList for
> >> libcamera and v4l2 controls.
> >
> > Note that the interface based on numerical IDs shouldn't be used by
> > applications. I'm not necessarily opposed to this change, but I would
> > like to assess its implications in a broader context. Please see below
> > for two additional comments.
> >
> >>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>>> ---
> >>>>  include/libcamera/controls.h       | 23 ++++++++++++++++++-
> >>>>  src/libcamera/controls.cpp         | 37 +++++++++++++++++-------------
> >>>>  test/ipa/ipa_wrappers_test.cpp     |  6 ++---
> >>>>  test/v4l2_videodevice/controls.cpp | 12 +++++-----
> >>>>  4 files changed, 52 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>>> index b1b73367e874..0b55359890e6 100644
> >>>> --- a/include/libcamera/controls.h
> >>>> +++ b/include/libcamera/controls.h
> >>>> @@ -220,6 +220,18 @@ public:
> >>>>  		return val->get<T>();
> >>>>  	}
> >>>>
> >>>> +	template<typename T>
> >>>> +	const T &get(unsigned int id) const
> >>>> +	{
> >>>> +		const ControlValue *val = find(id);
> >>>> +		if (!val) {
> >>>> +			static T t(0);
> >>>> +			return t;
> >
> > This is the part that bothers me the most in our template interface, and
> > I wish we could solve it differently instead of duplicating it :-)
> 
> Why is this worse than:
> 
> >>>> -const ControlValue &ControlList::get(unsigned int id) const
> >>>> -{
> >>>> -	static ControlValue zero;
> >>>> -
> >>>> -	const ControlValue *val = find(id);
> >>>> -	if (!val)
> >>>> -		return zero;
> >>>> -
> >>>> -	return *val;
> >>>> -}

It's not, and that's my point, that's the part that bothers me the most
in our current interface. Your patch doesn't make it worse.

-- 
Regards,

Laurent Pinchart