[RFC PATCH v1 8/8] utils: codegen: ipc: Simplify deserialization
Paul Elder
paul.elder at ideasonboard.com
Wed May 21 19:05:58 CEST 2025
<snip>
> >> - static std::map<K, V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> >> - std::vector<uint8_t>::const_iterator dataEnd,
> >> - std::vector<SharedFD>::const_iterator fdsBegin,
> >> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> >> - ControlSerializer *cs = nullptr)
> >> - {
> >> std::map<K, V> ret;
> >>
> >> - uint32_t mapLen = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> >> -
> >> - std::vector<uint8_t>::const_iterator dataIter = dataBegin + 4;
> >> - std::vector<SharedFD>::const_iterator fdIter = fdsBegin;
> >> for (uint32_t i = 0; i < mapLen; i++) {
> >> - uint32_t sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> >> - uint32_t sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> >> - dataIter += 8;
> >> -
> >> - K key = IPADataSerializer<K>::deserialize(dataIter,
> >> - dataIter + sizeofData,
> >> - fdIter,
> >> - fdIter + sizeofFds,
> >> - cs);
> >> -
> >> - dataIter += sizeofData;
> >> - fdIter += sizeofFds;
> >> - sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> >> - sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> >> - dataIter += 8;
> >> -
> >> - const V value = IPADataSerializer<V>::deserialize(dataIter,
> >> - dataIter + sizeofData,
> >> - fdIter,
> >> - fdIter + sizeofFds,
> >> - cs);
> >> - ret.insert({ key, value });
> >> -
> >> - dataIter += sizeofData;
> >> - fdIter += sizeofFds;
> >> + auto key = IPADataSerializer<K>::deserialize(reader, cs);
> >> + if (!key)
> >> + return {};
> >> +
> >> + auto value = IPADataSerializer<V>::deserialize(reader, cs);
> >> + if (!value)
> >> + return {};
> >> +
> >> + ret.try_emplace(std::move(*key), std::move(*value));
> >
> > I'm curious, what's the reason for using try_emplace() here?
>
> Alternatives:
> * operator[]
> * requires default constructibility
> * must default construct and then (move) assign
> * emplace()
> * may need to allocate and construct a node (even if the key exists)
> * insert_or_assign()
> * could be used if the latest value is desired
> * must generate conditional code for (move) assignment
>
> So my rule of thumb is to basically always use `try_emplace()` or `insert_or_assign()`,
> but I think I will modify the map deserialization code to reject duplicate keys,
> so that leaves `try_emplace()` as the best option in my opinion.
Ah, I see.
<snip>
> >
> >> +
> >> + template<typename... Ts>
> >> + [[nodiscard]] auto read()
> >> + {
> >> + std::tuple<Ts...> xs;
> >> + bool ok = std::apply([&](auto &...vs) {
> >> + return read(vs...);
> >> + }, xs);
> >> +
> >> + if constexpr (sizeof...(Ts) == 1)
> >> + return ok ? std::optional(std::get<0>(xs)) : std::nullopt;
> >> + else
> >> + return ok ? std::optional(xs) : std::nullopt;
> >
> > This means that each std::nullopt is of different types no? afaict you only use
> > this function when sizeof...(Ts) == 1.
>
> Well, `std::nullopt` is always `std::nullopt`, but you're right that the conditional
> operator will resolve to different types. I thought it is best for usability to have
> `read<T>()` return `std::optional<T>` and `read<Ts...>()` return `std::optional<std::tuple<Ts...>>`.
> But yes, this is overload not really used, so it could be be removed.
Ah not the nullopt but the std::optional type will be different, ok.
<snip>
> >> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >> index 9a3aadbd2..843260b4b 100644
> >> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >> @@ -107,13 +107,13 @@ namespace {{ns}} {
> >> {% if interface_event.methods|length > 0 %}
> >> void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >> {
> >> - size_t dataSize = data.data().size();
> >> + SeriReader reader = data.reader();
> >> {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
> >>
> >> switch (_cmd) {
> >> {%- for method in interface_event.methods %}
> >> case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> >> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());
> >> + {{method.mojom_name}}IPC(reader);
> >> break;
> >> }
> >> {%- endfor %}
> >> @@ -211,18 +211,23 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >> return;
> >> {%- endif %}
> >> }
> >> +{% if has_output %}
> >> + SeriReader _outputReader = _ipcOutputBuf.reader();
> >> +{% endif -%}
> >> {% if method|method_return_value != "void" %}
> >> - {{method|method_return_value}} _retValue = IPADataSerializer<{{method|method_return_value}}>::deserialize(_ipcOutputBuf.data(), 0);
> >> -
> >> -{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset = method|method_return_value|byte_width|int)}}
> >> + auto _retValue = IPADataSerializer<{{method|method_return_value}}>::deserialize(_outputReader);
> >> + ASSERT(_retValue);
> >> +{% endif -%}
> >>
> >> - return _retValue;
> >> +{% if has_output %}
> >
> > I think this should be {% if method|method_param_outputs|length > 0 %} because
> > has_output also covers `method|method_return_value != "void"`, so you'd be
> > deserializing the single _retValue twice.
>
> The generated code looks ok to me, and as far as I can see the return value is not
> part of `method_param_outputs`, so the `proxy_funcs.deserialize_call()` should expand
> to nothing because it receives an empty list.
Indeed, you are right.
>
>
> >
> > Or maybe an ASSERT(_outputReader.empty()) might be valuable after
> > ASSERT(_retValue) above.
> >
> >> + {{proxy_funcs.deserialize_call(method|method_param_outputs, '_outputReader')}}
> >> + ASSERT(_outputReader.empty());
> >> +{% endif -%}
> >>
> >> -{% elif method|method_param_outputs|length > 0 %}
> >> -{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()')}}
> >> +{% if method|method_return_value != "void" %}
> >> + return std::move(*_retValue);
> >> {% endif -%}
> >> }
> >> -
> >> {% endfor %}
> >>
> >> {% for method in interface_event.methods %}
> >> @@ -232,12 +237,9 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> >> }
> >>
> >> -void {{proxy_name}}::{{method.mojom_name}}IPC(
> >> - [[maybe_unused]] std::vector<uint8_t>::const_iterator data,
> >> - [[maybe_unused]] size_t dataSize,
> >> - [[maybe_unused]] const std::vector<SharedFD> &fds)
> >> +void {{proxy_name}}::{{method.mojom_name}}IPC([[maybe_unused]] SeriReader &reader)
> >> {
> >> -{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, true, true, 'dataSize')}}
> >> + {{proxy_funcs.deserialize_call(method.parameters, 'reader', false, true)}}
> >
> > I think this is one level too much indentation? Same for the other places you
> > add this call.
>
> The generated code looks good to me. I think the indentiation of this macro call
> does not matter because it has `{%-`, so the preceeding whitespaces will be stripped.
Ok.
<snip>
I just saw Jacopo's review and was reminded that the serialization format
documentation needs to updated. afaict that's the only significant thing left.
Thanks,
Paul
More information about the libcamera-devel
mailing list