| Message ID | 20250606164156.1442682-10-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Fri, Jun 06, 2025 at 06:41:42PM +0200, Barnabás Pőcze wrote: > Define the type in `core.mojom` with external (de)serialization, and > add the necessary `IPADataSerializer` template specialization. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/ipa/core.mojom | 1 + > src/libcamera/ipa_data_serializer.cpp | 84 +++++++++++++++++++ > .../core_ipa_interface.h.tmpl | 1 + > 3 files changed, 86 insertions(+) > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom > index bce797245..754e4065c 100644 > --- a/include/libcamera/ipa/core.mojom > +++ b/include/libcamera/ipa/core.mojom > @@ -83,6 +83,7 @@ module libcamera; > [skipSerdes, skipHeader] struct ControlInfoMap {}; > [skipSerdes, skipHeader] struct ControlList {}; > [skipSerdes, skipHeader] struct SharedFD {}; > +[skipSerdes, skipHeader] struct MetadataListPlan {}; > > [skipHeader] struct Point { > int32 x; > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp > index 0537f785b..4646f4aa9 100644 > --- a/src/libcamera/ipa_data_serializer.cpp > +++ b/src/libcamera/ipa_data_serializer.cpp > @@ -11,6 +11,8 @@ > > #include <libcamera/base/log.h> > > +#include <libcamera/metadata_list_plan.h> > + > #include "libcamera/internal/byte_stream_buffer.h" > > /** > @@ -620,6 +622,88 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(const std::vector<uint8_t> &d > return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs); > } > > +template<> > +std::tuple<std::vector<uint8_t>, std::vector<SharedFD>> > +IPADataSerializer<MetadataListPlan>::serialize([[maybe_unused]] const MetadataListPlan &data, data is defintely used ;) > + [[maybe_unused]] ControlSerializer *cs) > +{ > + std::vector<uint8_t> dataVec; > + > + appendPOD<uint32_t>(dataVec, data.size()); > + > + for (auto &&[tag, e] : data) { Why using a forwarding reference and not "const auto &[]" ? > + appendPOD<uint32_t>(dataVec, tag); > + appendPOD<uint32_t>(dataVec, e.size); > + appendPOD<uint32_t>(dataVec, e.alignment); > + appendPOD<uint8_t>(dataVec, e.type); > + appendPOD<uint8_t>(dataVec, e.isArray); > + } > + > + return { dataVec, {} }; > +} > + > +template<> > +MetadataListPlan > +IPADataSerializer<MetadataListPlan>::deserialize([[maybe_unused]] std::vector<uint8_t>::const_iterator dataBegin, > + [[maybe_unused]] std::vector<uint8_t>::const_iterator dataEnd, > + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin, > + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd, > + [[maybe_unused]] ControlSerializer *cs) Also here you can probably remove some [[maybe_unused]] > +{ > + MetadataListPlan ret; > + std::size_t offset = 0; > + > + auto count = readPOD<uint32_t>(dataBegin, 0, dataEnd); dunno, I'm always a bit meh when I see 'auto' being used when the type is known. One could say the type is knows, so using auto is the right thing to do :) > + offset += sizeof(count); > + > + while (count--) { > + auto tag = readPOD<uint32_t>(dataBegin, offset, dataEnd); > + offset += sizeof(tag); > + > + auto size = readPOD<uint32_t>(dataBegin, offset, dataEnd); > + offset += sizeof(size); > + > + auto alignment = readPOD<uint32_t>(dataBegin, offset, dataEnd); > + offset += sizeof(alignment); > + > + auto type = readPOD<uint8_t>(dataBegin, offset, dataEnd); > + offset += sizeof(type); > + > + auto isArray = readPOD<uint8_t>(dataBegin, offset, dataEnd); > + offset += sizeof(isArray); > + > + ret.add(tag, size, 1, alignment, static_cast<ControlType>(type), isArray); > + } > + > + return ret; > +} > + > +template<> > +MetadataListPlan > +IPADataSerializer<MetadataListPlan>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, > + std::vector<uint8_t>::const_iterator dataEnd, > + ControlSerializer *cs) > +{ > + return deserialize(dataBegin, dataEnd, {}, {}, cs); > +} > + > +template<> > +MetadataListPlan > +IPADataSerializer<MetadataListPlan>::deserialize(const std::vector<uint8_t> &data, > + ControlSerializer *cs) > +{ > + return deserialize(data.cbegin(), data.end(), cs); > +} > + > +template<> > +MetadataListPlan > +IPADataSerializer<MetadataListPlan>::deserialize(const std::vector<uint8_t> &data, > + const std::vector<SharedFD> &fds, > + ControlSerializer *cs) > +{ > + return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs); > +} > + > #endif /* __DOXYGEN__ */ > > } /* namespace libcamera */ > diff --git a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > index 3942e5708..b3774cd64 100644 > --- a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > +++ b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > @@ -21,6 +21,7 @@ > #include <libcamera/controls.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > +#include <libcamera/metadata_list_plan.h> Do you need this in the .cpp file then ? All minors, Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > #include <libcamera/ipa/ipa_interface.h> > > -- > 2.49.0 >
Hi 2025. 06. 19. 12:47 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, Jun 06, 2025 at 06:41:42PM +0200, Barnabás Pőcze wrote: >> Define the type in `core.mojom` with external (de)serialization, and >> add the necessary `IPADataSerializer` template specialization. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/ipa/core.mojom | 1 + >> src/libcamera/ipa_data_serializer.cpp | 84 +++++++++++++++++++ >> .../core_ipa_interface.h.tmpl | 1 + >> 3 files changed, 86 insertions(+) >> >> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom >> index bce797245..754e4065c 100644 >> --- a/include/libcamera/ipa/core.mojom >> +++ b/include/libcamera/ipa/core.mojom >> @@ -83,6 +83,7 @@ module libcamera; >> [skipSerdes, skipHeader] struct ControlInfoMap {}; >> [skipSerdes, skipHeader] struct ControlList {}; >> [skipSerdes, skipHeader] struct SharedFD {}; >> +[skipSerdes, skipHeader] struct MetadataListPlan {}; >> >> [skipHeader] struct Point { >> int32 x; >> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp >> index 0537f785b..4646f4aa9 100644 >> --- a/src/libcamera/ipa_data_serializer.cpp >> +++ b/src/libcamera/ipa_data_serializer.cpp >> @@ -11,6 +11,8 @@ >> >> #include <libcamera/base/log.h> >> >> +#include <libcamera/metadata_list_plan.h> >> + >> #include "libcamera/internal/byte_stream_buffer.h" >> >> /** >> @@ -620,6 +622,88 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(const std::vector<uint8_t> &d >> return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs); >> } >> >> +template<> >> +std::tuple<std::vector<uint8_t>, std::vector<SharedFD>> >> +IPADataSerializer<MetadataListPlan>::serialize([[maybe_unused]] const MetadataListPlan &data, > > data is defintely used ;) Oops, yes, you're right. > >> + [[maybe_unused]] ControlSerializer *cs) >> +{ >> + std::vector<uint8_t> dataVec; >> + >> + appendPOD<uint32_t>(dataVec, data.size()); >> + >> + for (auto &&[tag, e] : data) { > > Why using a forwarding reference and not "const auto &[]" ? I'm just used to doing that. But a const lref is enough, so I replaced it with that. > >> + appendPOD<uint32_t>(dataVec, tag); >> + appendPOD<uint32_t>(dataVec, e.size); >> + appendPOD<uint32_t>(dataVec, e.alignment); >> + appendPOD<uint8_t>(dataVec, e.type); >> + appendPOD<uint8_t>(dataVec, e.isArray); >> + } >> + >> + return { dataVec, {} }; >> +} >> + >> +template<> >> +MetadataListPlan >> +IPADataSerializer<MetadataListPlan>::deserialize([[maybe_unused]] std::vector<uint8_t>::const_iterator dataBegin, >> + [[maybe_unused]] std::vector<uint8_t>::const_iterator dataEnd, >> + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin, >> + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd, >> + [[maybe_unused]] ControlSerializer *cs) > > Also here you can probably remove some [[maybe_unused]] Indeed. > >> +{ >> + MetadataListPlan ret; >> + std::size_t offset = 0; >> + >> + auto count = readPOD<uint32_t>(dataBegin, 0, dataEnd); > > dunno, I'm always a bit meh when I see 'auto' being used when the > type is known. > > One could say the type is knows, so using auto is the right thing to > do :) I definitely prefer to use auto if the type is clear from the initializer, such as `static_cast`, or here. > >> + offset += sizeof(count); >> + >> + while (count--) { >> + auto tag = readPOD<uint32_t>(dataBegin, offset, dataEnd); >> + offset += sizeof(tag); >> + >> + auto size = readPOD<uint32_t>(dataBegin, offset, dataEnd); >> + offset += sizeof(size); >> + >> + auto alignment = readPOD<uint32_t>(dataBegin, offset, dataEnd); >> + offset += sizeof(alignment); >> + >> + auto type = readPOD<uint8_t>(dataBegin, offset, dataEnd); >> + offset += sizeof(type); >> + >> + auto isArray = readPOD<uint8_t>(dataBegin, offset, dataEnd); >> + offset += sizeof(isArray); >> + >> + ret.add(tag, size, 1, alignment, static_cast<ControlType>(type), isArray); >> + } >> + >> + return ret; >> +} >> + >> +template<> >> +MetadataListPlan >> +IPADataSerializer<MetadataListPlan>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, >> + std::vector<uint8_t>::const_iterator dataEnd, >> + ControlSerializer *cs) >> +{ >> + return deserialize(dataBegin, dataEnd, {}, {}, cs); >> +} >> + >> +template<> >> +MetadataListPlan >> +IPADataSerializer<MetadataListPlan>::deserialize(const std::vector<uint8_t> &data, >> + ControlSerializer *cs) >> +{ >> + return deserialize(data.cbegin(), data.end(), cs); >> +} >> + >> +template<> >> +MetadataListPlan >> +IPADataSerializer<MetadataListPlan>::deserialize(const std::vector<uint8_t> &data, >> + const std::vector<SharedFD> &fds, >> + ControlSerializer *cs) >> +{ >> + return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs); >> +} >> + >> #endif /* __DOXYGEN__ */ >> >> } /* namespace libcamera */ >> diff --git a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl >> index 3942e5708..b3774cd64 100644 >> --- a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl >> +++ b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl >> @@ -21,6 +21,7 @@ >> #include <libcamera/controls.h> >> #include <libcamera/framebuffer.h> >> #include <libcamera/geometry.h> >> +#include <libcamera/metadata_list_plan.h> > > Do you need this in the .cpp file then ? Can you clarify what you mean here? Which cpp file? Regards, Barnabás Pőcze > > All minors, > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> >> #include <libcamera/ipa/ipa_interface.h> >> >> -- >> 2.49.0 >>
Hi Barnabás On Mon, Jun 23, 2025 at 02:46:08PM +0200, Barnabás Pőcze wrote: > Hi > > > --- a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > > > +++ b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl > > > @@ -21,6 +21,7 @@ > > > #include <libcamera/controls.h> > > > #include <libcamera/framebuffer.h> > > > #include <libcamera/geometry.h> > > > +#include <libcamera/metadata_list_plan.h> > > > > Do you need this in the .cpp file then ? > > Can you clarify what you mean here? Which cpp file? > I thought src/libcamera/ipa_data_serializer.cpp was going to include this header but apparently it does not. Ignore this comment please :) > > Regards, > Barnabás Pőcze > > > > > > All minors, > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > > > > > > #include <libcamera/ipa/ipa_interface.h> > > > > > > -- > > > 2.49.0 > > > >
diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom index bce797245..754e4065c 100644 --- a/include/libcamera/ipa/core.mojom +++ b/include/libcamera/ipa/core.mojom @@ -83,6 +83,7 @@ module libcamera; [skipSerdes, skipHeader] struct ControlInfoMap {}; [skipSerdes, skipHeader] struct ControlList {}; [skipSerdes, skipHeader] struct SharedFD {}; +[skipSerdes, skipHeader] struct MetadataListPlan {}; [skipHeader] struct Point { int32 x; diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp index 0537f785b..4646f4aa9 100644 --- a/src/libcamera/ipa_data_serializer.cpp +++ b/src/libcamera/ipa_data_serializer.cpp @@ -11,6 +11,8 @@ #include <libcamera/base/log.h> +#include <libcamera/metadata_list_plan.h> + #include "libcamera/internal/byte_stream_buffer.h" /** @@ -620,6 +622,88 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(const std::vector<uint8_t> &d return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs); } +template<> +std::tuple<std::vector<uint8_t>, std::vector<SharedFD>> +IPADataSerializer<MetadataListPlan>::serialize([[maybe_unused]] const MetadataListPlan &data, + [[maybe_unused]] ControlSerializer *cs) +{ + std::vector<uint8_t> dataVec; + + appendPOD<uint32_t>(dataVec, data.size()); + + for (auto &&[tag, e] : data) { + appendPOD<uint32_t>(dataVec, tag); + appendPOD<uint32_t>(dataVec, e.size); + appendPOD<uint32_t>(dataVec, e.alignment); + appendPOD<uint8_t>(dataVec, e.type); + appendPOD<uint8_t>(dataVec, e.isArray); + } + + return { dataVec, {} }; +} + +template<> +MetadataListPlan +IPADataSerializer<MetadataListPlan>::deserialize([[maybe_unused]] std::vector<uint8_t>::const_iterator dataBegin, + [[maybe_unused]] std::vector<uint8_t>::const_iterator dataEnd, + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin, + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd, + [[maybe_unused]] ControlSerializer *cs) +{ + MetadataListPlan ret; + std::size_t offset = 0; + + auto count = readPOD<uint32_t>(dataBegin, 0, dataEnd); + offset += sizeof(count); + + while (count--) { + auto tag = readPOD<uint32_t>(dataBegin, offset, dataEnd); + offset += sizeof(tag); + + auto size = readPOD<uint32_t>(dataBegin, offset, dataEnd); + offset += sizeof(size); + + auto alignment = readPOD<uint32_t>(dataBegin, offset, dataEnd); + offset += sizeof(alignment); + + auto type = readPOD<uint8_t>(dataBegin, offset, dataEnd); + offset += sizeof(type); + + auto isArray = readPOD<uint8_t>(dataBegin, offset, dataEnd); + offset += sizeof(isArray); + + ret.add(tag, size, 1, alignment, static_cast<ControlType>(type), isArray); + } + + return ret; +} + +template<> +MetadataListPlan +IPADataSerializer<MetadataListPlan>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, + std::vector<uint8_t>::const_iterator dataEnd, + ControlSerializer *cs) +{ + return deserialize(dataBegin, dataEnd, {}, {}, cs); +} + +template<> +MetadataListPlan +IPADataSerializer<MetadataListPlan>::deserialize(const std::vector<uint8_t> &data, + ControlSerializer *cs) +{ + return deserialize(data.cbegin(), data.end(), cs); +} + +template<> +MetadataListPlan +IPADataSerializer<MetadataListPlan>::deserialize(const std::vector<uint8_t> &data, + const std::vector<SharedFD> &fds, + ControlSerializer *cs) +{ + return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs); +} + #endif /* __DOXYGEN__ */ } /* namespace libcamera */ diff --git a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl index 3942e5708..b3774cd64 100644 --- a/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl +++ b/utils/codegen/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl @@ -21,6 +21,7 @@ #include <libcamera/controls.h> #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> +#include <libcamera/metadata_list_plan.h> #include <libcamera/ipa/ipa_interface.h>
Define the type in `core.mojom` with external (de)serialization, and add the necessary `IPADataSerializer` template specialization. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/ipa/core.mojom | 1 + src/libcamera/ipa_data_serializer.cpp | 84 +++++++++++++++++++ .../core_ipa_interface.h.tmpl | 1 + 3 files changed, 86 insertions(+)