[v2,1/2] libcamera: control_serializer: Add array info to serialized ControlValue
diff mbox series

Message ID 20251007102747.2746478-2-paul.elder@ideasonboard.com
State New
Headers show
Series
  • Fix ControlSerializer deserializing array controls
Related show

Commit Message

Paul Elder Oct. 7, 2025, 10:27 a.m. UTC 
Array controls (eg. ColourCorrectionMatrix, FrameDurationLimits,
ColourGains) are serialized properly by the ControlSerializer, but are
not deserialized properly. This is because their arrayness and size are
not considered during deserialization.

Fix this by adding arrayness and size to the serialized form of all
ControlValues.

This was not noticed before as the default value of the ControlInfo of
other array controls had been set to scalar values similar to min/max,
and ColourCorrectionMatrix was the first control to properly define a
non-scalar default value.

Other array controls that define a scalar default value need to be
fixed to define a properly sized default ControlInfo value.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=285
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- make it so that the *serialized form* of ControlValue includes
  arrayness and size instead
  - Compared to v1, this ties the arrayness and size information to
    ControlValue instead of ControlId, which as a side effect allows us
    to support scalar and array min/max values, not just def values.
    This also gives us support for variable-length arrays
---
 .../libcamera/internal/control_serializer.h   |  8 +++--
 src/libcamera/control_serializer.cpp          | 30 ++++++++++++++-----
 2 files changed, 29 insertions(+), 9 deletions(-)

Comments

Barnabás Pőcze Oct. 7, 2025, 10:45 a.m. UTC | #1 
2025. 10. 07. 12:27 keltezéssel, Paul Elder írta:
> Array controls (eg. ColourCorrectionMatrix, FrameDurationLimits,
> ColourGains) are serialized properly by the ControlSerializer, but are
> not deserialized properly. This is because their arrayness and size are
> not considered during deserialization.
> 
> Fix this by adding arrayness and size to the serialized form of all
> ControlValues.
> 
> This was not noticed before as the default value of the ControlInfo of
> other array controls had been set to scalar values similar to min/max,
> and ColourCorrectionMatrix was the first control to properly define a
> non-scalar default value.
> 
> Other array controls that define a scalar default value need to be
> fixed to define a properly sized default ControlInfo value.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=285
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - make it so that the *serialized form* of ControlValue includes
>    arrayness and size instead
>    - Compared to v1, this ties the arrayness and size information to
>      ControlValue instead of ControlId, which as a side effect allows us
>      to support scalar and array min/max values, not just def values.
>      This also gives us support for variable-length arrays
> ---
>   .../libcamera/internal/control_serializer.h   |  8 +++--
>   src/libcamera/control_serializer.cpp          | 30 ++++++++++++++-----
>   2 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> index 8a63ae44a13e..683bb13df92f 100644
> --- a/include/libcamera/internal/control_serializer.h
> +++ b/include/libcamera/internal/control_serializer.h
> @@ -41,14 +41,18 @@ public:
>   	bool isCached(const ControlInfoMap &infoMap);
>   
>   private:
> +	struct ControlValueHeader {
> +		bool isArray;
> +		std::size_t numElements;

Why not have `type` in this as well?


> +	};
> +
>   	static size_t binarySize(const ControlValue &value);
>   	static size_t binarySize(const ControlInfo &info);
>   
>   	static void store(const ControlValue &value, ByteStreamBuffer &buffer);
>   	static void store(const ControlInfo &info, ByteStreamBuffer &buffer);
>   
> -	ControlValue loadControlValue(ByteStreamBuffer &buffer,
> -				      bool isArray = false, unsigned int count = 1);
> +	ControlValue loadControlValue(ByteStreamBuffer &buffer);
>   	ControlInfo loadControlInfo(ByteStreamBuffer &buffer);
>   
>   	unsigned int serial_;
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index 050f8512bd52..463f6ab9118d 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -144,7 +144,12 @@ void ControlSerializer::reset()
>   
>   size_t ControlSerializer::binarySize(const ControlValue &value)
>   {
> -	return sizeof(ControlType) + value.data().size_bytes();
> +	/*
> +	 * Allocate extra space to save isArray and the number of elements, to
> +	 * support array-type ControlValues.
> +	 */
> +	return sizeof(ControlType) + sizeof(ControlValueHeader) +
> +	       value.data().size_bytes();
>   }
>   
>   size_t ControlSerializer::binarySize(const ControlInfo &info)
> @@ -197,6 +202,13 @@ void ControlSerializer::store(const ControlValue &value,
>   {
>   	const ControlType type = value.type();
>   	buffer.write(&type);
> +
> +	ControlValueHeader cvh = {
> +		value.isArray(),
> +		value.numElements(),
> +	};
> +	buffer.write(&cvh);
> +
>   	buffer.write(value.data());
>   }
>   
> @@ -368,6 +380,10 @@ int ControlSerializer::serialize(const ControlList &list,
>   		struct ipa_control_value_entry entry;
>   		entry.id = id;
>   		entry.type = value.type();
> +		/*
> +		 * .is_array and .count are now unused as these have been moved
> +		 * to ControlSerializer::store(const ControlValue &, ByteStreamBuffer &)
> +		 */
>   		entry.is_array = value.isArray();
>   		entry.count = value.numElements();

I think `type` is unused as well. So I wonder if there are any downsides to removing
the unused fields from `ipa_control_value_entry` and maybe even `ipa_control_info_entry`?


Regards,
Barnabás Pőcze


>   		entry.offset = values.offset();
> @@ -382,16 +398,16 @@ int ControlSerializer::serialize(const ControlList &list,
>   	return 0;
>   }
>   
> -ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
> -						 bool isArray,
> -						 unsigned int count)
> +ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer)
>   {
>   	ControlType type;
>   	buffer.read(&type);
>   
> -	ControlValue value;
> +	ControlValueHeader cvh;
> +	buffer.read(&cvh);
>   
> -	value.reserve(type, isArray, count);
> +	ControlValue value;
> +	value.reserve(type, cvh.isArray, cvh.numElements);
>   	buffer.read(value.data());
>   
>   	return value;
> @@ -633,7 +649,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>   		}
>   
>   		ctrls.set(entry->id,
> -			  loadControlValue(values, entry->is_array, entry->count));
> +			  loadControlValue(values));
>   	}
>   
>   	return ctrls;
Paul Elder Oct. 8, 2025, 4:54 a.m. UTC | #2 
Quoting Barnabás Pőcze (2025-10-07 19:45:44)
> 2025. 10. 07. 12:27 keltezéssel, Paul Elder írta:
> > Array controls (eg. ColourCorrectionMatrix, FrameDurationLimits,
> > ColourGains) are serialized properly by the ControlSerializer, but are
> > not deserialized properly. This is because their arrayness and size are
> > not considered during deserialization.
> > 
> > Fix this by adding arrayness and size to the serialized form of all
> > ControlValues.
> > 
> > This was not noticed before as the default value of the ControlInfo of
> > other array controls had been set to scalar values similar to min/max,
> > and ColourCorrectionMatrix was the first control to properly define a
> > non-scalar default value.
> > 
> > Other array controls that define a scalar default value need to be
> > fixed to define a properly sized default ControlInfo value.
> > 
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=285
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - make it so that the *serialized form* of ControlValue includes
> >    arrayness and size instead
> >    - Compared to v1, this ties the arrayness and size information to
> >      ControlValue instead of ControlId, which as a side effect allows us
> >      to support scalar and array min/max values, not just def values.
> >      This also gives us support for variable-length arrays
> > ---
> >   .../libcamera/internal/control_serializer.h   |  8 +++--
> >   src/libcamera/control_serializer.cpp          | 30 ++++++++++++++-----
> >   2 files changed, 29 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> > index 8a63ae44a13e..683bb13df92f 100644
> > --- a/include/libcamera/internal/control_serializer.h
> > +++ b/include/libcamera/internal/control_serializer.h
> > @@ -41,14 +41,18 @@ public:
> >       bool isCached(const ControlInfoMap &infoMap);
> >   
> >   private:
> > +     struct ControlValueHeader {
> > +             bool isArray;
> > +             std::size_t numElements;
> 
> Why not have `type` in this as well?

type is already serialized in ipa_control_{info,value}_entry, so I thought we
don't need to duplicate/move it here (just kidding, it was an oversight).

I suppose an ABI breaking release is our chance to change the serialized
format. If we're going to redesign all of this... I feel like instead of moving
things from ipa_control_{info,value}_entry here, it would be better to move
this there. Maybe it would be better to reuse ipa_control_value_entry for
ControlInfos, since a ControlInfo is just a collection of three ControlValues.

What do you think?

> 
> 
> > +     };
> > +
> >       static size_t binarySize(const ControlValue &value);
> >       static size_t binarySize(const ControlInfo &info);
> >   
> >       static void store(const ControlValue &value, ByteStreamBuffer &buffer);
> >       static void store(const ControlInfo &info, ByteStreamBuffer &buffer);
> >   
> > -     ControlValue loadControlValue(ByteStreamBuffer &buffer,
> > -                                   bool isArray = false, unsigned int count = 1);
> > +     ControlValue loadControlValue(ByteStreamBuffer &buffer);
> >       ControlInfo loadControlInfo(ByteStreamBuffer &buffer);
> >   
> >       unsigned int serial_;
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index 050f8512bd52..463f6ab9118d 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -144,7 +144,12 @@ void ControlSerializer::reset()
> >   
> >   size_t ControlSerializer::binarySize(const ControlValue &value)
> >   {
> > -     return sizeof(ControlType) + value.data().size_bytes();
> > +     /*
> > +      * Allocate extra space to save isArray and the number of elements, to
> > +      * support array-type ControlValues.
> > +      */
> > +     return sizeof(ControlType) + sizeof(ControlValueHeader) +
> > +            value.data().size_bytes();
> >   }
> >   
> >   size_t ControlSerializer::binarySize(const ControlInfo &info)
> > @@ -197,6 +202,13 @@ void ControlSerializer::store(const ControlValue &value,
> >   {
> >       const ControlType type = value.type();
> >       buffer.write(&type);
> > +
> > +     ControlValueHeader cvh = {
> > +             value.isArray(),
> > +             value.numElements(),
> > +     };
> > +     buffer.write(&cvh);
> > +
> >       buffer.write(value.data());
> >   }
> >   
> > @@ -368,6 +380,10 @@ int ControlSerializer::serialize(const ControlList &list,
> >               struct ipa_control_value_entry entry;
> >               entry.id = id;
> >               entry.type = value.type();
> > +             /*
> > +              * .is_array and .count are now unused as these have been moved
> > +              * to ControlSerializer::store(const ControlValue &, ByteStreamBuffer &)
> > +              */
> >               entry.is_array = value.isArray();
> >               entry.count = value.numElements();
> 
> I think `type` is unused as well. So I wonder if there are any downsides to removing
> the unused fields from `ipa_control_value_entry` and maybe even `ipa_control_info_entry`?

afaict it is used in ControlSerializer::loadControlValue(ByteStreamBuffer &buffer)
and ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer)
to deserialize ControlValues.


Thanks,

Paul

> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> >               entry.offset = values.offset();
> > @@ -382,16 +398,16 @@ int ControlSerializer::serialize(const ControlList &list,
> >       return 0;
> >   }
> >   
> > -ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
> > -                                              bool isArray,
> > -                                              unsigned int count)
> > +ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer)
> >   {
> >       ControlType type;
> >       buffer.read(&type);
> >   
> > -     ControlValue value;
> > +     ControlValueHeader cvh;
> > +     buffer.read(&cvh);
> >   
> > -     value.reserve(type, isArray, count);
> > +     ControlValue value;
> > +     value.reserve(type, cvh.isArray, cvh.numElements);
> >       buffer.read(value.data());
> >   
> >       return value;
> > @@ -633,7 +649,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
> >               }
> >   
> >               ctrls.set(entry->id,
> > -                       loadControlValue(values, entry->is_array, entry->count));
> > +                       loadControlValue(values));
> >       }
> >   
> >       return ctrls;
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
index 8a63ae44a13e..683bb13df92f 100644
--- a/include/libcamera/internal/control_serializer.h
+++ b/include/libcamera/internal/control_serializer.h
@@ -41,14 +41,18 @@  public:
 	bool isCached(const ControlInfoMap &infoMap);
 
 private:
+	struct ControlValueHeader {
+		bool isArray;
+		std::size_t numElements;
+	};
+
 	static size_t binarySize(const ControlValue &value);
 	static size_t binarySize(const ControlInfo &info);
 
 	static void store(const ControlValue &value, ByteStreamBuffer &buffer);
 	static void store(const ControlInfo &info, ByteStreamBuffer &buffer);
 
-	ControlValue loadControlValue(ByteStreamBuffer &buffer,
-				      bool isArray = false, unsigned int count = 1);
+	ControlValue loadControlValue(ByteStreamBuffer &buffer);
 	ControlInfo loadControlInfo(ByteStreamBuffer &buffer);
 
 	unsigned int serial_;
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index 050f8512bd52..463f6ab9118d 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -144,7 +144,12 @@  void ControlSerializer::reset()
 
 size_t ControlSerializer::binarySize(const ControlValue &value)
 {
-	return sizeof(ControlType) + value.data().size_bytes();
+	/*
+	 * Allocate extra space to save isArray and the number of elements, to
+	 * support array-type ControlValues.
+	 */
+	return sizeof(ControlType) + sizeof(ControlValueHeader) +
+	       value.data().size_bytes();
 }
 
 size_t ControlSerializer::binarySize(const ControlInfo &info)
@@ -197,6 +202,13 @@  void ControlSerializer::store(const ControlValue &value,
 {
 	const ControlType type = value.type();
 	buffer.write(&type);
+
+	ControlValueHeader cvh = {
+		value.isArray(),
+		value.numElements(),
+	};
+	buffer.write(&cvh);
+
 	buffer.write(value.data());
 }
 
@@ -368,6 +380,10 @@  int ControlSerializer::serialize(const ControlList &list,
 		struct ipa_control_value_entry entry;
 		entry.id = id;
 		entry.type = value.type();
+		/*
+		 * .is_array and .count are now unused as these have been moved
+		 * to ControlSerializer::store(const ControlValue &, ByteStreamBuffer &)
+		 */
 		entry.is_array = value.isArray();
 		entry.count = value.numElements();
 		entry.offset = values.offset();
@@ -382,16 +398,16 @@  int ControlSerializer::serialize(const ControlList &list,
 	return 0;
 }
 
-ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
-						 bool isArray,
-						 unsigned int count)
+ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer)
 {
 	ControlType type;
 	buffer.read(&type);
 
-	ControlValue value;
+	ControlValueHeader cvh;
+	buffer.read(&cvh);
 
-	value.reserve(type, isArray, count);
+	ControlValue value;
+	value.reserve(type, cvh.isArray, cvh.numElements);
 	buffer.read(value.data());
 
 	return value;
@@ -633,7 +649,7 @@  ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
 		}
 
 		ctrls.set(entry->id,
-			  loadControlValue(values, entry->is_array, entry->count));
+			  loadControlValue(values));
 	}
 
 	return ctrls;