[libcamera-devel,v2,11/13] android: camera_device: Use CameraStream buffers
diff mbox series

Message ID 20201006144432.22908-12-jacopo@jmondi.org
State Accepted
Headers show
Series
  • CameraStream refactor
Related show

Commit Message

Jacopo Mondi Oct. 6, 2020, 2:44 p.m. UTC 
Now that CameraStream that require internal memory allocation
have been instrumented with a FrameBuffer pool, use them to create
intermediate buffers in the CameraDevice.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 51 ++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 10 deletions(-)

Comments

Kieran Bingham Oct. 6, 2020, 8:01 p.m. UTC | #1 
Hi Jacopo,

On 06/10/2020 15:44, Jacopo Mondi wrote:
> Now that CameraStream that require internal memory allocation
> have been instrumented with a FrameBuffer pool, use them to create
> intermediate buffers in the CameraDevice.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 51 ++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ecdc0922e90b..b2fec7040038 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1387,24 +1387,48 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>  
> -		/* Software streams are handled after hardware streams complete. */
> -		if (cameraStream->camera3Stream().format == HAL_PIXEL_FORMAT_BLOB)
> -			continue;
> -
>  		/*
> -		 * Create a libcamera buffer using the dmabuf descriptors of
> -		 * the camera3Buffer for each stream. The FrameBuffer is
> -		 * directly associated with the Camera3RequestDescriptor for
> -		 * lifetime management only.
> +		 * Inspect the camera stream type, create buffers opportunely
> +		 * and add them to the Request if required.
>  		 */
> -		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> +		FrameBuffer *buffer;
> +		switch (cameraStream->type()) {
> +		case CameraStream::Type::Mapped:
> +			/*
> +			 * Mapped streams don't need buffers added to the
> +			 * Request.
> +			 */
> +			continue;
> +
> +		case CameraStream::Type::Direct:
> +			/*
> +			 * Create a libcamera buffer using the dmabuf
> +			 * descriptors of the camera3Buffer for each stream and
> +			 * associate it with the Camera3RequestDescriptor for
> +			 * lifetime management only.
> +			 */
> +			buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> +			descriptor->frameBuffers.emplace_back(buffer);
> +			break;
> +
> +		case CameraStream::Type::Internal:
> +			/*
> +			 * Get the frame buffer from the CameraStream internal
> +			 * buffer pool.
> +			 *
> +			 * The buffer has to be returned to the CameraStream
> +			 * once it has been processed.
> +			 */
> +			buffer = cameraStream->getBuffer();
> +			break;
> +		}
> +
>  		if (!buffer) {
>  			LOG(HAL, Error) << "Failed to create buffer";
>  			delete request;
>  			delete descriptor;
>  			return -ENOMEM;
>  		}
> -		descriptor->frameBuffers.emplace_back(buffer);
>  
>  		request->addBuffer(cameraStream->stream(), buffer);
>  	}
> @@ -1476,6 +1500,13 @@ void CameraDevice::requestComplete(Request *request)
>  			status = CAMERA3_BUFFER_STATUS_ERROR;
>  			continue;
>  		}
> +
> +		/*
> +		 * Return the FrameBuffer to the CameraStream now that we're
> +		 * done processing it.
> +		 */
> +		if (cameraStream->type() == CameraStream::Type::Internal)
> +			cameraStream->putBuffer(buffer);
>  	}
>  
>  	/* Prepare to call back the Android camera stack. */
>
Umang Jain Oct. 7, 2020, 8:05 a.m. UTC | #2 
Hi Jacopo,

On 10/6/20 8:14 PM, Jacopo Mondi wrote:
> Now that CameraStream that require internal memory allocation
> have been instrumented with a FrameBuffer pool, use them to create
> intermediate buffers in the CameraDevice.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   src/android/camera_device.cpp | 51 ++++++++++++++++++++++++++++-------
>   1 file changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ecdc0922e90b..b2fec7040038 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1387,24 +1387,48 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>   		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>   
> -		/* Software streams are handled after hardware streams complete. */
> -		if (cameraStream->camera3Stream().format == HAL_PIXEL_FORMAT_BLOB)
> -			continue;
> -
>   		/*
> -		 * Create a libcamera buffer using the dmabuf descriptors of
> -		 * the camera3Buffer for each stream. The FrameBuffer is
> -		 * directly associated with the Camera3RequestDescriptor for
> -		 * lifetime management only.
> +		 * Inspect the camera stream type, create buffers opportunely
> +		 * and add them to the Request if required.
>   		 */
> -		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> +		FrameBuffer *buffer;
I would not leave this un-initialize and initialize this to nullptr 
anyway (So that the `if (!buffer)` below is more tight). For now, buffer 
will get automatically get assigned to nullptr in the relevant 
switch-case-block on any errors though, so this is not really a blocker.

Thanks.
Reviewed-by: Umang Jain <email@uajain.com>
> +		switch (cameraStream->type()) {
> +		case CameraStream::Type::Mapped:
> +			/*
> +			 * Mapped streams don't need buffers added to the
> +			 * Request.
> +			 */
> +			continue;
> +
> +		case CameraStream::Type::Direct:
> +			/*
> +			 * Create a libcamera buffer using the dmabuf
> +			 * descriptors of the camera3Buffer for each stream and
> +			 * associate it with the Camera3RequestDescriptor for
> +			 * lifetime management only.
> +			 */
> +			buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> +			descriptor->frameBuffers.emplace_back(buffer);
> +			break;
> +
> +		case CameraStream::Type::Internal:
> +			/*
> +			 * Get the frame buffer from the CameraStream internal
> +			 * buffer pool.
> +			 *
> +			 * The buffer has to be returned to the CameraStream
> +			 * once it has been processed.
> +			 */
> +			buffer = cameraStream->getBuffer();
> +			break;
> +		}
> +
>   		if (!buffer) {
>   			LOG(HAL, Error) << "Failed to create buffer";
>   			delete request;
>   			delete descriptor;
>   			return -ENOMEM;
>   		}
> -		descriptor->frameBuffers.emplace_back(buffer);
>   
>   		request->addBuffer(cameraStream->stream(), buffer);
>   	}
> @@ -1476,6 +1500,13 @@ void CameraDevice::requestComplete(Request *request)
>   			status = CAMERA3_BUFFER_STATUS_ERROR;
>   			continue;
>   		}
> +
> +		/*
> +		 * Return the FrameBuffer to the CameraStream now that we're
> +		 * done processing it.
> +		 */
> +		if (cameraStream->type() == CameraStream::Type::Internal)
> +			cameraStream->putBuffer(buffer);
>   	}
>   
>   	/* Prepare to call back the Android camera stack. */
Kieran Bingham Oct. 7, 2020, 9:01 a.m. UTC | #3 
Hello,

On 07/10/2020 09:05, Umang Jain wrote:
> Hi Jacopo,
> 
> On 10/6/20 8:14 PM, Jacopo Mondi wrote:
>> Now that CameraStream that require internal memory allocation
>> have been instrumented with a FrameBuffer pool, use them to create
>> intermediate buffers in the CameraDevice.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>   src/android/camera_device.cpp | 51 ++++++++++++++++++++++++++++-------
>>   1 file changed, 41 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp
>> b/src/android/camera_device.cpp
>> index ecdc0922e90b..b2fec7040038 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1387,24 +1387,48 @@ int
>> CameraDevice::processCaptureRequest(camera3_capture_request_t
>> *camera3Reques
>>           descriptor->buffers[i].stream = camera3Buffers[i].stream;
>>           descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>>   -        /* Software streams are handled after hardware streams
>> complete. */
>> -        if (cameraStream->camera3Stream().format ==
>> HAL_PIXEL_FORMAT_BLOB)
>> -            continue;
>> -
>>           /*
>> -         * Create a libcamera buffer using the dmabuf descriptors of
>> -         * the camera3Buffer for each stream. The FrameBuffer is
>> -         * directly associated with the Camera3RequestDescriptor for
>> -         * lifetime management only.
>> +         * Inspect the camera stream type, create buffers opportunely
>> +         * and add them to the Request if required.
>>            */
>> -        FrameBuffer *buffer =
>> createFrameBuffer(*camera3Buffers[i].buffer);
>> +        FrameBuffer *buffer;
> I would not leave this un-initialize and initialize this to nullptr
> anyway (So that the `if (!buffer)` below is more tight). For now, buffer
> will get automatically get assigned to nullptr in the relevant
> switch-case-block on any errors though, so this is not really a blocker.
> 

Good spot.

Indeed, this doesn't get used uninitialised, as the
CameraStream::Type::Mapped will hit the continue statement, but I think
initialisign to nullptr would be safer code, protecting against any
future changes in here that might occur.

Agreed, not a blocker, but possibly a nice-to-have.
--
Kieran


> Thanks.
> Reviewed-by: Umang Jain <email@uajain.com>
>> +        switch (cameraStream->type()) {
>> +        case CameraStream::Type::Mapped:
>> +            /*
>> +             * Mapped streams don't need buffers added to the
>> +             * Request.
>> +             */
>> +            continue;
>> +
>> +        case CameraStream::Type::Direct:
>> +            /*
>> +             * Create a libcamera buffer using the dmabuf
>> +             * descriptors of the camera3Buffer for each stream and
>> +             * associate it with the Camera3RequestDescriptor for
>> +             * lifetime management only.
>> +             */
>> +            buffer = createFrameBuffer(*camera3Buffers[i].buffer);
>> +            descriptor->frameBuffers.emplace_back(buffer);
>> +            break;
>> +
>> +        case CameraStream::Type::Internal:
>> +            /*
>> +             * Get the frame buffer from the CameraStream internal
>> +             * buffer pool.
>> +             *
>> +             * The buffer has to be returned to the CameraStream
>> +             * once it has been processed.
>> +             */
>> +            buffer = cameraStream->getBuffer();
>> +            break;
>> +        }
>> +
>>           if (!buffer) {
>>               LOG(HAL, Error) << "Failed to create buffer";
>>               delete request;
>>               delete descriptor;
>>               return -ENOMEM;
>>           }
>> -        descriptor->frameBuffers.emplace_back(buffer);
>>             request->addBuffer(cameraStream->stream(), buffer);
>>       }
>> @@ -1476,6 +1500,13 @@ void CameraDevice::requestComplete(Request
>> *request)
>>               status = CAMERA3_BUFFER_STATUS_ERROR;
>>               continue;
>>           }
>> +
>> +        /*
>> +         * Return the FrameBuffer to the CameraStream now that we're
>> +         * done processing it.
>> +         */
>> +        if (cameraStream->type() == CameraStream::Type::Internal)
>> +            cameraStream->putBuffer(buffer);
>>       }
>>         /* Prepare to call back the Android camera stack. */
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index ecdc0922e90b..b2fec7040038 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1387,24 +1387,48 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		descriptor->buffers[i].stream = camera3Buffers[i].stream;
 		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
 
-		/* Software streams are handled after hardware streams complete. */
-		if (cameraStream->camera3Stream().format == HAL_PIXEL_FORMAT_BLOB)
-			continue;
-
 		/*
-		 * Create a libcamera buffer using the dmabuf descriptors of
-		 * the camera3Buffer for each stream. The FrameBuffer is
-		 * directly associated with the Camera3RequestDescriptor for
-		 * lifetime management only.
+		 * Inspect the camera stream type, create buffers opportunely
+		 * and add them to the Request if required.
 		 */
-		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
+		FrameBuffer *buffer;
+		switch (cameraStream->type()) {
+		case CameraStream::Type::Mapped:
+			/*
+			 * Mapped streams don't need buffers added to the
+			 * Request.
+			 */
+			continue;
+
+		case CameraStream::Type::Direct:
+			/*
+			 * Create a libcamera buffer using the dmabuf
+			 * descriptors of the camera3Buffer for each stream and
+			 * associate it with the Camera3RequestDescriptor for
+			 * lifetime management only.
+			 */
+			buffer = createFrameBuffer(*camera3Buffers[i].buffer);
+			descriptor->frameBuffers.emplace_back(buffer);
+			break;
+
+		case CameraStream::Type::Internal:
+			/*
+			 * Get the frame buffer from the CameraStream internal
+			 * buffer pool.
+			 *
+			 * The buffer has to be returned to the CameraStream
+			 * once it has been processed.
+			 */
+			buffer = cameraStream->getBuffer();
+			break;
+		}
+
 		if (!buffer) {
 			LOG(HAL, Error) << "Failed to create buffer";
 			delete request;
 			delete descriptor;
 			return -ENOMEM;
 		}
-		descriptor->frameBuffers.emplace_back(buffer);
 
 		request->addBuffer(cameraStream->stream(), buffer);
 	}
@@ -1476,6 +1500,13 @@  void CameraDevice::requestComplete(Request *request)
 			status = CAMERA3_BUFFER_STATUS_ERROR;
 			continue;
 		}
+
+		/*
+		 * Return the FrameBuffer to the CameraStream now that we're
+		 * done processing it.
+		 */
+		if (cameraStream->type() == CameraStream::Type::Internal)
+			cameraStream->putBuffer(buffer);
 	}
 
 	/* Prepare to call back the Android camera stack. */