| Message ID | 20211019114802.665980-10-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi On Tue, Oct 19, 2021 at 05:17:59PM +0530, Umang Jain wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The camera3_stream_buffer_t structure is meant to communicate between > the camera service and the HAL. They are short-live structures that > don't outlive the .process_capture_request() operation (when queuing > requests) or the .process_capture_result() callback. > > We currently store copies of the camera3_stream_buffer_t passed to > .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to > store the structure members that the HAL need, and reuse them when > calling the .process_capture_result() callback. This is conceptually not > right, as the camera3_stream_buffer_t pass to the callback are not the > same objects as the ones received in .process_capture_request(). > > Store individual fields of the camera3_stream_buffer_t in StreamBuffer > instead of copying the whole structure. This gives the HAL full control > of how data is stored, and properly decouples request queueing from > result reporting. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com> I'm not sure I get what is functionally different, we were copying in the received camera3_stream_buffer_t, so there was no dependency with the ones passed in by the service. Anyway, it shortne names, so I think it's good :) Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/android/camera_device.cpp | 73 ++++++++++++++++++---------------- > src/android/camera_request.cpp | 19 +++++++-- > src/android/camera_request.h | 12 +++--- > src/android/camera_stream.cpp | 6 +-- > 4 files changed, 63 insertions(+), 47 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 0bb547ae..0a2d3826 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > { > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > - for (auto &buffer : descriptor->buffers_) { > - /* > - * Signal to the framework it has to handle fences that have not > - * been waited on by setting the release fence to the acquire > - * fence value. > - */ > - buffer.buffer.release_fence = buffer.buffer.acquire_fence; > - buffer.buffer.acquire_fence = -1; > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > - } > + for (auto &buffer : descriptor->buffers_) > + buffer.status = Camera3RequestDescriptor::Status::Error; > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } > @@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > << " with " << descriptor->buffers_.size() << " streams"; > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > - camera3_stream *camera3Stream = buffer.buffer.stream; > - CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > + CameraStream *cameraStream = buffer.stream; > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > std::stringstream ss; > ss << i << " - (" << camera3Stream->width << "x" > @@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * lifetime management only. > */ > buffer.frameBuffer = > - createFrameBuffer(*buffer.buffer.buffer, > + createFrameBuffer(*buffer.camera3Buffer, > cameraStream->configuration().pixelFormat, > cameraStream->configuration().size); > frameBuffer = buffer.frameBuffer.get(); > - acquireFence = buffer.buffer.acquire_fence; > + acquireFence = buffer.fence; > LOG(HAL, Debug) << ss.str() << " (direct)"; > break; > > @@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request) > /* > * Prepare the capture result for the Android camera stack. > * > - * The buffer status is set to OK and later changed to ERROR if > + * The buffer status is set to Success and later changed to Error if > * post-processing/compression fails. > */ > for (auto &buffer : descriptor->buffers_) { > - CameraStream *cameraStream = > - static_cast<CameraStream *>(buffer.buffer.stream->priv); > + CameraStream *stream = buffer.stream; > > /* > * Streams of type Direct have been queued to the > @@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request) > * \todo Instrument the CameraWorker to set the acquire > * fence to -1 once it has handled it and remove this check. > */ > - if (cameraStream->type() == CameraStream::Type::Direct) > - buffer.buffer.acquire_fence = -1; > - buffer.buffer.release_fence = -1; > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK; > + if (stream->type() == CameraStream::Type::Direct) > + buffer.fence = -1; > + buffer.status = Camera3RequestDescriptor::Status::Success; > } > > /* > @@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request) > > /* Handle post-processing. */ > for (auto &buffer : descriptor->buffers_) { > - CameraStream *cameraStream = > - static_cast<CameraStream *>(buffer.buffer.stream->priv); > + CameraStream *stream = buffer.stream; > > - if (cameraStream->type() == CameraStream::Type::Direct) > + if (stream->type() == CameraStream::Type::Direct) > continue; > > - FrameBuffer *src = request->findBuffer(cameraStream->stream()); > + FrameBuffer *src = request->findBuffer(stream->stream()); > if (!src) { > LOG(HAL, Error) << "Failed to find a source stream buffer"; > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > - notifyError(descriptor->frameNumber_, buffer.buffer.stream, > + buffer.status = Camera3RequestDescriptor::Status::Error; > + notifyError(descriptor->frameNumber_, stream->camera3Stream(), > CAMERA3_MSG_ERROR_BUFFER); > continue; > } > > - int ret = cameraStream->process(*src, buffer, descriptor); > + int ret = stream->process(*src, buffer, descriptor); > > /* > * Return the FrameBuffer to the CameraStream now that we're > * done processing it. > */ > - if (cameraStream->type() == CameraStream::Type::Internal) > - cameraStream->putBuffer(src); > + if (stream->type() == CameraStream::Type::Internal) > + stream->putBuffer(src); > > if (ret) { > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > - notifyError(descriptor->frameNumber_, buffer.buffer.stream, > + buffer.status = Camera3RequestDescriptor::Status::Error; > + notifyError(descriptor->frameNumber_, stream->camera3Stream(), > CAMERA3_MSG_ERROR_BUFFER); > } > } > @@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults() > captureResult.result = descriptor->resultMetadata_->get(); > > std::vector<camera3_stream_buffer_t> resultBuffers; > - for (const auto &buffer : descriptor->buffers_) > - resultBuffers.emplace_back(buffer.buffer); > + resultBuffers.reserve(descriptor->buffers_.size()); > + > + for (const auto &buffer : descriptor->buffers_) { > + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR; > + > + if (buffer.status == Camera3RequestDescriptor::Status::Success) > + status = CAMERA3_BUFFER_STATUS_OK; > + > + /* > + * Pass the buffer fence back to the camera framework as > + * a release fence. This instructs the framework to wait > + * on the acquire fence in case we haven't done so > + * ourselves for any reason. > + */ > + resultBuffers.push_back({ buffer.stream->camera3Stream(), > + buffer.camera3Buffer, status, > + -1, buffer.fence }); > + } > > captureResult.num_output_buffers = resultBuffers.size(); > captureResult.output_buffers = resultBuffers.data(); > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > index 614baed4..faa85ada 100644 > --- a/src/android/camera_request.cpp > +++ b/src/android/camera_request.cpp > @@ -7,6 +7,8 @@ > > #include "camera_request.h" > > +#include <libcamera/base/span.h> > + > using namespace libcamera; > > /* > @@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > frameNumber_ = camera3Request->frame_number; > > /* Copy the camera3 request stream information for later access. */ > - const uint32_t numBuffers = camera3Request->num_output_buffers; > + const Span<const camera3_stream_buffer_t> buffers{ > + camera3Request->output_buffers, > + camera3Request->num_output_buffers > + }; > + > + buffers_.reserve(buffers.size()); > + > + for (const camera3_stream_buffer_t &buffer : buffers) { > + CameraStream *stream = > + static_cast<CameraStream *>(buffer.stream->priv); > > - buffers_.resize(numBuffers); > - for (uint32_t i = 0; i < numBuffers; i++) > - buffers_[i].buffer = camera3Request->output_buffers[i]; > + buffers_.push_back({ stream, buffer.buffer, nullptr, > + buffer.acquire_fence, Status::Pending }); > + } > > /* Clone the controls associated with the camera3 request. */ > settings_ = CameraMetadata(camera3Request->settings); > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > index a030febf..05dabf89 100644 > --- a/src/android/camera_request.h > +++ b/src/android/camera_request.h > @@ -20,6 +20,8 @@ > #include "camera_metadata.h" > #include "camera_worker.h" > > +class CameraStream; > + > class Camera3RequestDescriptor > { > public: > @@ -30,13 +32,11 @@ public: > }; > > struct StreamBuffer { > - camera3_stream_buffer_t buffer; > - /* > - * FrameBuffer instances created by wrapping a camera3 provided > - * dmabuf are emplaced in this vector of unique_ptr<> for > - * lifetime management. > - */ > + CameraStream *stream; > + buffer_handle_t *camera3Buffer; > std::unique_ptr<libcamera::FrameBuffer> frameBuffer; > + int fence; > + Status status; > }; > > Camera3RequestDescriptor(libcamera::Camera *camera, > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index f3cc77e7..9b5cd0c4 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source, > Camera3RequestDescriptor *request) > { > /* Handle waiting on fences on the destination buffer. */ > - int fence = dest.buffer.acquire_fence; > + int fence = dest.fence; > if (fence != -1) { > int ret = waitFence(fence); > ::close(fence); > - dest.buffer.acquire_fence = -1; > + dest.fence = -1; > if (ret < 0) { > LOG(HAL, Error) << "Failed waiting for fence: " > << fence << ": " << strerror(-ret); > @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source, > * separate thread. > */ > const StreamConfiguration &output = configuration(); > - CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat, > + CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat, > output.size, PROT_READ | PROT_WRITE); > if (!destBuffer.isValid()) { > LOG(HAL, Error) << "Failed to create destination buffer"; > -- > 2.31.0 >
Hi Jacopo, On Tue, Oct 19, 2021 at 02:12:08PM +0200, Jacopo Mondi wrote: > On Tue, Oct 19, 2021 at 05:17:59PM +0530, Umang Jain wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The camera3_stream_buffer_t structure is meant to communicate between > > the camera service and the HAL. They are short-live structures that > > don't outlive the .process_capture_request() operation (when queuing > > requests) or the .process_capture_result() callback. > > > > We currently store copies of the camera3_stream_buffer_t passed to > > .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to > > store the structure members that the HAL need, and reuse them when > > calling the .process_capture_result() callback. This is conceptually not > > right, as the camera3_stream_buffer_t pass to the callback are not the > > same objects as the ones received in .process_capture_request(). > > > > Store individual fields of the camera3_stream_buffer_t in StreamBuffer > > instead of copying the whole structure. This gives the HAL full control > > of how data is stored, and properly decouples request queueing from > > result reporting. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com> > > I'm not sure I get what is functionally different, we were copying in > the received camera3_stream_buffer_t, so there was no dependency with > the ones passed in by the service. We don't use the same instances as we copy them, but we reuse the value, which I don't think is conceptually correct. camera3_stream_buffer_t is meant to pass buffer information in the API, it's not a buffer object in itself, so I don't think it's the right level of abstraction (and we have to extend it with additional fields anyway). > Anyway, it shortne names, so I think it's good :) > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > src/android/camera_device.cpp | 73 ++++++++++++++++++---------------- > > src/android/camera_request.cpp | 19 +++++++-- > > src/android/camera_request.h | 12 +++--- > > src/android/camera_stream.cpp | 6 +-- > > 4 files changed, 63 insertions(+), 47 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 0bb547ae..0a2d3826 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > { > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > - for (auto &buffer : descriptor->buffers_) { > > - /* > > - * Signal to the framework it has to handle fences that have not > > - * been waited on by setting the release fence to the acquire > > - * fence value. > > - */ > > - buffer.buffer.release_fence = buffer.buffer.acquire_fence; > > - buffer.buffer.acquire_fence = -1; > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > - } > > + for (auto &buffer : descriptor->buffers_) > > + buffer.status = Camera3RequestDescriptor::Status::Error; > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > } > > @@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > << " with " << descriptor->buffers_.size() << " streams"; > > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > - camera3_stream *camera3Stream = buffer.buffer.stream; > > - CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > > + CameraStream *cameraStream = buffer.stream; > > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > > std::stringstream ss; > > ss << i << " - (" << camera3Stream->width << "x" > > @@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * lifetime management only. > > */ > > buffer.frameBuffer = > > - createFrameBuffer(*buffer.buffer.buffer, > > + createFrameBuffer(*buffer.camera3Buffer, > > cameraStream->configuration().pixelFormat, > > cameraStream->configuration().size); > > frameBuffer = buffer.frameBuffer.get(); > > - acquireFence = buffer.buffer.acquire_fence; > > + acquireFence = buffer.fence; > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > break; > > > > @@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request) > > /* > > * Prepare the capture result for the Android camera stack. > > * > > - * The buffer status is set to OK and later changed to ERROR if > > + * The buffer status is set to Success and later changed to Error if > > * post-processing/compression fails. > > */ > > for (auto &buffer : descriptor->buffers_) { > > - CameraStream *cameraStream = > > - static_cast<CameraStream *>(buffer.buffer.stream->priv); > > + CameraStream *stream = buffer.stream; > > > > /* > > * Streams of type Direct have been queued to the > > @@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request) > > * \todo Instrument the CameraWorker to set the acquire > > * fence to -1 once it has handled it and remove this check. > > */ > > - if (cameraStream->type() == CameraStream::Type::Direct) > > - buffer.buffer.acquire_fence = -1; > > - buffer.buffer.release_fence = -1; > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK; > > + if (stream->type() == CameraStream::Type::Direct) > > + buffer.fence = -1; > > + buffer.status = Camera3RequestDescriptor::Status::Success; > > } > > > > /* > > @@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request) > > > > /* Handle post-processing. */ > > for (auto &buffer : descriptor->buffers_) { > > - CameraStream *cameraStream = > > - static_cast<CameraStream *>(buffer.buffer.stream->priv); > > + CameraStream *stream = buffer.stream; > > > > - if (cameraStream->type() == CameraStream::Type::Direct) > > + if (stream->type() == CameraStream::Type::Direct) > > continue; > > > > - FrameBuffer *src = request->findBuffer(cameraStream->stream()); > > + FrameBuffer *src = request->findBuffer(stream->stream()); > > if (!src) { > > LOG(HAL, Error) << "Failed to find a source stream buffer"; > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > - notifyError(descriptor->frameNumber_, buffer.buffer.stream, > > + buffer.status = Camera3RequestDescriptor::Status::Error; > > + notifyError(descriptor->frameNumber_, stream->camera3Stream(), > > CAMERA3_MSG_ERROR_BUFFER); > > continue; > > } > > > > - int ret = cameraStream->process(*src, buffer, descriptor); > > + int ret = stream->process(*src, buffer, descriptor); > > > > /* > > * Return the FrameBuffer to the CameraStream now that we're > > * done processing it. > > */ > > - if (cameraStream->type() == CameraStream::Type::Internal) > > - cameraStream->putBuffer(src); > > + if (stream->type() == CameraStream::Type::Internal) > > + stream->putBuffer(src); > > > > if (ret) { > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > - notifyError(descriptor->frameNumber_, buffer.buffer.stream, > > + buffer.status = Camera3RequestDescriptor::Status::Error; > > + notifyError(descriptor->frameNumber_, stream->camera3Stream(), > > CAMERA3_MSG_ERROR_BUFFER); > > } > > } > > @@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults() > > captureResult.result = descriptor->resultMetadata_->get(); > > > > std::vector<camera3_stream_buffer_t> resultBuffers; > > - for (const auto &buffer : descriptor->buffers_) > > - resultBuffers.emplace_back(buffer.buffer); > > + resultBuffers.reserve(descriptor->buffers_.size()); > > + > > + for (const auto &buffer : descriptor->buffers_) { > > + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR; > > + > > + if (buffer.status == Camera3RequestDescriptor::Status::Success) > > + status = CAMERA3_BUFFER_STATUS_OK; > > + > > + /* > > + * Pass the buffer fence back to the camera framework as > > + * a release fence. This instructs the framework to wait > > + * on the acquire fence in case we haven't done so > > + * ourselves for any reason. > > + */ > > + resultBuffers.push_back({ buffer.stream->camera3Stream(), > > + buffer.camera3Buffer, status, > > + -1, buffer.fence }); > > + } > > > > captureResult.num_output_buffers = resultBuffers.size(); > > captureResult.output_buffers = resultBuffers.data(); > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > > index 614baed4..faa85ada 100644 > > --- a/src/android/camera_request.cpp > > +++ b/src/android/camera_request.cpp > > @@ -7,6 +7,8 @@ > > > > #include "camera_request.h" > > > > +#include <libcamera/base/span.h> > > + > > using namespace libcamera; > > > > /* > > @@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > frameNumber_ = camera3Request->frame_number; > > > > /* Copy the camera3 request stream information for later access. */ > > - const uint32_t numBuffers = camera3Request->num_output_buffers; > > + const Span<const camera3_stream_buffer_t> buffers{ > > + camera3Request->output_buffers, > > + camera3Request->num_output_buffers > > + }; > > + > > + buffers_.reserve(buffers.size()); > > + > > + for (const camera3_stream_buffer_t &buffer : buffers) { > > + CameraStream *stream = > > + static_cast<CameraStream *>(buffer.stream->priv); > > > > - buffers_.resize(numBuffers); > > - for (uint32_t i = 0; i < numBuffers; i++) > > - buffers_[i].buffer = camera3Request->output_buffers[i]; > > + buffers_.push_back({ stream, buffer.buffer, nullptr, > > + buffer.acquire_fence, Status::Pending }); > > + } > > > > /* Clone the controls associated with the camera3 request. */ > > settings_ = CameraMetadata(camera3Request->settings); > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > index a030febf..05dabf89 100644 > > --- a/src/android/camera_request.h > > +++ b/src/android/camera_request.h > > @@ -20,6 +20,8 @@ > > #include "camera_metadata.h" > > #include "camera_worker.h" > > > > +class CameraStream; > > + > > class Camera3RequestDescriptor > > { > > public: > > @@ -30,13 +32,11 @@ public: > > }; > > > > struct StreamBuffer { > > - camera3_stream_buffer_t buffer; > > - /* > > - * FrameBuffer instances created by wrapping a camera3 provided > > - * dmabuf are emplaced in this vector of unique_ptr<> for > > - * lifetime management. > > - */ > > + CameraStream *stream; > > + buffer_handle_t *camera3Buffer; > > std::unique_ptr<libcamera::FrameBuffer> frameBuffer; > > + int fence; > > + Status status; > > }; > > > > Camera3RequestDescriptor(libcamera::Camera *camera, > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > > index f3cc77e7..9b5cd0c4 100644 > > --- a/src/android/camera_stream.cpp > > +++ b/src/android/camera_stream.cpp > > @@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source, > > Camera3RequestDescriptor *request) > > { > > /* Handle waiting on fences on the destination buffer. */ > > - int fence = dest.buffer.acquire_fence; > > + int fence = dest.fence; > > if (fence != -1) { > > int ret = waitFence(fence); > > ::close(fence); > > - dest.buffer.acquire_fence = -1; > > + dest.fence = -1; > > if (ret < 0) { > > LOG(HAL, Error) << "Failed waiting for fence: " > > << fence << ": " << strerror(-ret); > > @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source, > > * separate thread. > > */ > > const StreamConfiguration &output = configuration(); > > - CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat, > > + CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat, > > output.size, PROT_READ | PROT_WRITE); > > if (!destBuffer.isValid()) { > > LOG(HAL, Error) << "Failed to create destination buffer";
Hi Laurent, thank you for the patch. On Tue, Oct 19, 2021 at 9:16 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jacopo, > > On Tue, Oct 19, 2021 at 02:12:08PM +0200, Jacopo Mondi wrote: > > On Tue, Oct 19, 2021 at 05:17:59PM +0530, Umang Jain wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > The camera3_stream_buffer_t structure is meant to communicate between > > > the camera service and the HAL. They are short-live structures that > > > don't outlive the .process_capture_request() operation (when queuing > > > requests) or the .process_capture_result() callback. > > > > > > We currently store copies of the camera3_stream_buffer_t passed to > > > .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to > > > store the structure members that the HAL need, and reuse them when > > > calling the .process_capture_result() callback. This is conceptually not > > > right, as the camera3_stream_buffer_t pass to the callback are not the > > > same objects as the ones received in .process_capture_request(). > > > > > > Store individual fields of the camera3_stream_buffer_t in StreamBuffer > > > instead of copying the whole structure. This gives the HAL full control > > > of how data is stored, and properly decouples request queueing from > > > result reporting. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com> > > > > I'm not sure I get what is functionally different, we were copying in > > the received camera3_stream_buffer_t, so there was no dependency with > > the ones passed in by the service. > > We don't use the same instances as we copy them, but we reuse the value, > which I don't think is conceptually correct. camera3_stream_buffer_t is > meant to pass buffer information in the API, it's not a buffer object in > itself, so I don't think it's the right level of abstraction (and we > have to extend it with additional fields anyway). > > > Anyway, it shortne names, so I think it's good :) > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > src/android/camera_device.cpp | 73 ++++++++++++++++++---------------- > > > src/android/camera_request.cpp | 19 +++++++-- > > > src/android/camera_request.h | 12 +++--- > > > src/android/camera_stream.cpp | 6 +-- > > > 4 files changed, 63 insertions(+), 47 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 0bb547ae..0a2d3826 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > > { > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > > > - for (auto &buffer : descriptor->buffers_) { > > > - /* > > > - * Signal to the framework it has to handle fences that have not > > > - * been waited on by setting the release fence to the acquire > > > - * fence value. > > > - */ > > > - buffer.buffer.release_fence = buffer.buffer.acquire_fence; > > > - buffer.buffer.acquire_fence = -1; > > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > - } > > > + for (auto &buffer : descriptor->buffers_) > > > + buffer.status = Camera3RequestDescriptor::Status::Error; > > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > } > > > @@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > << " with " << descriptor->buffers_.size() << " streams"; > > > > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > - camera3_stream *camera3Stream = buffer.buffer.stream; > > > - CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > > > + CameraStream *cameraStream = buffer.stream; > > > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > > > > std::stringstream ss; > > > ss << i << " - (" << camera3Stream->width << "x" > > > @@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > * lifetime management only. > > > */ > > > buffer.frameBuffer = > > > - createFrameBuffer(*buffer.buffer.buffer, > > > + createFrameBuffer(*buffer.camera3Buffer, > > > cameraStream->configuration().pixelFormat, > > > cameraStream->configuration().size); > > > frameBuffer = buffer.frameBuffer.get(); > > > - acquireFence = buffer.buffer.acquire_fence; > > > + acquireFence = buffer.fence; > > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > > break; > > > > > > @@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request) > > > /* > > > * Prepare the capture result for the Android camera stack. > > > * > > > - * The buffer status is set to OK and later changed to ERROR if > > > + * The buffer status is set to Success and later changed to Error if > > > * post-processing/compression fails. > > > */ > > > for (auto &buffer : descriptor->buffers_) { > > > - CameraStream *cameraStream = > > > - static_cast<CameraStream *>(buffer.buffer.stream->priv); > > > + CameraStream *stream = buffer.stream; > > > > > > /* > > > * Streams of type Direct have been queued to the > > > @@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request) > > > * \todo Instrument the CameraWorker to set the acquire > > > * fence to -1 once it has handled it and remove this check. > > > */ > > > - if (cameraStream->type() == CameraStream::Type::Direct) > > > - buffer.buffer.acquire_fence = -1; > > > - buffer.buffer.release_fence = -1; > > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK; > > > + if (stream->type() == CameraStream::Type::Direct) > > > + buffer.fence = -1; > > > + buffer.status = Camera3RequestDescriptor::Status::Success; > > > } > > > > > > /* > > > @@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request) > > > > > > /* Handle post-processing. */ > > > for (auto &buffer : descriptor->buffers_) { > > > - CameraStream *cameraStream = > > > - static_cast<CameraStream *>(buffer.buffer.stream->priv); > > > + CameraStream *stream = buffer.stream; > > > > > > - if (cameraStream->type() == CameraStream::Type::Direct) > > > + if (stream->type() == CameraStream::Type::Direct) > > > continue; > > > > > > - FrameBuffer *src = request->findBuffer(cameraStream->stream()); > > > + FrameBuffer *src = request->findBuffer(stream->stream()); > > > if (!src) { > > > LOG(HAL, Error) << "Failed to find a source stream buffer"; > > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > - notifyError(descriptor->frameNumber_, buffer.buffer.stream, > > > + buffer.status = Camera3RequestDescriptor::Status::Error; > > > + notifyError(descriptor->frameNumber_, stream->camera3Stream(), > > > CAMERA3_MSG_ERROR_BUFFER); > > > continue; > > > } > > > > > > - int ret = cameraStream->process(*src, buffer, descriptor); > > > + int ret = stream->process(*src, buffer, descriptor); > > > > > > /* > > > * Return the FrameBuffer to the CameraStream now that we're > > > * done processing it. > > > */ > > > - if (cameraStream->type() == CameraStream::Type::Internal) > > > - cameraStream->putBuffer(src); > > > + if (stream->type() == CameraStream::Type::Internal) > > > + stream->putBuffer(src); > > > > > > if (ret) { > > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > - notifyError(descriptor->frameNumber_, buffer.buffer.stream, > > > + buffer.status = Camera3RequestDescriptor::Status::Error; > > > + notifyError(descriptor->frameNumber_, stream->camera3Stream(), > > > CAMERA3_MSG_ERROR_BUFFER); > > > } > > > } > > > @@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults() > > > captureResult.result = descriptor->resultMetadata_->get(); > > > > > > std::vector<camera3_stream_buffer_t> resultBuffers; > > > - for (const auto &buffer : descriptor->buffers_) > > > - resultBuffers.emplace_back(buffer.buffer); > > > + resultBuffers.reserve(descriptor->buffers_.size()); > > > + If I read the change of this patch series correctly, when abortRequest() is called, this for-loop is not reachable? > > > + for (const auto &buffer : descriptor->buffers_) { > > > + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR; > > > + > > > + if (buffer.status == Camera3RequestDescriptor::Status::Success) > > > + status = CAMERA3_BUFFER_STATUS_OK; > > > + > > > + /* > > > + * Pass the buffer fence back to the camera framework as > > > + * a release fence. This instructs the framework to wait > > > + * on the acquire fence in case we haven't done so > > > + * ourselves for any reason. > > > + */ > > > + resultBuffers.push_back({ buffer.stream->camera3Stream(), > > > + buffer.camera3Buffer, status, > > > + -1, buffer.fence }); > > > + } > > > > > > captureResult.num_output_buffers = resultBuffers.size(); > > > captureResult.output_buffers = resultBuffers.data(); > > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > > > index 614baed4..faa85ada 100644 > > > --- a/src/android/camera_request.cpp > > > +++ b/src/android/camera_request.cpp > > > @@ -7,6 +7,8 @@ > > > > > > #include "camera_request.h" > > > > > > +#include <libcamera/base/span.h> > > > + > > > using namespace libcamera; > > > > > > /* > > > @@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > > frameNumber_ = camera3Request->frame_number; > > > > > > /* Copy the camera3 request stream information for later access. */ > > > - const uint32_t numBuffers = camera3Request->num_output_buffers; > > > + const Span<const camera3_stream_buffer_t> buffers{ > > > + camera3Request->output_buffers, > > > + camera3Request->num_output_buffers > > > + }; > > > + > > > + buffers_.reserve(buffers.size()); > > > + > > > + for (const camera3_stream_buffer_t &buffer : buffers) { > > > + CameraStream *stream = > > > + static_cast<CameraStream *>(buffer.stream->priv); > > > > > > - buffers_.resize(numBuffers); > > > - for (uint32_t i = 0; i < numBuffers; i++) > > > - buffers_[i].buffer = camera3Request->output_buffers[i]; > > > + buffers_.push_back({ stream, buffer.buffer, nullptr, > > > + buffer.acquire_fence, Status::Pending }); > > > + } > > > > > > /* Clone the controls associated with the camera3 request. */ > > > settings_ = CameraMetadata(camera3Request->settings); > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > index a030febf..05dabf89 100644 > > > --- a/src/android/camera_request.h > > > +++ b/src/android/camera_request.h > > > @@ -20,6 +20,8 @@ > > > #include "camera_metadata.h" > > > #include "camera_worker.h" > > > > > > +class CameraStream; > > > + > > > class Camera3RequestDescriptor > > > { > > > public: > > > @@ -30,13 +32,11 @@ public: > > > }; > > > > > > struct StreamBuffer { > > > - camera3_stream_buffer_t buffer; > > > - /* > > > - * FrameBuffer instances created by wrapping a camera3 provided > > > - * dmabuf are emplaced in this vector of unique_ptr<> for > > > - * lifetime management. > > > - */ Would you mind commenting the ownership of these variables? Reviewed-by: Hirokazu Honda <hiroh@chromium.org> Thanks, -Hiro > > > + CameraStream *stream; > > > + buffer_handle_t *camera3Buffer; > > > std::unique_ptr<libcamera::FrameBuffer> frameBuffer; > > > + int fence; > > > + Status status; > > > }; > > > > > > Camera3RequestDescriptor(libcamera::Camera *camera, > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > > > index f3cc77e7..9b5cd0c4 100644 > > > --- a/src/android/camera_stream.cpp > > > +++ b/src/android/camera_stream.cpp > > > @@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source, > > > Camera3RequestDescriptor *request) > > > { > > > /* Handle waiting on fences on the destination buffer. */ > > > - int fence = dest.buffer.acquire_fence; > > > + int fence = dest.fence; > > > if (fence != -1) { > > > int ret = waitFence(fence); > > > ::close(fence); > > > - dest.buffer.acquire_fence = -1; > > > + dest.fence = -1; > > > if (ret < 0) { > > > LOG(HAL, Error) << "Failed waiting for fence: " > > > << fence << ": " << strerror(-ret); > > > @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source, > > > * separate thread. > > > */ > > > const StreamConfiguration &output = configuration(); > > > - CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat, > > > + CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat, > > > output.size, PROT_READ | PROT_WRITE); > > > if (!destBuffer.isValid()) { > > > LOG(HAL, Error) << "Failed to create destination buffer"; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 0bb547ae..0a2d3826 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const { notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); - for (auto &buffer : descriptor->buffers_) { - /* - * Signal to the framework it has to handle fences that have not - * been waited on by setting the release fence to the acquire - * fence value. - */ - buffer.buffer.release_fence = buffer.buffer.acquire_fence; - buffer.buffer.acquire_fence = -1; - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - } + for (auto &buffer : descriptor->buffers_) + buffer.status = Camera3RequestDescriptor::Status::Error; descriptor->status_ = Camera3RequestDescriptor::Status::Error; } @@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques << " with " << descriptor->buffers_.size() << " streams"; for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { - camera3_stream *camera3Stream = buffer.buffer.stream; - CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); + CameraStream *cameraStream = buffer.stream; + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); std::stringstream ss; ss << i << " - (" << camera3Stream->width << "x" @@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * lifetime management only. */ buffer.frameBuffer = - createFrameBuffer(*buffer.buffer.buffer, + createFrameBuffer(*buffer.camera3Buffer, cameraStream->configuration().pixelFormat, cameraStream->configuration().size); frameBuffer = buffer.frameBuffer.get(); - acquireFence = buffer.buffer.acquire_fence; + acquireFence = buffer.fence; LOG(HAL, Debug) << ss.str() << " (direct)"; break; @@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request) /* * Prepare the capture result for the Android camera stack. * - * The buffer status is set to OK and later changed to ERROR if + * The buffer status is set to Success and later changed to Error if * post-processing/compression fails. */ for (auto &buffer : descriptor->buffers_) { - CameraStream *cameraStream = - static_cast<CameraStream *>(buffer.buffer.stream->priv); + CameraStream *stream = buffer.stream; /* * Streams of type Direct have been queued to the @@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request) * \todo Instrument the CameraWorker to set the acquire * fence to -1 once it has handled it and remove this check. */ - if (cameraStream->type() == CameraStream::Type::Direct) - buffer.buffer.acquire_fence = -1; - buffer.buffer.release_fence = -1; - buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK; + if (stream->type() == CameraStream::Type::Direct) + buffer.fence = -1; + buffer.status = Camera3RequestDescriptor::Status::Success; } /* @@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request) /* Handle post-processing. */ for (auto &buffer : descriptor->buffers_) { - CameraStream *cameraStream = - static_cast<CameraStream *>(buffer.buffer.stream->priv); + CameraStream *stream = buffer.stream; - if (cameraStream->type() == CameraStream::Type::Direct) + if (stream->type() == CameraStream::Type::Direct) continue; - FrameBuffer *src = request->findBuffer(cameraStream->stream()); + FrameBuffer *src = request->findBuffer(stream->stream()); if (!src) { LOG(HAL, Error) << "Failed to find a source stream buffer"; - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - notifyError(descriptor->frameNumber_, buffer.buffer.stream, + buffer.status = Camera3RequestDescriptor::Status::Error; + notifyError(descriptor->frameNumber_, stream->camera3Stream(), CAMERA3_MSG_ERROR_BUFFER); continue; } - int ret = cameraStream->process(*src, buffer, descriptor); + int ret = stream->process(*src, buffer, descriptor); /* * Return the FrameBuffer to the CameraStream now that we're * done processing it. */ - if (cameraStream->type() == CameraStream::Type::Internal) - cameraStream->putBuffer(src); + if (stream->type() == CameraStream::Type::Internal) + stream->putBuffer(src); if (ret) { - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - notifyError(descriptor->frameNumber_, buffer.buffer.stream, + buffer.status = Camera3RequestDescriptor::Status::Error; + notifyError(descriptor->frameNumber_, stream->camera3Stream(), CAMERA3_MSG_ERROR_BUFFER); } } @@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults() captureResult.result = descriptor->resultMetadata_->get(); std::vector<camera3_stream_buffer_t> resultBuffers; - for (const auto &buffer : descriptor->buffers_) - resultBuffers.emplace_back(buffer.buffer); + resultBuffers.reserve(descriptor->buffers_.size()); + + for (const auto &buffer : descriptor->buffers_) { + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR; + + if (buffer.status == Camera3RequestDescriptor::Status::Success) + status = CAMERA3_BUFFER_STATUS_OK; + + /* + * Pass the buffer fence back to the camera framework as + * a release fence. This instructs the framework to wait + * on the acquire fence in case we haven't done so + * ourselves for any reason. + */ + resultBuffers.push_back({ buffer.stream->camera3Stream(), + buffer.camera3Buffer, status, + -1, buffer.fence }); + } captureResult.num_output_buffers = resultBuffers.size(); captureResult.output_buffers = resultBuffers.data(); diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp index 614baed4..faa85ada 100644 --- a/src/android/camera_request.cpp +++ b/src/android/camera_request.cpp @@ -7,6 +7,8 @@ #include "camera_request.h" +#include <libcamera/base/span.h> + using namespace libcamera; /* @@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( frameNumber_ = camera3Request->frame_number; /* Copy the camera3 request stream information for later access. */ - const uint32_t numBuffers = camera3Request->num_output_buffers; + const Span<const camera3_stream_buffer_t> buffers{ + camera3Request->output_buffers, + camera3Request->num_output_buffers + }; + + buffers_.reserve(buffers.size()); + + for (const camera3_stream_buffer_t &buffer : buffers) { + CameraStream *stream = + static_cast<CameraStream *>(buffer.stream->priv); - buffers_.resize(numBuffers); - for (uint32_t i = 0; i < numBuffers; i++) - buffers_[i].buffer = camera3Request->output_buffers[i]; + buffers_.push_back({ stream, buffer.buffer, nullptr, + buffer.acquire_fence, Status::Pending }); + } /* Clone the controls associated with the camera3 request. */ settings_ = CameraMetadata(camera3Request->settings); diff --git a/src/android/camera_request.h b/src/android/camera_request.h index a030febf..05dabf89 100644 --- a/src/android/camera_request.h +++ b/src/android/camera_request.h @@ -20,6 +20,8 @@ #include "camera_metadata.h" #include "camera_worker.h" +class CameraStream; + class Camera3RequestDescriptor { public: @@ -30,13 +32,11 @@ public: }; struct StreamBuffer { - camera3_stream_buffer_t buffer; - /* - * FrameBuffer instances created by wrapping a camera3 provided - * dmabuf are emplaced in this vector of unique_ptr<> for - * lifetime management. - */ + CameraStream *stream; + buffer_handle_t *camera3Buffer; std::unique_ptr<libcamera::FrameBuffer> frameBuffer; + int fence; + Status status; }; Camera3RequestDescriptor(libcamera::Camera *camera, diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index f3cc77e7..9b5cd0c4 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source, Camera3RequestDescriptor *request) { /* Handle waiting on fences on the destination buffer. */ - int fence = dest.buffer.acquire_fence; + int fence = dest.fence; if (fence != -1) { int ret = waitFence(fence); ::close(fence); - dest.buffer.acquire_fence = -1; + dest.fence = -1; if (ret < 0) { LOG(HAL, Error) << "Failed waiting for fence: " << fence << ": " << strerror(-ret); @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source, * separate thread. */ const StreamConfiguration &output = configuration(); - CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat, + CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat, output.size, PROT_READ | PROT_WRITE); if (!destBuffer.isValid()) { LOG(HAL, Error) << "Failed to create destination buffer";