[libcamera-devel] [PATCH v3 5/8] android: camera_device: Create the Request and Camera3RequestDescriptor together

[Kieran Bingham]

Hi Jacopo,

On 06/07/2020 11:25, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Jul 06, 2020 at 10:36:33AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 06/07/2020 09:05, Jacopo Mondi wrote:
>>> On Fri, Jul 03, 2020 at 01:39:16PM +0100, Kieran Bingham wrote:
>>>> Move the construction of the Request higher in the code flow
>>>> so that multiple buffers and streams can be added where required.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>> ---
>>>>  src/android/camera_device.cpp | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>>> index 73cfab532cf5..4e77a92dbea5 100644
>>>> --- a/src/android/camera_device.cpp
>>>> +++ b/src/android/camera_device.cpp
>>>> @@ -1066,6 +1066,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>  	Camera3RequestDescriptor *descriptor =
>>>>  		new Camera3RequestDescriptor(camera3Request->frame_number,
>>>>  					     camera3Request->num_output_buffers);
>>>> +
>>>> +	Request *request =
>>>> +		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>>>> +
>>>>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>>>>  		/*
>>>>  		 * Keep track of which stream the request belongs to and store
>>>> @@ -1085,12 +1089,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>  	FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
>>>>  	if (!buffer) {
>>>>  		LOG(HAL, Error) << "Failed to create buffer";
>>>> +		delete request;
>>>>  		delete descriptor;
>>>
>>> This is a replic of the below queueRequest() error path. Might be
>>> worth creating a label and jump to it.
>>>
>>
>> Given the need to add the FrameBuffer's to the Camera3RequestDescriptor,
>> how about we add a unique_ptr<Request *> to that, so that we only have
>> to track the lifetime of the Camera3RequestDescriptor manually?
> 
> Seem like a good idea :)

Turns out it was a really bad idea ;-)

I had forgotten that the Request is deleted by the libcamera core after
completion, so by tracking the Request as a unique object, I introduced
a double-free.

Ooops.

So we should only delete Requests in this function if they are not
successfully submitted.

--
v5....

> 
> Would you like to send a v4 or fixes on top ?
> 
>>
>> --
>> Kieran
>>
>>
>>> Anywya
>>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>>>
>>> Thanks
>>>    j
>>>
>>>>  		return -ENOMEM;
>>>>  	}
>>>>
>>>> -	Request *request =
>>>> -		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>>>>  	request->addBuffer(stream, buffer);
>>>>
>>>>  	int ret = camera_->queueRequest(request);
>>>> --
>>>> 2.25.1
>>>>
>>>> _______________________________________________
>>>> libcamera-devel mailing list
>>>> libcamera-devel at lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran