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

[Jacopo Mondi]

Hi Laurent

On Thu, Jan 02, 2020 at 12:40:23PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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;
> > > > -}

Thanks
   j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200102/658ff435/attachment.sig>