[libcamera-devel] [PATCH v4 1/2] libcamera: control_serializer: store/load all ControlValue types

[Umang Jain]

Hi Christian

Thank you for the patch,

On 9/4/22 3:03 AM, 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 their 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.
>
> Fixes: https://bugs.libcamera.org/show_bug.cgi?id=137
>
> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
> Tested-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

Adding a unit test for this in 
test/serialization/control_serialization.cpp might be good idea,

but for this patch,

Reviewed-by: Umang Jain <umang.jain 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
>