Message ID | 20250422142010.13064-5-jaslo@ziska.de |
---|---|
State | New |
Headers | show |
Hi, Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit : > When the libcamerasrc element is stopped and started again, the > GstCameraControls::setCamera() function is called, but now it is > populated with metadata returned by the previous requests. Because > GstCameraControls::setCamera() filters the controls, this would cause > warnings to be emitted, as the returned metadata are not valid controls > which can be set. Does it also fixes on top the issue where controls that get applied with delays get resets and forgotten ? > > This is fixed by introducing a new member which only holds the metadata > returned by the requests, so that the controls and the metadata are now > completely independent from each other. > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > --- > src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------ > src/gstreamer/gstlibcamera-controls.h | 4 +++- > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > index 89c530da..490e835e 100644 > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}( > bool GstCameraControls::getProperty(guint propId, GValue *value, > [[maybe_unused]] GParamSpec *pspec) > { > - if (!controls_acc_.contains(propId)) { > + const ControlValue *cv; > + if (controls_acc_.contains(propId)) { > + cv = &controls_acc_.get(propId); > + } else if (metadata_.contains(propId)) { > + cv = &metadata_.get(propId); > + } else { > GST_WARNING("Control '%s' is not available, default value will " > "be returned", > controls::controls.at(propId)->name().c_str()); > return true; > } > - const ControlValue &cv = controls_acc_.get(propId); > > switch (propId) { > {%- for vendor, ctrls in controls %} > {%- for ctrl in ctrls %} > > case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { > - auto control = cv.get<{{ ctrl.type }}>(); > + auto control = cv->get<{{ ctrl.type }}>(); > > {%- if ctrl.is_array %} > for (size_t i = 0; i < control.size(); ++i) { > @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam) > "camera and will be ignored", > cid->name().c_str()); > } > - > controls_acc_ = new_controls; > controls_ = new_controls; > + > + /* > + * Clear metadata as this might already be populated if the element has > + * been running before. > + */ > + metadata_.clear(); > } > > void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &request) > @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &reque > > void GstCameraControls::readMetadata(libcamera::Request *request) > { > - controls_acc_.merge(request->metadata(), > - ControlList::MergePolicy::OverwriteExisting); > + metadata_.merge(request->metadata(), > + ControlList::MergePolicy::OverwriteExisting); > + > } > diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h > index 749220b5..ee97e61e 100644 > --- a/src/gstreamer/gstlibcamera-controls.h > +++ b/src/gstreamer/gstlibcamera-controls.h > @@ -36,8 +36,10 @@ private: > ControlInfoMap capabilities_; > /* Set of user modified controls. */ > ControlList controls_; > - /* Accumulator of all controls ever set and metadata returned by camera */ > + /* Accumulator of all controls ever */ > ControlList controls_acc_; > + /* Metadata returned by requests */ > + ControlList metadata_; > }; > > } /* namespace libcamera */ Looks good to me, but I'd like someone from the core to review this please. Nicolas
Quoting Nicolas Dufresne (2025-04-22 15:42:41) > Hi, > > Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit : > > When the libcamerasrc element is stopped and started again, the > > GstCameraControls::setCamera() function is called, but now it is > > populated with metadata returned by the previous requests. Because > > GstCameraControls::setCamera() filters the controls, this would cause > > warnings to be emitted, as the returned metadata are not valid controls > > which can be set. > > Does it also fixes on top the issue where controls that get applied > with delays get resets and forgotten ? > > > > > This is fixed by introducing a new member which only holds the metadata > > returned by the requests, so that the controls and the metadata are now > > completely independent from each other. > > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > > --- > > src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------ > > src/gstreamer/gstlibcamera-controls.h | 4 +++- > > 2 files changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in > > index 89c530da..490e835e 100644 > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in > > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}( > > bool GstCameraControls::getProperty(guint propId, GValue *value, > > [[maybe_unused]] GParamSpec *pspec) > > { > > - if (!controls_acc_.contains(propId)) { > > + const ControlValue *cv; > > + if (controls_acc_.contains(propId)) { > > + cv = &controls_acc_.get(propId); > > + } else if (metadata_.contains(propId)) { > > + cv = &metadata_.get(propId); > > + } else { > > GST_WARNING("Control '%s' is not available, default value will " > > "be returned", > > controls::controls.at(propId)->name().c_str()); > > return true; > > } > > - const ControlValue &cv = controls_acc_.get(propId); > > > > switch (propId) { > > {%- for vendor, ctrls in controls %} > > {%- for ctrl in ctrls %} > > > > case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { > > - auto control = cv.get<{{ ctrl.type }}>(); > > + auto control = cv->get<{{ ctrl.type }}>(); > > > > {%- if ctrl.is_array %} > > for (size_t i = 0; i < control.size(); ++i) { > > @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam) > > "camera and will be ignored", > > cid->name().c_str()); > > } > > - > > controls_acc_ = new_controls; > > controls_ = new_controls; > > + > > + /* > > + * Clear metadata as this might already be populated if the element has > > + * been running before. > > + */ > > + metadata_.clear(); > > } > > > > void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &request) > > @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &reque > > > > void GstCameraControls::readMetadata(libcamera::Request *request) > > { > > - controls_acc_.merge(request->metadata(), > > - ControlList::MergePolicy::OverwriteExisting); > > + metadata_.merge(request->metadata(), > > + ControlList::MergePolicy::OverwriteExisting); > > + > > } > > diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h > > index 749220b5..ee97e61e 100644 > > --- a/src/gstreamer/gstlibcamera-controls.h > > +++ b/src/gstreamer/gstlibcamera-controls.h > > @@ -36,8 +36,10 @@ private: > > ControlInfoMap capabilities_; > > /* Set of user modified controls. */ > > ControlList controls_; > > - /* Accumulator of all controls ever set and metadata returned by camera */ > > + /* Accumulator of all controls ever */ > > ControlList controls_acc_; Do we need a controls_acc_ at all ? What does this do ? I'm weary because some of the controls are 'triggers' so they should only be set once - not on every frame ... ? > > + /* Metadata returned by requests */ > > + ControlList metadata_; > > }; > > > > } /* namespace libcamera */ > > Looks good to me, but I'd like someone from the core to review this > please. > > Nicolas
diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in index 89c530da..490e835e 100644 --- a/src/gstreamer/gstlibcamera-controls.cpp.in +++ b/src/gstreamer/gstlibcamera-controls.cpp.in @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}( bool GstCameraControls::getProperty(guint propId, GValue *value, [[maybe_unused]] GParamSpec *pspec) { - if (!controls_acc_.contains(propId)) { + const ControlValue *cv; + if (controls_acc_.contains(propId)) { + cv = &controls_acc_.get(propId); + } else if (metadata_.contains(propId)) { + cv = &metadata_.get(propId); + } else { GST_WARNING("Control '%s' is not available, default value will " "be returned", controls::controls.at(propId)->name().c_str()); return true; } - const ControlValue &cv = controls_acc_.get(propId); switch (propId) { {%- for vendor, ctrls in controls %} {%- for ctrl in ctrls %} case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: { - auto control = cv.get<{{ ctrl.type }}>(); + auto control = cv->get<{{ ctrl.type }}>(); {%- if ctrl.is_array %} for (size_t i = 0; i < control.size(); ++i) { @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam) "camera and will be ignored", cid->name().c_str()); } - controls_acc_ = new_controls; controls_ = new_controls; + + /* + * Clear metadata as this might already be populated if the element has + * been running before. + */ + metadata_.clear(); } void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &request) @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &reque void GstCameraControls::readMetadata(libcamera::Request *request) { - controls_acc_.merge(request->metadata(), - ControlList::MergePolicy::OverwriteExisting); + metadata_.merge(request->metadata(), + ControlList::MergePolicy::OverwriteExisting); + } diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h index 749220b5..ee97e61e 100644 --- a/src/gstreamer/gstlibcamera-controls.h +++ b/src/gstreamer/gstlibcamera-controls.h @@ -36,8 +36,10 @@ private: ControlInfoMap capabilities_; /* Set of user modified controls. */ ControlList controls_; - /* Accumulator of all controls ever set and metadata returned by camera */ + /* Accumulator of all controls ever */ ControlList controls_acc_; + /* Metadata returned by requests */ + ControlList metadata_; }; } /* namespace libcamera */
When the libcamerasrc element is stopped and started again, the GstCameraControls::setCamera() function is called, but now it is populated with metadata returned by the previous requests. Because GstCameraControls::setCamera() filters the controls, this would cause warnings to be emitted, as the returned metadata are not valid controls which can be set. This is fixed by introducing a new member which only holds the metadata returned by the requests, so that the controls and the metadata are now completely independent from each other. Signed-off-by: Jaslo Ziska <jaslo@ziska.de> --- src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------ src/gstreamer/gstlibcamera-controls.h | 4 +++- 2 files changed, 19 insertions(+), 7 deletions(-)