[libcamera-devel] [PATCH v5 7/8] android: camera_device: Add buffers for each stream to Requests
Jacopo Mondi
jacopo at jmondi.org
Tue Jul 7 11:29:13 CEST 2020
Hi Kieran
On Tue, Jul 07, 2020 at 10:14:48AM +0100, Kieran Bingham wrote:
> 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.
As you wish, it's really only a suggestion
>
> >> +
> >> /*
> >> * 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.
> """
Good, thanks!
>
>
>
>
> > 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