[libcamera-devel,v3,5/8] android: camera_device: Create the Request and Camera3RequestDescriptor together
diff mbox series

Message ID 20200703123919.2223048-6-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • android: Multi-stream support
Related show

Commit Message

Kieran Bingham July 3, 2020, 12:39 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/android/camera_device.cpp | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart July 3, 2020, 2:32 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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;
>  		return -ENOMEM;
>  	}
>  
> -	Request *request =
> -		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>  	request->addBuffer(stream, buffer);
>  
>  	int ret = camera_->queueRequest(request);
Jacopo Mondi July 6, 2020, 8:05 a.m. UTC | #2 
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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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.

Anywya
Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham July 6, 2020, 9:36 a.m. UTC | #3 
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@ideasonboard.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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?

--
Kieran


> Anywya
> Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi July 6, 2020, 10:25 a.m. UTC | #4 
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@ideasonboard.com>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund@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 :)

Would you like to send a v4 or fixes on top ?

>
> --
> Kieran
>
>
> > Anywya
> > Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran
Kieran Bingham July 6, 2020, 10:28 p.m. UTC | #5 
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@ideasonboard.com>
>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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@lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran

Patch
diff mbox series 

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;
 		return -ENOMEM;
 	}
 
-	Request *request =
-		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
 	request->addBuffer(stream, buffer);
 
 	int ret = camera_->queueRequest(request);