[libcamera-devel] [PATCH v6 1/9] libcamera: control_serializer: Save serialized ControlInfoMap in a cache

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Dec 28, 2020 at 06:48:11PM +0900, paul.elder at ideasonboard.com wrote:
> On Sun, Dec 27, 2020 at 12:36:15PM +0100, Niklas Söderlund wrote:
> > On 2020-12-24 17:15:26 +0900, Paul Elder wrote:
> > > The ControlSerializer saves all ControlInfoMaps that it has already
> > > (de)serialized, in order to (de)serialize ControlLists that contain the
> > > ControlInfoMaps. Leverage this to cache ControlInfoMaps, such that the
> > > ControlSerializer will not re-(de)serialize a ControlInfoMap that it has
> > > previously (de)serialized.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > 
> > > ---
> > > New in v6
> > > ---
> > >  .../libcamera/internal/control_serializer.h   |  1 +
> > >  src/libcamera/control_serializer.cpp          | 30 +++++++++++++++++++
> > >  2 files changed, 31 insertions(+)
> > > 
> > > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> > > index 0ab29d9a..76cb3c10 100644
> > > --- a/include/libcamera/internal/control_serializer.h
> > > +++ b/include/libcamera/internal/control_serializer.h
> > > @@ -33,6 +33,7 @@ public:
> > >  	template<typename T>
> > >  	T deserialize(ByteStreamBuffer &buffer);
> > >  
> > > +	bool isCached(const ControlInfoMap *infoMap);

Blank line here ?

> > >  private:
> > >  	static size_t binarySize(const ControlValue &value);
> > >  	static size_t binarySize(const ControlInfo &info);
> > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > > index 258db6df..4cf1c720 100644
> > > --- a/src/libcamera/control_serializer.cpp
> > > +++ b/src/libcamera/control_serializer.cpp
> > > @@ -173,6 +173,11 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)
> > >  int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > >  				 ByteStreamBuffer &buffer)
> > >  {
> > > +	if (isCached(&infoMap)) {
> > > +		LOG(Serializer, Info) << "Serializing ControlInfoMap from cache";
> > 
> > I wonder if this and the one below should be Debug messages?
> 
> Yeah it should be.

And I think should mention that serialization is skipped, not that the
ControlInfoMap is serialized from the cache.

		LOG(Serializer, Debug)
			<< "Skipping already serialized ControlInfoMap";

> > > +		return 0;
> > > +	}
> > > +
> > >  	/* Compute entries and data required sizes. */
> > >  	size_t entriesSize = infoMap.size()
> > >  			   * sizeof(struct ipa_control_info_entry);
> > > @@ -347,6 +352,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > >  		return {};
> > >  	}
> > >  
> > > +	auto iter = infoMaps_.find(hdr->handle);
> > > +	if (iter != infoMaps_.end()) {
> > > +		LOG(Serializer, Info) << "Deserializing ControlInfoMap from cache";

Debug here too, and maybe

		LOG(Serializer, Info) << "Use cached ControlInfoMap";

as you don't deserialize it.

> > > +		return iter->second;
> > > +	}
> > 
> > I think you should either fold the check from isChached() in above or 
> > add a isCached(unsigned int) and use it here. I'm leaning towards the 
> > former as then if we switch to C++20 we could use 
> > std::map<>::contains().
> 
> I don't see how the former would work. The latter would just be the same
> as what's here, and it's only used once.
> 
> > > +
> > >  	if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {
> > >  		LOG(Serializer, Error)
> > >  			<< "Unsupported controls format version "
> > > @@ -485,4 +496,23 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
> > >  	return ctrls;
> > >  }
> > >  
> > > +/**
> > > + * \brief Check if some ControlInfoMap is cached
> > 
> > s/some/a/
> > 
> > > + * \param[in] infoMap The ControlInfoMap to check
> > > + *
> > > + * The ControlSerializer caches all ControlInfoMaps that it has (de)serialized.
> > > + * This function checks if \a infoMap is in the cache.
> > > + *
> > > + * \return True if \a infoMap is in the cache or if \a infoMap is
> > > + * controls::controls, false otherwise
> > 
> > controls::controls ?
> 
> Oh, it should be nullptr here. I was thinking about ControlLists that
> use controls::controls.

And I'd drop this, as you never pass nullptr to this function.

 * \return True if \a infoMap is in the cache or false otherwise

> > > + */
> > > +bool ControlSerializer::isCached(const ControlInfoMap *infoMap)

And I'd pass a const reference instead of a const pointer, to make sure
it can never be null.

> > > +{
> > > +	if (!infoMap)
> > > +		return true;
> > > +

This can be dropped.

> > > +	auto iter = infoMapHandles_.find(infoMap);
> > > +	return iter != infoMapHandles_.end();

You can just return

	return infoMapHandles_.count(infoMap);

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > +}
> > > +
> > >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart