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

[Niklas Söderlund]

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?

> +		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().

> +
>  	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 ?

> + */
> +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