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

[Laurent Pinchart]

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 at 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();
> >  }
> >  
> >  /**

-- 
Regards,

Laurent Pinchart