| Message ID | 20200229164254.23604-27-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On 29/02/2020 16:42, Laurent Pinchart wrote: > Use the zero-copy variant of ByteStreamBuffer::read() to read packet > haders and control entries. This enhance performance of ControlList and /haders/headers/ /enhance/enhances the/ or /enhance/will enhance the/ > ControlInfoMap deserialization. > > Deserialization of the actual ControlValue is untouched for now and will > be optimized later. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Very happy to see removal of copies from our serialization :) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/control_serializer.cpp | 74 +++++++++++++++++----------- > 1 file changed, 44 insertions(+), 30 deletions(-) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index dc87b96f384b..6cac70739468 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -364,39 +364,46 @@ ControlRange ControlSerializer::load<ControlRange>(ControlType type, > template<> > ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > { > - struct ipa_controls_header hdr; > - buffer.read(&hdr); > + const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); Aha, now I see why it doesn't return a span ... This looks like a lot of template additions ... couldn't they be just as easily added for this functionality with taking a size parameter and returning a void pointer? (or does C++ not like that) > + if (!hdr) { > + LOG(Serializer, Error) << "Out of data"; > + return {}; > + } > > - if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > + if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > LOG(Serializer, Error) > << "Unsupported controls format version " > - << hdr.version; > + << hdr->version; > return {}; > } > > - ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > - ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > + ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > + ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > > if (buffer.overflow()) { > - LOG(Serializer, Error) << "Serialized packet too small"; > + LOG(Serializer, Error) << "Out of data"; > return {}; > } > > ControlInfoMap::Map ctrls; > > - for (unsigned int i = 0; i < hdr.entries; ++i) { > - struct ipa_control_range_entry entry; > - entries.read(&entry); > + for (unsigned int i = 0; i < hdr->entries; ++i) { > + const struct ipa_control_range_entry *entry = > + entries.read<decltype(*entry)>(); > + if (!entry) { > + LOG(Serializer, Error) << "Out of data"; > + return {}; > + } > > /* Create and cache the individual ControlId. */ > - ControlType type = static_cast<ControlType>(entry.type); > + ControlType type = static_cast<ControlType>(entry->type); > /** > * \todo Find a way to preserve the control name for debugging > * purpose. > */ > - controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, "", type)); > + controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type)); > > - if (entry.offset != values.offset()) { > + if (entry->offset != values.offset()) { > LOG(Serializer, Error) > << "Bad data, entry offset mismatch (entry " > << i << ")"; > @@ -412,8 +419,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > * Create the ControlInfoMap in the cache, and store the map to handle > * association. > */ > - ControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls); > - infoMapHandles_[&map] = hdr.handle; > + ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls); > + infoMapHandles_[&map] = hdr->handle; > > return map; > } > @@ -430,21 +437,24 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > template<> > ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > { > - struct ipa_controls_header hdr; > - buffer.read(&hdr); > + const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > + if (!hdr) { > + LOG(Serializer, Error) << "Out of data"; > + return {}; > + } > > - if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > + if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > LOG(Serializer, Error) > << "Unsupported controls format version " > - << hdr.version; > + << hdr->version; > return {}; > } > > - ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > - ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > + ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > + ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > > if (buffer.overflow()) { > - LOG(Serializer, Error) << "Serialized packet too small"; > + LOG(Serializer, Error) << "Out of data"; > return {}; > } > > @@ -456,10 +466,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > * use the global control::control idmap. > */ > const ControlInfoMap *infoMap; > - if (hdr.handle) { > + if (hdr->handle) { > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > [&](decltype(infoMapHandles_)::value_type &entry) { > - return entry.second == hdr.handle; > + return entry.second == hdr->handle; > }); > if (iter == infoMapHandles_.end()) { > LOG(Serializer, Error) > @@ -474,19 +484,23 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > > - for (unsigned int i = 0; i < hdr.entries; ++i) { > - struct ipa_control_value_entry entry; > - entries.read(&entry); > + for (unsigned int i = 0; i < hdr->entries; ++i) { > + const struct ipa_control_value_entry *entry = > + entries.read<decltype(*entry)>(); > + if (!entry) { > + LOG(Serializer, Error) << "Out of data"; > + return {}; > + } > > - if (entry.offset != values.offset()) { > + if (entry->offset != values.offset()) { > LOG(Serializer, Error) > << "Bad data, entry offset mismatch (entry " > << i << ")"; > return {}; > } > > - ControlType type = static_cast<ControlType>(entry.type); > - ctrls.set(entry.id, load<ControlValue>(type, values)); > + ControlType type = static_cast<ControlType>(entry->type); > + ctrls.set(entry->id, load<ControlValue>(type, values)); > } > > return ctrls; >
Hi Kieran, On Thu, Mar 05, 2020 at 05:19:51PM +0000, Kieran Bingham wrote: > On 29/02/2020 16:42, Laurent Pinchart wrote: > > Use the zero-copy variant of ByteStreamBuffer::read() to read packet > > haders and control entries. This enhance performance of ControlList and > > /haders/headers/ > > /enhance/enhances the/ or /enhance/will enhance the/ > > > ControlInfoMap deserialization. > > > > Deserialization of the actual ControlValue is untouched for now and will > > be optimized later. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Very happy to see removal of copies from our serialization :) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > src/libcamera/control_serializer.cpp | 74 +++++++++++++++++----------- > > 1 file changed, 44 insertions(+), 30 deletions(-) > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > index dc87b96f384b..6cac70739468 100644 > > --- a/src/libcamera/control_serializer.cpp > > +++ b/src/libcamera/control_serializer.cpp > > @@ -364,39 +364,46 @@ ControlRange ControlSerializer::load<ControlRange>(ControlType type, > > template<> > > ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > > { > > - struct ipa_controls_header hdr; > > - buffer.read(&hdr); > > + const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > > Aha, now I see why it doesn't return a span ... > > This looks like a lot of template additions ... couldn't they be just as > easily added for this functionality with taking a size parameter and > returning a void pointer? > > (or does C++ not like that) That would require casting in the caller, which is potentially unsafe. With the above code the compiler will tell you if the variable to which you assigned the returned pointer is of a different type than the type passed to the read<> template function. It would be nice if C++ could perform template type deduction based no the return value. > > + if (!hdr) { > > + LOG(Serializer, Error) << "Out of data"; > > + return {}; > > + } > > > > - if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > > + if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > > LOG(Serializer, Error) > > << "Unsupported controls format version " > > - << hdr.version; > > + << hdr->version; > > return {}; > > } > > > > - ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > > - ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > > + ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > > + ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > > > > if (buffer.overflow()) { > > - LOG(Serializer, Error) << "Serialized packet too small"; > > + LOG(Serializer, Error) << "Out of data"; > > return {}; > > } > > > > ControlInfoMap::Map ctrls; > > > > - for (unsigned int i = 0; i < hdr.entries; ++i) { > > - struct ipa_control_range_entry entry; > > - entries.read(&entry); > > + for (unsigned int i = 0; i < hdr->entries; ++i) { > > + const struct ipa_control_range_entry *entry = > > + entries.read<decltype(*entry)>(); > > + if (!entry) { > > + LOG(Serializer, Error) << "Out of data"; > > + return {}; > > + } > > > > /* Create and cache the individual ControlId. */ > > - ControlType type = static_cast<ControlType>(entry.type); > > + ControlType type = static_cast<ControlType>(entry->type); > > /** > > * \todo Find a way to preserve the control name for debugging > > * purpose. > > */ > > - controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, "", type)); > > + controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type)); > > > > - if (entry.offset != values.offset()) { > > + if (entry->offset != values.offset()) { > > LOG(Serializer, Error) > > << "Bad data, entry offset mismatch (entry " > > << i << ")"; > > @@ -412,8 +419,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > * Create the ControlInfoMap in the cache, and store the map to handle > > * association. > > */ > > - ControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls); > > - infoMapHandles_[&map] = hdr.handle; > > + ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls); > > + infoMapHandles_[&map] = hdr->handle; > > > > return map; > > } > > @@ -430,21 +437,24 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > template<> > > ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > > { > > - struct ipa_controls_header hdr; > > - buffer.read(&hdr); > > + const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > > + if (!hdr) { > > + LOG(Serializer, Error) << "Out of data"; > > + return {}; > > + } > > > > - if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > > + if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > > LOG(Serializer, Error) > > << "Unsupported controls format version " > > - << hdr.version; > > + << hdr->version; > > return {}; > > } > > > > - ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > > - ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > > + ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > > + ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > > > > if (buffer.overflow()) { > > - LOG(Serializer, Error) << "Serialized packet too small"; > > + LOG(Serializer, Error) << "Out of data"; > > return {}; > > } > > > > @@ -456,10 +466,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > * use the global control::control idmap. > > */ > > const ControlInfoMap *infoMap; > > - if (hdr.handle) { > > + if (hdr->handle) { > > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > > [&](decltype(infoMapHandles_)::value_type &entry) { > > - return entry.second == hdr.handle; > > + return entry.second == hdr->handle; > > }); > > if (iter == infoMapHandles_.end()) { > > LOG(Serializer, Error) > > @@ -474,19 +484,23 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > > ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > > > > - for (unsigned int i = 0; i < hdr.entries; ++i) { > > - struct ipa_control_value_entry entry; > > - entries.read(&entry); > > + for (unsigned int i = 0; i < hdr->entries; ++i) { > > + const struct ipa_control_value_entry *entry = > > + entries.read<decltype(*entry)>(); > > + if (!entry) { > > + LOG(Serializer, Error) << "Out of data"; > > + return {}; > > + } > > > > - if (entry.offset != values.offset()) { > > + if (entry->offset != values.offset()) { > > LOG(Serializer, Error) > > << "Bad data, entry offset mismatch (entry " > > << i << ")"; > > return {}; > > } > > > > - ControlType type = static_cast<ControlType>(entry.type); > > - ctrls.set(entry.id, load<ControlValue>(type, values)); > > + ControlType type = static_cast<ControlType>(entry->type); > > + ctrls.set(entry->id, load<ControlValue>(type, values)); > > } > > > > return ctrls;
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index dc87b96f384b..6cac70739468 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -364,39 +364,46 @@ ControlRange ControlSerializer::load<ControlRange>(ControlType type, template<> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) { - struct ipa_controls_header hdr; - buffer.read(&hdr); + const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); + if (!hdr) { + LOG(Serializer, Error) << "Out of data"; + return {}; + } - if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { + if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { LOG(Serializer, Error) << "Unsupported controls format version " - << hdr.version; + << hdr->version; return {}; } - ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); - ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); + ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); + ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); if (buffer.overflow()) { - LOG(Serializer, Error) << "Serialized packet too small"; + LOG(Serializer, Error) << "Out of data"; return {}; } ControlInfoMap::Map ctrls; - for (unsigned int i = 0; i < hdr.entries; ++i) { - struct ipa_control_range_entry entry; - entries.read(&entry); + for (unsigned int i = 0; i < hdr->entries; ++i) { + const struct ipa_control_range_entry *entry = + entries.read<decltype(*entry)>(); + if (!entry) { + LOG(Serializer, Error) << "Out of data"; + return {}; + } /* Create and cache the individual ControlId. */ - ControlType type = static_cast<ControlType>(entry.type); + ControlType type = static_cast<ControlType>(entry->type); /** * \todo Find a way to preserve the control name for debugging * purpose. */ - controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, "", type)); + controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type)); - if (entry.offset != values.offset()) { + if (entry->offset != values.offset()) { LOG(Serializer, Error) << "Bad data, entry offset mismatch (entry " << i << ")"; @@ -412,8 +419,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & * Create the ControlInfoMap in the cache, and store the map to handle * association. */ - ControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls); - infoMapHandles_[&map] = hdr.handle; + ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls); + infoMapHandles_[&map] = hdr->handle; return map; } @@ -430,21 +437,24 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & template<> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) { - struct ipa_controls_header hdr; - buffer.read(&hdr); + const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); + if (!hdr) { + LOG(Serializer, Error) << "Out of data"; + return {}; + } - if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { + if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { LOG(Serializer, Error) << "Unsupported controls format version " - << hdr.version; + << hdr->version; return {}; } - ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); - ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); + ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); + ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); if (buffer.overflow()) { - LOG(Serializer, Error) << "Serialized packet too small"; + LOG(Serializer, Error) << "Out of data"; return {}; } @@ -456,10 +466,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer * use the global control::control idmap. */ const ControlInfoMap *infoMap; - if (hdr.handle) { + if (hdr->handle) { auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), [&](decltype(infoMapHandles_)::value_type &entry) { - return entry.second == hdr.handle; + return entry.second == hdr->handle; }); if (iter == infoMapHandles_.end()) { LOG(Serializer, Error) @@ -474,19 +484,23 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); - for (unsigned int i = 0; i < hdr.entries; ++i) { - struct ipa_control_value_entry entry; - entries.read(&entry); + for (unsigned int i = 0; i < hdr->entries; ++i) { + const struct ipa_control_value_entry *entry = + entries.read<decltype(*entry)>(); + if (!entry) { + LOG(Serializer, Error) << "Out of data"; + return {}; + } - if (entry.offset != values.offset()) { + if (entry->offset != values.offset()) { LOG(Serializer, Error) << "Bad data, entry offset mismatch (entry " << i << ")"; return {}; } - ControlType type = static_cast<ControlType>(entry.type); - ctrls.set(entry.id, load<ControlValue>(type, values)); + ControlType type = static_cast<ControlType>(entry->type); + ctrls.set(entry->id, load<ControlValue>(type, values)); } return ctrls;
Use the zero-copy variant of ByteStreamBuffer::read() to read packet haders and control entries. This enhance performance of ControlList and ControlInfoMap deserialization. Deserialization of the actual ControlValue is untouched for now and will be optimized later. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/control_serializer.cpp | 74 +++++++++++++++++----------- 1 file changed, 44 insertions(+), 30 deletions(-)