[libcamera-devel] [PATCH v5 7/8] android: camera_device: Add buffers for each stream to Requests
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 7 11:14:48 CEST 2020
Hi Jacopo,
On 07/07/2020 08:47, Jacopo Mondi wrote:
> Hi Kieran,
> two nits you can fix when applying
>
> On Tue, Jul 07, 2020 at 12:02:39AM +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, and
>> FrameBuffers have their lifetime tracked in the Camera3RequestDescriptor
>> to ensure they are released when the Request is completed.
>>
>> 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>
>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>> src/android/camera_device.cpp | 54 +++++++++++++++++++----------------
>> src/android/camera_device.h | 1 +
>> 2 files changed, 31 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 6d060e0c40e5..b13729e84abf 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -98,6 +98,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>> : frameNumber(frameNumber), numBuffers(numBuffers)
>> {
>> buffers = new camera3_stream_buffer_t[numBuffers];
>> + frameBuffers.reserve(numBuffers);
>> }
>>
>> CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
>> @@ -990,6 +991,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>> config_->addConfiguration(streamConfiguration);
>>
>> streams_[i].index = streamIndex++;
>> + stream->priv = static_cast<void *>(&streams_[i]);
>> }
>>
>> switch (config_->validate()) {
>> @@ -1048,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;
>> @@ -1088,30 +1087,35 @@ 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);
>
> This is really long. I would break it after =
>
:( I think I prefer the single line, but I can do so.
Even formatting as:
> for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> CameraStream *cameraStream =
> static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
goes up to 85 chars.
Do you think the line break really helps readability ?
For me adding new lines looks awkward, as it turns a single line
statement into ... well ... two lines ;-)
Anyway, I don't want to spend time worrying about line lengths.
I'll apply this with the newline as shown above.
>> +
>> /*
>> * 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";
>> + /*
>> + * Create a libcamera buffer using the dmabuf descriptors of the
>> + * first and (currently) only supported request buffer.
>
> This is not true anymore, isn't it ?
Good point, I'll fix this as: (combining both the original statement and
my addition):
"""
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.
"""
> The rest looks good as the whole series does!
> Thanks
> j
>
>> + * The FrameBuffer is directly associated with the
>> + * Camera3RequestDescriptor for lifetime management only.
>> + */
>> + FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
>> + if (!buffer) {
>> + LOG(HAL, Error) << "Failed to create buffer";
>> delete request;
>> - delete descriptor;
>> - return -ENOMEM;
>> - }
>> + delete descriptor;
>> + return -ENOMEM;
>> + }
>> + descriptor->frameBuffers.emplace_back(buffer);
>>
>> - request->addBuffer(stream, buffer);
>> + StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
>> + Stream *stream = streamConfiguration->stream();
>> +
>> + request->addBuffer(stream, buffer);
>> + }
>>
>> int ret = camera_->queueRequest(request);
>> if (ret) {
>> @@ -1127,7 +1131,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;
>>
>> @@ -1145,10 +1148,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;
>> @@ -1156,6 +1155,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);
>> @@ -1180,7 +1187,6 @@ void CameraDevice::requestComplete(Request *request)
>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>
>> delete descriptor;
>> - delete buffer;
>> }
>>
>> std::string CameraDevice::logPrefix() const
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index d00f617b09a6..5b8b9c3e26e2 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -70,6 +70,7 @@ private:
>> uint32_t frameNumber;
>> uint32_t numBuffers;
>> camera3_stream_buffer_t *buffers;
>> + std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
>> };
>>
>> struct Camera3StreamConfiguration {
>> --
>> 2.25.1
>>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list