[libcamera-devel] [PATCH v3 1/2] libcamera: serialiser: store/load all ControlValue types

[paul.elder at ideasonboard.com]

Hi Christian,

Thank you for the patch.

In the subject:

s/serialiser/control_serializer/

On Sat, Sep 03, 2022 at 12:49:38AM +0200, Christian Rauch via libcamera-devel wrote:
> The min/max/def ControlValue of a ControlInfo can take arbitrary types that
> are different from each other and different from the ControlId type.
> The serialiser serialises these ControlValue separately by there type but

s/there/their/

> does not store the type. The deserialiser assumes that ControlValue types
> match the ControlId type. If this is not the case, deserialisation will try
> to deserialise values of the wrong type.
> 
> Fix this by serialising each of the min/max/def ControlValue's ControlType
> and storing it just before the serialised ControlValue.
> 
> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>

This fixes bug 137 (not sure if there's a specific "Fixes:" tag for
this)

Tested-by: Paul Elder <paul.elder at ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> ---
>  .../libcamera/internal/control_serializer.h   |  4 +--
>  src/libcamera/control_serializer.cpp          | 28 +++++++++----------
>  2 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> index 99e57fee..a38ca6b0 100644
> --- a/include/libcamera/internal/control_serializer.h
> +++ b/include/libcamera/internal/control_serializer.h
> @@ -47,9 +47,9 @@ private:
>  	static void store(const ControlValue &value, ByteStreamBuffer &buffer);
>  	static void store(const ControlInfo &info, ByteStreamBuffer &buffer);
> 
> -	ControlValue loadControlValue(ControlType type, ByteStreamBuffer &buffer,
> +	ControlValue loadControlValue(ByteStreamBuffer &buffer,
>  				      bool isArray = false, unsigned int count = 1);
> -	ControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer);
> +	ControlInfo loadControlInfo(ByteStreamBuffer &buffer);
> 
>  	unsigned int serial_;
>  	unsigned int serialSeed_;
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index e87d2362..0cf719bd 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -144,7 +144,7 @@ void ControlSerializer::reset()
> 
>  size_t ControlSerializer::binarySize(const ControlValue &value)
>  {
> -	return value.data().size_bytes();
> +	return sizeof(ControlType) + value.data().size_bytes();
>  }
> 
>  size_t ControlSerializer::binarySize(const ControlInfo &info)
> @@ -195,6 +195,8 @@ size_t ControlSerializer::binarySize(const ControlList &list)
>  void ControlSerializer::store(const ControlValue &value,
>  			      ByteStreamBuffer &buffer)
>  {
> +	const ControlType type = value.type();
> +	buffer.write(&type);
>  	buffer.write(value.data());
>  }
> 
> @@ -379,11 +381,13 @@ int ControlSerializer::serialize(const ControlList &list,
>  	return 0;
>  }
> 
> -ControlValue ControlSerializer::loadControlValue(ControlType type,
> -						 ByteStreamBuffer &buffer,
> +ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
>  						 bool isArray,
>  						 unsigned int count)
>  {
> +	ControlType type;
> +	buffer.read(&type);
> +
>  	ControlValue value;
> 
>  	value.reserve(type, isArray, count);
> @@ -392,15 +396,11 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
>  	return value;
>  }
> 
> -ControlInfo ControlSerializer::loadControlInfo(ControlType type,
> -					       ByteStreamBuffer &b)
> +ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)
>  {
> -	if (type == ControlTypeString)
> -		type = ControlTypeInteger32;
> -
> -	ControlValue min = loadControlValue(type, b);
> -	ControlValue max = loadControlValue(type, b);
> -	ControlValue def = loadControlValue(type, b);
> +	ControlValue min = loadControlValue(b);
> +	ControlValue max = loadControlValue(b);
> +	ControlValue def = loadControlValue(b);
> 
>  	return ControlInfo(min, max, def);
>  }
> @@ -513,7 +513,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  		}
> 
>  		/* Create and store the ControlInfo. */
> -		ctrls.emplace(controlId, loadControlInfo(type, values));
> +		ctrls.emplace(controlId, loadControlInfo(values));
>  	}
> 
>  	/*
> @@ -624,10 +624,8 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  			return {};
>  		}
> 
> -		ControlType type = static_cast<ControlType>(entry->type);
>  		ctrls.set(entry->id,
> -			  loadControlValue(type, values, entry->is_array,
> -					   entry->count));
> +			  loadControlValue(values, entry->is_array, entry->count));
>  	}
> 
>  	return ctrls;
> --
> 2.34.1
>