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

[Laurent Pinchart]

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 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);

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.

-- 
Regards,

Laurent Pinchart