[libcamera-devel,v6,1/9] libcamera: control_serializer: Save serialized ControlInfoMap in a cache
diff mbox series

Message ID 20201224081534.41601-2-paul.elder@ideasonboard.com
State Changes Requested
Delegated to: Paul Elder
Headers show
Series
  • IPA isolation: Part 1: Core components
Related show

Commit Message

Paul Elder Dec. 24, 2020, 8:15 a.m. UTC 
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@ideasonboard.com>

---
New in v6
---
 .../libcamera/internal/control_serializer.h   |  1 +
 src/libcamera/control_serializer.cpp          | 30 +++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

Niklas Söderlund Dec. 27, 2020, 11:36 a.m. UTC | #1 
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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Paul Elder Dec. 28, 2020, 9:48 a.m. UTC | #2 
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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> 
> -- 
> Regards,
> Niklas Söderlund
Laurent Pinchart Feb. 1, 2021, 9:40 p.m. UTC | #3 
Hi Paul,

Thank you for the patch.

On Mon, Dec 28, 2020 at 06:48:11PM +0900, paul.elder@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@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@ideasonboard.com>

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

Patch
diff mbox series 

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";
+		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;
+	}
+
 	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
+ * \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
+ */
+bool ControlSerializer::isCached(const ControlInfoMap *infoMap)
+{
+	if (!infoMap)
+		return true;
+
+	auto iter = infoMapHandles_.find(infoMap);
+	return iter != infoMapHandles_.end();
+}
+
 } /* namespace libcamera */