[libcamera-devel] [PATCH 26/31] libcamera: control_serializer: Use zero-copy ByteStreamBuffer::read()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 6 15:58:31 CET 2020
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 at ideasonboard.com>
>
> Very happy to see removal of copies from our serialization :)
>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list