[libcamera-devel] [PATCH 09/10] libcamera: controls: Merge ControlInfoMap and V4L2ControlInfoMap

[Laurent Pinchart]

Hi Niklas,

On Tue, Oct 15, 2019 at 02:40:49AM +0200, Niklas Söderlund wrote:
> On 2019-10-14 02:27:55 +0300, Laurent Pinchart wrote:
> > The ControlInfoMap and V4L2ControlInfoMap classes are very similar, with
> > the latter adding convenience accessors based on numerical IDs for the
> > former, as well as a cached idmap. Both features can be useful for
> > ControlInfoMap in the context of serialisation, and merging the two
> > classes will further simplify the IPA API.
> > 
> > Import all the features of V4L2ControlInfoMap into ControlInfoMap,
> > turning the latter into a real class. A few new constructors and
> > assignment operators are added for completeness.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> The change to pipeline handler implementations to shorten the line 
> length could be done in a separate patch ;-) With out without that 
> addressed,

They're not there to shorten the line length, they actually change the
behaviour.

> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> > ---
> >  include/ipa/ipa_interface.h              |   2 +-
> >  include/libcamera/controls.h             |  45 +++++++-
> >  src/ipa/ipa_vimc.cpp                     |   2 +-
> >  src/ipa/rkisp1/rkisp1.cpp                |   6 +-
> >  src/libcamera/camera_sensor.cpp          |   2 +-
> >  src/libcamera/controls.cpp               | 139 ++++++++++++++++++++++-
> >  src/libcamera/include/camera_sensor.h    |   4 +-
> >  src/libcamera/include/v4l2_controls.h    |  36 +-----
> >  src/libcamera/include/v4l2_device.h      |   4 +-
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  11 +-
> >  src/libcamera/pipeline/uvcvideo.cpp      |  12 +-
> >  src/libcamera/pipeline/vimc.cpp          |  11 +-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp  |   2 +-
> >  src/libcamera/v4l2_controls.cpp          |  94 +--------------
> >  src/libcamera/v4l2_device.cpp            |   2 +-
> >  test/v4l2_videodevice/controls.cpp       |   2 +-
> >  16 files changed, 220 insertions(+), 154 deletions(-)

[snip]

> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 5534a2edb567..80414c6f0748 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -118,7 +118,50 @@ private:
> >  };
> >  
> >  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;
> > -using ControlInfoMap = std::unordered_map<const ControlId *, ControlRange>;
> > +
> > +class ControlInfoMap : private std::unordered_map<const ControlId *, ControlRange>
> > +{
> > +public:
> > +	using Map = std::unordered_map<const ControlId *, ControlRange>;
> > +
> > +	ControlInfoMap() = default;
> > +	ControlInfoMap(const ControlInfoMap &other) = default;
> > +	ControlInfoMap(std::initializer_list<Map::value_type> init);
> > +
> > +	ControlInfoMap &operator=(const ControlInfoMap &other) = default;
> > +	ControlInfoMap &operator=(std::initializer_list<Map::value_type> init);
> > +	ControlInfoMap &operator=(Map &&info);
> > +
> > +	using Map::key_type;
> > +	using Map::mapped_type;
> > +	using Map::value_type;
> > +	using Map::size_type;
> > +	using Map::iterator;
> > +	using Map::const_iterator;
> > +
> > +	using Map::begin;
> > +	using Map::cbegin;
> > +	using Map::end;
> > +	using Map::cend;
> > +	using Map::at;
> > +	using Map::empty;
> > +	using Map::size;
> > +	using Map::count;
> > +	using Map::find;
> > +
> > +	mapped_type &at(unsigned int key);
> > +	const mapped_type &at(unsigned int key) const;
> > +	size_type count(unsigned int key) const;
> > +	iterator find(unsigned int key);
> > +	const_iterator find(unsigned int key) const;
> > +
> > +	const ControlIdMap &idmap() const { return idmap_; }
> > +
> > +private:
> > +	void generateIdmap();
> > +
> > +	ControlIdMap idmap_;
> > +};

[snip]

> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 9b19bde8a274..7a28b03b8d38 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp

[snip]

> > @@ -875,9 +875,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >  	std::unique_ptr<RkISP1CameraData> data =
> >  		utils::make_unique<RkISP1CameraData>(this);
> >  
> > -	data->controlInfo_.emplace(std::piecewise_construct,
> > -				   std::forward_as_tuple(&controls::AeEnable),
> > -				   std::forward_as_tuple(false, true));
> > +	ControlInfoMap::Map ctrls;
> > +	ctrls.emplace(std::piecewise_construct,
> > +		      std::forward_as_tuple(&controls::AeEnable),
> > +		      std::forward_as_tuple(false, true));
> > +
> > +	data->controlInfo_ = std::move(ctrls);

ctrls is a ControlInfoMap::Map, and data->controlInfo_ is a
ControlInfoMap. The latter is constructed from the former using the
ControlInfoMap::operator=(Map &&info) assignment operator. This is
needed as ControlInfoMap doesn't provide accessors that allow modifying
its contents.

> >  
> >  	data->sensor_ = new CameraSensor(sensor);
> >  	ret = data->sensor_->init();

-- 
Regards,

Laurent Pinchart