| Message ID | 20250721104622.1550908-11-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Jul 21, 2025 at 12:46:10PM +0200, Barnabás Pőcze wrote: > Just like `ControlInfoMap controlInfo_` is a public member of the private > camera data for pipeline handlers to populate, add a `MetadataListPlan` as > well for the pipeline handlers to fill. This will be used to initialize > the `MetadataList` of each request created by the camera. > > Also add `Camera::metadata()`, which makes it accessible for applications. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > changes in v2: > * make it available in `Camera::metadata()` > --- > include/libcamera/camera.h | 1 + > include/libcamera/internal/camera.h | 2 ++ > src/libcamera/camera.cpp | 21 +++++++++++++++++++++ > 3 files changed, 24 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 94cee7bd8..e3e40aa89 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h This commit fails to build ../include/libcamera/camera.h:134:15: error: ‘MetadataListPlan’ does not name a type Do you need to forward declare MetadataListPlan ? > @@ -131,6 +131,7 @@ public: > > const ControlInfoMap &controls() const; > const ControlList &properties() const; > + const MetadataListPlan &metadata() const; just nitpicking on the name as it might create confusion between Request::metadata() and Camera::metadata(), but as we have Camera::controls() I think this is actually fine > > const std::set<Stream *> &streams() const; > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > index 8a2e9ed58..016c1ae51 100644 > --- a/include/libcamera/internal/camera.h > +++ b/include/libcamera/internal/camera.h > @@ -18,6 +18,7 @@ > #include <libcamera/base/class.h> > > #include <libcamera/camera.h> > +#include <libcamera/metadata_list_plan.h> > > namespace libcamera { > > @@ -40,6 +41,7 @@ public: > std::queue<Request *> waitingRequests_; > ControlInfoMap controlInfo_; > ControlList properties_; > + MetadataListPlan metadataPlan_; > > uint32_t requestSequence_; > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 5c78f36c1..fa57d46c9 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -669,6 +669,14 @@ Camera::Private::~Private() > * over a single capture session. > */ > > +/** > + * \var Camera::Private::metadataPlan_ > + * \brief The set of metadata supported by the camera > + * > + * The metadata information shall be initialised by the pipeline handler when > + * creating the camera. > + */ nit: the member is declared in the class before requestSequence_. You can move its documentation up, before the requestSequence_ one. > + > static const char *const camera_state_names[] = { > "Available", > "Acquired", > @@ -1074,6 +1082,19 @@ const ControlInfoMap &Camera::controls() const > return _d()->controlInfo_; > } > > +/** > + * \brief Retrieve the set of metadata supported by the camera > + * > + * The list of metadata controls that may be reported by the camera > + * for a \ref Request::metadata() "request". > + * > + * \return A MetadataListPlan listing the metadata controls supported by the camera nit: s/A/The nits apart Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > + */ > +const MetadataListPlan &Camera::metadata() const > +{ > + return _d()->metadataPlan_; > +} > + > /** > * \brief Retrieve the list of properties of the camera > * > -- > 2.50.1 >
Hi 2025. 07. 28. 11:28 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Jul 21, 2025 at 12:46:10PM +0200, Barnabás Pőcze wrote: >> Just like `ControlInfoMap controlInfo_` is a public member of the private >> camera data for pipeline handlers to populate, add a `MetadataListPlan` as >> well for the pipeline handlers to fill. This will be used to initialize >> the `MetadataList` of each request created by the camera. >> >> Also add `Camera::metadata()`, which makes it accessible for applications. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> changes in v2: >> * make it available in `Camera::metadata()` >> --- >> include/libcamera/camera.h | 1 + >> include/libcamera/internal/camera.h | 2 ++ >> src/libcamera/camera.cpp | 21 +++++++++++++++++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h >> index 94cee7bd8..e3e40aa89 100644 >> --- a/include/libcamera/camera.h >> +++ b/include/libcamera/camera.h > > This commit fails to build > ../include/libcamera/camera.h:134:15: error: ‘MetadataListPlan’ does not name a type > > Do you need to forward declare MetadataListPlan ? Yes, I think that is sufficient. > >> @@ -131,6 +131,7 @@ public: >> >> const ControlInfoMap &controls() const; >> const ControlList &properties() const; >> + const MetadataListPlan &metadata() const; > > just nitpicking on the name as it might create confusion between > Request::metadata() and Camera::metadata(), but as we have > Camera::controls() I think this is actually fine That was my thinking as well. > >> >> const std::set<Stream *> &streams() const; >> >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h >> index 8a2e9ed58..016c1ae51 100644 >> --- a/include/libcamera/internal/camera.h >> +++ b/include/libcamera/internal/camera.h >> @@ -18,6 +18,7 @@ >> #include <libcamera/base/class.h> >> >> #include <libcamera/camera.h> >> +#include <libcamera/metadata_list_plan.h> >> >> namespace libcamera { >> >> @@ -40,6 +41,7 @@ public: >> std::queue<Request *> waitingRequests_; >> ControlInfoMap controlInfo_; >> ControlList properties_; >> + MetadataListPlan metadataPlan_; >> >> uint32_t requestSequence_; >> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index 5c78f36c1..fa57d46c9 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -669,6 +669,14 @@ Camera::Private::~Private() >> * over a single capture session. >> */ >> >> +/** >> + * \var Camera::Private::metadataPlan_ >> + * \brief The set of metadata supported by the camera >> + * >> + * The metadata information shall be initialised by the pipeline handler when >> + * creating the camera. >> + */ > > nit: the member is declared in the class before requestSequence_. You > can move its documentation up, before the requestSequence_ one. done > >> + >> static const char *const camera_state_names[] = { >> "Available", >> "Acquired", >> @@ -1074,6 +1082,19 @@ const ControlInfoMap &Camera::controls() const >> return _d()->controlInfo_; >> } >> >> +/** >> + * \brief Retrieve the set of metadata supported by the camera >> + * >> + * The list of metadata controls that may be reported by the camera >> + * for a \ref Request::metadata() "request". >> + * >> + * \return A MetadataListPlan listing the metadata controls supported by the camera > > nit: s/A/The The documentation for `controls()` also says "A", I copied it from there, please confirm that I should change it. Regards, Barnabás Pőcze > > nits apart > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> + */ >> +const MetadataListPlan &Camera::metadata() const >> +{ >> + return _d()->metadataPlan_; >> +} >> + >> /** >> * \brief Retrieve the list of properties of the camera >> * >> -- >> 2.50.1 >>
Hi Barnabás On Mon, Jul 28, 2025 at 05:12:36PM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 07. 28. 11:28 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Jul 21, 2025 at 12:46:10PM +0200, Barnabás Pőcze wrote: > > > Just like `ControlInfoMap controlInfo_` is a public member of the private > > > camera data for pipeline handlers to populate, add a `MetadataListPlan` as > > > well for the pipeline handlers to fill. This will be used to initialize > > > the `MetadataList` of each request created by the camera. > > > > > > Also add `Camera::metadata()`, which makes it accessible for applications. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > changes in v2: > > > * make it available in `Camera::metadata()` > > > --- > > > include/libcamera/camera.h | 1 + > > > include/libcamera/internal/camera.h | 2 ++ > > > src/libcamera/camera.cpp | 21 +++++++++++++++++++++ > > > 3 files changed, 24 insertions(+) > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > index 94cee7bd8..e3e40aa89 100644 > > > --- a/include/libcamera/camera.h > > > +++ b/include/libcamera/camera.h > > > > This commit fails to build > > ../include/libcamera/camera.h:134:15: error: ‘MetadataListPlan’ does not name a type > > > > Do you need to forward declare MetadataListPlan ? > > Yes, I think that is sufficient. > > > > > > > @@ -131,6 +131,7 @@ public: > > > > > > const ControlInfoMap &controls() const; > > > const ControlList &properties() const; > > > + const MetadataListPlan &metadata() const; > > > > just nitpicking on the name as it might create confusion between > > Request::metadata() and Camera::metadata(), but as we have > > Camera::controls() I think this is actually fine > > That was my thinking as well. > > > > > > > > > > const std::set<Stream *> &streams() const; > > > > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > > > index 8a2e9ed58..016c1ae51 100644 > > > --- a/include/libcamera/internal/camera.h > > > +++ b/include/libcamera/internal/camera.h > > > @@ -18,6 +18,7 @@ > > > #include <libcamera/base/class.h> > > > > > > #include <libcamera/camera.h> > > > +#include <libcamera/metadata_list_plan.h> > > > > > > namespace libcamera { > > > > > > @@ -40,6 +41,7 @@ public: > > > std::queue<Request *> waitingRequests_; > > > ControlInfoMap controlInfo_; > > > ControlList properties_; > > > + MetadataListPlan metadataPlan_; > > > > > > uint32_t requestSequence_; > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 5c78f36c1..fa57d46c9 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -669,6 +669,14 @@ Camera::Private::~Private() > > > * over a single capture session. > > > */ > > > > > > +/** > > > + * \var Camera::Private::metadataPlan_ > > > + * \brief The set of metadata supported by the camera > > > + * > > > + * The metadata information shall be initialised by the pipeline handler when > > > + * creating the camera. > > > + */ > > > > nit: the member is declared in the class before requestSequence_. You > > can move its documentation up, before the requestSequence_ one. > > done > > > > > > > + > > > static const char *const camera_state_names[] = { > > > "Available", > > > "Acquired", > > > @@ -1074,6 +1082,19 @@ const ControlInfoMap &Camera::controls() const > > > return _d()->controlInfo_; > > > } > > > > > > +/** > > > + * \brief Retrieve the set of metadata supported by the camera > > > + * > > > + * The list of metadata controls that may be reported by the camera > > > + * for a \ref Request::metadata() "request". > > > + * > > > + * \return A MetadataListPlan listing the metadata controls supported by the camera > > > > nit: s/A/The > > The documentation for `controls()` also says "A", I copied it from there, > please confirm that I should change it. > Ok. My thinking was that the metadatalist plan is a single one and we return a reference to it, and that's better conveyed by "The" instead of "A". But for sake of consistency, feel free to keep it as it is! > > Regards, > Barnabás Pőcze > > > > > > nits apart > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > > > + */ > > > +const MetadataListPlan &Camera::metadata() const > > > +{ > > > + return _d()->metadataPlan_; > > > +} > > > + > > > /** > > > * \brief Retrieve the list of properties of the camera > > > * > > > -- > > > 2.50.1 > > > >
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 94cee7bd8..e3e40aa89 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -131,6 +131,7 @@ public: const ControlInfoMap &controls() const; const ControlList &properties() const; + const MetadataListPlan &metadata() const; const std::set<Stream *> &streams() const; diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h index 8a2e9ed58..016c1ae51 100644 --- a/include/libcamera/internal/camera.h +++ b/include/libcamera/internal/camera.h @@ -18,6 +18,7 @@ #include <libcamera/base/class.h> #include <libcamera/camera.h> +#include <libcamera/metadata_list_plan.h> namespace libcamera { @@ -40,6 +41,7 @@ public: std::queue<Request *> waitingRequests_; ControlInfoMap controlInfo_; ControlList properties_; + MetadataListPlan metadataPlan_; uint32_t requestSequence_; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 5c78f36c1..fa57d46c9 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -669,6 +669,14 @@ Camera::Private::~Private() * over a single capture session. */ +/** + * \var Camera::Private::metadataPlan_ + * \brief The set of metadata supported by the camera + * + * The metadata information shall be initialised by the pipeline handler when + * creating the camera. + */ + static const char *const camera_state_names[] = { "Available", "Acquired", @@ -1074,6 +1082,19 @@ const ControlInfoMap &Camera::controls() const return _d()->controlInfo_; } +/** + * \brief Retrieve the set of metadata supported by the camera + * + * The list of metadata controls that may be reported by the camera + * for a \ref Request::metadata() "request". + * + * \return A MetadataListPlan listing the metadata controls supported by the camera + */ +const MetadataListPlan &Camera::metadata() const +{ + return _d()->metadataPlan_; +} + /** * \brief Retrieve the list of properties of the camera *
Just like `ControlInfoMap controlInfo_` is a public member of the private camera data for pipeline handlers to populate, add a `MetadataListPlan` as well for the pipeline handlers to fill. This will be used to initialize the `MetadataList` of each request created by the camera. Also add `Camera::metadata()`, which makes it accessible for applications. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- changes in v2: * make it available in `Camera::metadata()` --- include/libcamera/camera.h | 1 + include/libcamera/internal/camera.h | 2 ++ src/libcamera/camera.cpp | 21 +++++++++++++++++++++ 3 files changed, 24 insertions(+)