[PATCH v3 1/2] libcamera: control_serializer: Add array info to serialized ControlValue
Paul Elder
paul.elder at ideasonboard.com
Sat Oct 11 07:44:31 CEST 2025
Quoting Paul Elder (2025-10-11 14:01:17)
> Hi Barnabás,
>
> Thanks for the review.
>
> Quoting Barnabás Pőcze (2025-10-10 21:38:38)
> > Hi
> >
> >
> > 2025. 10. 10. 13:33 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 is achieved by adding the already-existing struct
> > > ipa_control_value_entry to the serialized form of ControlInfoMap to
> > > contain all the metadata associated with each ControlValue in
> > > the min/max/def of each ControlInfo.
> > >
> > > The issue 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 at ideasonboard.com>
> > >
> > > ---
> > > Changes in v3:
> > > - instead of adding an extra header to store isArray and numElements
> > > like in v2, just reuse struct ipa_control_value_entry, and add these
> > > entries to the serialized form of ControlInfoMap to store information
> > > for each of the three ControlValues that make of min and max and def
> > > in ControlInfoMap
> > >
> > > 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 | 6 +-
> > > src/libcamera/control_serializer.cpp | 76 +++++++++++++------
> > > 2 files changed, 58 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> > > index 8a63ae44a13e..755ee68e1efe 100644
> > > --- a/include/libcamera/internal/control_serializer.h
> > > +++ b/include/libcamera/internal/control_serializer.h
> > > @@ -47,9 +47,13 @@ private:
> > > static void store(const ControlValue &value, ByteStreamBuffer &buffer);
> > > static void store(const ControlInfo &info, ByteStreamBuffer &buffer);
> > >
> > > + void populateControlValueEntry(struct ipa_control_value_entry &entry,
> > > + unsigned int id,
> > > + const ControlValue &value,
> > > + uint32_t offset);
> > > +
> > > ControlValue loadControlValue(ByteStreamBuffer &buffer,
> > > bool isArray = false, unsigned int count = 1);
> > > - 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 050f8512bd52..f15050901412 100644
> > > --- a/src/libcamera/control_serializer.cpp
> > > +++ b/src/libcamera/control_serializer.cpp
> > > @@ -164,7 +164,8 @@ size_t ControlSerializer::binarySize(const ControlInfo &info)
> > > size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)
> > > {
> > > size_t size = sizeof(struct ipa_controls_header)
> > > - + infoMap.size() * sizeof(struct ipa_control_info_entry);
> > > + + infoMap.size() * (sizeof(struct ipa_control_info_entry) +
> > > + 3 * sizeof(struct ipa_control_value_entry));
> > >
> > > for (const auto &ctrl : infoMap)
> > > size += binarySize(ctrl.second);
> > > @@ -197,14 +198,20 @@ void ControlSerializer::store(const ControlValue &value,
> > > {
> > > const ControlType type = value.type();
> > > buffer.write(&type);
> >
> > We could get rid of this serialized type as well and use the one in the entry header.
> > I feel like the commit that introduced it (cbc2be34ed9e47f5b17d0955bf3496d735359795)
> > was trying to solve the same problem that this change solves, but it stopped at
> > just handling different types, and did not consider different array-ness or size.
>
> Oh good pint yeah we can do that.
>
> >
> >
> > > +
> > > buffer.write(value.data());
> > > }
> > >
> > > -void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)
> > > +void ControlSerializer::populateControlValueEntry(struct ipa_control_value_entry &entry,
> > > + unsigned int id,
> > > + const ControlValue &value,
> > > + uint32_t offset)
> > > {
> > > - store(info.min(), buffer);
> > > - store(info.max(), buffer);
> > > - store(info.def(), buffer);
> > > + entry.id = id;
> >
> > Could we remove the `id`? I believe it's not used for control values.
>
> I looked a bit more and now I see what you mean.
>
> >
> > I would do the following:
> >
> > struct ipa_control_value_entry { /* everything but the id */ };
> > struct ipa_control_list_entry { uint32_t id; struct ipa_control_value_entry value; };
>
> Good idea.
>
> >
> >
> > > + entry.type = value.type();
> > > + entry.is_array = value.isArray();
> > > + entry.count = value.numElements();
> > > + entry.offset = offset;
> > > }
> > >
> > > /**
> > > @@ -232,7 +239,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > >
> > > /* Compute entries and data required sizes. */
> > > size_t entriesSize = infoMap.size()
> > > - * sizeof(struct ipa_control_info_entry);
> > > + * (sizeof(struct ipa_control_info_entry) +
> > > + 3 * sizeof(struct ipa_control_value_entry));
> > > size_t valuesSize = 0;
> > > for (const auto &ctrl : infoMap)
> > > valuesSize += binarySize(ctrl.second);
> > > @@ -284,7 +292,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > > entry.direction = static_cast<ControlId::DirectionFlags::Type>(id->direction());
> > > entries.write(&entry);
> > >
> >
> > I'm wondering if `ipa_control_info_entry::offset` is of any use anymore?
>
> It indeed does not look like it is.
>
> >
> >
> > > - store(info, values);
> > > + /*
> > > + * Write the metadata for the ControlValue entries as well,
> > > + * since we need type, isArray, and numElements information for
> > > + * min/max/def of the ControlInfo. Doing it this way is the
> > > + * least intrusive in terms of changing the structs in
> > > + * ipa_controls.h
> > > + */
> > > + struct ipa_control_value_entry valueEntry;
> > > +
> > > + populateControlValueEntry(valueEntry, id->id(), info.min(),
> > > + values.offset());
> > > + entries.write(&valueEntry);
> > > + store(info.min(), values);
> > > +
> > > + populateControlValueEntry(valueEntry, id->id(), info.max(),
> > > + values.offset());
> > > + entries.write(&valueEntry);
> > > + store(info.max(), values);
> > > +
> > > + populateControlValueEntry(valueEntry, id->id(), info.def(),
> > > + values.offset());
> > > + entries.write(&valueEntry);
> > > + store(info.def(), values);
> > > }
> > >
> > > if (buffer.overflow())
> > > @@ -366,11 +396,7 @@ int ControlSerializer::serialize(const ControlList &list,
> > > const ControlValue &value = ctrl.second;
> > >
> > > struct ipa_control_value_entry entry;
> > > - entry.id = id;
> > > - entry.type = value.type();
> > > - entry.is_array = value.isArray();
> > > - entry.count = value.numElements();
> > > - entry.offset = values.offset();
> > > + populateControlValueEntry(entry, id, value, values.offset());
> > > entries.write(&entry);
> > >
> > > store(value, values);
> > > @@ -397,15 +423,6 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
> > > return value;
> > > }
> > >
> > > -ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)
> > > -{
> > > - ControlValue min = loadControlValue(b);
> > > - ControlValue max = loadControlValue(b);
> > > - ControlValue def = loadControlValue(b);
> > > -
> > > - return ControlInfo(min, max, def);
> > > -}
> > > -
> > > /**
> > > * \fn template<typename T> T ControlSerializer::deserialize(ByteStreamBuffer &buffer)
> > > * \brief Deserialize an object from a binary buffer
> > > @@ -485,7 +502,13 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > > for (unsigned int i = 0; i < hdr->entries; ++i) {
> > > const struct ipa_control_info_entry *entry =
> > > entries.read<decltype(*entry)>();
> > > - if (!entry) {
> > > + const struct ipa_control_value_entry *min_entry =
> > > + entries.read<decltype(*min_entry)>();
> >
> > const auto *x = entries.read<const ipa_control_value_entry>();
> >
> > looks a bit better to me.
>
> That is indeed nicer.
>
> >
> >
> > > + const struct ipa_control_value_entry *max_entry =
> > > + entries.read<decltype(*max_entry)>();
> > > + const struct ipa_control_value_entry *def_entry =
> > > + entries.read<decltype(*def_entry)>();
> >
> > Have you tried adding these three to `ipa_control_info_entry` directly?
>
> Originally I had wanted to avoid changing the ipa_controls.h header but ig
> since we're aligning this with an ABI breaking release (and since nobody
> probably depends on that interface anyway) it's probably fine to change it.
On second thought, this is going to get a bit hairy relating to reusing the
serializing functions for ControlValue so I'm going to leave it as-is.
Paul
>
> >
> >
> > > + if (!entry || !min_entry || !max_entry || !def_entry) {
> > > LOG(Serializer, Error) << "Out of data";
> > > return {};
> > > }
> > > @@ -518,8 +541,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > > return {};
> > > }
> > >
> > > + ControlValue min = loadControlValue(values, min_entry->is_array,
> > > + min_entry->count);
> > > + ControlValue max = loadControlValue(values, max_entry->is_array,
> > > + max_entry->count);
> > > + ControlValue def = loadControlValue(values, def_entry->is_array,
> > > + def_entry->count);
> > > +
> >
> > Should `*_entry->offset` be checked here?
>
> Yes it probably should.
>
>
> Thanks,
>
> Paul
>
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
> >
> > > /* Create and store the ControlInfo. */
> > > - ctrls.emplace(controlId, loadControlInfo(values));
> > > + ctrls.emplace(controlId, ControlInfo(min, max, def));
> > > }
> > >
> > > /*
> >
More information about the libcamera-devel
mailing list