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

[Jaslo Ziska]

Hi Nicolas, Kieran, Laurent,

Laurent Pinchart <laurent.pinchart at 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 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 ?

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.