[libcamera-devel] [PATCH v3.1 7/8] android: camera_device: Add buffers for each stream to Requests
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jul 6 11:30:36 CEST 2020
Hi Jacopo,
On 06/07/2020 09:03, Jacopo Mondi wrote:
> Hi Kieran,
>
> On Fri, Jul 03, 2020 at 03:54:04PM +0100, Kieran Bingham wrote:
>> Construct a FrameBuffer for every buffer given in the camera3Request
>> and add it to the libcamera Request on the appropriate stream.
>>
>> The correct stream is obtained from the private data of the camera3_stream
>> associated with the camera3_buffer.
>>
>> Comments regarding supporting only one buffer are now removed.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>> src/android/camera_device.cpp | 58 ++++++++++++++++++++---------------
>> 1 file changed, 33 insertions(+), 25 deletions(-)
>>
>> Rather than resend a v4 just for the change in this patch - sending a
>> v3.1 for direct review of this one.
>>
>> Within requestComplete(), the 'first' buffer pointer, which is needed to
>> extract the timestamp is moved to directly before it is used, and a
>> comment added to highlight that this is using a single buffer purely for
>> the timestamp.
>>
>> Then, after calling the process_capture_result callback to the android
>> hal, we iterate all buffers in the request and delete them to prevent
>> leaks.
>>
>>
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 28334751a26e..72be4ab86e50 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -991,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>
>> /* Maintain internal state of all stream mappings. */
>> streams_[i].index = streamIndex++;
>> + stream->priv = static_cast<void *>(&streams_[i]);
>> }
>>
>> switch (config_->validate()) {
>> @@ -1049,9 +1050,6 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>>
>> int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>> {
>> - StreamConfiguration *streamConfiguration = &config_->at(0);
>> - Stream *stream = streamConfiguration->stream();
>> -
>> if (camera3Request->num_output_buffers != 1) {
>> LOG(HAL, Error) << "Invalid number of output buffers: "
>> << camera3Request->num_output_buffers;
>> @@ -1089,30 +1087,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>> camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>>
>> for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>> + CameraStream *cameraStream = static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
>> +
>> /*
>> * Keep track of which stream the request belongs to and store
>> * the native buffer handles.
>> - *
>> - * \todo Currently we only support one capture buffer. Copy
>> - * all of them to be ready once we'll support more.
>> */
>> descriptor->buffers[i].stream = camera3Buffers[i].stream;
>> descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>> - }
>>
>> - /*
>> - * Create a libcamera buffer using the dmabuf descriptors of the first
>> - * and (currently) only supported request buffer.
>> - */
>> - FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
>> - if (!buffer) {
>> - LOG(HAL, Error) << "Failed to create buffer";
>> - delete request;
>> - delete descriptor;
>> - return -ENOMEM;
>> - }
>> + /*
>> + * Create a libcamera buffer using the dmabuf descriptors of the
>> + * first and (currently) only supported request buffer.
>> + */
>> + FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[0].buffer);
>> + if (!buffer) {
>> + LOG(HAL, Error) << "Failed to create buffer";
>> + delete request;
>> + delete descriptor;
>> + return -ENOMEM;
>> + }
>>
>> - request->addBuffer(stream, buffer);
>> + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
>> + Stream *stream = streamConfiguration->stream();
>> +
>> + request->addBuffer(stream, buffer);
>> + }
>>
>> int ret = camera_->queueRequest(request);
>
> I still don't see the framebuffers added to the request been deleted
> if queuing the request fails. We delete the request, but the class
> destructor does not destroy the associated buffer. I understand this
> was already the case, so it might be fixed on top.
Argh, maybe it could be fixed on top, but now I've seen it I can't unsee it.
How about we store a vector of the buffers created by createFrameBuffer
in the Camera3RequestDescriptor so we can delete them on destruction of
that ?
Then it's a bit more automatic and tracked along with the request, which
is the same lifetime...
>> if (ret) {
>> @@ -1128,7 +1128,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>> void CameraDevice::requestComplete(Request *request)
>> {
>> const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>> - FrameBuffer *buffer = buffers.begin()->second;
>> camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>> std::unique_ptr<CameraMetadata> resultMetadata;
>>
>> @@ -1146,10 +1145,6 @@ void CameraDevice::requestComplete(Request *request)
>> captureResult.frame_number = descriptor->frameNumber;
>> captureResult.num_output_buffers = descriptor->numBuffers;
>> for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>> - /*
>> - * \todo Currently we only support one capture buffer. Prepare
>> - * all of them to be ready once we'll support more.
>> - */
>> descriptor->buffers[i].acquire_fence = -1;
>> descriptor->buffers[i].release_fence = -1;
>> descriptor->buffers[i].status = status;
>> @@ -1157,6 +1152,14 @@ void CameraDevice::requestComplete(Request *request)
>> captureResult.output_buffers =
>> const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
>>
>> + /*
>> + * \todo The timestamp used for the metadata is currently always taken
>> + * from the first buffer (which may be the first stream) in the Request.
>> + * It might be appropriate to return a 'correct' (as determined by
>> + * pipeline handlers) timestamp in the Request itself.
>> + */
>> + FrameBuffer *buffer = buffers.begin()->second;
>> +
>> if (status == CAMERA3_BUFFER_STATUS_OK) {
>> notifyShutter(descriptor->frameNumber,
>> buffer->metadata().timestamp);
>
> This shall also be changed soon. We use the buffer timestamp to notify
> the shutter event. We currently don't have a way to notify the HAL
> layer that an SOF even has been emitted from the device, and so we use
> the request complete handler to also notify the shutter event, which
> might (and should) be sent to the framework as early as possible.
>
> More to fix on top.
>
> With the handling of buffers in case of queueRequest failue clarified
I think the simplest way to handle the failure case as well is to make
sure the Camera3RequestDescriptor tracks the lifetimes.
I'll make a v4 with that included.
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
And grab this, but I'll away a final confirmation/check on the v4 before
integration anyway.
--
Thanks
Kieran
>
> Thanks
> j
>
>> @@ -1180,8 +1183,13 @@ void CameraDevice::requestComplete(Request *request)
>>
>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>
>> + /* Release all buffers created with createFrameBuffer(). */
>> + for (auto it : buffers) {
>> + FrameBuffer *buffer = it.second;
>> + delete buffer;
>> + }
>> +
>> delete descriptor;
>> - delete buffer;
>> }
>>
>> std::string CameraDevice::logPrefix() const
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list