| Message ID | 20251011063333.2169364-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 10. 11. 8: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 fully serializing the min/max/def > ControlValue's metadata associated with each ControlInfo entry in the > ControlInfoMap. > > While at it, clean up the serialization format of ControlValues and > ControlLists: > - ControlValue's id is only used by ControlList, so add a new struct for > ControlList entries to contain it, and remove id from ControlValue > - Remove offset from ControlInfo's entry, as it is no longer needed, > since the serialized data of a ControlInfo has now been converted to > simply three serialized ControlValues > - Remove the type from the serialized data of ControlValue, as it is > already in the metadata entry > > The issue regarding array controls was not noticed before because the > default value of the ControlInfo of other array controls had been set to > scalar values similar to how min/max are set, and ColourCorrectionMatrix > was the first control to properly define a non-scalar default value. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=285 > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v4: > - remove id from ipa_control_value_entry, as it is only used by > ControlList > - move id into a new struct ipa_control_list_entry > - remove offset from ipa_control_info_entry, as it is no longer used > - since ControlInfo is no longer serialized, and instead it is three > ControlValues serialized individually > - remove type from the serialized data portion of ControlValue, as the > information is in the metadata entries > - error-check the offsets when deserializing ControlInfoMap > > 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 +- > include/libcamera/ipa/ipa_controls.h | 12 +- > src/libcamera/control_serializer.cpp | 117 ++++++++++++------ > src/libcamera/ipa_controls.cpp | 22 +++- > 4 files changed, 109 insertions(+), 48 deletions(-) > > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h > index 8a63ae44a13e..d1ecfbf99113 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, > + const ControlValue &value, > + uint32_t offset); > + > ControlValue loadControlValue(ByteStreamBuffer &buffer, > + ControlType type, > bool isArray = false, unsigned int count = 1); The default arguments don't seem to be used anywhere, so maybe they could be removed to avoid potential issues there? > - ControlInfo loadControlInfo(ByteStreamBuffer &buffer); > > unsigned int serial_; > unsigned int serialSeed_; > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 980668c86bcc..820908eea023 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -15,7 +15,7 @@ namespace libcamera { > extern "C" { > #endif > > -#define IPA_CONTROLS_FORMAT_VERSION 1 > +#define IPA_CONTROLS_FORMAT_VERSION 2 > > enum ipa_controls_id_map_type { > IPA_CONTROL_ID_MAP_CONTROLS, > @@ -34,7 +34,7 @@ struct ipa_controls_header { > }; > > struct ipa_control_value_entry { > - uint32_t id; > + uint32_t reserved; Why not remove it completely or add to `padding` at the end? > uint8_t type; > uint8_t is_array; > uint16_t count; > @@ -42,10 +42,16 @@ struct ipa_control_value_entry { > uint32_t padding[1]; > }; > > +struct ipa_control_list_entry { > + uint32_t id; > + uint32_t reserved; It's not clear to me why this `reserved` is added? If it wasn't there then `ipa_control_list_entry` could pretty much be "compatible" with the previous `ipa_control_value_entry` definition. > + struct ipa_control_value_entry value; > +}; > + > struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > - uint32_t offset; > + uint32_t reserved; Same here, why not remove / add to padding at the end? > uint8_t direction; > uint8_t padding[3]; > }; > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 050f8512bd52..8d7a91680fd6 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); > @@ -184,7 +185,7 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) > size_t ControlSerializer::binarySize(const ControlList &list) > { > size_t size = sizeof(struct ipa_controls_header) > - + list.size() * sizeof(struct ipa_control_value_entry); > + + list.size() * sizeof(struct ipa_control_list_entry); > > for (const auto &ctrl : list) > size += binarySize(ctrl.second); > @@ -195,16 +196,17 @@ size_t ControlSerializer::binarySize(const ControlList &list) > void ControlSerializer::store(const ControlValue &value, > ByteStreamBuffer &buffer) > { > - const ControlType type = value.type(); > - buffer.write(&type); This needs a corresponding adjustment in `ControlSerializer::binarySize(const ControlValue&)`. > buffer.write(value.data()); > } > > -void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) > +void ControlSerializer::populateControlValueEntry(struct ipa_control_value_entry &entry, > + const ControlValue &value, > + uint32_t offset) > { > - store(info.min(), buffer); > - store(info.max(), buffer); > - store(info.def(), buffer); > + entry.type = value.type(); > + entry.is_array = value.isArray(); > + entry.count = value.numElements(); > + entry.offset = offset; > } > > /** > @@ -232,7 +234,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); > @@ -280,11 +283,32 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > struct ipa_control_info_entry entry; > entry.id = id->id(); > entry.type = id->type(); > - entry.offset = values.offset(); > entry.direction = static_cast<ControlId::DirectionFlags::Type>(id->direction()); > entries.write(&entry); > > - 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, info.min(), > + values.offset()); > + entries.write(&valueEntry); > + store(info.min(), values); > + > + populateControlValueEntry(valueEntry, info.max(), > + values.offset()); > + entries.write(&valueEntry); > + store(info.max(), values); > + > + populateControlValueEntry(valueEntry, info.def(), > + values.offset()); > + entries.write(&valueEntry); > + store(info.def(), values); > } > > if (buffer.overflow()) > @@ -341,7 +365,7 @@ int ControlSerializer::serialize(const ControlList &list, > else > idMapType = IPA_CONTROL_ID_MAP_V4L2; > > - size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > + size_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry); > size_t valuesSize = 0; > for (const auto &ctrl : list) > valuesSize += binarySize(ctrl.second); > @@ -365,12 +389,9 @@ int ControlSerializer::serialize(const ControlList &list, > unsigned int id = ctrl.first; > const ControlValue &value = ctrl.second; > > - struct ipa_control_value_entry entry; > + struct ipa_control_list_entry entry; > entry.id = id; > - entry.type = value.type(); > - entry.is_array = value.isArray(); > - entry.count = value.numElements(); > - entry.offset = values.offset(); > + populateControlValueEntry(entry.value, value, values.offset()); > entries.write(&entry); > > store(value, values); > @@ -383,12 +404,10 @@ int ControlSerializer::serialize(const ControlList &list, > } > > ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer, > + ControlType type, > bool isArray, > unsigned int count) > { > - ControlType type; > - buffer.read(&type); > - > ControlValue value; > > value.reserve(type, isArray, count); > @@ -397,15 +416,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 > @@ -483,9 +493,11 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > ControlInfoMap::Map ctrls; > for (unsigned int i = 0; i < hdr->entries; ++i) { > - const struct ipa_control_info_entry *entry = > - entries.read<decltype(*entry)>(); > - if (!entry) { > + const auto *entry = entries.read<const ipa_control_info_entry>(); > + const auto *min_entry = entries.read<const ipa_control_value_entry>(); > + const auto *max_entry = entries.read<const ipa_control_value_entry>(); > + const auto *def_entry = entries.read<const ipa_control_value_entry>(); > + if (!entry || !min_entry || !max_entry || !def_entry) { > LOG(Serializer, Error) << "Out of data"; > return {}; > } > @@ -511,15 +523,39 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > const ControlId *controlId = idMap->at(entry->id); > ASSERT(controlId); > > - if (entry->offset != values.offset()) { > + if (min_entry->offset != values.offset()) { > LOG(Serializer, Error) > - << "Bad data, entry offset mismatch (entry " > + << "Bad data, entry offset mismatch (min entry " > + << i << ")"; > + return {}; > + } > + ControlValue min = > + loadControlValue(values, static_cast<ControlType>(min_entry->type), > + min_entry->is_array, min_entry->count); > + > + if (max_entry->offset != values.offset()) { > + LOG(Serializer, Error) > + << "Bad data, entry offset mismatch (max entry " > << i << ")"; > return {}; > } > + ControlValue max = > + loadControlValue(values, static_cast<ControlType>(max_entry->type), > + max_entry->is_array, max_entry->count); > + > + if (def_entry->offset != values.offset()) { > + LOG(Serializer, Error) > + << "Bad data, entry offset mismatch (def entry " > + << i << ")"; > + return {}; > + } > + ControlValue def = > + loadControlValue(values, static_cast<ControlType>(def_entry->type), > + def_entry->is_array, def_entry->count); > + > > /* Create and store the ControlInfo. */ > - ctrls.emplace(controlId, loadControlInfo(values)); > + ctrls.emplace(controlId, ControlInfo(min, max, def)); > } > > /* > @@ -618,12 +654,12 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > ControlList ctrls(*idMap); > > for (unsigned int i = 0; i < hdr->entries; ++i) { > - const struct ipa_control_value_entry *entry = > - entries.read<decltype(*entry)>(); > - if (!entry) { > + auto *list_entry = entries.read<const ipa_control_list_entry>(); > + if (!list_entry) { > LOG(Serializer, Error) << "Out of data"; > return {}; > } > + const ipa_control_value_entry *entry = &list_entry->value; > > if (entry->offset != values.offset()) { > LOG(Serializer, Error) > @@ -632,8 +668,9 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > return {}; > } > > - ctrls.set(entry->id, > - loadControlValue(values, entry->is_array, entry->count)); > + ctrls.set(list_entry->id, > + loadControlValue(values, static_cast<ControlType>(entry->type), > + entry->is_array, entry->count)); > } > > return ctrls; > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp > index 12d92ebe894d..9133aec54b27 100644 > --- a/src/libcamera/ipa_controls.cpp > +++ b/src/libcamera/ipa_controls.cpp > @@ -192,8 +192,8 @@ static_assert(sizeof(ipa_controls_header) == 32, > /** > * \struct ipa_control_value_entry > * \brief Description of a serialized ControlValue entry > - * \var ipa_control_value_entry::id > - * The numerical ID of the control > + * \var ipa_control_value_entry::reserved > + * Reserved for future extensions > * \var ipa_control_value_entry::type > * The type of the control (defined by enum ControlType) > * \var ipa_control_value_entry::is_array > @@ -210,6 +210,20 @@ static_assert(sizeof(ipa_controls_header) == 32, > static_assert(sizeof(ipa_control_value_entry) == 16, > "Invalid ABI size change for struct ipa_control_value_entry"); > > +/** > + * \struct ipa_control_list_entry > + * \brief Description of a serialized ControlList entry > + * \var ipa_control_list_entry::id > + * The numerical ID of the control > + * \var ipa_control_list_entry::reserved > + * Reserved for future extensions > + * \var ipa_control_list_entry::value > + * The description of the serialized ControlValue > + */ > + > +static_assert(sizeof(ipa_control_list_entry) == 24, > + "Invalid ABI size change for struct ipa_control_list_entry"); We might want to check `alignof()` as well. Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # rkisp1 Regards, Barnabás Pőcze > + > /** > * \struct ipa_control_info_entry > * \brief Description of a serialized ControlInfo entry > @@ -217,8 +231,8 @@ static_assert(sizeof(ipa_control_value_entry) == 16, > * The numerical ID of the control > * \var ipa_control_info_entry::type > * The type of the control (defined by enum ControlType) > - * \var ipa_control_info_entry::offset > - * The offset in bytes from the beginning of the data section to the control > + * \var ipa_control_info_entry::reserved > + * Reserved for future extensions > * info data (shall be a multiple of 8 bytes) > * \var ipa_control_info_entry::direction > * The directions in which the control is allowed to be sent. This is a flags > -- > 2.47.2 >
diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h index 8a63ae44a13e..d1ecfbf99113 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, + const ControlValue &value, + uint32_t offset); + ControlValue loadControlValue(ByteStreamBuffer &buffer, + ControlType type, bool isArray = false, unsigned int count = 1); - ControlInfo loadControlInfo(ByteStreamBuffer &buffer); unsigned int serial_; unsigned int serialSeed_; diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h index 980668c86bcc..820908eea023 100644 --- a/include/libcamera/ipa/ipa_controls.h +++ b/include/libcamera/ipa/ipa_controls.h @@ -15,7 +15,7 @@ namespace libcamera { extern "C" { #endif -#define IPA_CONTROLS_FORMAT_VERSION 1 +#define IPA_CONTROLS_FORMAT_VERSION 2 enum ipa_controls_id_map_type { IPA_CONTROL_ID_MAP_CONTROLS, @@ -34,7 +34,7 @@ struct ipa_controls_header { }; struct ipa_control_value_entry { - uint32_t id; + uint32_t reserved; uint8_t type; uint8_t is_array; uint16_t count; @@ -42,10 +42,16 @@ struct ipa_control_value_entry { uint32_t padding[1]; }; +struct ipa_control_list_entry { + uint32_t id; + uint32_t reserved; + struct ipa_control_value_entry value; +}; + struct ipa_control_info_entry { uint32_t id; uint32_t type; - uint32_t offset; + uint32_t reserved; uint8_t direction; uint8_t padding[3]; }; diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index 050f8512bd52..8d7a91680fd6 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); @@ -184,7 +185,7 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) size_t ControlSerializer::binarySize(const ControlList &list) { size_t size = sizeof(struct ipa_controls_header) - + list.size() * sizeof(struct ipa_control_value_entry); + + list.size() * sizeof(struct ipa_control_list_entry); for (const auto &ctrl : list) size += binarySize(ctrl.second); @@ -195,16 +196,17 @@ 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()); } -void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) +void ControlSerializer::populateControlValueEntry(struct ipa_control_value_entry &entry, + const ControlValue &value, + uint32_t offset) { - store(info.min(), buffer); - store(info.max(), buffer); - store(info.def(), buffer); + entry.type = value.type(); + entry.is_array = value.isArray(); + entry.count = value.numElements(); + entry.offset = offset; } /** @@ -232,7 +234,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); @@ -280,11 +283,32 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, struct ipa_control_info_entry entry; entry.id = id->id(); entry.type = id->type(); - entry.offset = values.offset(); entry.direction = static_cast<ControlId::DirectionFlags::Type>(id->direction()); entries.write(&entry); - 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, info.min(), + values.offset()); + entries.write(&valueEntry); + store(info.min(), values); + + populateControlValueEntry(valueEntry, info.max(), + values.offset()); + entries.write(&valueEntry); + store(info.max(), values); + + populateControlValueEntry(valueEntry, info.def(), + values.offset()); + entries.write(&valueEntry); + store(info.def(), values); } if (buffer.overflow()) @@ -341,7 +365,7 @@ int ControlSerializer::serialize(const ControlList &list, else idMapType = IPA_CONTROL_ID_MAP_V4L2; - size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); + size_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry); size_t valuesSize = 0; for (const auto &ctrl : list) valuesSize += binarySize(ctrl.second); @@ -365,12 +389,9 @@ int ControlSerializer::serialize(const ControlList &list, unsigned int id = ctrl.first; const ControlValue &value = ctrl.second; - struct ipa_control_value_entry entry; + struct ipa_control_list_entry entry; entry.id = id; - entry.type = value.type(); - entry.is_array = value.isArray(); - entry.count = value.numElements(); - entry.offset = values.offset(); + populateControlValueEntry(entry.value, value, values.offset()); entries.write(&entry); store(value, values); @@ -383,12 +404,10 @@ int ControlSerializer::serialize(const ControlList &list, } ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer, + ControlType type, bool isArray, unsigned int count) { - ControlType type; - buffer.read(&type); - ControlValue value; value.reserve(type, isArray, count); @@ -397,15 +416,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 @@ -483,9 +493,11 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & ControlInfoMap::Map ctrls; for (unsigned int i = 0; i < hdr->entries; ++i) { - const struct ipa_control_info_entry *entry = - entries.read<decltype(*entry)>(); - if (!entry) { + const auto *entry = entries.read<const ipa_control_info_entry>(); + const auto *min_entry = entries.read<const ipa_control_value_entry>(); + const auto *max_entry = entries.read<const ipa_control_value_entry>(); + const auto *def_entry = entries.read<const ipa_control_value_entry>(); + if (!entry || !min_entry || !max_entry || !def_entry) { LOG(Serializer, Error) << "Out of data"; return {}; } @@ -511,15 +523,39 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & const ControlId *controlId = idMap->at(entry->id); ASSERT(controlId); - if (entry->offset != values.offset()) { + if (min_entry->offset != values.offset()) { LOG(Serializer, Error) - << "Bad data, entry offset mismatch (entry " + << "Bad data, entry offset mismatch (min entry " + << i << ")"; + return {}; + } + ControlValue min = + loadControlValue(values, static_cast<ControlType>(min_entry->type), + min_entry->is_array, min_entry->count); + + if (max_entry->offset != values.offset()) { + LOG(Serializer, Error) + << "Bad data, entry offset mismatch (max entry " << i << ")"; return {}; } + ControlValue max = + loadControlValue(values, static_cast<ControlType>(max_entry->type), + max_entry->is_array, max_entry->count); + + if (def_entry->offset != values.offset()) { + LOG(Serializer, Error) + << "Bad data, entry offset mismatch (def entry " + << i << ")"; + return {}; + } + ControlValue def = + loadControlValue(values, static_cast<ControlType>(def_entry->type), + def_entry->is_array, def_entry->count); + /* Create and store the ControlInfo. */ - ctrls.emplace(controlId, loadControlInfo(values)); + ctrls.emplace(controlId, ControlInfo(min, max, def)); } /* @@ -618,12 +654,12 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer ControlList ctrls(*idMap); for (unsigned int i = 0; i < hdr->entries; ++i) { - const struct ipa_control_value_entry *entry = - entries.read<decltype(*entry)>(); - if (!entry) { + auto *list_entry = entries.read<const ipa_control_list_entry>(); + if (!list_entry) { LOG(Serializer, Error) << "Out of data"; return {}; } + const ipa_control_value_entry *entry = &list_entry->value; if (entry->offset != values.offset()) { LOG(Serializer, Error) @@ -632,8 +668,9 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer return {}; } - ctrls.set(entry->id, - loadControlValue(values, entry->is_array, entry->count)); + ctrls.set(list_entry->id, + loadControlValue(values, static_cast<ControlType>(entry->type), + entry->is_array, entry->count)); } return ctrls; diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp index 12d92ebe894d..9133aec54b27 100644 --- a/src/libcamera/ipa_controls.cpp +++ b/src/libcamera/ipa_controls.cpp @@ -192,8 +192,8 @@ static_assert(sizeof(ipa_controls_header) == 32, /** * \struct ipa_control_value_entry * \brief Description of a serialized ControlValue entry - * \var ipa_control_value_entry::id - * The numerical ID of the control + * \var ipa_control_value_entry::reserved + * Reserved for future extensions * \var ipa_control_value_entry::type * The type of the control (defined by enum ControlType) * \var ipa_control_value_entry::is_array @@ -210,6 +210,20 @@ static_assert(sizeof(ipa_controls_header) == 32, static_assert(sizeof(ipa_control_value_entry) == 16, "Invalid ABI size change for struct ipa_control_value_entry"); +/** + * \struct ipa_control_list_entry + * \brief Description of a serialized ControlList entry + * \var ipa_control_list_entry::id + * The numerical ID of the control + * \var ipa_control_list_entry::reserved + * Reserved for future extensions + * \var ipa_control_list_entry::value + * The description of the serialized ControlValue + */ + +static_assert(sizeof(ipa_control_list_entry) == 24, + "Invalid ABI size change for struct ipa_control_list_entry"); + /** * \struct ipa_control_info_entry * \brief Description of a serialized ControlInfo entry @@ -217,8 +231,8 @@ static_assert(sizeof(ipa_control_value_entry) == 16, * The numerical ID of the control * \var ipa_control_info_entry::type * The type of the control (defined by enum ControlType) - * \var ipa_control_info_entry::offset - * The offset in bytes from the beginning of the data section to the control + * \var ipa_control_info_entry::reserved + * Reserved for future extensions * info data (shall be a multiple of 8 bytes) * \var ipa_control_info_entry::direction * The directions in which the control is allowed to be sent. This is a flags
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 fully serializing the min/max/def ControlValue's metadata associated with each ControlInfo entry in the ControlInfoMap. While at it, clean up the serialization format of ControlValues and ControlLists: - ControlValue's id is only used by ControlList, so add a new struct for ControlList entries to contain it, and remove id from ControlValue - Remove offset from ControlInfo's entry, as it is no longer needed, since the serialized data of a ControlInfo has now been converted to simply three serialized ControlValues - Remove the type from the serialized data of ControlValue, as it is already in the metadata entry The issue regarding array controls was not noticed before because the default value of the ControlInfo of other array controls had been set to scalar values similar to how min/max are set, and ColourCorrectionMatrix was the first control to properly define a non-scalar default value. Bug: https://bugs.libcamera.org/show_bug.cgi?id=285 Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v4: - remove id from ipa_control_value_entry, as it is only used by ControlList - move id into a new struct ipa_control_list_entry - remove offset from ipa_control_info_entry, as it is no longer used - since ControlInfo is no longer serialized, and instead it is three ControlValues serialized individually - remove type from the serialized data portion of ControlValue, as the information is in the metadata entries - error-check the offsets when deserializing ControlInfoMap 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 +- include/libcamera/ipa/ipa_controls.h | 12 +- src/libcamera/control_serializer.cpp | 117 ++++++++++++------ src/libcamera/ipa_controls.cpp | 22 +++- 4 files changed, 109 insertions(+), 48 deletions(-)