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

[Nicolas Dufresne]

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

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.