[v1] libcamera: request: Make `controls_`/`metadata_` members

22948 diff mbox series

The lifetimes of these two `ControlList`s are tied entirely to the request
object, so simplify the code by making them member variables instead of
manually managing their dynamic lifetime.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/request.h |  8 ++++----
 src/libcamera/request.cpp   | 17 ++++-------------
 2 files changed, 8 insertions(+), 17 deletions(-)

Quoting Barnabás Pőcze (2025-03-10 17:03:43)
> The lifetimes of these two `ControlList`s are tied entirely to the request
> object, so simplify the code by making them member variables instead of
> manually managing their dynamic lifetime.
> 

This one looks reasonable, but I suspect it changes the ABI in some
form?

Could you run 

./utils/abi-compat.sh

And check the report please? I'd like to ensure that any patches which
affect ABI or API breakage include that statement in the commit message
so we can identify them in the release notes. (Another candidate for the
CI builds too of course)

--
Kieran

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/request.h |  8 ++++----
>  src/libcamera/request.cpp   | 17 ++++-------------
>  2 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index e214a9d13..3061e2fb0 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -49,8 +49,8 @@ public:
>  
>         void reuse(ReuseFlag flags = Default);
>  
> -       ControlList &controls() { return *controls_; }
> -       ControlList &metadata() { return *metadata_; }
> +       ControlList &controls() { return controls_; }
> +       ControlList &metadata() { return metadata_; }
>         const BufferMap &buffers() const { return bufferMap_; }
>         int addBuffer(const Stream *stream, FrameBuffer *buffer,
>                       std::unique_ptr<Fence> fence = nullptr);
> @@ -67,8 +67,8 @@ public:
>  private:
>         LIBCAMERA_DISABLE_COPY(Request)
>  
> -       ControlList *controls_;
> -       ControlList *metadata_;
> +       ControlList controls_;
> +       ControlList metadata_;

If we're modifying Request public ABI - should they in fact go into the
Request::Private class though ?

>         BufferMap bufferMap_;
>  
>         const uint64_t cookie_;
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 6fa1801a0..59fc3fdf9 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -351,16 +351,10 @@ void Request::Private::timeout()
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
>         : Extensible(std::make_unique<Private>(camera)),
> +         controls_(controls::controls, camera->_d()->validator()),
> +         metadata_(controls::controls), /* \todo Add a validator for metadata controls. */
>           cookie_(cookie), status_(RequestPending)
>  {
> -       controls_ = new ControlList(controls::controls,
> -                                   camera->_d()->validator());
> -
> -       /**
> -        * \todo Add a validator for metadata controls.
> -        */
> -       metadata_ = new ControlList(controls::controls);
> -
>         LIBCAMERA_TRACEPOINT(request_construct, this);
>  
>         LOG(Request, Debug) << "Created request - cookie: " << cookie_;
> @@ -369,9 +363,6 @@ Request::Request(Camera *camera, uint64_t cookie)
>  Request::~Request()
>  {
>         LIBCAMERA_TRACEPOINT(request_destroy, this);
> -
> -       delete metadata_;
> -       delete controls_;
>  }
>  
>  /**
> @@ -402,8 +393,8 @@ void Request::reuse(ReuseFlag flags)
>  
>         status_ = RequestPending;
>  
> -       controls_->clear();
> -       metadata_->clear();
> +       controls_.clear();
> +       metadata_.clear();
>  }
>  
>  /**
> -- 
> 2.48.1
>

On Sun, Mar 16, 2025 at 12:19:57PM +0000, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-03-10 17:03:43)
> > The lifetimes of these two `ControlList`s are tied entirely to the request
> > object, so simplify the code by making them member variables instead of
> > manually managing their dynamic lifetime.
> > 
> 
> This one looks reasonable, but I suspect it changes the ABI in some
> form?
> 
> Could you run 
> 
> ./utils/abi-compat.sh
> 
> And check the report please? I'd like to ensure that any patches which
> affect ABI or API breakage include that statement in the commit message
> so we can identify them in the release notes. (Another candidate for the
> CI builds too of course)

Unless there's something depending on this patch, we could also postpone
this change until we need to merge other ABI breakages.

> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  include/libcamera/request.h |  8 ++++----
> >  src/libcamera/request.cpp   | 17 ++++-------------
> >  2 files changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index e214a9d13..3061e2fb0 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -49,8 +49,8 @@ public:
> >  
> >         void reuse(ReuseFlag flags = Default);
> >  
> > -       ControlList &controls() { return *controls_; }
> > -       ControlList &metadata() { return *metadata_; }
> > +       ControlList &controls() { return controls_; }
> > +       ControlList &metadata() { return metadata_; }
> >         const BufferMap &buffers() const { return bufferMap_; }
> >         int addBuffer(const Stream *stream, FrameBuffer *buffer,
> >                       std::unique_ptr<Fence> fence = nullptr);
> > @@ -67,8 +67,8 @@ public:
> >  private:
> >         LIBCAMERA_DISABLE_COPY(Request)
> >  
> > -       ControlList *controls_;
> > -       ControlList *metadata_;
> > +       ControlList controls_;
> > +       ControlList metadata_;
> 
> If we're modifying Request public ABI - should they in fact go into the
> Request::Private class though ?

And while at it, move the rest of the members to the Request::Private
class ?

> >         BufferMap bufferMap_;
> >  
> >         const uint64_t cookie_;
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 6fa1801a0..59fc3fdf9 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -351,16 +351,10 @@ void Request::Private::timeout()
> >   */
> >  Request::Request(Camera *camera, uint64_t cookie)
> >         : Extensible(std::make_unique<Private>(camera)),
> > +         controls_(controls::controls, camera->_d()->validator()),
> > +         metadata_(controls::controls), /* \todo Add a validator for metadata controls. */
> >           cookie_(cookie), status_(RequestPending)
> >  {
> > -       controls_ = new ControlList(controls::controls,
> > -                                   camera->_d()->validator());
> > -
> > -       /**
> > -        * \todo Add a validator for metadata controls.
> > -        */
> > -       metadata_ = new ControlList(controls::controls);
> > -
> >         LIBCAMERA_TRACEPOINT(request_construct, this);
> >  
> >         LOG(Request, Debug) << "Created request - cookie: " << cookie_;
> > @@ -369,9 +363,6 @@ Request::Request(Camera *camera, uint64_t cookie)
> >  Request::~Request()
> >  {
> >         LIBCAMERA_TRACEPOINT(request_destroy, this);
> > -
> > -       delete metadata_;
> > -       delete controls_;
> >  }
> >  
> >  /**
> > @@ -402,8 +393,8 @@ void Request::reuse(ReuseFlag flags)
> >  
> >         status_ = RequestPending;
> >  
> > -       controls_->clear();
> > -       metadata_->clear();
> > +       controls_.clear();
> > +       metadata_.clear();
> >  }
> >  
> >  /**

Hi


2025. 03. 16. 13:19 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-03-10 17:03:43)
>> The lifetimes of these two `ControlList`s are tied entirely to the request
>> object, so simplify the code by making them member variables instead of
>> manually managing their dynamic lifetime.
>>
> 
> This one looks reasonable, but I suspect it changes the ABI in some
> form?
> 
> Could you run
> 
> ./utils/abi-compat.sh
> 
> And check the report please? I'd like to ensure that any patches which
> affect ABI or API breakage include that statement in the commit message
> so we can identify them in the release notes. (Another candidate for the
> CI builds too of course)

It definitely changes the ABI and API.


> 
> --
> Kieran
> 
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/request.h |  8 ++++----
>>   src/libcamera/request.cpp   | 17 ++++-------------
>>   2 files changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>> index e214a9d13..3061e2fb0 100644
>> --- a/include/libcamera/request.h
>> +++ b/include/libcamera/request.h
>> @@ -49,8 +49,8 @@ public:
>>   
>>          void reuse(ReuseFlag flags = Default);
>>   
>> -       ControlList &controls() { return *controls_; }
>> -       ControlList &metadata() { return *metadata_; }
>> +       ControlList &controls() { return controls_; }
>> +       ControlList &metadata() { return metadata_; }
>>          const BufferMap &buffers() const { return bufferMap_; }
>>          int addBuffer(const Stream *stream, FrameBuffer *buffer,
>>                        std::unique_ptr<Fence> fence = nullptr);
>> @@ -67,8 +67,8 @@ public:
>>   private:
>>          LIBCAMERA_DISABLE_COPY(Request)
>>   
>> -       ControlList *controls_;
>> -       ControlList *metadata_;
>> +       ControlList controls_;
>> +       ControlList metadata_;
> 
> If we're modifying Request public ABI - should they in fact go into the
> Request::Private class though ?

That's also an option, I went with this because this was the "simplest simplification".


Regards,
Barnabás Pőcze


> 
>>          BufferMap bufferMap_;
>>   
>>          const uint64_t cookie_;
>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>> index 6fa1801a0..59fc3fdf9 100644
>> --- a/src/libcamera/request.cpp
>> +++ b/src/libcamera/request.cpp
>> @@ -351,16 +351,10 @@ void Request::Private::timeout()
>>    */
>>   Request::Request(Camera *camera, uint64_t cookie)
>>          : Extensible(std::make_unique<Private>(camera)),
>> +         controls_(controls::controls, camera->_d()->validator()),
>> +         metadata_(controls::controls), /* \todo Add a validator for metadata controls. */
>>            cookie_(cookie), status_(RequestPending)
>>   {
>> -       controls_ = new ControlList(controls::controls,
>> -                                   camera->_d()->validator());
>> -
>> -       /**
>> -        * \todo Add a validator for metadata controls.
>> -        */
>> -       metadata_ = new ControlList(controls::controls);
>> -
>>          LIBCAMERA_TRACEPOINT(request_construct, this);
>>   
>>          LOG(Request, Debug) << "Created request - cookie: " << cookie_;
>> @@ -369,9 +363,6 @@ Request::Request(Camera *camera, uint64_t cookie)
>>   Request::~Request()
>>   {
>>          LIBCAMERA_TRACEPOINT(request_destroy, this);
>> -
>> -       delete metadata_;
>> -       delete controls_;
>>   }
>>   
>>   /**
>> @@ -402,8 +393,8 @@ void Request::reuse(ReuseFlag flags)
>>   
>>          status_ = RequestPending;
>>   
>> -       controls_->clear();
>> -       metadata_->clear();
>> +       controls_.clear();
>> +       metadata_.clear();
>>   }
>>   
>>   /**
>> -- 
>> 2.48.1
>>