| 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
On Tue, Apr 22, 2025 at 04:33:02PM +0100, Kieran Bingham wrote: > Quoting Nicolas Dufresne (2025-04-22 15:42:41) > > 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); Why do controls take precedence here ? Taking a step back, does GSreamer expect the value of properties to be changed dynamically by the device ? If, for instance, auto-exposure is enabled, does GStreamer expect/allow the exposure time and gain properties to be modified to reflect the AGC calculation ? > > > + } 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()); > > > } > > > - Please keep the blank line here. > > > 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 */ s/ever/ever set/ > > > 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 ... ? There are also dependencies between some controls, and I don't think we guarantee that setting A=a, B=b in the same request will have the same effect as setting A=a in a first request and B=b in a second request. Accumulating the controls may not match the actual state of the device. For instance, look at the documentation of ColourGains and ColourTemperature. They respectively state If ColourGains is set in a request but ColourTemperature is not, the implementation shall calculate and set the ColourTemperature based on the ColourGains. and If ColourTemperature is set in a request but ColourGains is not, the implementation shall calculate and set the ColourGains based on the given ColourTemperature. Setting ColourGains=cg in a first request and and ColourTemperature=ct in a second request means that ColourGains will end up being calculated as CG(ct). Setting them both in the same request means they will both have the values specified by the user. By accumulating controls, we lose this information. > > > + /* Metadata returned by requests */ > > > + ControlList metadata_; > > > }; > > > > > > } /* namespace libcamera */ > > > > Looks good to me, but I'd like someone from the core to review this > > please.
Hi Nicolas, Kieran, Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Tue, Apr 22, 2025 at 04:33:02PM +0100, Kieran Bingham wrote: >> Quoting Nicolas Dufresne (2025-04-22 15:42:41) >> > 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 ? Sorry, I don't know which issue you are referring to. >> > >> > > 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); > > Why do controls take precedence here ? I thought it would make sense, but I was wrong, see below. > > Taking a step back, does GSreamer expect the value of properties > to be > changed dynamically by the device ? If, for instance, > auto-exposure is > enabled, does GStreamer expect/allow the exposure time and gain > properties to be modified to reflect the AGC calculation ? In theory these modified values should be available as properties if they are in the metadata returned by the requests. But from the rest of your comments I see that there is a mistake in my logic, see below. > >> > > + } 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()); >> > > } >> > > - > > Please keep the blank line here. > >> > > 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 */ > > s/ever/ever set/ > >> > > 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 ... ? Now I'm confused, I thought all controls should only be set once when they changed? controls_acc_ was introduced for this. The controls_ member holds the controls which will be set on the next request and then gets cleared (so that they only get set once). But the control_acc_ variable accumulates the controls so that they can be returned to GSreamer in getProperty(). > > There are also dependencies between some controls, and I don't > think we > guarantee that setting A=a, B=b in the same request will have > the same > effect as setting A=a in a first request and B=b in a second > request. > Accumulating the controls may not match the actual state of the > device. > > For instance, look at the documentation of ColourGains and > ColourTemperature. They respectively state > > If ColourGains is set in a request but ColourTemperature > is not, the > implementation shall calculate and set the > ColourTemperature based on > the ColourGains. > > and > > If ColourTemperature is set in a request but ColourGains > is not, the > implementation shall calculate and set the ColourGains > based on the > given ColourTemperature. > > Setting ColourGains=cg in a first request and and > ColourTemperature=ct > in a second request means that ColourGains will end up being > calculated > as CG(ct). Setting them both in the same request means they will > both > have the values specified by the user. By accumulating controls, > we lose > this information. Thanks for the explanation. Then it seems like it was correct the way it was before? Because when ColourGains is set in the first request controls_acc_ holds that value and if at a later point ColourTemperature is set, ColourGains gets overwritten by the value returned by the metadata of the request. Or am I mistaken? Best Regards Jaslo > >> > > + /* Metadata returned by requests */ >> > > + ControlList metadata_; >> > > }; >> > > >> > > } /* namespace libcamera */ >> > >> > Looks good to me, but I'd like someone from the core to >> > review this >> > please.
Le mercredi 23 avril 2025 à 02:34 +0300, Laurent Pinchart a écrit : > On Tue, Apr 22, 2025 at 04:33:02PM +0100, Kieran Bingham wrote: > > Quoting Nicolas Dufresne (2025-04-22 15:42:41) > > > 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); > > Why do controls take precedence here ? > > Taking a step back, does GSreamer expect the value of properties to be > changed dynamically by the device ? If, for instance, auto-exposure is > enabled, does GStreamer expect/allow the exposure time and gain > properties to be modified to reflect the AGC calculation ? Properties can be used for that yes. When they change, I'd expect a call to g_object_notify() to notify users of the change. I think what we are missing is the clear split between the properties are are set by the application, vs the one that are set by libcamera. This is why we excluded the second bread in the initial control support. Nicolas > > > > > + } 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()); > > > > } > > > > - > > Please keep the blank line here. > > > > > 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 */ > > s/ever/ever set/ > > > > > 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 ... ? > > There are also dependencies between some controls, and I don't think we > guarantee that setting A=a, B=b in the same request will have the same > effect as setting A=a in a first request and B=b in a second request. > Accumulating the controls may not match the actual state of the device. > > For instance, look at the documentation of ColourGains and > ColourTemperature. They respectively state > > If ColourGains is set in a request but ColourTemperature is not, the > implementation shall calculate and set the ColourTemperature based on > the ColourGains. > > and > > If ColourTemperature is set in a request but ColourGains is not, the > implementation shall calculate and set the ColourGains based on the > given ColourTemperature. > > Setting ColourGains=cg in a first request and and ColourTemperature=ct > in a second request means that ColourGains will end up being calculated > as CG(ct). Setting them both in the same request means they will both > have the values specified by the user. By accumulating controls, we lose > this information. > > > > > + /* Metadata returned by requests */ > > > > + ControlList metadata_; > > > > }; > > > > > > > > } /* namespace libcamera */ > > > > > > Looks good to me, but I'd like someone from the core to review this > > > please.
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(-)