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

[paul.elder at ideasonboard.com]

Hi Niklas,

Thank you for the review.

On Sun, Dec 27, 2020 at 12:36:15PM +0100, Niklas Söderlund wrote:
> Hi Paul,
> 
> Thanks for your work.
> 
> 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);
> >  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.

> > +		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";
> > +		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.


Thanks,

Paul

> > + */
> > +bool ControlSerializer::isCached(const ControlInfoMap *infoMap)
> > +{
> > +	if (!infoMap)
> > +		return true;
> > +
> > +	auto iter = infoMapHandles_.find(infoMap);
> > +	return iter != infoMapHandles_.end();
> > +}
> > +
> >  } /* namespace libcamera */
> > -- 
> > 2.27.0
> > 
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> 
> -- 
> Regards,
> Niklas Söderlund