[libcamera-devel] [PATCH 26/31] libcamera: control_serializer: Use zero-copy ByteStreamBuffer::read()

[Kieran Bingham]

On 29/02/2020 16:42, Laurent Pinchart wrote:
> Use the zero-copy variant of ByteStreamBuffer::read() to read packet
> haders and control entries. This enhance performance of ControlList and

/haders/headers/

/enhance/enhances the/ or /enhance/will enhance the/


> ControlInfoMap deserialization.
> 
> Deserialization of the actual ControlValue is untouched for now and will
> be optimized later.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Very happy to see removal of copies from our serialization :)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/libcamera/control_serializer.cpp | 74 +++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index dc87b96f384b..6cac70739468 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -364,39 +364,46 @@ ControlRange ControlSerializer::load<ControlRange>(ControlType type,
>  template<>
>  ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer)
>  {
> -	struct ipa_controls_header hdr;
> -	buffer.read(&hdr);
> +	const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>();

Aha, now I see why it doesn't return a span ...

This looks like a lot of template additions ... couldn't they be just as
easily added for this functionality with taking a size parameter and
returning a void pointer?

(or does C++ not like that)


> +	if (!hdr) {
> +		LOG(Serializer, Error) << "Out of data";
> +		return {};
> +	}
>  
> -	if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {
> +	if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {
>  		LOG(Serializer, Error)
>  			<< "Unsupported controls format version "
> -			<< hdr.version;
> +			<< hdr->version;
>  		return {};
>  	}
>  
> -	ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));
> -	ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);
> +	ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));
> +	ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);
>  
>  	if (buffer.overflow()) {
> -		LOG(Serializer, Error) << "Serialized packet too small";
> +		LOG(Serializer, Error) << "Out of data";
>  		return {};
>  	}
>  
>  	ControlInfoMap::Map ctrls;
>  
> -	for (unsigned int i = 0; i < hdr.entries; ++i) {
> -		struct ipa_control_range_entry entry;
> -		entries.read(&entry);
> +	for (unsigned int i = 0; i < hdr->entries; ++i) {
> +		const struct ipa_control_range_entry *entry =
> +			entries.read<decltype(*entry)>();
> +		if (!entry) {
> +			LOG(Serializer, Error) << "Out of data";
> +			return {};
> +		}
>  
>  		/* Create and cache the individual ControlId. */
> -		ControlType type = static_cast<ControlType>(entry.type);
> +		ControlType type = static_cast<ControlType>(entry->type);
>  		/**
>  		 * \todo Find a way to preserve the control name for debugging
>  		 * purpose.
>  		 */
> -		controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, "", type));
> +		controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
>  
> -		if (entry.offset != values.offset()) {
> +		if (entry->offset != values.offset()) {
>  			LOG(Serializer, Error)
>  				<< "Bad data, entry offset mismatch (entry "
>  				<< i << ")";
> @@ -412,8 +419,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  	 * Create the ControlInfoMap in the cache, and store the map to handle
>  	 * association.
>  	 */
> -	ControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls);
> -	infoMapHandles_[&map] = hdr.handle;
> +	ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);
> +	infoMapHandles_[&map] = hdr->handle;
>  
>  	return map;
>  }
> @@ -430,21 +437,24 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  template<>
>  ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer)
>  {
> -	struct ipa_controls_header hdr;
> -	buffer.read(&hdr);
> +	const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>();
> +	if (!hdr) {
> +		LOG(Serializer, Error) << "Out of data";
> +		return {};
> +	}
>  
> -	if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {
> +	if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {
>  		LOG(Serializer, Error)
>  			<< "Unsupported controls format version "
> -			<< hdr.version;
> +			<< hdr->version;
>  		return {};
>  	}
>  
> -	ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));
> -	ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);
> +	ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));
> +	ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);
>  
>  	if (buffer.overflow()) {
> -		LOG(Serializer, Error) << "Serialized packet too small";
> +		LOG(Serializer, Error) << "Out of data";
>  		return {};
>  	}
>  
> @@ -456,10 +466,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  	 * use the global control::control idmap.
>  	 */
>  	const ControlInfoMap *infoMap;
> -	if (hdr.handle) {
> +	if (hdr->handle) {
>  		auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),
>  					 [&](decltype(infoMapHandles_)::value_type &entry) {
> -						 return entry.second == hdr.handle;
> +						 return entry.second == hdr->handle;
>  					 });
>  		if (iter == infoMapHandles_.end()) {
>  			LOG(Serializer, Error)
> @@ -474,19 +484,23 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  
>  	ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);
>  
> -	for (unsigned int i = 0; i < hdr.entries; ++i) {
> -		struct ipa_control_value_entry entry;
> -		entries.read(&entry);
> +	for (unsigned int i = 0; i < hdr->entries; ++i) {
> +		const struct ipa_control_value_entry *entry =
> +			entries.read<decltype(*entry)>();
> +		if (!entry) {
> +			LOG(Serializer, Error) << "Out of data";
> +			return {};
> +		}
>  
> -		if (entry.offset != values.offset()) {
> +		if (entry->offset != values.offset()) {
>  			LOG(Serializer, Error)
>  				<< "Bad data, entry offset mismatch (entry "
>  				<< i << ")";
>  			return {};
>  		}
>  
> -		ControlType type = static_cast<ControlType>(entry.type);
> -		ctrls.set(entry.id, load<ControlValue>(type, values));
> +		ControlType type = static_cast<ControlType>(entry->type);
> +		ctrls.set(entry->id, load<ControlValue>(type, values));
>  	}
>  
>  	return ctrls;
> 

-- 
Regards
--
Kieran