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

17282 diff mbox series

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
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@gmx.de>
---
 .../libcamera/internal/control_serializer.h   |  4 +--
 src/libcamera/control_serializer.cpp          | 28 +++++++++----------
 2 files changed, 15 insertions(+), 17 deletions(-)

--
2.34.1

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@gmx.de>

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

Tested-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@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
>