| Message ID | 20250606164156.1442682-1-barnabas.pocze@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás let me reply here with the most recent outcome of the discussions we had in the team to keep the public plan up-to-date and provide a summary of the design decisions we're making to others. On Fri, Jun 06, 2025 at 06:41:33PM +0200, Barnabás Pőcze wrote: > The main purpose of this patch series is to introduce a mechanism for pipeline handlers > by which they can report metadata items before a request completes. That part is based > on https://patchwork.libcamera.org/cover/22226/, which itself was created in response > to https://patchwork.libcamera.org/cover/22111/. Most of the former patch series is > integrated here. > > In contrast, this patch series introduces a new type `MetadataList` that is used for > storing the metadata items, instead of a `ControlList`. This is needed to solve the > inherent synchronization issues caused by the fact that user callbacks (e.g. `requestComplete`) > are invoked synchronously. Specifically, the newly introduced `metadataAvailable` signal > should provide a way for users to defer the processing of the completed metadata items > to a separate thread (as blocking the internal libcamera thread is undesired). Using > a `ControlList` would not be ideal. > > This patch series also adds `Documentation/design/metadata-list.rst`. I believe reading > that (first) provides useful information to better understand the reasons and design > of the implemented mechanism. I tried to put most of the information in the commit > messages or actual documentation as this cover letter will be lost to time. > > ### TODOs > > There are some TODOs in the code at the moment, those still has to be discussed. > > The old `ControlList` metadata list is not removed yet for testing, > but everything uses the new metadata list in `Request::metadata2()`. > > The pipeline handlers guide is also not updated yet. > > > ### user interface > > An important question remaining to be answered is what exactly should be exposed to user applications > and how. For example, should `MetadataListPLan` be accessible? There are arguments to be made for that: > for example, if an application has to know beforehand which metadata items are reported in order to > allocate memory for them (e.g. pipewire). I see this as very similar to the `ControlInfoMap` of the camera. > > Similarly, e.g. `MetadataList::iterator` has an inline `operator++`. I don't think going through a shared > library for an operator increment is ideal, but an argument could be made that doing so enables > the structure to be changed. I believe it is more likely than not that it will have to be changed > at least initially. Since libcamera does not promise any stability at the moment, I think this is > not a great concern at the moment. > Based on our latest discussion the ability for the camera to report the list of metadata it can report seems a useful feature. Looking at this conversation you had with Wim@Pipewire it seems being able to pre-size the Pipewire metadata buffer is a desired feature: https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/2355 With the introduction of MetadataListPlan we now have this information. The type as it is structured today allows insertion/removal of metadata entries, something we don't want to expose in the public API where we probably only need a RO interface. A possibility is to use the d-pointer (PIMPL) pattern as we do in multiple places already, or define a new type that from a MetadataListPlan gives us something to expose in the public API Exposing the metadata information from the Camera as we currently do for Camera::controls() also impacts the MetadataListPlan mangement inside the library. This patch series makes the IPA initialize the MetadataListPlan at IPA::init() time, something we need if we want to expose the metadata information from the Camera before the Camera is configured. This opens the question if we ever need to update the MetadataListPlan in IPA::configure() as well, as we currently do for controls. At the moment I would say it is probably not necesary, but I might be overlooking some detail here. > > ### rpi > > The rpi related changes conflict with https://patchwork.libcamera.org/cover/23474/ > some things can be simplified after that is accepted. > > > ### testing > > I was only able to test the uvc, rkisp1, rpi/vc4, vimc, virtual pipeline handlers. > The rest compile and all, but I am not entirely sure everything is in the right > place (especially in the `simple` pipeline handler). > > > ### debug metadata > > One could argue, that this implementation is based on one of the simplest ideas considered, > but that naturally has some drawbacks. One such issues arises when considering "debug metadata". > The current implementation requires the set of all possibly reported metadata items to be known. > This means that using debug metadata will become a lot less convenient, as the new metadata has > to be added to a `MetadataListPlan` object. As opposed to the current situation where metadata > items can be just set, and `gen-debug-controls.py` will automagically generate them. > A couple ideas to address this situation: > > (1) do nothing > * by far the simplest > * inconvenient for debug metadata > (2) decouple "debug metadata" from "normal metadata" > * no early reporting (probably not too important) > * can keep using `ControlList` > * how to expose and to which (all?) applications? > * needs some, possibly quite a bit of, modifications in the IPA interface > (3) different `MetadataList` implementation > * I have a prototype for another implementation > - somewhat more complex > - needs to allocate on demand > - provides the essentially same interface and guarantees > - can accommodate any number of metadata items just like a `ControlList` > - no `MetadataListPlan` needed > Let me here resume the plan we sketched out with Stefan. In order to use DebugMetadata libcamera need to be recompiled with the DebugMetadata controls defined by gen-debug-controls.py for us by parsing the code. The DebugMetadata will have their 'vendor' id set to 'debug', so it is "easy" to filter them out. The Camera can then add a fixed margin to the MetadataListPlan after it has been created by the IPA for each defined DebugMetadata. The fixed size reserved for each DebugMetadata can be globally defined as a constant size. Initially we can fix it in the library, if needed we can make it configurable through, in example, gen-debug-controls.py or some other means. Does this match your understanding ? > > ### alternative implementation > > There is a prototype for an alternative implementation that implements basically the > same interface, but is a bit more complicated. Comparison of the here proposed ("contiguous") > metadata list, and the more complex ("chunked") metadata list is as follows: > > cont. chunk. > ==============================+===================================+==========================+ > complexity | simpler | more complex | > memory allocation | single one when constructed | on demand | > contiguous<1> | yes | no | > backing data structure<2> | sorted array | chaining hash table<3> | > iterator step | constant time | constant time | > wasted space | <4> | <5> | > > <1>: The reason why this is highlighted is that initially I expected that if > a serialized version of the `ControlList` is ever introduced, then it should > use the same format as this metadata list. This would allow the currently completed > items in the metadata list to be accessed through a hypothetical non-owning > `ControlListView` type referring to the storage of the metadata list. Such a > `ControlList` would most likely use a contiguous chunk of memory, so having > contiguous storage helps compatibility. Nonetheless, I am becoming less sure > that is is actually useful; and with sufficient complexity the non-contiguous > version can also be supported. > > <2>: for lookup, wrt. the numeric identifier of the control > > <3>: fixed number buckets, making it resizable is not trivial > > <4>: Space for all metadata items is allocated, so if there are metadata items with large space requirements > (e.g. `rpi::PispStatsOutput` - 23200 bytes) that are seldom reported, then that space is essentially wasted. > > <5>: (I think) bounded by (items / 2 * chunk_size), but with the right parameters > all "usually" reported metadata can fit in one chunk. Let's try to stick with the "contiguous" solution for the moment as it will make it easier to introduce a C ABI later on. I haven't find here a discussion point for the MetadataList serialization format, so I'll move some discussions which happened in the series review here. ------------------------------------------------------------------------------- > > Entry = The metadata descriptor in the first half of the serialized > > buffer > > > > ValueHeader = The header before the actual metadata value in the > > second half of the serialized buffer > > > > Correct ? > > > > In this case, looking at the fields in those two types > > > > struct Entry { > > const std::uint32_t tag; > > const std::uint32_t capacity; > > const std::uint32_t alignment; > > const ControlType type; > > bool isArray; > > > > ... > > > > }; > > > > struct ValueHeader { > > std::uint32_t tag; > > std::uint32_t size; > > std::uint32_t alignment; > > > > struct ValueParams { > > ControlType type; > > bool isArray; > > std::uint32_t numElements; > > }; > > }; > > > > I see quite an overlap. > > > > Basically for an Entry to match a ValueHeader the only missing > > information are size and numElements. This makes sense as these are > > the 'runtime' information while everything else is static and comes > > from the control definition. > > > > Now, do we need to duplicate those information ? Can we just use Entry > > and only store data in second half of the buffer ? > > > > This won't allow you to reconstruct a "second half" without the "first > > half" but is this actually a use case ? > > I mentioned it in the cover letter that I think this is an open question, > it seems useful, and having some kind of header simplifies the iterators. ------------------------------------------------------------------------------- As I read this, my suggestion was to consider if we need ValueHeader at all in the second half of the list, as the "first" part which currently contains only static information can be easily augmented to include the "runtime" information (actual data size and the actual number of entries). This would reduce the size of the serializaed buffer and avoid duplicating information in multiple places. Of course, this means you can't handle a "second half" without the "first half". What do you think a use case for handling a "second half" without the "first half" is ? If Entry is able to support for Metadata and Controls the same underlying format can be used to serialize Controls as well ? Also, FYI, separating the metadata list in a first half with only the entries description and a second half with actual data is what the Android metadata serialization format does as well. I'll continue the review of the rest of the series but won't go in too much details as I suspect the decision we have to make here on the design will impact the rest of the patch series. Thanks again, lot of great work going on here! > > Barnabás Pőcze (17): > libcamera: controls: Strings are arrays > libcamera: controls: Add `ControlValueView` > libcamera: base: Add file for C++20 polyfills > libcamera: base: cxx20: Add `type_identity{,_t}` > libcamera: base: cxx20: Add `has_single_bit()` > libcamera: base: Add alignment utility functions > libcamera: Add `MetadataList` > Documentation: design: Document `MetadataList` > libcamera: ipa_data_serializer: Support `MetadataListPlan` > libcamera: camera: Store `MetadataListPlan` in `Camera::Private` > libcamera: request: Store `MetadataList` > [DNI] apps: cam: Print `MetadataList` of `Request` as well > libcamera: pipeline: Fill `MetadataListPlan` of cameras > libcamera: pipeline: rpi: Queue metadata until completion > libcamera: pipeline: rpi: Use `metadataAvailable()` > py: Use `Request::metadata2()` > treewide: Use `Request::metadata2()` > > Jacopo Mondi (6): > libcamera: camera: Introduce metadataAvailable signal > guides: application: Document Camera::metadataAvailable > libcamera: pipeline_handler: Add metadataAvailable() function > guides: pipeline_handler: Document PipelineHandler::metadataAvailable > [DNI] apps: cam: Use Camera::metadataAvailable signal > libcamera: pipeline: Use `metadataAvailable()` > > Documentation/design/metadata-list.rst | 234 +++++++ > .../guides/application-developer.rst | 8 + > Documentation/guides/pipeline-handler.rst | 26 +- > Documentation/index.rst | 1 + > Documentation/meson.build | 1 + > include/libcamera/base/details/align.h | 72 ++ > include/libcamera/base/details/cxx20.h | 25 + > include/libcamera/base/meson.build | 1 + > include/libcamera/camera.h | 1 + > include/libcamera/controls.h | 71 +- > include/libcamera/internal/camera.h | 3 + > include/libcamera/internal/pipeline_handler.h | 21 + > .../internal/software_isp/software_isp.h | 3 +- > include/libcamera/ipa/core.mojom | 1 + > include/libcamera/ipa/ipu3.mojom | 3 +- > include/libcamera/ipa/mali-c55.mojom | 3 +- > include/libcamera/ipa/raspberrypi.mojom | 1 + > include/libcamera/ipa/rkisp1.mojom | 2 +- > include/libcamera/ipa/soft.mojom | 3 +- > include/libcamera/meson.build | 2 + > include/libcamera/metadata_list.h | 619 ++++++++++++++++++ > include/libcamera/metadata_list_plan.h | 109 +++ > include/libcamera/request.h | 5 + > src/android/camera_device.cpp | 4 +- > src/apps/cam/camera_session.cpp | 19 + > src/apps/cam/file_sink.cpp | 4 +- > src/apps/cam/file_sink.h | 4 +- > src/apps/common/dng_writer.cpp | 2 +- > src/apps/common/dng_writer.h | 4 +- > src/apps/qcam/main_window.cpp | 4 +- > src/apps/qcam/main_window.h | 4 +- > src/gstreamer/gstlibcamera-controls.cpp.in | 4 +- > src/gstreamer/gstlibcamerasrc.cpp | 2 +- > src/ipa/ipu3/algorithms/agc.cpp | 4 + > src/ipa/ipu3/algorithms/awb.cpp | 12 + > src/ipa/ipu3/algorithms/awb.h | 1 + > src/ipa/ipu3/ipa_context.cpp | 3 + > src/ipa/ipu3/ipa_context.h | 3 + > src/ipa/ipu3/ipu3.cpp | 8 +- > src/ipa/mali-c55/algorithms/agc.cpp | 5 + > src/ipa/mali-c55/algorithms/awb.cpp | 7 + > src/ipa/mali-c55/algorithms/awb.h | 1 + > src/ipa/mali-c55/algorithms/blc.cpp | 2 + > src/ipa/mali-c55/ipa_context.h | 3 + > src/ipa/mali-c55/mali-c55.cpp | 6 +- > src/ipa/rkisp1/algorithms/agc.cpp | 10 + > src/ipa/rkisp1/algorithms/awb.cpp | 4 + > src/ipa/rkisp1/algorithms/blc.cpp | 2 + > src/ipa/rkisp1/ipa_context.h | 1 + > src/ipa/rkisp1/rkisp1.cpp | 8 +- > src/ipa/rpi/common/ipa_base.cpp | 34 + > src/ipa/rpi/pisp/pisp.cpp | 5 +- > src/ipa/rpi/vc4/vc4.cpp | 4 +- > src/ipa/simple/algorithms/agc.cpp | 8 + > src/ipa/simple/algorithms/agc.h | 1 + > src/ipa/simple/algorithms/awb.cpp | 8 + > src/ipa/simple/algorithms/awb.h | 1 + > src/ipa/simple/algorithms/blc.cpp | 3 + > src/ipa/simple/algorithms/ccm.cpp | 3 + > src/ipa/simple/algorithms/lut.cpp | 3 + > src/ipa/simple/ipa_context.h | 2 + > src/ipa/simple/soft_simple.cpp | 8 +- > src/libcamera/camera.cpp | 62 ++ > src/libcamera/controls.cpp | 194 ++++++ > src/libcamera/ipa_data_serializer.cpp | 84 +++ > src/libcamera/meson.build | 1 + > src/libcamera/metadata_list.cpp | 315 +++++++++ > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 7 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 23 +- > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 4 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +- > .../pipeline/rpi/common/pipeline_base.cpp | 36 +- > .../pipeline/rpi/common/pipeline_base.h | 4 +- > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 8 +- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 8 +- > src/libcamera/pipeline/simple/simple.cpp | 7 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- > src/libcamera/pipeline/vimc/vimc.cpp | 5 +- > .../pipeline/virtual/config_parser.cpp | 3 + > src/libcamera/pipeline/virtual/virtual.cpp | 2 +- > src/libcamera/pipeline_handler.cpp | 61 ++ > src/libcamera/request.cpp | 2 + > src/libcamera/software_isp/software_isp.cpp | 6 +- > src/py/libcamera/py_helpers.cpp | 4 +- > src/py/libcamera/py_helpers.h | 2 +- > src/py/libcamera/py_main.cpp | 9 +- > test/controls/meson.build | 1 + > test/controls/metadata_list.cpp | 171 +++++ > .../core_ipa_interface.h.tmpl | 1 + > 89 files changed, 2361 insertions(+), 101 deletions(-) > create mode 100644 Documentation/design/metadata-list.rst > create mode 100644 include/libcamera/base/details/align.h > create mode 100644 include/libcamera/base/details/cxx20.h > create mode 100644 include/libcamera/metadata_list.h > create mode 100644 include/libcamera/metadata_list_plan.h > create mode 100644 src/libcamera/metadata_list.cpp > create mode 100644 test/controls/metadata_list.cpp > > -- > 2.49.0
Hi 2025. 06. 19. 11:57 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > let me reply here with the most recent outcome of the discussions we > had in the team to keep the public plan up-to-date and provide a > summary of the design decisions we're making to others. > > On Fri, Jun 06, 2025 at 06:41:33PM +0200, Barnabás Pőcze wrote: >> The main purpose of this patch series is to introduce a mechanism for pipeline handlers >> by which they can report metadata items before a request completes. That part is based >> on https://patchwork.libcamera.org/cover/22226/, which itself was created in response >> to https://patchwork.libcamera.org/cover/22111/. Most of the former patch series is >> integrated here. >> >> In contrast, this patch series introduces a new type `MetadataList` that is used for >> storing the metadata items, instead of a `ControlList`. This is needed to solve the >> inherent synchronization issues caused by the fact that user callbacks (e.g. `requestComplete`) >> are invoked synchronously. Specifically, the newly introduced `metadataAvailable` signal >> should provide a way for users to defer the processing of the completed metadata items >> to a separate thread (as blocking the internal libcamera thread is undesired). Using >> a `ControlList` would not be ideal. >> >> This patch series also adds `Documentation/design/metadata-list.rst`. I believe reading >> that (first) provides useful information to better understand the reasons and design >> of the implemented mechanism. I tried to put most of the information in the commit >> messages or actual documentation as this cover letter will be lost to time. >> >> ### TODOs >> >> There are some TODOs in the code at the moment, those still has to be discussed. >> >> The old `ControlList` metadata list is not removed yet for testing, >> but everything uses the new metadata list in `Request::metadata2()`. >> >> The pipeline handlers guide is also not updated yet. >> >> >> ### user interface >> >> An important question remaining to be answered is what exactly should be exposed to user applications >> and how. For example, should `MetadataListPLan` be accessible? There are arguments to be made for that: >> for example, if an application has to know beforehand which metadata items are reported in order to >> allocate memory for them (e.g. pipewire). I see this as very similar to the `ControlInfoMap` of the camera. >> >> Similarly, e.g. `MetadataList::iterator` has an inline `operator++`. I don't think going through a shared >> library for an operator increment is ideal, but an argument could be made that doing so enables >> the structure to be changed. I believe it is more likely than not that it will have to be changed >> at least initially. Since libcamera does not promise any stability at the moment, I think this is >> not a great concern at the moment. >> > > Based on our latest discussion the ability for the camera to report > the list of metadata it can report seems a useful feature. > > Looking at this conversation you had with Wim@Pipewire it seems being > able to pre-size the Pipewire metadata buffer is a desired feature: > https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/2355 > > With the introduction of MetadataListPlan we now have this > information. The type as it is structured today allows > insertion/removal of metadata entries, something we don't want to > expose in the public API where we probably only need a RO interface. > A possibility is to use the d-pointer (PIMPL) pattern as we do in > multiple places already, or define a new type that from a > MetadataListPlan gives us something to expose in the public API I believe the simplest idea would be adding the necessary methods to the current type and exposing only a const lref to it. If we wish to hide the type, then I would first explore the idea of introducing a new type. This could be as simple as storing a single pointer to the camera (and accessing the "real" metadata plan in the appropriate non-inline methods where Camera::Private is accessible), or a pointer to the internal map of the "real" metadata plan could also be stored. > > Exposing the metadata information from the Camera as we currently do > for Camera::controls() also impacts the MetadataListPlan mangement > inside the library. This patch series makes the IPA initialize the > MetadataListPlan at IPA::init() time, something we need if we want to > expose the metadata information from the Camera before the Camera is > configured. This opens the question if we ever need to update the > MetadataListPlan in IPA::configure() as well, as we currently do for > controls. At the moment I would say it is probably not necesary, but I > might be overlooking some detail here. At the moment allowing changes from an IPA at camera configuration time does not seem necessary. If required, this can be added later, even on a per pipeline handler basis. > >> >> ### rpi >> >> The rpi related changes conflict with https://patchwork.libcamera.org/cover/23474/ >> some things can be simplified after that is accepted. >> >> >> ### testing >> >> I was only able to test the uvc, rkisp1, rpi/vc4, vimc, virtual pipeline handlers. >> The rest compile and all, but I am not entirely sure everything is in the right >> place (especially in the `simple` pipeline handler). >> >> >> ### debug metadata >> >> One could argue, that this implementation is based on one of the simplest ideas considered, >> but that naturally has some drawbacks. One such issues arises when considering "debug metadata". >> The current implementation requires the set of all possibly reported metadata items to be known. >> This means that using debug metadata will become a lot less convenient, as the new metadata has >> to be added to a `MetadataListPlan` object. As opposed to the current situation where metadata >> items can be just set, and `gen-debug-controls.py` will automagically generate them. >> A couple ideas to address this situation: >> >> (1) do nothing >> * by far the simplest >> * inconvenient for debug metadata >> (2) decouple "debug metadata" from "normal metadata" >> * no early reporting (probably not too important) >> * can keep using `ControlList` >> * how to expose and to which (all?) applications? >> * needs some, possibly quite a bit of, modifications in the IPA interface >> (3) different `MetadataList` implementation >> * I have a prototype for another implementation >> - somewhat more complex >> - needs to allocate on demand >> - provides the essentially same interface and guarantees >> - can accommodate any number of metadata items just like a `ControlList` >> - no `MetadataListPlan` needed >> > > Let me here resume the plan we sketched out with Stefan. > > In order to use DebugMetadata libcamera need to be recompiled with the > DebugMetadata controls defined by gen-debug-controls.py for us by > parsing the code. > > The DebugMetadata will have their 'vendor' id set to 'debug', so it is > "easy" to filter them out. The Camera can then add a fixed margin to > the MetadataListPlan after it has been created by the IPA for each > defined DebugMetadata. The fixed size reserved for each DebugMetadata > can be globally defined as a constant size. Initially we can fix it in > the library, if needed we can make it configurable through, in > example, gen-debug-controls.py or some other means. > > Does this match your understanding ? I believe so. > >> >> ### alternative implementation >> >> There is a prototype for an alternative implementation that implements basically the >> same interface, but is a bit more complicated. Comparison of the here proposed ("contiguous") >> metadata list, and the more complex ("chunked") metadata list is as follows: >> >> cont. chunk. >> ==============================+===================================+==========================+ >> complexity | simpler | more complex | >> memory allocation | single one when constructed | on demand | >> contiguous<1> | yes | no | >> backing data structure<2> | sorted array | chaining hash table<3> | >> iterator step | constant time | constant time | >> wasted space | <4> | <5> | >> >> <1>: The reason why this is highlighted is that initially I expected that if >> a serialized version of the `ControlList` is ever introduced, then it should >> use the same format as this metadata list. This would allow the currently completed >> items in the metadata list to be accessed through a hypothetical non-owning >> `ControlListView` type referring to the storage of the metadata list. Such a >> `ControlList` would most likely use a contiguous chunk of memory, so having >> contiguous storage helps compatibility. Nonetheless, I am becoming less sure >> that is is actually useful; and with sufficient complexity the non-contiguous >> version can also be supported. >> >> <2>: for lookup, wrt. the numeric identifier of the control >> >> <3>: fixed number buckets, making it resizable is not trivial >> >> <4>: Space for all metadata items is allocated, so if there are metadata items with large space requirements >> (e.g. `rpi::PispStatsOutput` - 23200 bytes) that are seldom reported, then that space is essentially wasted. >> >> <5>: (I think) bounded by (items / 2 * chunk_size), but with the right parameters >> all "usually" reported metadata can fit in one chunk. > > Let's try to stick with the "contiguous" solution for the moment as it > will make it easier to introduce a C ABI later on. > > I haven't find here a discussion point for the MetadataList > serialization format, so I'll move some discussions which happened in > the series review here. > > ------------------------------------------------------------------------------- >>> Entry = The metadata descriptor in the first half of the serialized >>> buffer >>> >>> ValueHeader = The header before the actual metadata value in the >>> second half of the serialized buffer >>> >>> Correct ? >>> >>> In this case, looking at the fields in those two types >>> >>> struct Entry { >>> const std::uint32_t tag; >>> const std::uint32_t capacity; >>> const std::uint32_t alignment; >>> const ControlType type; >>> bool isArray; >>> >>> ... >>> >>> }; >>> >>> struct ValueHeader { >>> std::uint32_t tag; >>> std::uint32_t size; >>> std::uint32_t alignment; >>> >>> struct ValueParams { >>> ControlType type; >>> bool isArray; >>> std::uint32_t numElements; >>> }; >>> }; >>> >>> I see quite an overlap. >>> >>> Basically for an Entry to match a ValueHeader the only missing >>> information are size and numElements. This makes sense as these are >>> the 'runtime' information while everything else is static and comes >>> from the control definition. >>> >>> Now, do we need to duplicate those information ? Can we just use Entry >>> and only store data in second half of the buffer ? >>> >>> This won't allow you to reconstruct a "second half" without the "first >>> half" but is this actually a use case ? >> >> I mentioned it in the cover letter that I think this is an open question, >> it seems useful, and having some kind of header simplifies the iterators. > ------------------------------------------------------------------------------- > > As I read this, my suggestion was to consider if we need ValueHeader > at all in the second half of the list, as the "first" part which > currently contains only static information can be easily augmented to > include the "runtime" information (actual data size and the actual number of > entries). This would reduce the size of the serializaed buffer and > avoid duplicating information in multiple places. Of course, this > means you can't handle a "second half" without the "first half". > > What do you think a use case for handling a "second half" without the > "first half" is ? If Entry is able to support for Metadata and > Controls the same underlying format can be used to serialize Controls > as well ? The `Entry` type is specific to the special needs of the MetadataList, I would not reuse that for just any control. `ValueHeader` is more for that purpose. There are multiple options if we want to decrease the memory footprint, for example: (a) make `ValueHeader` minimal (e.g. containing a single offset to the `Entry`) - iteration is not significantly more complicated - less memory used (b) extend the first half minimally to store metadata completion order - iteration is just as easy - `ValueHeader` can be removed I still believe having a self-contained second half is useful, but possibly not as much as I had initially thought. It is certainly not required at the moment. Regards, Barnabás Pőcze > > Also, FYI, separating the metadata list in a first half with only > the entries description and a second half with actual data is what the > Android metadata serialization format does as well. > > I'll continue the review of the rest of the series but won't go in too > much details as I suspect the decision we have to make here on the > design will impact the rest of the patch series. > > Thanks again, lot of great work going on here! > > >> >> Barnabás Pőcze (17): >> libcamera: controls: Strings are arrays >> libcamera: controls: Add `ControlValueView` >> libcamera: base: Add file for C++20 polyfills >> libcamera: base: cxx20: Add `type_identity{,_t}` >> libcamera: base: cxx20: Add `has_single_bit()` >> libcamera: base: Add alignment utility functions >> libcamera: Add `MetadataList` >> Documentation: design: Document `MetadataList` >> libcamera: ipa_data_serializer: Support `MetadataListPlan` >> libcamera: camera: Store `MetadataListPlan` in `Camera::Private` >> libcamera: request: Store `MetadataList` >> [DNI] apps: cam: Print `MetadataList` of `Request` as well >> libcamera: pipeline: Fill `MetadataListPlan` of cameras >> libcamera: pipeline: rpi: Queue metadata until completion >> libcamera: pipeline: rpi: Use `metadataAvailable()` >> py: Use `Request::metadata2()` >> treewide: Use `Request::metadata2()` >> >> Jacopo Mondi (6): >> libcamera: camera: Introduce metadataAvailable signal >> guides: application: Document Camera::metadataAvailable >> libcamera: pipeline_handler: Add metadataAvailable() function >> guides: pipeline_handler: Document PipelineHandler::metadataAvailable >> [DNI] apps: cam: Use Camera::metadataAvailable signal >> libcamera: pipeline: Use `metadataAvailable()` >> >> Documentation/design/metadata-list.rst | 234 +++++++ >> .../guides/application-developer.rst | 8 + >> Documentation/guides/pipeline-handler.rst | 26 +- >> Documentation/index.rst | 1 + >> Documentation/meson.build | 1 + >> include/libcamera/base/details/align.h | 72 ++ >> include/libcamera/base/details/cxx20.h | 25 + >> include/libcamera/base/meson.build | 1 + >> include/libcamera/camera.h | 1 + >> include/libcamera/controls.h | 71 +- >> include/libcamera/internal/camera.h | 3 + >> include/libcamera/internal/pipeline_handler.h | 21 + >> .../internal/software_isp/software_isp.h | 3 +- >> include/libcamera/ipa/core.mojom | 1 + >> include/libcamera/ipa/ipu3.mojom | 3 +- >> include/libcamera/ipa/mali-c55.mojom | 3 +- >> include/libcamera/ipa/raspberrypi.mojom | 1 + >> include/libcamera/ipa/rkisp1.mojom | 2 +- >> include/libcamera/ipa/soft.mojom | 3 +- >> include/libcamera/meson.build | 2 + >> include/libcamera/metadata_list.h | 619 ++++++++++++++++++ >> include/libcamera/metadata_list_plan.h | 109 +++ >> include/libcamera/request.h | 5 + >> src/android/camera_device.cpp | 4 +- >> src/apps/cam/camera_session.cpp | 19 + >> src/apps/cam/file_sink.cpp | 4 +- >> src/apps/cam/file_sink.h | 4 +- >> src/apps/common/dng_writer.cpp | 2 +- >> src/apps/common/dng_writer.h | 4 +- >> src/apps/qcam/main_window.cpp | 4 +- >> src/apps/qcam/main_window.h | 4 +- >> src/gstreamer/gstlibcamera-controls.cpp.in | 4 +- >> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >> src/ipa/ipu3/algorithms/agc.cpp | 4 + >> src/ipa/ipu3/algorithms/awb.cpp | 12 + >> src/ipa/ipu3/algorithms/awb.h | 1 + >> src/ipa/ipu3/ipa_context.cpp | 3 + >> src/ipa/ipu3/ipa_context.h | 3 + >> src/ipa/ipu3/ipu3.cpp | 8 +- >> src/ipa/mali-c55/algorithms/agc.cpp | 5 + >> src/ipa/mali-c55/algorithms/awb.cpp | 7 + >> src/ipa/mali-c55/algorithms/awb.h | 1 + >> src/ipa/mali-c55/algorithms/blc.cpp | 2 + >> src/ipa/mali-c55/ipa_context.h | 3 + >> src/ipa/mali-c55/mali-c55.cpp | 6 +- >> src/ipa/rkisp1/algorithms/agc.cpp | 10 + >> src/ipa/rkisp1/algorithms/awb.cpp | 4 + >> src/ipa/rkisp1/algorithms/blc.cpp | 2 + >> src/ipa/rkisp1/ipa_context.h | 1 + >> src/ipa/rkisp1/rkisp1.cpp | 8 +- >> src/ipa/rpi/common/ipa_base.cpp | 34 + >> src/ipa/rpi/pisp/pisp.cpp | 5 +- >> src/ipa/rpi/vc4/vc4.cpp | 4 +- >> src/ipa/simple/algorithms/agc.cpp | 8 + >> src/ipa/simple/algorithms/agc.h | 1 + >> src/ipa/simple/algorithms/awb.cpp | 8 + >> src/ipa/simple/algorithms/awb.h | 1 + >> src/ipa/simple/algorithms/blc.cpp | 3 + >> src/ipa/simple/algorithms/ccm.cpp | 3 + >> src/ipa/simple/algorithms/lut.cpp | 3 + >> src/ipa/simple/ipa_context.h | 2 + >> src/ipa/simple/soft_simple.cpp | 8 +- >> src/libcamera/camera.cpp | 62 ++ >> src/libcamera/controls.cpp | 194 ++++++ >> src/libcamera/ipa_data_serializer.cpp | 84 +++ >> src/libcamera/meson.build | 1 + >> src/libcamera/metadata_list.cpp | 315 +++++++++ >> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 7 +- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 23 +- >> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 4 +- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +- >> .../pipeline/rpi/common/pipeline_base.cpp | 36 +- >> .../pipeline/rpi/common/pipeline_base.h | 4 +- >> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 8 +- >> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 8 +- >> src/libcamera/pipeline/simple/simple.cpp | 7 +- >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- >> src/libcamera/pipeline/vimc/vimc.cpp | 5 +- >> .../pipeline/virtual/config_parser.cpp | 3 + >> src/libcamera/pipeline/virtual/virtual.cpp | 2 +- >> src/libcamera/pipeline_handler.cpp | 61 ++ >> src/libcamera/request.cpp | 2 + >> src/libcamera/software_isp/software_isp.cpp | 6 +- >> src/py/libcamera/py_helpers.cpp | 4 +- >> src/py/libcamera/py_helpers.h | 2 +- >> src/py/libcamera/py_main.cpp | 9 +- >> test/controls/meson.build | 1 + >> test/controls/metadata_list.cpp | 171 +++++ >> .../core_ipa_interface.h.tmpl | 1 + >> 89 files changed, 2361 insertions(+), 101 deletions(-) >> create mode 100644 Documentation/design/metadata-list.rst >> create mode 100644 include/libcamera/base/details/align.h >> create mode 100644 include/libcamera/base/details/cxx20.h >> create mode 100644 include/libcamera/metadata_list.h >> create mode 100644 include/libcamera/metadata_list_plan.h >> create mode 100644 src/libcamera/metadata_list.cpp >> create mode 100644 test/controls/metadata_list.cpp >> >> -- >> 2.49.0
The main purpose of this patch series is to introduce a mechanism for pipeline handlers by which they can report metadata items before a request completes. That part is based on https://patchwork.libcamera.org/cover/22226/, which itself was created in response to https://patchwork.libcamera.org/cover/22111/. Most of the former patch series is integrated here. In contrast, this patch series introduces a new type `MetadataList` that is used for storing the metadata items, instead of a `ControlList`. This is needed to solve the inherent synchronization issues caused by the fact that user callbacks (e.g. `requestComplete`) are invoked synchronously. Specifically, the newly introduced `metadataAvailable` signal should provide a way for users to defer the processing of the completed metadata items to a separate thread (as blocking the internal libcamera thread is undesired). Using a `ControlList` would not be ideal. This patch series also adds `Documentation/design/metadata-list.rst`. I believe reading that (first) provides useful information to better understand the reasons and design of the implemented mechanism. I tried to put most of the information in the commit messages or actual documentation as this cover letter will be lost to time. ### TODOs There are some TODOs in the code at the moment, those still has to be discussed. The old `ControlList` metadata list is not removed yet for testing, but everything uses the new metadata list in `Request::metadata2()`. The pipeline handlers guide is also not updated yet. ### user interface An important question remaining to be answered is what exactly should be exposed to user applications and how. For example, should `MetadataListPLan` be accessible? There are arguments to be made for that: for example, if an application has to know beforehand which metadata items are reported in order to allocate memory for them (e.g. pipewire). I see this as very similar to the `ControlInfoMap` of the camera. Similarly, e.g. `MetadataList::iterator` has an inline `operator++`. I don't think going through a shared library for an operator increment is ideal, but an argument could be made that doing so enables the structure to be changed. I believe it is more likely than not that it will have to be changed at least initially. Since libcamera does not promise any stability at the moment, I think this is not a great concern at the moment. ### rpi The rpi related changes conflict with https://patchwork.libcamera.org/cover/23474/ some things can be simplified after that is accepted. ### testing I was only able to test the uvc, rkisp1, rpi/vc4, vimc, virtual pipeline handlers. The rest compile and all, but I am not entirely sure everything is in the right place (especially in the `simple` pipeline handler). ### debug metadata One could argue, that this implementation is based on one of the simplest ideas considered, but that naturally has some drawbacks. One such issues arises when considering "debug metadata". The current implementation requires the set of all possibly reported metadata items to be known. This means that using debug metadata will become a lot less convenient, as the new metadata has to be added to a `MetadataListPlan` object. As opposed to the current situation where metadata items can be just set, and `gen-debug-controls.py` will automagically generate them. A couple ideas to address this situation: (1) do nothing * by far the simplest * inconvenient for debug metadata (2) decouple "debug metadata" from "normal metadata" * no early reporting (probably not too important) * can keep using `ControlList` * how to expose and to which (all?) applications? * needs some, possibly quite a bit of, modifications in the IPA interface (3) different `MetadataList` implementation * I have a prototype for another implementation - somewhat more complex - needs to allocate on demand - provides the essentially same interface and guarantees - can accommodate any number of metadata items just like a `ControlList` - no `MetadataListPlan` needed ### alternative implementation There is a prototype for an alternative implementation that implements basically the same interface, but is a bit more complicated. Comparison of the here proposed ("contiguous") metadata list, and the more complex ("chunked") metadata list is as follows: cont. chunk. ==============================+===================================+==========================+ complexity | simpler | more complex | memory allocation | single one when constructed | on demand | contiguous<1> | yes | no | backing data structure<2> | sorted array | chaining hash table<3> | iterator step | constant time | constant time | wasted space | <4> | <5> | <1>: The reason why this is highlighted is that initially I expected that if a serialized version of the `ControlList` is ever introduced, then it should use the same format as this metadata list. This would allow the currently completed items in the metadata list to be accessed through a hypothetical non-owning `ControlListView` type referring to the storage of the metadata list. Such a `ControlList` would most likely use a contiguous chunk of memory, so having contiguous storage helps compatibility. Nonetheless, I am becoming less sure that is is actually useful; and with sufficient complexity the non-contiguous version can also be supported. <2>: for lookup, wrt. the numeric identifier of the control <3>: fixed number buckets, making it resizable is not trivial <4>: Space for all metadata items is allocated, so if there are metadata items with large space requirements (e.g. `rpi::PispStatsOutput` - 23200 bytes) that are seldom reported, then that space is essentially wasted. <5>: (I think) bounded by (items / 2 * chunk_size), but with the right parameters all "usually" reported metadata can fit in one chunk. Barnabás Pőcze (17): libcamera: controls: Strings are arrays libcamera: controls: Add `ControlValueView` libcamera: base: Add file for C++20 polyfills libcamera: base: cxx20: Add `type_identity{,_t}` libcamera: base: cxx20: Add `has_single_bit()` libcamera: base: Add alignment utility functions libcamera: Add `MetadataList` Documentation: design: Document `MetadataList` libcamera: ipa_data_serializer: Support `MetadataListPlan` libcamera: camera: Store `MetadataListPlan` in `Camera::Private` libcamera: request: Store `MetadataList` [DNI] apps: cam: Print `MetadataList` of `Request` as well libcamera: pipeline: Fill `MetadataListPlan` of cameras libcamera: pipeline: rpi: Queue metadata until completion libcamera: pipeline: rpi: Use `metadataAvailable()` py: Use `Request::metadata2()` treewide: Use `Request::metadata2()` Jacopo Mondi (6): libcamera: camera: Introduce metadataAvailable signal guides: application: Document Camera::metadataAvailable libcamera: pipeline_handler: Add metadataAvailable() function guides: pipeline_handler: Document PipelineHandler::metadataAvailable [DNI] apps: cam: Use Camera::metadataAvailable signal libcamera: pipeline: Use `metadataAvailable()` Documentation/design/metadata-list.rst | 234 +++++++ .../guides/application-developer.rst | 8 + Documentation/guides/pipeline-handler.rst | 26 +- Documentation/index.rst | 1 + Documentation/meson.build | 1 + include/libcamera/base/details/align.h | 72 ++ include/libcamera/base/details/cxx20.h | 25 + include/libcamera/base/meson.build | 1 + include/libcamera/camera.h | 1 + include/libcamera/controls.h | 71 +- include/libcamera/internal/camera.h | 3 + include/libcamera/internal/pipeline_handler.h | 21 + .../internal/software_isp/software_isp.h | 3 +- include/libcamera/ipa/core.mojom | 1 + include/libcamera/ipa/ipu3.mojom | 3 +- include/libcamera/ipa/mali-c55.mojom | 3 +- include/libcamera/ipa/raspberrypi.mojom | 1 + include/libcamera/ipa/rkisp1.mojom | 2 +- include/libcamera/ipa/soft.mojom | 3 +- include/libcamera/meson.build | 2 + include/libcamera/metadata_list.h | 619 ++++++++++++++++++ include/libcamera/metadata_list_plan.h | 109 +++ include/libcamera/request.h | 5 + src/android/camera_device.cpp | 4 +- src/apps/cam/camera_session.cpp | 19 + src/apps/cam/file_sink.cpp | 4 +- src/apps/cam/file_sink.h | 4 +- src/apps/common/dng_writer.cpp | 2 +- src/apps/common/dng_writer.h | 4 +- src/apps/qcam/main_window.cpp | 4 +- src/apps/qcam/main_window.h | 4 +- src/gstreamer/gstlibcamera-controls.cpp.in | 4 +- src/gstreamer/gstlibcamerasrc.cpp | 2 +- src/ipa/ipu3/algorithms/agc.cpp | 4 + src/ipa/ipu3/algorithms/awb.cpp | 12 + src/ipa/ipu3/algorithms/awb.h | 1 + src/ipa/ipu3/ipa_context.cpp | 3 + src/ipa/ipu3/ipa_context.h | 3 + src/ipa/ipu3/ipu3.cpp | 8 +- src/ipa/mali-c55/algorithms/agc.cpp | 5 + src/ipa/mali-c55/algorithms/awb.cpp | 7 + src/ipa/mali-c55/algorithms/awb.h | 1 + src/ipa/mali-c55/algorithms/blc.cpp | 2 + src/ipa/mali-c55/ipa_context.h | 3 + src/ipa/mali-c55/mali-c55.cpp | 6 +- src/ipa/rkisp1/algorithms/agc.cpp | 10 + src/ipa/rkisp1/algorithms/awb.cpp | 4 + src/ipa/rkisp1/algorithms/blc.cpp | 2 + src/ipa/rkisp1/ipa_context.h | 1 + src/ipa/rkisp1/rkisp1.cpp | 8 +- src/ipa/rpi/common/ipa_base.cpp | 34 + src/ipa/rpi/pisp/pisp.cpp | 5 +- src/ipa/rpi/vc4/vc4.cpp | 4 +- src/ipa/simple/algorithms/agc.cpp | 8 + src/ipa/simple/algorithms/agc.h | 1 + src/ipa/simple/algorithms/awb.cpp | 8 + src/ipa/simple/algorithms/awb.h | 1 + src/ipa/simple/algorithms/blc.cpp | 3 + src/ipa/simple/algorithms/ccm.cpp | 3 + src/ipa/simple/algorithms/lut.cpp | 3 + src/ipa/simple/ipa_context.h | 2 + src/ipa/simple/soft_simple.cpp | 8 +- src/libcamera/camera.cpp | 62 ++ src/libcamera/controls.cpp | 194 ++++++ src/libcamera/ipa_data_serializer.cpp | 84 +++ src/libcamera/meson.build | 1 + src/libcamera/metadata_list.cpp | 315 +++++++++ src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 7 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 23 +- src/libcamera/pipeline/mali-c55/mali-c55.cpp | 4 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +- .../pipeline/rpi/common/pipeline_base.cpp | 36 +- .../pipeline/rpi/common/pipeline_base.h | 4 +- src/libcamera/pipeline/rpi/pisp/pisp.cpp | 8 +- src/libcamera/pipeline/rpi/vc4/vc4.cpp | 8 +- src/libcamera/pipeline/simple/simple.cpp | 7 +- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- src/libcamera/pipeline/vimc/vimc.cpp | 5 +- .../pipeline/virtual/config_parser.cpp | 3 + src/libcamera/pipeline/virtual/virtual.cpp | 2 +- src/libcamera/pipeline_handler.cpp | 61 ++ src/libcamera/request.cpp | 2 + src/libcamera/software_isp/software_isp.cpp | 6 +- src/py/libcamera/py_helpers.cpp | 4 +- src/py/libcamera/py_helpers.h | 2 +- src/py/libcamera/py_main.cpp | 9 +- test/controls/meson.build | 1 + test/controls/metadata_list.cpp | 171 +++++ .../core_ipa_interface.h.tmpl | 1 + 89 files changed, 2361 insertions(+), 101 deletions(-) create mode 100644 Documentation/design/metadata-list.rst create mode 100644 include/libcamera/base/details/align.h create mode 100644 include/libcamera/base/details/cxx20.h create mode 100644 include/libcamera/metadata_list.h create mode 100644 include/libcamera/metadata_list_plan.h create mode 100644 src/libcamera/metadata_list.cpp create mode 100644 test/controls/metadata_list.cpp -- 2.49.0