[libcamera-devel] [RFC v2 2/4] HACK: expose Camera* from Request

[Tomi Valkeinen]

On 30/11/2020 01:42, Laurent Pinchart wrote:
> Hi Kieran and Tomi,
> 
> On Sun, Nov 29, 2020 at 09:08:28PM +0000, Kieran Bingham wrote:
>> On 27/11/2020 13:37, Tomi Valkeinen wrote:
>>> Py bindings need to find out which camera is the source of the completed
>>> Request. Request already has a private field for the Camera, so we can
>>> just expose it via a getter.
> 
> Note that the camera_ member is actually not used. We should either use
> it, or remove it :-)
> 
>> Interestingly, some time ago I posted a similar (but simpler?) patch for
>> this.
>>
>> [PATCH] libcamera: request: Facilitate retrieval of the camera
>>
>> https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/012064.html
>>
>> Which was partially rejected by Niklas, however he did give me an RB tag.
>>
>> I sort of thought this was beneficial as it helps get directly to the
>> 'correct' Camera in any callbacks.
>>
>> Interesting that you've used a shared_ptr rather than where I directly
>> pass the pointer, and I think that's likely more correct as the expected
>> use might be to take the event in a callback, and immediately pass it on
>> to another thread - so a shared pointer makes a bit more sense then to
>> keep things safe.
> 
> I'm not entirely sure about that. shared_ptr<> has a tendency to spread,
> and sometimes borrowed references are enough. In this particular case,
> requests should all complete before the application receives the camera
> disconnection signal. Borrowed references to the camera in the
> completion handler should thus be valid. Request processing is then
> typically deferred to the main thread, but for cancelled requests, a
> different code path can be used.

We can return Camera* here, and I can convert it to a shared_ptr in the binding code.

I don't know what you mean with "has tendency to spread". The camera is allocated as shared_ptr, so
generally speaking, it should always be shared_ptr<Camera>.

In some cases we could pass it as Camera* as an optimization, e.g. for a function that we know only
does a thing X, and does not store the Camera pointer anywhere. Storing a Camera* is not correct in
my opinion, although it can of course work if the lifetime or that pointer is well defined and
enforced. But it's pretty easy to get that wrong, and it should be considered if such optimization
is worth it (maybe it is here with Requests).

But when talking about the python bindings, a Camera* is not ok, we can't pass such a thing to
python. Either python owns the instance (unique_ptr, which we can create from a pointer) or it
doesn't (shared_ptr). Well, we can also build more elaborate ownership containers, but the rules
must be clear and defined in the container code.

In this case, as Camera is a shared_ptr, and we store the Camera to a list which is processed later,
I think it's clear that we should use shared_ptr (either as I do in this patch, or convert Camera*
to shared_ptr<Camera> in the binding code).

> I'll review the patch that makes use of this to see what's best.
> 
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
>>> ---
>>>  include/libcamera/request.h | 1 +
>>>  src/libcamera/request.cpp   | 5 +++++
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>>> index 655b1324..f98ef767 100644
>>> --- a/include/libcamera/request.h
>>> +++ b/include/libcamera/request.h
>>> @@ -56,6 +56,7 @@ public:
>>>  
>>>  	bool hasPendingBuffers() const { return !pending_.empty(); }
>>>  
>>> +        std::shared_ptr<Camera> camera() const;
> 
> Doesn't checkstyle.py warn about spaces uses for indentation ? There's
> also a missing blank line.

It does, I just didn't run it... Not sure how I managed to indent with spaces, though.

 Tomi