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

[Barnabás Pőcze]

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

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