[PATCH 4/4] gstreamer: Split metadata controls into new member

[Kieran Bingham]

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 at 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